Re: [PATCH 0/7 v3] mm/memcontrol, page_counter: move stock from mem_cgroup to page_counter

[Joshua Hahn <joshua.hahnjy@gmail.com>]

On Mon, 25 May 2026 12:04:47 -0700 Joshua Hahn <joshua.hahnjy@gmail.com> wrote:

> Memcg currently keeps a "stock" of 64 pages per-cpu to cache pre-charged
> allocations, allowing small allocations to avoid walking the expensive
> mem_cgroup hierarchy traversal and atomic operations on each charge.
> This design introduces a fastpath, but there is room for improvement:

Hello everyone,

Sashiko has left some great comments on the series, some of which are
things I need to address for the next iteration, while others are false
positives. There aren't a bunch, so I'll go over all of them below.
Note that the same warning brought up in 1/7 is duplicated for the rest
of the series; I've addressed my thoughts for that in [1].

> Joshua Hahn (7):
>   mm/page_counter: introduce per-page_counter stock

Sashiko raised a concern about how zeroing per-cpu stock during the
nolock drain could race with in-flight charges that read the value
before it gets zeroed, leading to duplicate drains. I think this is
solvable by just changing the order in the callsites (disable, then drain).
More details can be found in [1].

>   mm/page_counter: use page_counter_stock in page_counter_try_charge

Sashiko raises the same concern as [1].

>   mm/page_counter: introduce stock drain APIs

Same concern as [1].

>   mm/memcontrol: convert memcg to use page_counter_stock

Sashiko raises 4 concerns, of which I think 3 of them are false positives
(or not as serious as Sashiko makes it out to be).

(1) Sashiko asks whether the synchronous draining with the percpu_charge_mutex
    lock held could lead to more time spent holding the lock, which means
    more callers of drain_all_stock would fail the trylock and just skip draining.

    To clarify, even in the original code, two tasks simultaneously calling
    drain_all_stock would serialize and only one of them would schedule the
    drain work, so this problem definitely existed before as well. It's just that
    the window for this race is a bit longer now.

    I do think that there is actually a behavior change here (for the better).
    Previously, drain_all_stock had no guarantees on whether the stock was
    drained before retrying. Now, if the caller can get the trylock, they have
    a stronger guarantee that the stock is drained before retrying the drain.

    On the note of premature OOMs, each retry loop takes much longer than the
    draining itself; I would imagine that by the time the next retry loop happens,
    there's a better chance that the trylock succeeds in the next iteration.

(2) Sashiko also raises another concern about a potential ABBA deadlock with
    the mmap_lock. I think this concern is not really true, the synchronous
    work being done (drain_stock_on_cpu) only takes a local lock. Hopefully I'm
    not missing anything here.

(3) I think Sashiko's concerns about NOHZ / CPU isolation is real. But it shouldn't
    be too bad, all I need is a cpu_is_isolated() check in the for_each_online_cpu
    iterator. Again, not draining a CPU is not fatal here, so it shouldn't be too
    big of a problem to skip some of them.

    I also just wanted to note here explicitly that we don't need the
    migrate_disable() for the memcg stock drain, since we don't differentiate
    between local drain work & remote drain scheduling (like objcg_stock).

(4) Finally Sashiko asks if we should enable the memcg->memsw stock here.
    That's included in the very next patch : -) I separated them so that they
    can be reviewed separately, since they are separate ideas. 

>   mm/memcontrol: optimize memsw stock for cgroup v1

Both concerns here are addressed in the previous section.

>   mm/memcontrol: optimize stock usage for cgroup v2

Sashiko raises 3 concerns, of which I think all of them are actually OK.

(1) If we drain the parent memcg stock on first child creation, then this would
    mean that there will be additional synchronous work being done with the
    cgroup_mutex lock held. I personally think this is fine, since it happens
    once per parent cgroup, and the draining work is pretty cheap. But I would
    appreciate it if other reviewers could chime in here.

(2) Sashiko also asks whether we need cpus_read_lock during the iteration.
    I think it's fine without it; if a CPU happens to go offline during the
    iteration, then that work will be scheduled on another CPU. That's fine,
    duplicate draining work on 1 CPU isn't the end of the world (and preferable
    to taking a cpus_read_lock here). As for the dying CPU, it will drain its
    own stock during the destruction path anyways, so no stock is lost.

(3) This one is not related to this series, so I'll move on.

>   mm/memcontrol: remove unused memcg_stock code

No comments for this patch.

I think that's all the comments that Sashiko raised for this patch. Most of them
had to do with performance tradeoffs, for which I hope that my testing results
in the cover letter were able to instill some confidence that a lot of these
tradeoffs aren't as bad as they seem. Regardless, I would really appreciate
reviewer feedback on whether they think it is acceptable.

There are definitely some real bugs that I want to address, so a v4 will be
incoming to address those (in a week or so).

Thank you Sashiko!
Joshua

[1] https://lore.kernel.org/linux-mm/20260525194506.3414995-1-joshua.hahnjy@gmail.com/