Messages in this thread | | | From | Namhyung Kim <> | Date | Sun, 15 Oct 2023 11:53:44 -0700 | Subject | Re: [PATCH v2] perf bench sched pipe: Add -G/--cgroups option |
| |
On Sat, Oct 14, 2023 at 1:44 AM Ingo Molnar <mingo@kernel.org> wrote: > > > * Namhyung Kim <namhyung@kernel.org> wrote: > > > + cgrp_send = cgroup__new(p, /*do_open=*/true); > > + if (cgrp_send == NULL) { > > + fprintf(stderr, "cannot open sender cgroup: %s", p); > > + goto out; > > + } > > Maybe in this case print out a small suggestion of how to create this > particular cgroup? > > Most distro users and even kernel developers don't ever have to create > new cgroups. > > Maybe even allow the creation of new cgroups for this testing, if they > don't already exist? As long as we don't delete any cgroups I don't think > much harm can be done - and the increase in usability is substantial.
I'm not sure if it's ok create a new cgroup and leave it after the use. Anyway, none of the existing subcommands create new cgroups IIUC and I think it'd be ok to print a message on how to create one.
> > > +static void enter_cgroup(struct cgroup *cgrp) > > +{ > > + char buf[32]; > > + int fd, len; > > + pid_t pid; > > + > > + if (cgrp == NULL) > > + return; > > + > > + if (threaded) > > + pid = syscall(__NR_gettid); > > + else > > + pid = getpid(); > > + > > + snprintf(buf, sizeof(buf), "%d\n", pid); > > + len = strlen(buf); > > + > > + /* try cgroup v2 interface first */ > > + if (threaded) > > + fd = openat(cgrp->fd, "cgroup.threads", O_WRONLY); > > + else > > + fd = openat(cgrp->fd, "cgroup.procs", O_WRONLY); > > + > > + /* try cgroup v1 if failed */ > > + if (fd < 0) > > + fd = openat(cgrp->fd, "tasks", O_WRONLY); > > + > > + if (fd < 0) { > > + printf("failed to open cgroup file in %s\n", cgrp->name); > > + return; > > + } > > + > > + if (write(fd, buf, len) != len) > > + printf("cannot enter to cgroup: %s\n", cgrp->name); > > The failures here should probably result in termination of the run with an > error code, not just messages which are easy to skip in automated tests?
Right, I'll make the change.
Thanks, Namhyung
| |