| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 29 Apr 2024 15:45:44 +0800
From: "zhangpeng (AS)" <zhangpeng362@...wei.com>
To: Dennis Zhou <dennisszhou@...il.com>
CC: <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
<akpm@...ux-foundation.org>, <shakeelb@...gle.com>, <jack@...e.cz>,
<surenb@...gle.com>, <kent.overstreet@...ux.dev>, <mhocko@...e.cz>,
<vbabka@...e.cz>, <yuzhao@...gle.com>, <yu.ma@...el.com>,
<wangkefeng.wang@...wei.com>, <sunnanyong@...wei.com>
Subject: Re: [RFC PATCH v2 1/2] percpu_counter: introduce atomic mode for
percpu_counter
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@...wei.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@...e.cz>
>> Signed-off-by: ZhangPeng <zhangpeng362@...wei.com>
>> Signed-off-by: Kefeng Wang <wangkefeng.wang@...wei.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
Powered by blists - more mailing lists