Message-ID: <e027a330-8d51-44e5-badc-7c3ec4d41e23@linux.dev>
Date: Thu, 30 Oct 2025 15:42:12 -0700
From: Martin KaFai Lau <martin.lau@...ux.dev>
To: Song Liu <song@...nel.org>, Roman Gushchin <roman.gushchin@...ux.dev>
Cc: Amery Hung <ameryhung@...il.com>,
 Andrew Morton <akpm@...ux-foundation.org>, linux-kernel@...r.kernel.org,
 Alexei Starovoitov <ast@...nel.org>, Suren Baghdasaryan <surenb@...gle.com>,
 Michal Hocko <mhocko@...nel.org>, Shakeel Butt <shakeel.butt@...ux.dev>,
 Johannes Weiner <hannes@...xchg.org>, Andrii Nakryiko <andrii@...nel.org>,
 JP Kobryn <inwardvessel@...il.com>, linux-mm@...ck.org,
 cgroups@...r.kernel.org, bpf@...r.kernel.org,
 Martin KaFai Lau <martin.lau@...nel.org>,
 Kumar Kartikeya Dwivedi <memxor@...il.com>, Tejun Heo <tj@...nel.org>
Subject: Re: [PATCH v2 02/23] bpf: initial support for attaching struct ops to
 cgroups



On 10/30/25 2:34 PM, Song Liu wrote:
> Hi Roman,
> 
> On Thu, Oct 30, 2025 at 12:07 PM Roman Gushchin
> <roman.gushchin@...ux.dev> wrote:
> [...]
>>> In TCP congestion control and BPF qdisc's model:
>>>
>>> During link_create, both adds the struct_ops to a list, and the
>>> struct_ops can be indexed by name. The struct_ops are not "active" by
>>> this time.
>>> Then, each has their own interface to 'apply' the struct_ops to a
>>> socket or queue: setsockopt() or netlink.
>>>
>>> But maybe cgroup-related struct_ops are different.
>>
>> Both tcp congestion and qdisk cases are somewhat different because
>> there already is a way to select between multiple implementations, bpf
>> just adds another one. In the oom case, it's not true. As of today,
>> there is only one (global) oom killer. Of course we can create
>> interfaces to allow a user make a choice. But the question is do we want
>> to create such interface for the oom case specifically (and later for
>> each new case separately), or there is a place for some generalization?
> 
> Agreed that this approach requires a separate mechanism to attach
> the struct_ops to an entity.
> 
>> Ok, let me summarize the options we discussed here:
> 
> Thanks for the summary!
> 
>>
>> 1) Make the attachment details (e.g. cgroup_id) the part of struct ops
>> itself. The attachment is happening at the reg() time.
>>
>>    +: It's convenient for complex stateful struct ops'es, because a
>>        single entity represents a combination of code and data.
>>    -: No way to attach a single struct ops to multiple entities.
>>
>> This approach is used by Tejun for per-cgroup sched_ext prototype.
>>
>> 2) Make the attachment details a part of bpf_link creation. The
>> attachment is still happening at the reg() time.
>>
>>    +: A single struct ops can be attached to multiple entities.
>>    -: Implementing stateful struct ops'es is harder and requires passing
>>       an additional argument (some sort of "self") to all callbacks.
>> I'm using this approach in the bpf oom proposal.
>>
> 
> I think both 1) and 2) have the following issue. With cgroup_id in
> struct_ops or the link, the cgroup_id works more like a filter. The
> cgroup doesn't hold any reference to the struct_ops. The bpf link
> holds the reference to the struct_ops, so we need to keep the
> the link alive, either by keeping an active fd, or by pinning the
> link to bpffs. When the cgroup is removed, we need to clean up
> the bpf link separately.

The link can be detached (struct_ops's unreg) by the user space.

The link can also be detached from the subsystem (cgroup) here.
It was requested by scx:
https://lore.kernel.org/all/20240530065946.979330-7-thinker.li@gmail.com/

Not sure if scx has started using it.

> 
>> 3) Move the attachment out of .reg() scope entirely. reg() will register
>> the implementation system-wide and then some 3rd-party interface
>> (e.g. cgroupfs) should be used to select the implementation.
>>
>>    +: ?
>>    -: New hard-coded interfaces might be required to enable bpf-driven
>>       kernel customization. The "attachment" code is not shared between
>>       various struct ops cases.
>>       Implementing stateful struct ops'es is harder and requires passing
>>       an additional argument (some sort of "self") to all callbacks.
>>
>> This approach works well for cases when there is already a selection
>> of implementations (e.g. tcp congestion mechanisms), and bpf is adding
>> another one.
> 
> Another benefit of 3) is that it allows loading an OOM controller in a
> kernel module, just like loading a file system in a kernel module. This
> is possible with 3) because we paid the cost of adding a new select
> attach interface.
> 
> A semi-separate topic, option 2) enables attaching a BPF program
> to a kernel object (a cgroup here, but could be something else). This
> is an interesting idea, and we may find it useful in other cases (attach
> a BPF program to a task_struct, etc.).

Does it have plan for a pure kernel module oom implementation?
I think the link-to-cgrp support here does not necessary stop the
later write to cgroupfs support if a kernel module oom is indeed needed
in the future.

imo, cgroup-bpf has a eco-system around it, so it is sort of special. bpf user
has expectation on how a bpf prog is attached to a cgroup. The introspection,
auto detachment from the cgroup when the link is gone...etc.

If link-to-cgrp is used, I prefer (2). Stay with one way to attach
to a cgrp. It is also consistent with the current way of attaching a single
bpf prog to a cgroup. It is now attaching a map/set of bpf prog to a cgroup.
The individual struct_ops implementation can decide if it should
allow a struct_ops be attached multiple times.

Powered by blists - more mailing lists