[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