On Thu, 3 Nov 2022 17:14:07 +0000 Shakeel Butt <shakeelb@xxxxxxxxxx> wrote:
>
> ...
>
> Thanks for the report. It seems like there is a race between
> for_each_online_cpu() in __percpu_counter_sum() and
> percpu_counter_cpu_dead()/cpu-offlining. Normally this race is fine for
> percpu_counter users but for check_mm() is not happy with this race. Can
> you please try the following patch:
percpu-counters supposedly avoid such races via the hotplup notifier.
So can you please fully describe the race and let's see if it can be
fixed at the percpu_counter level?
>
> From: Shakeel Butt <shakeelb@xxxxxxxxxx>
> Date: Thu, 3 Nov 2022 06:05:13 +0000
> Subject: [PATCH] mm: percpu_counter: use race free percpu_counter sum
> interface
>
> percpu_counter_sum can race with cpu offlining. Add a new interface
> which does not race with it and use that for check_mm().
I'll grab this version for now, as others might be seeing this issue.
> ---
> include/linux/percpu_counter.h | 11 +++++++++++
> kernel/fork.c | 2 +-
> lib/percpu_counter.c | 24 ++++++++++++++++++------
> 3 files changed, 30 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/percpu_counter.h b/include/linux/percpu_counter.h
> index bde6c4c1f405..3070c1043acf 100644
> --- a/include/linux/percpu_counter.h
> +++ b/include/linux/percpu_counter.h
> @@ -45,6 +45,7 @@ void percpu_counter_set(struct percpu_counter *fbc, s64 amount);
> void percpu_counter_add_batch(struct percpu_counter *fbc, s64 amount,
> s32 batch);
> s64 __percpu_counter_sum(struct percpu_counter *fbc);
> +s64 __percpu_counter_sum_all(struct percpu_counter *fbc);
> int __percpu_counter_compare(struct percpu_counter *fbc, s64 rhs, s32 batch);
> void percpu_counter_sync(struct percpu_counter *fbc);
>
> @@ -85,6 +86,11 @@ static inline s64 percpu_counter_sum(struct percpu_counter *fbc)
> return __percpu_counter_sum(fbc);
> }
>
> +static inline s64 percpu_counter_sum_all(struct percpu_counter *fbc)
> +{
> + return __percpu_counter_sum_all(fbc);
> +}
We haven't been good about documenting these interfaces. Can we please
start now? ;)
>
> ...
>
> +
> +/*
> + * Add up all the per-cpu counts, return the result. This is a more accurate
> + * but much slower version of percpu_counter_read_positive()
> + */
> +s64 __percpu_counter_sum(struct percpu_counter *fbc)
> +{
> + return __percpu_counter_sum_mask(fbc, cpu_online_mask);
> +}
> EXPORT_SYMBOL(__percpu_counter_sum);
>
> +s64 __percpu_counter_sum_all(struct percpu_counter *fbc)
> +{
> + return __percpu_counter_sum_mask(fbc, cpu_possible_mask);
> +}
> +EXPORT_SYMBOL(__percpu_counter_sum_all);
Probably here is a good place to document it.
Is there any point in having the
percpu_counter_sum_all()->__percpu_counter_sum_all() inlined wrapper?
Why not name this percpu_counter_sum_all() directly?
> int __percpu_counter_init(struct percpu_counter *fbc, s64 amount, gfp_t gfp,
> struct lock_class_key *key)
> {
> --
> 2.38.1.431.g37b22c650d-goog
