Messages in this thread | | | Date | Mon, 29 Apr 2024 15:45:44 +0800 | Subject | Re: [RFC PATCH v2 1/2] percpu_counter: introduce atomic mode for percpu_counter | From | "zhangpeng (AS)" <> |
| |
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
| |