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

[Eduard Zingerman <eddyz87@gmail.com>]

On Thu, 2025-06-05 at 19:17 -0700, Alexei Starovoitov wrote:
> On Thu, Jun 5, 2025 at 6:04 PM Eduard Zingerman <eddyz87@gmail.com> wrote:
> > 
> > On Thu, 2025-06-05 at 16:06 -0700, Eduard Zingerman wrote:
> > 
> > [...]
> > 
> > > +/*
> > > + * Enters new cgroup namespace and mounts cgroupfs at /tmp/veristat-cgroup-mount-XXXXXX,
> > > + * enables "memory" controller for the root cgroup.
> > > + */
> > > +static int mount_cgroupfs(void)
> > > +{
> > > +     char buf[PATH_MAX + 1];
> > > +     int err;
> > > +
> > > +     env.memory_peak_fd = -1;
> > > +
> > > +     err = unshare(CLONE_NEWCGROUP);
> > > +     if (err < 0) {
> > > +             err = log_errno("unshare(CLONE_NEWCGROUP)");
> > > +             goto err_out;
> > > +     }
> > 
> > The `unshare` call is useless. I thought it would grant me a new
> > hierarchy with separate cgroup.subtree_control in the root.
> > But that's now how things work, hierarchy is shared across namespaces.
> > I'll drop this call and just complain if "memory" controller is not enabled.
> > 
> > The "mount" part can remain, I use it to avoid searching for cgroupfs
> > mount point. Alternatively I can inspect /proc/self/mountinfo and
> > complain if cgroupfs is not found. Please let me know which way is
> > preferred.
> 
> I would keep unshare and mount,

But what would be the purpose of the unshare?
I thought that it provides a new isolated new independent
cgroup.subtree_control, but that is not the case.
Outside from that the feature gains nothing from entering new namespace.

> and if possible share the code with setup_cgroup_environment()
> from cgroup_helpers.c

setup_cgroup_environment() does the following:
 a. creates a directory for cgroupfs mount
 b. creates a new mount namespace
 c. mounts a new empty root
 d. mounts cgroupfs inside new root
 e. creates a cgroup
 f. enables controllers

Of these only (a) and (d) are needed for veristat (and (f) is deligated to init).
Also my current version does unshare for cgroup namespace, while
setup_cgroup_environment() does not do that.

Things that might be reusable:
- get_root_cgroup
- remove_cgroup
- join_root_cgroup
- join_cgroup

These rely on CGROUP_MOUNT_PATH and CGROUP_WORK_DIR being constants,
I'll need to modify these helpers to parameterize this.