Re: [PATCH bpf-next v2 2/2] veristat: memory accounting for bpf programs

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-06-12 at 17:01 -0700, Andrii Nakryiko wrote:

[...]

> > +static void destroy_stat_cgroup(void)
> > +{
> > +       char buf[PATH_MAX];
> > +       int err;
> > +
> > +       close(env.memory_peak_fd);
> > +
> > +       if (env.orig_cgroup[0]) {
> > +               snprintf_trunc(buf, sizeof(buf), "%s/cgroup.procs", env.orig_cgroup);
> > +               err = write_one_line(buf, "%d\n", getpid());
> > +               if (err < 0)
> > +                       log_errno("moving self to original cgroup %s\n", env.orig_cgroup);
> > +       }
> > +
> > +       if (env.stat_cgroup[0]) {
> > +               err = rmdir(env.stat_cgroup);
> 
> We need to enter the original cgroup to successfully remove the one we
> created, is that right? Otherwise, why bother reentering if we are on
> our way out, no?

Yes, cgroup can't be removed if there are member processes.
I chose to organize this way because there would be a message printed
with a name of stale group.

> 
> > +               if (err < 0)
> > +                       log_errno("deletion of cgroup %s", env.stat_cgroup);
> > +       }
> > +
> > +       env.memory_peak_fd = -1;
> > +       env.orig_cgroup[0] = 0;
> > +       env.stat_cgroup[0] = 0;
> > +}
> > +
> > +/*
> > + * Creates a cgroup at /sys/fs/cgroup/veristat-accounting-<pid>,
> > + * moves current process to this cgroup.
> > + */
> > +static void create_stat_cgroup(void)
> > +{
> > +       char cgroup_fs_mount[PATH_MAX + 1];
> > +       char buf[PATH_MAX + 1];
> > +       int err;
> > +
> > +       env.memory_peak_fd = -1;
> > +
> > +       if (!output_stat_enabled(MEMORY_PEAK))
> > +               return;
> > +
> > +       err = scanf_one_line("/proc/self/mounts", 2, "%*s %" STR(PATH_MAX) "s cgroup2 %s",
> 
> let's just hard-code 1024 or something and not do that STR() magic,
> please (same below).
> 
> > +                            cgroup_fs_mount, buf);
> > +       if (err != 2) {
> > +               if (err < 0)
> > +                       log_errno("reading /proc/self/mounts");
> > +               else if (!env.quiet)
> > +                       fprintf(stderr, "Can't find cgroupfs v2 mount point.\n");
> > +               goto err_out;
> > +       }
> > +
> > +       /* cgroup-v2.rst promises the line "0::<group>" for cgroups v2 */
> > +       err = scanf_one_line("/proc/self/cgroup", 1, "0::%" STR(PATH_MAX) "s", buf);
> 
> do you think just hard-coding /sys/fs/cgroup would not work in
> practice? It just feels like we are trying to be a bit too flexible
> here...

Idk, removing this saves 10 lines.
On machines I have access to (one CentOS, one Fedora) the mount point
is /sys/fs/cgroup.

> 
> > +       if (err != 1) {
> > +               if (err < 0)
> > +                       log_errno("reading /proc/self/cgroup");
> > +               else if (!env.quiet)
> > +                       fprintf(stderr, "Can't infer veristat process cgroup.");
> > +               goto err_out;
> > +       }
> > +
> 
> [...]

Ack for other points, thank you for taking a look.