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

[zhangpeng (AS)]

On 2024/4/19 3:40, Andrew Morton wrote:

> 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.
Thanks for your reply!

Yes, of course, I would add before the union:

	/*
	 * Depending on whether counters is NULL, we can support two modes,
	 * atomic mode using count_atomic and perpcu mode using count.
	 * The single-thread processes should use atomic mode to reduce the
	 * memory consumption and performance regression.
	 * The multiple-thread processes should use percpu mode to reduce the
	 * error margin.
	 */
	union {
		s64 count;
		atomic64_t count_atomic;
	};

> > --- 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?

Yes. I'll add comments like:
 * Return: 0 if percpu_counter is already in atomic mode or successfully
 * switched to atomic mode; -ENOMEM if perpcu memory allocation fails,
 * perpcu_counter is still in atomic mode.

> > + */
> > +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.

Yes, we need both. For RT kernels, local_irq_save() doesn't mean
that preemption is disabled.

> > +	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?

At least for the current test scenario, we don't need to switch back
to atomic mode. The scenario for switching back to atomic mode may be
that the multi-threaded process destroys the last second thread. I could
add this API if needed later.

--
Best Regards,
Peng