Re: [PATCH 1/2] pcpcntr: add group allocation/free

[Vegard Nossum <vegard.nossum@oracle.com>]

Testing out a review style with very detailed comments. Let me know if
you hate it. Review notes:

On 8/21/23 22:28, Mateusz Guzik wrote:
> Allocations and frees are globally serialized on the pcpu lock (and the
> CPU hotplug lock if enabled, which is the case on Debian).
> 
> At least one frequent consumer allocates 4 back-to-back counters (and
> frees them in the same manner), exacerbating the problem.
> 
> While this does not fully remedy scalability issues, it is a step
> towards that goal and provides immediate relief.
> 
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> ---
>   include/linux/percpu_counter.h | 19 ++++++++---
>   lib/percpu_counter.c           | 61 ++++++++++++++++++++++++----------
>   2 files changed, 57 insertions(+), 23 deletions(-)
> 
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index 75b73c83bc9d..ff5850b07124 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -30,17 +30,26 @@ struct percpu_counter {
>   
>   extern int percpu_counter_batch;
>   
> -int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
> -			  struct lock_class_key *key);
> +int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
> +			  struct lock_class_key *key, u32 count);

renaming and adding a u32 count argument

>   
> -#define percpu_counter_init(fbc, value, gfp)				\
> +#define percpu_counter_init_many(fbc, value, gfp, count)		\

adding a count argument

>   	({								\
>   		static struct lock_class_key __key;			\
>   									\
> -		__percpu_counter_init(fbc, value, gfp, &__key);		\
> +		__percpu_counter_init_many(fbc, value, gfp, &__key, count);\

renaming and passing count along

>   	})
>   
> -void percpu_counter_destroy(struct percpu_counter *fbc);
> +
> +#define percpu_counter_init(fbc, value, gfp)				\
> +	percpu_counter_init_many(fbc, value, gfp, 1)
> +
> +void percpu_counter_destroy_many(struct percpu_counter *fbc, u32 count);
> +static inline void percpu_counter_destroy(struct percpu_counter *fbc)
> +{
> +	percpu_counter_destroy_many(fbc, 1);
> +}
> +

wrappers for the count == 1 case

