v1 changes:
- remove guard support for BPF
- split patch into parts

v0 link:
https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/

Jemmy Wong (3):
  cgroup: add lock guard support for cgroup_muetx
  cgroup: add lock guard support for css_set_lock and rcu
  cgroup: add lock guard support for others

 include/linux/cgroup.h          |   7 +
 kernel/cgroup/cgroup-internal.h |   8 +-
 kernel/cgroup/cgroup-v1.c       |  56 ++-
 kernel/cgroup/cgroup.c          | 720 +++++++++++++++-----------------
 kernel/cgroup/cpuset-v1.c       |  16 +-
 kernel/cgroup/debug.c           | 185 ++++----
 kernel/cgroup/freezer.c         |  28 +-
 kernel/cgroup/namespace.c       |   8 +-
 8 files changed, 483 insertions(+), 545 deletions(-)

-- 
2.43.0

Hello.

I understand why this might have been controversial but I'm surprised it
remains so when the lock guards are already used in the kernel code.
Documentation/process/coding-style.rst isn't partisan in either way.

Johannes:
> heebeejeebees - it's asymmetric and critical sections don't stand out
> visually at all.
- I'd say that's the point of it for functions that are made to
  manipulate data structures under the lock. Not to spoil the code.
- Yes, it's a different coding style that one has to get used to.

> extra indentation
- deeper indentation == deeper locking, wary of that
- although I admit, in some cases of the reworked code, it's overly deep

Further reasoning is laid out in include/linux/cleanup.h. I noticed
there exists no_free_ptr() macro and that suggests an idea for analogous
no_exit_class() (or unguard()) macro (essential an unlock + signal for
compiler to not call cleanup function after this BB).

Tejun:
> There are no practical benefits to converting the code base at this point.

I'd expect future backports (into such code) to be more robust wrt
pairing errors.
At the same time this is also my biggest concern about this change, the
wide-spread diff would make current backporting more difficult.  (But
I'd counter argue that one should think forward here.)


Further benefits I see:
- it is fewer lines (code is to the point),
- reliable cleanup paths,
- it is modern and evolution step forward (given such constructs
  eventually emerge in various languages).


On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote:
> v1 changes:
> - remove guard support for BPF
> - split patch into parts
> 
> v0 link:
> https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/
> 
> Jemmy Wong (3):
>   cgroup: add lock guard support for cgroup_muetx
>   cgroup: add lock guard support for css_set_lock and rcu
>   cgroup: add lock guard support for others

As for the series in general
- I'm still in favor of pursuing it (with remarks to individual
  patches),
- it's a good opportunity to also to append sparse __acquires/__releases
  cleanup to it (see also [1]).

Regards,
Michal

[1] https://lore.kernel.org/r/Z_6z2-qqLI7dbl8h@slm.duckdns.org

Hi Michal, Tejun, Johannes,

Thank you, Michal, for supporting this modernization effort to adopt guard constructs.

With 3,326 instances already in use across the kernel upstream,
guards offer improved safety and readability.

I look forward to working with the community to integrate them into cgroup 
and welcome feedback to ensure a smooth transition.

Best,
Jemmy

> On Jun 17, 2025, at 5:08 PM, Michal Koutný <mkoutny@suse.com> wrote:
> 
> Hello.
> 
> I understand why this might have been controversial but I'm surprised it
> remains so when the lock guards are already used in the kernel code.
> Documentation/process/coding-style.rst isn't partisan in either way.
> 
> Johannes:
>> heebeejeebees - it's asymmetric and critical sections don't stand out
>> visually at all.
> - I'd say that's the point of it for functions that are made to
>  manipulate data structures under the lock. Not to spoil the code.
> - Yes, it's a different coding style that one has to get used to.
> 
>> extra indentation
> - deeper indentation == deeper locking, wary of that
> - although I admit, in some cases of the reworked code, it's overly deep
> 
> Further reasoning is laid out in include/linux/cleanup.h. I noticed
> there exists no_free_ptr() macro and that suggests an idea for analogous
> no_exit_class() (or unguard()) macro (essential an unlock + signal for
> compiler to not call cleanup function after this BB).
> 
> Tejun:
>> There are no practical benefits to converting the code base at this point.
> 
> I'd expect future backports (into such code) to be more robust wrt
> pairing errors.
> At the same time this is also my biggest concern about this change, the
> wide-spread diff would make current backporting more difficult.  (But
> I'd counter argue that one should think forward here.)
> 
> 
> Further benefits I see:
> - it is fewer lines (code is to the point),
> - reliable cleanup paths,
> - it is modern and evolution step forward (given such constructs
>  eventually emerge in various languages).
> 
> 
> On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong <jemmywong512@gmail.com> wrote:
>> v1 changes:
>> - remove guard support for BPF
>> - split patch into parts
>> 
>> v0 link:
>> https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/
>> 
>> Jemmy Wong (3):
>>  cgroup: add lock guard support for cgroup_muetx
>>  cgroup: add lock guard support for css_set_lock and rcu
>>  cgroup: add lock guard support for others
> 
> As for the series in general
> - I'm still in favor of pursuing it (with remarks to individual
>  patches),
> - it's a good opportunity to also to append sparse __acquires/__releases
>  cleanup to it (see also [1]).
> 
> Regards,
> Michal
> 
> [1] https://lore.kernel.org/r/Z_6z2-qqLI7dbl8h@slm.duckdns.org
> 

