Re: [RFC PATCH bpf-next 0/8] bpf, cgroup: Add bpf support for cgroup controller

[Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

Hello,

On Sun, Sep 24, 2023 at 02:32:14PM +0800, Yafang Shao wrote:
> On Sat, Sep 23, 2023 at 12:52 AM Tejun Heo <tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
> >
> > Hello,
> >
> > On Fri, Sep 22, 2023 at 11:28:38AM +0000, Yafang Shao wrote:
> > > - bpf_cgroup_id_from_task_within_controller
> > >   Retrieves the cgroup ID from a task within a specific cgroup controller.
> > > - bpf_cgroup_acquire_from_id_within_controller
> > >   Acquires the cgroup from a cgroup ID within a specific cgroup controller.
> > > - bpf_cgroup_ancestor_id_from_task_within_controller
> > >   Retrieves the ancestor cgroup ID from a task within a specific cgroup
> > >   controller.
> > >
> > > The advantage of these new BPF kfuncs is their ability to abstract away the
> > > complexities of cgroup hierarchies, irrespective of whether they involve
> > > cgroup1 or cgroup2.
> >
> > I'm afraid this is more likely to bring the unnecessary complexities of
> > cgroup1 into cgroup2.
> 
> I concur with the idea that we should avoid introducing the
> complexities of cgroup1 into cgroup2. Which specific change do you
> believe might introduce these complexities into cgroup2? Is it the
> modification within task_under_cgroup_hierarchy() or
> cgroup_get_from_id()?

The helpers you are adding only makes sense for cgroup1. e.g.
bpf_cgroup_ancestor_id_from_task_within_controller() makes no sense in
cgroup2. The ancestor ids don't change according to controllers. The only
thing you would ask in cgroup2 is the level at which a given controller is
enabled at along with the straight-forward "where am I in the hierarchy?"
questions. I really don't want to expose interfaces which assume that the
hierarchies change according to the controller in question.

Also, as pointed out before, this doesn't cover cgroup1 named hierarchies
which leaves out a good potion of cgroup1 use cases.

> In fact, we have the option to utilize
> bpf_cgroup_ancestor_id_from_task_within_controller() as a substitute
> for bpf_task_under_cgroup(), which allows us to sidestep the need for
> changes within task_under_cgroup_hierarchy() altogether.

I don't think this is the direction we should take. If you really want,
please tie the interface directly to the hierarchies. Don't hitch hierarchy
identificdation on the controllers. e.g. Introduce cgroup1 only interface
which takes both hierarchy ID and cgroup ID to operate on them.

Thanks.

-- 
tejun