On Thu, 18 Apr 2024 22:20:07 +0800 Peng Zhang <zhangpeng362@xxxxxxxxxx> wrote:
> From: ZhangPeng <zhangpeng362@xxxxxxxxxx>
>
> 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.
>
I think it would be better if we had a detailed code comment in an
appropriate place which fully describes the tradeoffs here. Tell
people when they would benefit from using one mode versus the other.
> --- 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.
Describe what happens if that GFP_ATOMIC allocation fails. We remain
in atomic mode, yes?
> + */
> +int percpu_counter_switch_to_pcpu_many(struct percpu_counter *fbc,
> + u32 nr_counters)
> +{
> + static struct lock_class_key __key;
> + unsigned long flags;
> + bool ret = 0;
> +
> + if (percpu_counter_initialized(fbc))
> + return 0;
> +
> + preempt_disable();
> + local_irq_save(flags);
Do we need both? Does local_irq_save() always disable preemption?
This might not be the case for RT kernels, I always forget.
> + 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;
> +}
> +
Why is there no API for switching back to atomic mode?