On Fri, Jun 20, 2025 at 06:45:54PM +0800, Jemmy Wong wrote:
...
> > Tejun:
> >> There are no practical benefits to converting the code base at this point.
> > 
> > I'd expect future backports (into such code) to be more robust wrt
> > pairing errors.
> > At the same time this is also my biggest concern about this change, the
> > wide-spread diff would make current backporting more difficult.  (But
> > I'd counter argue that one should think forward here.)

Well, I'm not necessarily against it but I generally dislike wholesale
cleanups which create big patch application boundaries. If there are enough
practical benefits, sure, we should do it, but when it's things like this -
maybe possibly it's a bit better in the long term - the calculus isn't clear
cut. People can argue these things to high heavens on abstract grounds, but
if you break it down to practical gains vs. costs, it's not a huge
difference.

But, again, I'm not against it. Johannes, any second thoughts?

Thanks.

-- 
tejun

On Fri, Jun 20, 2025 at 04:52:03PM -1000, Tejun Heo wrote:
> On Fri, Jun 20, 2025 at 06:45:54PM +0800, Jemmy Wong wrote:
> ...
> > > Tejun:
> > >> There are no practical benefits to converting the code base at this point.
> > > 
> > > I'd expect future backports (into such code) to be more robust wrt
> > > pairing errors.
> > > At the same time this is also my biggest concern about this change, the
> > > wide-spread diff would make current backporting more difficult.  (But
> > > I'd counter argue that one should think forward here.)
> 
> Well, I'm not necessarily against it but I generally dislike wholesale
> cleanups which create big patch application boundaries. If there are enough
> practical benefits, sure, we should do it, but when it's things like this -
> maybe possibly it's a bit better in the long term - the calculus isn't clear
> cut. People can argue these things to high heavens on abstract grounds, but
> if you break it down to practical gains vs. costs, it's not a huge
> difference.
> 
> But, again, I'm not against it. Johannes, any second thoughts?

Yeah, letting the primitives get used organically in new code and
patches sounds better to me than retrofitting it into an existing
function graph that wasn't designed with these in mind.

On Mon, Jun 23, 2025 at 04:03:21PM +0200, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > People can argue these things to high heavens on abstract grounds,
> > but if you break it down to practical gains vs. costs, it's not a
> > huge difference.

This makes it sound like we were discussing tabs-vs-spaces (at least I
perceive there are more benefits of guard locks ;-))

(I also believe that the encouraged separation per lock (locking type)
would allow easier backporting of this transformation.)

> > But, again, I'm not against it. Johannes, any second thoughts?
> 
> Yeah, letting the primitives get used organically in new code and
> patches sounds better to me than retrofitting it into an existing
> function graph that wasn't designed with these in mind.

But OK, it sounds there's no objection against combining *_lock calling-
and guarded code at one time, so in the future the ratio of those two
may be more favorable for one-time switch to the latter.

I thank Jemmy for giving the preview of the transformation.


Michal

On Sat, Jun 07, 2025 at 12:18:38AM +0800, Jemmy Wong wrote:
> v1 changes:
> - remove guard support for BPF
> - split patch into parts
> 
> v0 link:
> https://lore.kernel.org/all/20250605211053.19200-1-jemmywong512@gmail.com/
> 
> Jemmy Wong (3):
>   cgroup: add lock guard support for cgroup_muetx
>   cgroup: add lock guard support for css_set_lock and rcu
>   cgroup: add lock guard support for others

So, I'm rather ambivalent about this patchset but leaning towards not
applying them. The lock guards are fine but I'm not sure what converting the
existing code base wholesale buys us. We're already pretty good at detecting
locking problems with lockdep and all and the code being modified hasn't
seen significant locking changes in ages. There are no practical benefits to
converting the code base at this point.

Thanks.

-- 
tejun