Re: [PATCH v2 1/6] memcg: Remove unnecessary READ_ONCE() under memcg_stock.lock.

[Kuniyuki Iwashima <kuniyu@google.com>]

On Wed, 28 Oct 2026 17:48:26 -0700, Shakeel Butt wrote:
> On Wed, Oct 28, 2026 at 06:50:13PM +0000, Kuniyuki Iwashima wrote:
> > this_cpu_ptr(&memcg_stock) is always modified under
> > local_lock(&memcg_stock.lock).
> 
> You need to be more precise here. Mainly the cached[] and nr_pages[] are only
> modified under the local lock. There is flags field which is being modified
> without it.

Will update.

> 
> > 
> > Unlike is_memcg_drain_needed() under RCU, consume_stock(),
> > drain_stock(), and refill_stock() do not need READ_ONCE().
> > 
> > Especially, in refill_stock(), READ_ONCE() is used twice for
> > stock->cached[i], which forces the compiler to emit two volatile
> > loads from stock->cached[i].
> > 
> > Let's remove the unnecessary READ_ONCE() and eliminate the
> > redundant loads in refill_stock():
> 
> What are we gaining with that? Is this measurable?

I haven't measured, but here clang unrolled 7 loops and
each iteration had two loads, where it should be a single load.


> With READ_ONCE() at least we
> are very explicit that this field can read and modified concurrently by
> different CPUs i.e. better readability.

For me, this is rather confusing.

READ_ONCE() means this field is modified concurrently,
and WRITE_ONCE() means this field is read concurrently
(or modified if the writer is not under lock).

So the genral rule is "use READ_ONCE() in a lockless context
and WRITE_ONCE() under lock", and at least we should not use
READ_ONCE() under lock, which adds unnecessary volatile load
and implies non-existing concurrent writer.