[PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Song Hu]

refill_stock() takes a css reference for each cached memcg slot and
mem_cgroup_css_offline() relies on drain_all_stock() to release those
references before the cgroup can finish dying.

consume_stock() can drive a slot's nr_pages to zero while its cached[]
pointer stays set.  is_memcg_drain_needed() skips such empty slots, so
the offlining drain is a no-op for them: the pinned css reference is
released only if the slot happens to be evicted by later unrelated
charges on the same CPU, or by CPU hotplug.  The memcg then lingers
in the dying state indefinitely.  On a CPU with container churn, a
per-cpu stock can pin up to NR_MEMCG_STOCK (7) zombie memcgs.

The sibling obj_stock_flush_required() has no such gate: a cached
objcgs slot is flushed regardless of its byte count.  Drop the
nr_pages gate from is_memcg_drain_needed() accordingly.  Draining an
empty slot is a single css_put() and a NULL store, and it happens at
most once per slot per drain, so the extra work is negligible.

This is easily reproduced with short-lived cgroups pinned to one CPU:
12 charge/exit/rmdir cycles leave nr_dying_subsys_memory at +2, stable
across later slot displacement.

Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
Signed-off-by: Song Hu <husong@kylinos.cn>
---
 mm/memcontrol.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 17da1f43b7d3..b931ec16bb82 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2291,8 +2291,12 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
 		if (!memcg)
 			continue;
 
-		if (READ_ONCE(stock->nr_pages[i]) &&
-		    mem_cgroup_is_descendant(memcg, root_memcg)) {
+		/*
+		 * An empty slot still pins a css reference which
+		 * mem_cgroup_css_offline() relies on drain_all_stock()
+		 * to release.
+		 */
+		if (mem_cgroup_is_descendant(memcg, root_memcg)) {
 			flush = true;
 			break;
 		}
-- 
2.43.0

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Matthew Wilcox]

On Mon, Aug 17, 2026 at 10:59:17AM +0800, Song Hu wrote:
> refill_stock() takes a css reference for each cached memcg slot and
> mem_cgroup_css_offline() relies on drain_all_stock() to release those
> references before the cgroup can finish dying.
> 
> consume_stock() can drive a slot's nr_pages to zero while its cached[]
> pointer stays set.  is_memcg_drain_needed() skips such empty slots, so
> the offlining drain is a no-op for them: the pinned css reference is
> released only if the slot happens to be evicted by later unrelated
> charges on the same CPU, or by CPU hotplug.  The memcg then lingers
> in the dying state indefinitely.  On a CPU with container churn, a
> per-cpu stock can pin up to NR_MEMCG_STOCK (7) zombie memcgs.
> 
> The sibling obj_stock_flush_required() has no such gate: a cached
> objcgs slot is flushed regardless of its byte count.  Drop the
> nr_pages gate from is_memcg_drain_needed() accordingly.  Draining an
> empty slot is a single css_put() and a NULL store, and it happens at
> most once per slot per drain, so the extra work is negligible.
> 
> This is easily reproduced with short-lived cgroups pinned to one CPU:
> 12 charge/exit/rmdir cycles leave nr_dying_subsys_memory at +2, stable
> across later slot displacement.
> 
> Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
> Signed-off-by: Song Hu <husong@kylinos.cn>
> ---
>  mm/memcontrol.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..b931ec16bb82 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2291,8 +2291,12 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>  		if (!memcg)
>  			continue;
>  
> -		if (READ_ONCE(stock->nr_pages[i]) &&
> -		    mem_cgroup_is_descendant(memcg, root_memcg)) {
> +		/*
> +		 * An empty slot still pins a css reference which
> +		 * mem_cgroup_css_offline() relies on drain_all_stock()
> +		 * to release.
> +		 */

I don't think the comment adds any value.  It explains why something
_isn't_ there which makes no sense to someone reading the code.

> +		if (mem_cgroup_is_descendant(memcg, root_memcg)) {
>  			flush = true;
>  			break;
>  		}
> -- 
> 2.43.0
> 
> 

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Joshua Hahn]

On Mon, 17 Aug 2026 10:59:17 +0800 Song Hu <husong@kylinos.cn> wrote:

