Re: [PATCH v8 7/7] mm/memcontrol: restrict memory stock to leaf cgroups on v2

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] mm: foreign slot drain on fast path in page_counter_consume_stock()
--

commit 4a3e7ea8b6bee1d8310dd3c42d8ad7c15aae3cf4
Author: Joshua Hahn <joshua.hahnjy@gmail.com>

mm/memcontrol: restrict memory stock to leaf cgroups on v2

This commit disables memory page_counter stock on cgroup v2 non-leaf cgroups
when they gain a child, schedules per-CPU stock drains for online CPUs, and
lazily drains any remaining disabled cached counters during stock consumption.
When a parent cgroup becomes a leaf again on child offlining, its memory stock
is re-enabled.

> diff --git a/mm/page_counter.c b/mm/page_counter.c
> index e1feb2bd1e225..7a3b7cea0ff94 100644
> --- a/mm/page_counter.c
> +++ b/mm/page_counter.c
> @@ -158,6 +158,7 @@ static void page_counter_drain_stock(struct page_counter_stock_pcp *stock,
>   * Consume the cached charge if enough nr_pages are present, otherwise return
>   * failure. Also return failure for charge requests larger than
>   * PAGE_COUNTER_STOCK_BATCH or if the local lock is already taken.
> + * Drain disabled cached counters while looking up the requested counter.
>   */
>  static bool page_counter_consume_stock(struct page_counter *counter,
>  				       unsigned long nr_pages)
> @@ -176,12 +177,17 @@ static bool page_counter_consume_stock(struct page_counter *counter,
>  	for (i = 0; i < NR_PAGE_COUNTER_STOCK; i++) {
>  		struct page_counter *cached = pcp_stock->cached[i];
>  
> +		/* Drain any stock that is disabled but hasn't drained yet */
> +		if (cached && !READ_ONCE(cached->stock)) {
> +			page_counter_drain_stock(pcp_stock, i);
> +			continue;
> +		}
>  		if (counter != cached)
>  			continue;

[Severity: High]
Can draining an unrelated disabled slot in page_counter_consume_stock()
acquire spinning locks from an NMI or alloc_pages_nolock() context, and also
add foreign cacheline loads to every fast-path stock hit?

In mm/memcontrol.c:try_charge_memcg(), may_batch is set to false when
!gfpflags_allow_spinning(gfp_mask) so that charges from non-spinning contexts
such as alloc_pages_nolock(__GFP_ACCOUNT, ...) do not evict other stock slots
or take spinning locks:

mm/memcontrol.c:try_charge_memcg() {
	bool allow_spinning = gfpflags_allow_spinning(gfp_mask);
	/* Charges from reclaim or non-spinning contexts don't batch. */
	bool may_batch = allow_spinning && !(current->flags & PF_MEMALLOC);
}

However, mm/page_counter.c:page_counter_try_charge() calls
page_counter_consume_stock() unconditionally before checking may_batch:

mm/page_counter.c:page_counter_try_charge() {
retry:
	if (page_counter_consume_stock(counter, nr_pages)) {
		if (nr_charged)
			*nr_charged = 0;
		return true;
	}
}

For the counter being charged (counter == cached), the caller holds an active
CSS reference, so css_put(counter->stock_css) when depleting the matching slot
cannot drop the final CSS reference.

For an unrelated counter in pcp_stock->cached[i] (cached != counter) whose
stock was disabled via mem_cgroup_disable_stock(), the charging caller holds
no reference to cached->stock_css. If that cgroup and its child were
subsequently removed while its per-CPU stock slot remained populated (for
example on an isolated CPU where schedule_drain_work() returns false, or
after a racing refill or contended mutex_trylock(&percpu_charge_mutex) in
drain_all_stock()), pcp_stock->cached[i] can hold the final reference to a
dying CSS after css_killed_work_fn() completes.

If an NMI or non-spinning caller on that CPU then runs:

  alloc_pages_nolock(__GFP_ACCOUNT, ...)
    -> __memcg_kmem_charge_page()
      -> obj_cgroup_charge_pages()
        -> try_charge_memcg()
          -> page_counter_try_charge()
            -> page_counter_consume_stock()
              -> page_counter_drain_stock()
                -> css_put(counter->stock_css)
                  -> css_release()
                    -> queue_work(cgroup_release_wq, &css->destroy_work)

wouldn't queue_work() spin on workqueue pool and scheduler locks in a context
where spinning is forbidden?

In addition, the commit message notes that lazy cleanup happens on the next
local stock miss ("The next local stock miss on that CPU scans for any stock
that has been disabled, and will drain its stock"). Because the
!READ_ONCE(cached->stock) check is placed before the counter != cached check
inside page_counter_consume_stock(), every fast-path stock hit at index i will
also dereference cached->stock across foreign struct page_counter cachelines
for slots 0..i and drain any disabled slots before i while holding
stock->lock.

Should the lazy cleanup of disabled slots be moved to
page_counter_refill_stock() (which only runs on a stock miss when may_batch is
true and gfpflags_allow_spinning(gfp_mask) holds), while
page_counter_consume_stock() only checks counter == cached?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104233029.3282438-1-joshua.hahnjy@gmail.com?part=7