Re: [PATCH v2 4/5] mm/mglru: avoid redundant rotations of young memcgs

[Barry Song]

On Wed, Nov 4, 2026 at 4:27 PM Max Kellermann <max.kellermann@xxxxxxxxx> wrote:
>
> On Wed, Nov 4, 2026 at 6:18 AM Barry Song <baohua@xxxxxxxxxx> wrote:
> > > In my micro benchmark, this increases the request rate by 36% and
> > > reduces the average duration of each write request by 27%.
> >
> > Could you share your microbenchmark somewhere (or post it here
> > if it's not too lengthy), perhaps on GitHub, so others can
> > reproduce it? You could then refer to it in your changelog.
>
> Will do.  A note of warning: it's ugly LLM slop, and if I had written
> it manually, probably 99% of it could be removed because it's
> pointless garbage.  The LLM wrote it for me before I had any clue
> where the problem could be.  It helped me find the problem, but
> uploading this garbage triggers a bit of shame in me ;-)

Thanks!

>
> > I think this is a correct change. I wonder if we can
> > reproduce the issue even without concurrent reclaimers.
> > In other words, can a single thread running in a
> > memory-limited memcg trigger it as well?
>
> Your question is not really related to this [4/5] patch, is it?
> Because this one is just an incremental optimization on top of the
> real linked list scalability issue.

I think it is related. Although lock contention isn't a concern
with a single thread, the early goto unlock could still save
some CPU cycles.

I can imagine a scenario on a system with a large amount
of memory, where a single memcg (e.g. one running a kernel
build) is limited to 1 GB. If this is the only memcg
constantly being aged and rotated, it should remain in
YOUNG most of the time.

We could still skip things like:

        hlist_nulls_del_rcu(&lruvec->lrugen.list);
        hlist_nulls_add_tail_rcu(&lruvec->lrugen.list,
&pgdat->memcg_lru.fifo[new][bin]);

>
> A single thread means there is no spinlock contention and the linked
> list is only walked once.  I don't think the problem is visible then.
>
> > One thing I noticed is that you may not need to call
> > get_random_u32_below(MEMCG_NR_BINS) upfront. Could you
> > defer generating the random number until you actually need
> > to rotate the memcg? I wonder if this could further improve
> > your microbenchmark results.
>
> Good idea, optimization added for the v3 patch.  I don't think it
> makes much of a difference, but it really makes sense.
>
> (And I still hate that the Linux coding style requires having all
> variable declarations at the top.  Code would be simpler by
> declaring+initializing variables where you need them, and additionally
> it opens more opportunities for compiler optimizations, but... it's
> the way it is.)
>
> > Anyway, the compiler might be able to defer it for you.
> > I'm not sure, though. Could you take a look?
>
> The compiler is not allowed to because it is not allowed to make
> assumptions on what get_random_u8() does, therefore it must not
> reorder the call.
>
> (I wish C had a way to express a promise like "this function call
> won't modify global state reachable by the caller's pointers", there's
> just GCC's "pure" attribute which is too strong here.)
>
> Minor note: deferring the PRNG call will move it inside spinlock
> protection.  I think that's okay, but it can theoretically have a
> performance impact, and I'm not expert enough on this part of the
> kernel.

Thanks for pointing this out. I missed it.

It might be a problem, as it could increase lock contention
by holding the spinlock for longer.
Only testing can tell for sure. If it does increase lock
contention, I think we can simply keep the redundant
random number generation.

Best,
Barry