Re: [PATCH 3/4 v2] cgroup: separate rstat locks for subsystems

[JP Kobryn <inwardvessel@gmail.com>]

On 3/3/25 10:49 AM, Michal Koutný wrote:
> On Mon, Mar 03, 2025 at 06:29:53PM +0000, Yosry Ahmed <yosry.ahmed@linux.dev> wrote:
>> I thought about this, but this would have unnecessary memory overhead as
>> we only need one lock per-subsystem. So having a lock in every single
>> css is wasteful.
>>
>> Maybe we can put the lock in struct cgroup_subsys? Then we can still
>> initialize them in cgroup_init_subsys().
> 
> Ah, yes, muscle memory, of course I had struct cgroup_subsys\> in mind.
> 
>> I think it will be confusing to have cgroup_rstat_boot() only initialize
>> some of the locks.
>>
>> I think if we initialize the subsys locks in cgroup_init_subsys(), then
>> we should open code initializing the base locks in cgroup_init(), and
>> remove cgroup_rstat_boot().
> 
> Can this work?
> 
> DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_base_cpu_lock) =
> 	__RAW_SPIN_LOCK_INITIALIZER(cgroup_rstat_base_cpu_lock);
> 
> (I see other places in kernel that assign into the per-cpu definition
> but I have no idea whether that does expands and links to what's
> expected. Neglecting the fact that the lock initializer is apparently
> not for external use.)

I gave this a try. Using lockdep fields to verify, it expanded as
intended:
[    1.442498] [ss_rstat_init] cpu:0, lock.magic:dead4ead, 
lock.owner_cpu:-1, lock.owner:ffffffffffffffff
[    1.443027] [ss_rstat_init] cpu:1, lock.magic:dead4ead, 
lock.owner_cpu:-1, lock.owner:ffffffffffffffff
...

Unless anyone has objections on using the double under macro, I will use
this in v3.

> 
>> Alternatively, we can make cgroup_rstat_boot() take in a subsys and
>> initialize its lock. If we pass NULL, then it initialize the base locks.
>> In this case we can call cgroup_rstat_boot() for each subsystem that has
>> an rstat callback in cgroup_init() (or cgroup_init_subsys()), and then
>> once for the base locks.
>>
>> WDYT?
> 
> Such calls from cgroup_init_subsys() are fine too.

Using the lock initializer macro above simplifies this situation. I'm
currently working with these changes that should appear in v3:

move global subsys locks to ss struct:
	struct cgroup_subsys {
		...
		spinlock_t lock;
		raw_spinlock_t __percpu *percpu_lock;
	};

change:
	cgroup_rstat_boot(void)
to:
	ss_rstat_init(struct cgroup_subsys *ss)

... and only use ss_rstat_init(ss) to initialize the new subsystem
lock fields, since the locks for base stats are now initialized at
definition.

Note that these changes also have the added benefit of not having to
perform an allocation of the per-cpu lock for subsystems that do not
participate in rstat. Previously it was wasted static memory.