Re: [PATCH v4 5/5] cgroup: use subsystem-specific rstat locks to avoid contention

[JP Kobryn <inwardvessel@gmail.com>]

On 4/22/25 7:01 AM, Yosry Ahmed wrote:
> On Thu, Apr 03, 2025 at 06:10:50PM -0700, JP Kobryn wrote:
>> It is possible to eliminate contention between subsystems when
>> updating/flushing stats by using subsystem-specific locks. Let the existing
>> rstat locks be dedicated to the cgroup base stats and rename them to
>> reflect that. Add similar locks to the cgroup_subsys struct for use with
>> individual subsystems.
>>
>> Lock initialization is done in the new function ss_rstat_init(ss) which
>> replaces cgroup_rstat_boot(void). If NULL is passed to this function, the
>> global base stat locks will be initialized. Otherwise, the subsystem locks
>> will be initialized.
>>
>> Change the existing lock helper functions to accept a reference to a css.
>> Then within these functions, conditionally select the appropriate locks
>> based on the subsystem affiliation of the given css. Add helper functions
>> for this selection routine to avoid repeated code.
>>
>> Signed-off-by: JP Kobryn <inwardvessel@gmail.com>
>> ---
>>   block/blk-cgroup.c              |   2 +-
>>   include/linux/cgroup-defs.h     |  16 +++--
>>   include/trace/events/cgroup.h   |  12 +++-
>>   kernel/cgroup/cgroup-internal.h |   2 +-
>>   kernel/cgroup/cgroup.c          |  10 +++-
>>   kernel/cgroup/rstat.c           | 101 +++++++++++++++++++++++---------
>>   6 files changed, 103 insertions(+), 40 deletions(-)
>>
>> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
>> index 0560ea402856..62d0bf1e1a04 100644
>> --- a/block/blk-cgroup.c
>> +++ b/block/blk-cgroup.c
>> @@ -1074,7 +1074,7 @@ static void __blkcg_rstat_flush(struct blkcg *blkcg, int cpu)
>>   	/*
>>   	 * For covering concurrent parent blkg update from blkg_release().
>>   	 *
>> -	 * When flushing from cgroup, cgroup_rstat_lock is always held, so
>> +	 * When flushing from cgroup, the subsystem lock is always held, so
>>   	 * this lock won't cause contention most of time.
>>   	 */
>>   	raw_spin_lock_irqsave(&blkg_stat_lock, flags);
>> diff --git a/include/linux/cgroup-defs.h b/include/linux/cgroup-defs.h
>> index c58c21c2110a..bb5a355524d6 100644
>> --- a/include/linux/cgroup-defs.h
>> +++ b/include/linux/cgroup-defs.h
>> @@ -223,7 +223,10 @@ struct cgroup_subsys_state {
>>   	/*
>>   	 * A singly-linked list of css structures to be rstat flushed.
>>   	 * This is a scratch field to be used exclusively by
>> -	 * css_rstat_flush_locked() and protected by cgroup_rstat_lock.
>> +	 * css_rstat_flush_locked().
>> +	 *
>> +	 * Protected by rstat_base_lock when css is cgroup::self.
>> +	 * Protected by css->ss->rstat_ss_lock otherwise.
>>   	 */
>>   	struct cgroup_subsys_state *rstat_flush_next;
>>   };
>> @@ -359,11 +362,11 @@ struct css_rstat_cpu {
>>   	 * are linked on the parent's ->updated_children through
>>   	 * ->updated_next.
>>   	 *
>> -	 * In addition to being more compact, singly-linked list pointing
>> -	 * to the cgroup makes it unnecessary for each per-cpu struct to
>> -	 * point back to the associated cgroup.
>> +	 * In addition to being more compact, singly-linked list pointing to
>> +	 * the css makes it unnecessary for each per-cpu struct to point back
>> +	 * to the associated css.
>>   	 *
>> -	 * Protected by per-cpu cgroup_rstat_cpu_lock.
>> +	 * Protected by per-cpu css->ss->rstat_ss_cpu_lock.
>>   	 */
>>   	struct cgroup_subsys_state *updated_children;	/* terminated by self cgroup */
> 
> This rename belongs in the previous patch, also the comment about
> updated_children should probably say "self css" now.

Thanks, will adjust in next rev.

> 
>>   	struct cgroup_subsys_state *updated_next;	/* NULL iff not on the list */
>> @@ -793,6 +796,9 @@ struct cgroup_subsys {
>>   	 * specifies the mask of subsystems that this one depends on.
>>   	 */
>>   	unsigned int depends_on;
>> +
>> +	spinlock_t rstat_ss_lock;
>> +	raw_spinlock_t __percpu *rstat_ss_cpu_lock;
> 
> Can we use local_lock_t here instead? I guess it would be annoying
> because we won't be able to have common code for locking/unlocking. It's
> annoying because the local lock is a spinlock under the hood for non-RT
> kernels anyway..
> 
>>   };
>>   
>>   extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
> [..]
>> diff --git a/kernel/cgroup/rstat.c b/kernel/cgroup/rstat.c
>> index 37d9e5012b2d..bcc253aec774 100644
>> --- a/kernel/cgroup/rstat.c
>> +++ b/kernel/cgroup/rstat.c
>> @@ -9,8 +9,8 @@
>>   
>>   #include <trace/events/cgroup.h>
>>   
>> -static DEFINE_SPINLOCK(cgroup_rstat_lock);
>> -static DEFINE_PER_CPU(raw_spinlock_t, cgroup_rstat_cpu_lock);
>> +static DEFINE_SPINLOCK(rstat_base_lock);
>> +static DEFINE_PER_CPU(raw_spinlock_t, rstat_base_cpu_lock);
> 
> Can we do something like this (not sure the macro usage is correct):
> 
> static DEFINE_PER_CPU(raw_spinlock_t, rstat_base_cpu_lock) = __SPIN_LOCK_UNLOCKED(rstat_base_cpu_lock);
> 
> This should initialize the per-CPU spinlocks the same way
> DEFINE_SPINLOCK does IIUC.

Yes, this could work. There was some earlier discussion and it was
decided against though [0].

> 
>>   
>>   static void cgroup_base_stat_flush(struct cgroup *cgrp, int cpu);
>>   
> [..]
>> @@ -422,12 +443,36 @@ void css_rstat_exit(struct cgroup_subsys_state *css)
>>   	css->rstat_cpu = NULL;
>>   }
>>   
>> -void __init cgroup_rstat_boot(void)
>> +/**
>> + * ss_rstat_init - subsystem-specific rstat initialization
>> + * @ss: target subsystem
>> + *
>> + * If @ss is NULL, the static locks associated with the base stats
>> + * are initialized. If @ss is non-NULL, the subsystem-specific locks
>> + * are initialized.
>> + */
>> +int __init ss_rstat_init(struct cgroup_subsys *ss)
>>   {
>>   	int cpu;
>>   
>> +	if (!ss) {
>> +		spin_lock_init(&rstat_base_lock);
> 
> IIUC locks defined with DEFINE_SPINLOCK() do not need to be initialized,
> and I believe we can achieve the same for the per-CPU locks as I
> described above and eliminate this branch completely.

True. I'll remove in next rev, but will keep the initialization for the
per-cpu locks based on [0].

[0] 
https://lore.kernel.org/all/jtqkpzqv4xqy4vajm6fljin6ospot37qg252dfk3yldzq6aubu@icg3ndtg3k7i/