>   void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
>   void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
>   			      s32 batch);
> diff --git a/lib/percpu_counter.c b/lib/percpu_counter.c
> index 5004463c4f9f..2a33cf23df55 100644
> --- a/lib/percpu_counter.c
> +++ b/lib/percpu_counter.c
> @@ -151,48 +151,73 @@ s64 __percpu_counter_sum(struct percpu_counter *fbc)
>   }
>   EXPORT_SYMBOL(__percpu_counter_sum);
>   
> -int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
> -			  struct lock_class_key *key)
> +int __percpu_counter_init_many(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
> +			  struct lock_class_key *key, u32 count)
>   {
>   	unsigned long flags __maybe_unused;
> +	s32 __percpu *counters;
> +	u32 i;
>   
> -	raw_spin_lock_init(&fbc->lock);
> -	lockdep_set_class(&fbc->lock, key);
> -	fbc->count = amount;
> -	fbc->counters = alloc_percpu_gfp(s32, gfp);
> -	if (!fbc->counters)
> +	counters = __alloc_percpu_gfp(sizeof(*counters) * count,
> +				      sizeof(*counters), gfp);

The second argument here is the alignment. I see other callers using
__alignof__(type), which is what alloc_percpu_gfp() does as well. In
practice I think it shouldn't matter, but for clarity/consistency maybe
this should be __alignof__ as well?

Presumably multiplication overflow is not an issue here as it is with
kmalloc and friends since the count can't be controlled by userspace.

> +	if (!counters) {
> +		fbc[0].counters = NULL;
>   		return -ENOMEM;
> +	}

Checked that __alloc_percpu_gfp() returns NULL on failure.

Checked that nothing else before this in the function needs cleanup.

In the old code, fbc->count would have gotten initialized but it
shouldn't matter here, I think, as long as the counter is never activated.

I'm not sure why only fbc[0].counters is set to NULL, should this happen
for all the "count" members? [PS: percpu_counter_destroy_many() below
has a check for fbc[0].counters]

>   
> -	debug_percpu_counter_activate(fbc);
> +	for (i = 0; i < count; i++) {
> +		raw_spin_lock_init(&fbc[i].lock);
> +		lockdep_set_class(&fbc[i].lock, key);
> +#ifdef CONFIG_HOTPLUG_CPU
> +		INIT_LIST_HEAD(&fbc[i].list);
> +#endif
> +		fbc[i].count = amount;
> +		fbc[i].counters = &counters[i];
> +
> +		debug_percpu_counter_activate(&fbc[i]);

Checked that this can't return an error.

> +	}
>   
>   #ifdef CONFIG_HOTPLUG_CPU
> -	INIT_LIST_HEAD(&fbc->list);
>   	spin_lock_irqsave(&percpu_counters_lock, flags);
> -	list_add(&fbc->list, &percpu_counters);
> +	for (i = 0; i < count; i++) {
> +		list_add(&fbc[i].list, &percpu_counters);
> +	}
>   	spin_unlock_irqrestore(&percpu_counters_lock, flags);
>   #endif

each counter is added to the list while the spinlock is held

>   	return 0;

Nothing here can fail after the initial allocation so no cleanup/error
handling is needed before returning.

>   }
> -EXPORT_SYMBOL(__percpu_counter_init);
> +EXPORT_SYMBOL(__percpu_counter_init_many);
>   
> -void percpu_counter_destroy(struct percpu_counter *fbc)
> +void percpu_counter_destroy_many(struct percpu_counter *fbc, u32 count)
>   {
>   	unsigned long flags __maybe_unused;
> +	u32 i;
>   
> -	if (!fbc->counters)
> +	if (WARN_ON_ONCE(!fbc))
>   		return;

This change is misleading, but correct; the WARN_ON_ONCE() is newly
added and the old check is modified below:

>   
> -	debug_percpu_counter_deactivate(fbc);
> +	if (!fbc[0].counters)
> +		return;

(this explains why only fbc[0] was NULL-ed out above in the allocation
function...)

> +
> +	for (i = 0; i < count; i++) {
> +		debug_percpu_counter_deactivate(&fbc[i]);
> +	}

Double checked that _activate() was not called in the cases where we
would return early from this function.

>   
>   #ifdef CONFIG_HOTPLUG_CPU
>   	spin_lock_irqsave(&percpu_counters_lock, flags);
> -	list_del(&fbc->list);
> +	for (i = 0; i < count; i++) {
> +		list_del(&fbc[i].list);
> +	}
>   	spin_unlock_irqrestore(&percpu_counters_lock, flags);
>   #endif
> -	free_percpu(fbc->counters);
> -	fbc->counters = NULL;
> +
> +	free_percpu(fbc[0].counters);
> +
> +	for (i = 0; i < count; i++) {
> +		fbc[i].counters = NULL;
> +	}
>   }

Looks correct to me; fbc[0].counters is the actual allocation so only
that gets passed to free_percpu().

> -EXPORT_SYMBOL(percpu_counter_destroy);
> +EXPORT_SYMBOL(percpu_counter_destroy_many);
>   
>   int percpu_counter_batch __read_mostly = 32;
>   EXPORT_SYMBOL(percpu_counter_batch);

Reviewed-by: Vegard Nossum <vegard.nossum@oracle.com>

In summary, my only slight concern is sizeof(*counters) being passed as
the alignment to __alloc_percpu_gfp() when maybe it would be more
appropriate to pass __alignof__() -- not that it makes a difference at
runtime since both are 4 for s32.

One other thing: I find it a bit odd that the "amount" parameter
(initial value) is s64 when the counters themselves are s32. Maybe just
a leftover from an old version?


Vegard