| 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: Tue, 22 Aug 2023 15:37:30 +0200
From: Vegard Nossum <vegard.nossum@...cle.com>
To: Mateusz Guzik <mjguzik@...il.com>, linux-kernel@...r.kernel.org
Cc: dennis@...nel.org, tj@...nel.org, cl@...ux.com,
akpm@...ux-foundation.org, shakeelb@...gle.com, linux-mm@...ck.org
Subject: Re: [PATCH 1/2] pcpcntr: add group allocation/free
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@...il.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@...cle.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
Powered by blists - more mailing lists