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

[JP Kobryn <inwardvessel@gmail.com>]

On 3/3/25 10:29 AM, Yosry Ahmed wrote:
> On Mon, Mar 03, 2025 at 04:22:42PM +0100, Michal Koutný wrote:
>> On Thu, Feb 27, 2025 at 01:55:42PM -0800, inwardvessel <inwardvessel@gmail.com> wrote:
>>> From: JP Kobryn <inwardvessel@gmail.com>
>> ...
>>> +static inline bool is_base_css(struct cgroup_subsys_state *css)
>>> +{
>>> +	return css->ss == NULL;
>>> +}
>>
>> Similar predicate is also used in cgroup.c (various cgroup vs subsys
>> lifecycle functions, e.g. css_free_rwork_fn()). I think it'd better
>> unified, i.e. open code the predicate here or use the helper in both
>> cases (css_is_cgroup() or similar).

Sure. Thanks for pointing that one out.

>>
>>>   void __init cgroup_rstat_boot(void)
>>>   {
>>> -	int cpu;
>>> +	struct cgroup_subsys *ss;
>>> +	int cpu, ssid;
>>>   
>>> -	for_each_possible_cpu(cpu)
>>> -		raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_cpu_lock, cpu));
>>> +	for_each_subsys(ss, ssid) {
>>> +		spin_lock_init(&cgroup_rstat_subsys_lock[ssid]);
>>> +	}
>>
>> Hm, with this loop I realize it may be worth putting this lock into
>> struct cgroup_subsys_state and initializing them in
>> cgroup_init_subsys() to keep all per-subsys data in one pack.

Will give this a go in next rev.

> 
> 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().
> 
>>
>>> +
>>> +	for_each_possible_cpu(cpu) {
>>> +		raw_spin_lock_init(per_cpu_ptr(&cgroup_rstat_base_cpu_lock, cpu));
>>> +
>>> +		for_each_subsys(ss, ssid) {
>>> +			raw_spin_lock_init(
>>> +					per_cpu_ptr(cgroup_rstat_subsys_cpu_lock, cpu) + ssid);
>>> +		}
>>
>> Similar here, and keep cgroup_rstat_boot() for the base locks only.
> 
> 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().
> 
> 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?

I like this alternative idea of adjusting cgroup_rstat_boot() so it can
accept a subsys ref. Let's see how it looks when v3 goes out.