Re: [RFC PATCH v2 1/2] percpu_counter: introduce atomic mode for percpu_counter

["zhangpeng (AS)" <zhangpeng362@huawei.com>]

On 2024/4/26 16:11, Dennis Zhou wrote:

> On Thu, Apr 18, 2024 at 10:20:07PM +0800, Peng Zhang wrote:
>> From: ZhangPeng <zhangpeng362@huawei.com>
>>
>> Depending on whether counters is NULL, we can support two modes:
>> atomic mode and perpcu mode. We implement both modes by grouping
>> the s64 count and atomic64_t count_atomic in a union. At the same time,
>> we create the interface for adding and reading in atomic mode and for
>> switching atomic mode to percpu mode.
>>
>> Suggested-by: Jan Kara <jack@suse.cz>
>> Signed-off-by: ZhangPeng <zhangpeng362@huawei.com>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
>> ---
>>   include/linux/percpu_counter.h | 43 +++++++++++++++++++++++++++++++---
>>   lib/percpu_counter.c           | 31 ++++++++++++++++++++++--
>>   2 files changed, 69 insertions(+), 5 deletions(-)
>>
>> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
>> index 3a44dd1e33d2..160f9734c0bb 100644
>> --- a/include/linux/percpu_counter.h
>> +++ b/include/linux/percpu_counter.h
>> @@ -21,7 +21,13 @@
>>   
>>   struct percpu_counter {
>>   	raw_spinlock_t lock;
>> -	s64 count;
>> +	/* Depending on whether counters is NULL, we can support two modes,
>> +	 * atomic mode using count_atomic and perpcu mode using count.
>> +	 */
>> +	union {
>> +		s64 count;
>> +		atomic64_t count_atomic;
>> +	};
>>   #ifdef CONFIG_HOTPLUG_CPU
>>   	struct list_head list;	/* All percpu_counters are on a list */
>>   #endif
>> @@ -32,14 +38,14 @@ extern int percpu_counter_batch;
>>   
>>   int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount,
>>   			       gfp_t gfp, u32 nr_counters,
>> -			       struct lock_class_key *key);
>> +			       struct lock_class_key *key, bool switch_mode);
> Nit: the lock_class_key at the end.

Got it.

>>   
>>   #define percpu_counter_init_many(fbc, value, gfp, nr_counters)		\
>>   	({								\
>>   		static struct lock_class_key __key;			\
>>   									\
>>   		__percpu_counter_init_many(fbc, value, gfp, nr_counters,\
>> -					   &__key);			\
>> +					   &__key, false);		\
>>   	})
>>   
>>   
>> @@ -130,6 +136,20 @@ static inline bool percpu_counter_initialized(struct percpu_counter *fbc)
>>   	return (fbc->counters != NULL);
>>   }
>>   
>> +static inline s64 percpu_counter_atomic_read(struct percpu_counter *fbc)
>> +{
>> +	return atomic64_read(&fbc->count_atomic);
>> +}
>> +
>> +static inline void percpu_counter_atomic_add(struct percpu_counter *fbc,
>> +					     s64 amount)
>> +{
>> +	atomic64_add(amount, &fbc->count_atomic);
>> +}
>> +
>> +int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc,
>> +				       u32 nr_counters);
>> +
>>   #else /* !CONFIG_SMP */
>>   
>>   struct percpu_counter {
>> @@ -260,6 +280,23 @@ static inline bool percpu_counter_initialized(struct percpu_counter *fbc)
>>   static inline void percpu_counter_sync(struct percpu_counter *fbc)
>>   {
>>   }
>> +
>> +static inline s64 percpu_counter_atomic_read(struct percpu_counter *fbc)
>> +{
>> +	return fbc->count;
>> +}
>> +
>> +static inline void percpu_counter_atomic_add(struct percpu_counter *fbc,
>> +					     s64 amount)
>> +{
>> +	percpu_counter_add(fbc, amount);
>> +}
>> +
>> +static inline int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc,
>> +						     u32 nr_counters)
>> +{
>> +	return 0;
>> +}
>>   #endif	/* CONFIG_SMP */
>>   
>>   static inline void percpu_counter_inc(struct percpu_counter *fbc)
>> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
>> index 44dd133594d4..95c4e038051a 100644
>> --- a/lib/percpu_counter.c
>> +++ b/lib/percpu_counter.c
>> @@ -153,7 +153,7 @@ EXPORT_SYMBOL(__percpu_counter_sum);
>>   
>>   int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount,
>>   			       gfp_t gfp, u32 nr_counters,
>> -			       struct lock_class_key *key)
>> +			       struct lock_class_key *key, bool switch_mode)
>>   {
>>   	unsigned long flags __maybe_unused;
>>   	size_t counter_size;
>> @@ -174,7 +174,8 @@ int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount,
>>   #ifdef CONFIG_HOTPLUG_CPU
>>   		INIT_LIST_HEAD(&fbc[i].list);
>>   #endif
>> -		fbc[i].count = amount;
>> +		if (likely(!switch_mode))
>> +			fbc[i].count = amount;
>>   		fbc[i].counters = (void *)counters + (i * counter_size);
>>   
>>   		debug_percpu_counter_activate(&fbc[i]);
>> @@ -357,6 +358,32 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
>>   	return good;
>>   }
>>   
>> +/*
>> + * percpu_counter_switch_to_pcpu_many: Converts struct percpu_counters from
>> + * atomic mode to percpu mode.
>> + */
>> +int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc,
>> +				       u32 nr_counters)
>> +{
>> +	static struct lock_class_key __key;
> This is an improper use of lockdep. Now all of the percpu_counters
> initialized on this path will key off of this lock_class_key.

Sorry, I don't know much about lock_class_key. I may not understand the reason
why it's not appropriate here. Could you please help me with the details?

>> +	unsigned long flags;
>> +	bool ret = 0;
>> +
>> +	if (percpu_counter_initialized(fbc))
>> +		return 0;
>> +
>> +	preempt_disable();
>> +	local_irq_save(flags);
>> +	if (likely(!percpu_counter_initialized(fbc)))
>> +		ret = __percpu_counter_init_many(fbc, 0,
>> +					GFP_ATOMIC|__GFP_NOWARN|__GFP_ZERO,
>> +					nr_counters, &__key, true);
>> +	local_irq_restore(flags);
>> +	preempt_enable();
>> +
>> +	return ret;
>> +}
> I'm staring at this API and I'm not in love with it. I think it hinges
> on that there's one user of mm_stats prior hence it's safe. Generically
> though, I can't see why this is safe.
>
> I need to give this a little more thought, but my gut reaction is I'd
> rather this look like percpu_refcount where we can init dead minus the
> percpu allocation. Maybe that's not quite right, but I'd feel better
> about it than requiring external synchronization on a percpu_counter to
> ensure that it's correct.

Maybe it would be better if I change this API to the internal function of
mm counter.

Sorry again, maybe percpu_refcount is better, but I don't understand this
sentence "we can init dead minus the percpu allocation." Please forgive my
ignorance...

Thank you very much for your review and valuable comments!

>> +
>>   static int __init percpu_counter_startup(void)
>>   {
>>   	int ret;
>> -- 
>> 2.25.1
>>
> Thanks,
> Dennis

-- 
Best Regards,
Peng