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@xxxxxxxxxx> 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
