Re: [PATCH] lib/percpu_counter: fix data race in __percpu_counter_limited_add()

[Andrew Morton <akpm@linux-foundation.org>]

On Tue, 6 May 2025 13:56:13 +0200 Mateusz Guzik <mjguzik@gmail.com> wrote:

> >  	unknown = batch * num_online_cpus();
> >  	count = __this_cpu_read(*fbc->counters);
> >  
> > @@ -344,11 +345,10 @@ bool __percpu_counter_limited_add(struct percpu_counter *fbc,
> >  	    ((amount > 0 && fbc->count + unknown <= limit) ||
> >  	     (amount < 0 && fbc->count - unknown >= limit))) {
> >  		this_cpu_add(*fbc->counters, amount);
> > -		local_irq_restore(flags);
> > -		return true;
> > +		good = true;
> > +		goto out;
> >  	}
> >  
> > -	raw_spin_lock(&fbc->lock);
> >  	count = fbc->count + amount;
> >  
> >  	/* Skip percpu_counter_sum() when safe */
> > --
> > 
> 
> As this always takes the centralized lock in the fast path this defeats
> the point of using a per-cpu counter in the first place.

Well.  It partially "defeats the point" if the client code actually
uses percpu_counter_limited_add().  Only shmem.c does that.

> I noted this thing is buggy almost a year ago:
> https://lore.kernel.org/linux-mm/5eemkb4lo5eefp7ijgncgogwmadyzmvjfjmmmvfiki6cwdskfs@hi2z4drqeuz6/
> 
> per the e-mail I don't believe existence of this routine is warranted.
> 
> shmem is still the only consumer.

Totally.  It would be better to remove percpu_counter_limited_add() and
to implement its intended effect within shmem.c