> refill_stock() takes a css reference for each cached memcg slot and
> mem_cgroup_css_offline() relies on drain_all_stock() to release those
> references before the cgroup can finish dying.
> 
> consume_stock() can drive a slot's nr_pages to zero while its cached[]
> pointer stays set.  is_memcg_drain_needed() skips such empty slots, so
> the offlining drain is a no-op for them: the pinned css reference is
> released only if the slot happens to be evicted by later unrelated
> charges on the same CPU, or by CPU hotplug.  The memcg then lingers
> in the dying state indefinitely.  On a CPU with container churn, a
> per-cpu stock can pin up to NR_MEMCG_STOCK (7) zombie memcgs.
> 
> The sibling obj_stock_flush_required() has no such gate: a cached
> objcgs slot is flushed regardless of its byte count.  Drop the
> nr_pages gate from is_memcg_drain_needed() accordingly.  Draining an
> empty slot is a single css_put() and a NULL store, and it happens at
> most once per slot per drain, so the extra work is negligible.

Hi Song,

I feel that this above statement is quite misleading.
Scheduling a drain on a memcg that doesn't have stock eventually
performs a drain operation on every memcg cached in that CPU.

drain_local_memcg_stock
  drain_stock_fully
  {
    for (i = 0; i < NR_MEMCG_STOCK; ++i)
      drain_stock(stock, i);
  {

Sure, for any empty stock during this iteration drain_stock is trivial,
but for nonempty stock that happen to co-exist on the CPU, it's forcing
a cache miss on the next charge.

The problem that you note does seem quite real though. I think a less
invasive solution could be something like

if (mem_cgroup_is_descendant(memcg, root_memct) &&
    (READ_ONCE(stock->nr_pages[i]) || css_is_dying(&memcg->css))

By the way, I think the argument that "obj_stock_flush_required has no
such gate so is_memcg_drain_needed doesn't need one too" is also not
really correct since a dying objcg doesn't flush anyways. 

I hope you have a great day,
Joshua

> This is easily reproduced with short-lived cgroups pinned to one CPU:
> 12 charge/exit/rmdir cycles leave nr_dying_subsys_memory at +2, stable
> across later slot displacement.
> 
> Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
> Signed-off-by: Song Hu <husong@kylinos.cn>
> ---
>  mm/memcontrol.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..b931ec16bb82 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2291,8 +2291,12 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>  		if (!memcg)
>  			continue;
>  
> -		if (READ_ONCE(stock->nr_pages[i]) &&
> -		    mem_cgroup_is_descendant(memcg, root_memcg)) {
> +		/*
> +		 * An empty slot still pins a css reference which
> +		 * mem_cgroup_css_offline() relies on drain_all_stock()
> +		 * to release.
> +		 */
> +		if (mem_cgroup_is_descendant(memcg, root_memcg)) {
>  			flush = true;
>  			break;
>  		}
> -- 
> 2.43.0

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Michal Hocko]

On Mon 17-08-26 10:59:17, Song Hu wrote:
> refill_stock() takes a css reference for each cached memcg slot and
> mem_cgroup_css_offline() relies on drain_all_stock() to release those
> references before the cgroup can finish dying.
> 
> consume_stock() can drive a slot's nr_pages to zero while its cached[]
> pointer stays set.

Is there any specific reason why the memcg stays in the cache slot
without any pages?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Song Hu]

Hi,

在 2026/8/17 11:15, Matthew Wilcox 写道:
> On Mon, Aug 17, 2026 at 10:59:17AM +0800, Song Hu wrote:
>> refill_stock() takes a css reference for each cached memcg slot and
>> mem_cgroup_css_offline() relies on drain_all_stock() to release those
>> references before the cgroup can finish dying.
>>
>> consume_stock() can drive a slot's nr_pages to zero while its cached[]
>> pointer stays set.  is_memcg_drain_needed() skips such empty slots, so
>> the offlining drain is a no-op for them: the pinned css reference is
>> released only if the slot happens to be evicted by later unrelated
>> charges on the same CPU, or by CPU hotplug.  The memcg then lingers
>> in the dying state indefinitely.  On a CPU with container churn, a
>> per-cpu stock can pin up to NR_MEMCG_STOCK (7) zombie memcgs.
>>
>> The sibling obj_stock_flush_required() has no such gate: a cached
>> objcgs slot is flushed regardless of its byte count.  Drop the
>> nr_pages gate from is_memcg_drain_needed() accordingly.  Draining an
>> empty slot is a single css_put() and a NULL store, and it happens at
>> most once per slot per drain, so the extra work is negligible.
>>
>> This is easily reproduced with short-lived cgroups pinned to one CPU:
>> 12 charge/exit/rmdir cycles leave nr_dying_subsys_memory at +2, stable
>> across later slot displacement.
>>
>> Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
>> Signed-off-by: Song Hu <husong@kylinos.cn>
>> ---
>>  mm/memcontrol.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 17da1f43b7d3..b931ec16bb82 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2291,8 +2291,12 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>>  		if (!memcg)
>>  			continue;
>>  
>> -		if (READ_ONCE(stock->nr_pages[i]) &&
>> -		    mem_cgroup_is_descendant(memcg, root_memcg)) {
>> +		/*
>> +		 * An empty slot still pins a css reference which
>> +		 * mem_cgroup_css_offline() relies on drain_all_stock()
>> +		 * to release.
>> +		 */
> 
> I don't think the comment adds any value.  It explains why something
> _isn't_ there which makes no sense to someone reading the code.
> 
Fair enough, will drop it in v2.

Thanks,
Song>> +		if (mem_cgroup_is_descendant(memcg, root_memcg)) {
>>  			flush = true;
>>  			break;
>>  		}
>> -- 
>> 2.43.0
>>
>>

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Song Hu]

Hi, 

在 2026/8/17 12:30, Joshua Hahn 写道:
> On Mon, 17 Aug 2026 10:59:17 +0800 Song Hu <husong@kylinos.cn> wrote:
> 
>> refill_stock() takes a css reference for each cached memcg slot and
>> mem_cgroup_css_offline() relies on drain_all_stock() to release those
>> references before the cgroup can finish dying.
>>
>> consume_stock() can drive a slot's nr_pages to zero while its cached[]
>> pointer stays set.  is_memcg_drain_needed() skips such empty slots, so
>> the offlining drain is a no-op for them: the pinned css reference is
>> released only if the slot happens to be evicted by later unrelated
>> charges on the same CPU, or by CPU hotplug.  The memcg then lingers
>> in the dying state indefinitely.  On a CPU with container churn, a
>> per-cpu stock can pin up to NR_MEMCG_STOCK (7) zombie memcgs.
>>
>> The sibling obj_stock_flush_required() has no such gate: a cached
>> objcgs slot is flushed regardless of its byte count.  Drop the
>> nr_pages gate from is_memcg_drain_needed() accordingly.  Draining an
>> empty slot is a single css_put() and a NULL store, and it happens at
>> most once per slot per drain, so the extra work is negligible.
> 
> Hi Song,
> 
> I feel that this above statement is quite misleading.
> Scheduling a drain on a memcg that doesn't have stock eventually
> performs a drain operation on every memcg cached in that CPU.
> 
Right, I missed that drain_local_memcg_stock() flushes the whole
stock, so an unrelated charge below a dying memcg would throw away the
other slots on that CPU.  Will drop that claim and the objcg
comparison from the changelog; the objcg side has no reference to
release from the stock, so there is no symmetry to argue from.

> drain_local_memcg_stock
>   drain_stock_fully
>   {
>     for (i = 0; i < NR_MEMCG_STOCK; ++i)
>       drain_stock(stock, i);
>   {
> 
> Sure, for any empty stock during this iteration drain_stock is trivial,
> but for nonempty stock that happen to co-exist on the CPU, it's forcing
> a cache miss on the next charge.
> 
> The problem that you note does seem quite real though. I think a less
> invasive solution could be something like
> 
> if (mem_cgroup_is_descendant(memcg, root_memct) &&
>     (READ_ONCE(stock->nr_pages[i]) || css_is_dying(&memcg->css))
> 
Will do.  The ordering works out: kill_css_sync() sets CSS_DYING
before css_offline is queued, so css_is_dying() is already true when
mem_cgroup_css_offline() calls drain_all_stock().
> By the way, I think the argument that "obj_stock_flush_required has no
> such gate so is_memcg_drain_needed doesn't need one too" is also not
> really correct since a dying objcg doesn't flush anyways. 
> 
Thanks,
Song> I hope you have a great day,
> Joshua
> 
>> This is easily reproduced with short-lived cgroups pinned to one CPU:
>> 12 charge/exit/rmdir cycles leave nr_dying_subsys_memory at +2, stable
>> across later slot displacement.
>>
>> Fixes: d1a05b6973c7 ("memcg: do not try to drain per-cpu caches without pages")
>> Signed-off-by: Song Hu <husong@kylinos.cn>
>> ---
>>  mm/memcontrol.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 17da1f43b7d3..b931ec16bb82 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -2291,8 +2291,12 @@ static bool is_memcg_drain_needed(struct memcg_stock_pcp *stock,
>>  		if (!memcg)
>>  			continue;
>>  
>> -		if (READ_ONCE(stock->nr_pages[i]) &&
>> -		    mem_cgroup_is_descendant(memcg, root_memcg)) {
>> +		/*
>> +		 * An empty slot still pins a css reference which
>> +		 * mem_cgroup_css_offline() relies on drain_all_stock()
>> +		 * to release.
>> +		 */
>> +		if (mem_cgroup_is_descendant(memcg, root_memcg)) {
>>  			flush = true;
>>  			break;
>>  		}
>> -- 
>> 2.43.0

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Song Hu]

On Mon 17-08-26, Michal Hocko wrote:
> Is there any specific reason why the memcg stays in the cache slot
> without any pages?

consume_stock() doesn't release the slot when nr_pages hits zero.  It
is kept for the next charge of the same task and only gets displaced
by a charge under a different memcg or by CPU hotplug.  The problem is
that the offlining drain skips empty slots, so the css reference they
hold is never dropped unless something unrelated displaces them.

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Michal Hocko]

On Mon 17-08-26 21:12:21, Song Hu wrote:
> On Mon 17-08-26, Michal Hocko wrote:
> > Is there any specific reason why the memcg stays in the cache slot
> > without any pages?
> 
> consume_stock() doesn't release the slot when nr_pages hits zero.  It
> is kept for the next charge of the same task and only gets displaced
> by a charge under a different memcg or by CPU hotplug.  The problem is
> that the offlining drain skips empty slots, so the css reference they
> hold is never dropped unless something unrelated displaces them.

This doesn't answer my question, really, does it? Is there any good
reason for this implementation? Why do we need to drop references
remotely when we can do so when the last cached charge is consumed?

-- 
Michal Hocko
SUSE Labs

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Song Hu]

Hi,Michal

在 2026/8/17 21:29, Michal Hocko 写道:
> On Mon 17-08-26 21:12:21, Song Hu wrote:
>> On Mon 17-08-26, Michal Hocko wrote:
>>> Is there any specific reason why the memcg stays in the cache slot
>>> without any pages?
>>
>> consume_stock() doesn't release the slot when nr_pages hits zero.  It
>> is kept for the next charge of the same task and only gets displaced
>> by a charge under a different memcg or by CPU hotplug.  The problem is
>> that the offlining drain skips empty slots, so the css reference they
>> hold is never dropped unless something unrelated displaces them.
> 
> This doesn't answer my question, really, does it? Is there any good
> reason for this implementation? Why do we need to drop references
> remotely when we can do so when the last cached charge is consumed?
> 

Fair enough.  There is no strong reason.  Keeping the slot
populated after the last page is consumed only saves a
css_get()/css_put() pair when the same memcg charges again on that
CPU - a micro-optimization from the original single-slot
implementation.

Dropping the reference in consume_stock() when the slot empties is
the better place.  Empty slots stop existing, so the offlining drain
has nothing left to miss and is_memcg_drain_needed() stays as it
is.  This also makes Joshua's concern about the full-stock drain go
away entirely.  The cost is one refcount pair per emptied slot, at
most once per MEMCG_CHARGE_BATCH pages.

Joshua, this supersedes the css_is_dying gating you suggested and
that I said I would do - with no empty slots left, the check would
never fire, and the kill_css_sync() ordering argument becomes moot
as well.  Since you are reworking this code, I'd appreciate a
sanity check on releasing from consume_stock().

If this direction works for you both, I'll rework the patch
accordingly.

Thanks,
Song

Re: [PATCH] mm: memcg: flush empty per-cpu stock slots on memcg offlining

[Michal Hocko]

On Mon 17-08-26 21:58:39, Song Hu wrote:
> Hi,Michal
> 
> 在 2026/8/17 21:29, Michal Hocko 写道:
> > On Mon 17-08-26 21:12:21, Song Hu wrote:
> >> On Mon 17-08-26, Michal Hocko wrote:
> >>> Is there any specific reason why the memcg stays in the cache slot
> >>> without any pages?
> >>
> >> consume_stock() doesn't release the slot when nr_pages hits zero.  It
> >> is kept for the next charge of the same task and only gets displaced
> >> by a charge under a different memcg or by CPU hotplug.  The problem is
> >> that the offlining drain skips empty slots, so the css reference they
> >> hold is never dropped unless something unrelated displaces them.
> > 
> > This doesn't answer my question, really, does it? Is there any good
> > reason for this implementation? Why do we need to drop references
> > remotely when we can do so when the last cached charge is consumed?
> > 
> 
> Fair enough.  There is no strong reason.  Keeping the slot
> populated after the last page is consumed only saves a
> css_get()/css_put() pair when the same memcg charges again on that
> CPU - a micro-optimization from the original single-slot
> implementation.

This would also allow more effective use of the stock because it
wouldn't need to throw other stock away when refilling.

-- 
Michal Hocko
SUSE Labs