mm: mglru: fix stale batch updates after memcg reparenting

PATCH Superseded Qi Zheng 5 participants 15 messages Jun 23 – Jun 25

Subsystems (5)

Stable branch Memory management MM core MGLRU +1 more subsystems

lore

Share & tools

lore

Qi Zheng

author reviewer

2026-06-23 02:44 UTC #0 lore raw

PATCH v2 1/1 mm: mglru: fix stale batch updates after memcg reparenting

From: Qi Zheng <[email protected]>

The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock. The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.

The page table walker can run concurrently with the memcg reparenting path as follows:

CPU0 CPU1
==== ====

walk_mm
--> walk_page_range

    --> update_batch_size
        --> walk->nr_pages += delta

                              mem_cgroup_css_offline
                              --> memcg_reparent_objcgs
                                  --> lock lruvec
                                      lru_gen_reparent_memcg
                                      --> reparent child folios to parent
                                      unlock lruvec

    lock lruvec
    reset_batch_size
    --> child lrugen->nr_pages += delta

This will trigger the following warning in lru_gen_exit_memcg():

	VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
				   sizeof(lruvec->lrugen.nr_pages)));

To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.

Reported-by Peiyang He <[email protected]>

Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn open here

Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")

Cc <[email protected]>

Signed-off-by Qi Zheng <[email protected]>

Reviewed-by Barry Song <[email protected]>

---
 include/linux/mmzone.h |  4 ++++
 mm/vmscan.c            | 43 +++++++++++++++++++++++++++++++++++-------
 2 files changed, 40 insertions(+), 7 deletions(-)

include/linux/mmzone.h +4 -0

index ca2712187147..0d572db2ef64 100644
@@ -584,6 +584,10 @@ struct lru_gen_folio {
 	u8 gen;
 	/* the list segment this lru_gen_folio belongs to */
 	u8 seg;
+#ifdef CONFIG_MEMCG
+	/* the lruvec this lruvec has been reparented to */
+	struct lruvec *reparented;
+#endif
 	/* per-node lru_gen_folio list for global reclaim */
 	struct hlist_nulls_node list;
 };

mm/vmscan.c +36 -7

84 lines not on this page · raw message

Signature · 1 line

2.54.0

Qi Zheng

author reviewer

2026-06-23 02:56 UTC #1 lore raw

Quoted 38 lines · Qi Zheng

From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock. The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.
The page table walker can run concurrently with the memcg reparenting path as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
     --> update_batch_size
         --> walk->nr_pages += delta

                               mem_cgroup_css_offline
                               --> memcg_reparent_objcgs
                                   --> lock lruvec
                                       lru_gen_reparent_memcg
                                       --> reparent child folios to parent
                                       unlock lruvec

     lock lruvec
     reset_batch_size
     --> child lrugen->nr_pages += delta
This will trigger the following warning in lru_gen_exit_memcg():
	VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
				   sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.

Quoted 7 lines

Reported-by Peiyang He <[email protected]>
Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn open here
Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
Cc <[email protected]>
Signed-off-by Qi Zheng <[email protected]>
Reviewed-by Barry Song <[email protected]>
---

Changes in v2:
- update the commit message (pointed by Barry)
- collect Reviewed-by

Quoted 4 lines

  include/linux/mmzone.h |  4 ++++
  mm/vmscan.c            | 43 +++++++++++++++++++++++++++++++++++-------
  2 files changed, 40 insertions(+), 7 deletions(-)

Baolin Wang

reviewer Reviewed-by on v2 1/1

2026-06-23 04:03 UTC #2 lore raw

In reply to Qi Zheng

Quoted 38 lines · Qi Zheng

From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock. The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.
The page table walker can run concurrently with the memcg reparenting path as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
     --> update_batch_size
         --> walk->nr_pages += delta

                               mem_cgroup_css_offline
                               --> memcg_reparent_objcgs
                                   --> lock lruvec
                                       lru_gen_reparent_memcg
                                       --> reparent child folios to parent
                                       unlock lruvec

     lock lruvec
     reset_batch_size
     --> child lrugen->nr_pages += delta
This will trigger the following warning in lru_gen_exit_memcg():
	VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
				   sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.

Quoted 7 lines

Reported-by Peiyang He <[email protected]>
Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn open here
Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
Cc <[email protected]>
Signed-off-by Qi Zheng <[email protected]>
Reviewed-by Barry Song <[email protected]>
---

Looks reasonable to me. Thanks for the fix.

Reviewed-by Baolin Wang <[email protected]>

Harry Yoo

2026-06-23 06:17 UTC #3 lore raw

In reply to Qi Zheng

Quoted 6 lines · Qi Zheng

From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock. The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.

Ouch.

IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.

Perhaps worth mentioning in the changelog?

Quoted 8 lines

The page table walker can run concurrently with the memcg reparenting path as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range

Quoted 13 lines

    --> update_batch_size
        --> walk->nr_pages += delta

                              mem_cgroup_css_offline
                              --> memcg_reparent_objcgs
                                  --> lock lruvec
                                      lru_gen_reparent_memcg
                                      --> reparent child folios to parent
                                      unlock lruvec

    lock lruvec
    reset_batch_size
    --> child lrugen->nr_pages += delta

The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.

Quoted 8 lines

This will trigger the following warning in lru_gen_exit_memcg():
	VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
				   sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.

Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?

Quoted 7 lines

Reported-by Peiyang He <[email protected]>
Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn open here
Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
Cc <[email protected]>
Signed-off-by Qi Zheng <[email protected]>
Reviewed-by Barry Song <[email protected]>
---

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Qi Zheng

author reviewer

2026-06-23 07:16 UTC #4 lore raw

Hi Harry,

Quoted 10 lines · Harry Yoo

Quoted 6 lines · Qi Zheng
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock. The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.

Quoted 7 lines

Ouch.
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?

Maybe this should be placed before "To fix it...".

Quoted 34 lines

Quoted 9 lines
The page table walker can run concurrently with the memcg reparenting path as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 13 lines
     --> update_batch_size
         --> walk->nr_pages += delta

                               mem_cgroup_css_offline
                               --> memcg_reparent_objcgs
                                   --> lock lruvec
                                       lru_gen_reparent_memcg
                                       --> reparent child folios to parent
                                       unlock lruvec

     lock lruvec
     reset_batch_size
     --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 5 lines
This will trigger the following warning in lru_gen_exit_memcg():
	VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
				   sizeof(lruvec->lrugen.nr_pages)));

Quoted 7 lines

Quoted 3 lines
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?

Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag. I also considered using mem_cgroup_tryget_online(), but that only prevent the memcg from being freed. It's doesn't prevent the offlining.

So in the end, I chose the approach used in this patch. Simply adding a new field to mglru to track its reparenting status seems to be the most straightforward and effective approach.

Thanks,
Qi

[1]. https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn

Quoted 9 lines

Quoted 8 lines
Reported-by Peiyang He <[email protected]>
Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-a53a-1e28cc894f0b@smail.nju.edu.cn open here
Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
Cc <[email protected]>
Signed-off-by Qi Zheng <[email protected]>
Reviewed-by Barry Song <[email protected]>
---

Harry Yoo

2026-06-23 08:19 UTC #5 lore raw

Quoted 1 line · Qi Zheng

Hi Harry,

Hi Qi!

Quoted 12 lines · Harry Yoo

Quoted 11 lines · Qi Zheng
Quoted 7 lines
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock.
The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.
Ouch.

Quoted 7 lines

Quoted 5 lines
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?
Maybe this should be placed before "To fix it...".

Thanks!

Quoted 39 lines

Quoted 39 lines
Quoted 9 lines
The page table walker can run concurrently with the memcg reparenting path
as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 14 lines
     --> update_batch_size
         --> walk->nr_pages += delta

                               mem_cgroup_css_offline
                               --> memcg_reparent_objcgs
                                   --> lock lruvec
                                       lru_gen_reparent_memcg
                                       --> reparent child folios to
parent
                                       unlock lruvec

     lock lruvec
     reset_batch_size
     --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 8 lines
This will trigger the following warning in lru_gen_exit_memcg():
    VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
                   sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.

Quoted 6 lines

Quoted 3 lines
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?
Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag.

Right.

Quoted 2 lines

I also considered using mem_cgroup_tryget_online(), but that only prevent the memcg from being freed. It's doesn't prevent the offlining.

Right.

I think checking CSS_DYING under RCU and grabbing the lruvec of the first non-dying memcg should work (this pattern is already used where we use RCU to guarantee memcgs are not freed).

If we do not observe CSS_DYING flag, it is safe to charge deltas to the lruvec because RCU guarantees that reparenting cannot happen under us.

If we do observe CSS_DYING, we can walk up the hierarchy and charge deltas to the first non-dying memcg.

This requires introducing an API to grab the first non-dying lruvec lock like folio_lruvec_lock_irq(), but it can be called only when the caller has a reference to make sure it doens't go away. (I wonder if there are other places that requires similar semantics)

or am I missing something?

Quoted 3 lines

So in the end, I chose the approach used in this patch. Simply adding a new field to mglru to track its reparenting status seems to be the most straightforward and effective approach.

I think it would work, but we need some explanation on why this requires special handling compared to other stuff (e.g., reparenting of split queues).

Quoted 2 lines

Thanks,
Qi

Thanks for working on this, Qi!

Quoted 10 lines

[1]. https://lore.kernel.org/all/5A9E929D82717101+12fcf643-efb8-4b9a-
[email protected]
Quoted 7 lines
Quoted 7 lines
Reported-by Peiyang He <[email protected]>
Closes https://lore.kernel.org/all/5A9E929D82717101+12fcf643-
[email protected]
Fixes f304652609ea ("mm: vmscan: prepare for reparenting MGLRU folios")
Cc <[email protected]>
Signed-off-by Qi Zheng <[email protected]>
Reviewed-by Barry Song <[email protected]>

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Qi Zheng

author reviewer

2026-06-23 09:15 UTC #6 lore raw

Hi Harry,

Quoted 86 lines · Harry Yoo

Quoted 1 line · Qi Zheng
Hi Harry,
Hi Qi!
Quoted 12 lines · Harry Yoo
Quoted 11 lines · Qi Zheng
Quoted 7 lines
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock.
The reset_batch_size() later folds those deltas into walk->lruvec under the lruvec lock.
Ouch.
Quoted 7 lines
Quoted 5 lines
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?
Maybe this should be placed before "To fix it...".
Thanks!
Quoted 39 lines
Quoted 39 lines
Quoted 9 lines
The page table walker can run concurrently with the memcg reparenting path
as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 14 lines
      --> update_batch_size
          --> walk->nr_pages += delta

                                mem_cgroup_css_offline
                                --> memcg_reparent_objcgs
                                    --> lock lruvec
                                        lru_gen_reparent_memcg
                                        --> reparent child folios to
parent
                                        unlock lruvec

      lock lruvec
      reset_batch_size
      --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 8 lines
This will trigger the following warning in lru_gen_exit_memcg():
     VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
                    sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to that owner.
Quoted 6 lines
Quoted 3 lines
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?
Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag.
Right.
Quoted 2 lines
I also considered using mem_cgroup_tryget_online(), but that only prevent the memcg from being freed. It's doesn't prevent the offlining.
Right.
I think checking CSS_DYING under RCU and grabbing the lruvec of the first non-dying memcg should work (this pattern is already used where we use RCU to guarantee memcgs are not freed).

Quoted 6 lines

If we do not observe CSS_DYING flag, it is safe to charge deltas to the lruvec because RCU guarantees that reparenting cannot happen under us.
If we do observe CSS_DYING, we can walk up the hierarchy and charge deltas to the first non-dying memcg.

Checking CSS_DYING looks feasible, but the rcu lock alone cannot prevent reparenting. We should recheck CSS_DYING after acquiring the lruvec lock, otherwise we might run into the following race:

CPU0 reset_batch_size CPU1 memcg teardown
===================== ==================

read !CSS_DYING

set CSS_DYING memcg_reparent_objcgs() lock child lruvec move child to parent zero child nr_pages unlock child lruvec

lock child lruvec
charge stale delta to child

So it seems lock_batch_lruvec() should be implemented like this:

static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)

{
	struct mem_cgroup *memcg = lruvec_memcg(lruvec);

	rcu_read_lock();
retry:
	while (memcg && css_is_dying(&memcg->css))
		memcg = parent_mem_cgroup(memcg);

	lruvec = mem_cgroup_lruvec(memcg, pgdat);
	spin_lock_irq(&lruvec->lru_lock);
	if (memcg && unlikely(css_is_dying(&memcg->css))) {
		spin_unlock_irq(&lruvec->lru_lock);
		goto retry;
	}

	rcu_read_unlock();

	return lruvec;
}

This way, there is no need to add lrugen->reparented, right?

Thanks,
Qi

Harry Yoo

2026-06-24 04:30 UTC #7 lore raw

Quoted 94 lines · Qi Zheng

Hi Harry,
Quoted 91 lines · Harry Yoo
Quoted 1 line · Qi Zheng
Hi Harry,
Hi Qi!
Quoted 13 lines · Harry Yoo
Quoted 12 lines · Qi Zheng
Quoted 8 lines
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock.
The reset_batch_size() later folds those deltas into walk->lruvec under
the lruvec lock.
Ouch.
Quoted 7 lines
Quoted 5 lines
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?
Maybe this should be placed before "To fix it...".
Thanks!
Quoted 40 lines
Quoted 40 lines
Quoted 9 lines
The page table walker can run concurrently with the memcg reparenting path
as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 14 lines
      --> update_batch_size
          --> walk->nr_pages += delta

                                mem_cgroup_css_offline
                                --> memcg_reparent_objcgs
                                    --> lock lruvec
                                        lru_gen_reparent_memcg
                                        --> reparent child folios to
parent
                                        unlock lruvec

      lock lruvec
      reset_batch_size
      --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 9 lines
This will trigger the following warning in lru_gen_exit_memcg():
     VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
                    sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to
that owner.
Quoted 6 lines
Quoted 3 lines
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?
Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag.
Right.
Quoted 3 lines
I also considered using mem_cgroup_tryget_online(), but that only prevent
the memcg from being freed. It's doesn't prevent the offlining.
Right.
I think checking CSS_DYING under RCU and grabbing the lruvec of the first non-dying memcg should work (this pattern is already used where we use RCU to guarantee memcgs are not freed).
If we do not observe CSS_DYING flag, it is safe to charge deltas to the lruvec because RCU guarantees that reparenting cannot happen under us.

Quoted 6 lines

Quoted 2 lines
If we do observe CSS_DYING, we can walk up the hierarchy and charge deltas to the first non-dying memcg.
Checking CSS_DYING looks feasible, but the rcu lock alone cannot prevent reparenting. We should recheck CSS_DYING after acquiring the lruvec lock, otherwise we might run into the following race:

Haha, actually, I was thinking of checking CSS_DYING under both RCU and lruvec lock. (because that's the pattern)

Quoted 6 lines

CPU0 reset_batch_size CPU1 memcg teardown
===================== ==================
read !CSS_DYING
set CSS_DYING

Oh, I thought the entire critical section is covered by RCU. (I see lock_batch_lruvec() you suggested below doesn't do that)

Isn't RCU enough to prevent reparenting because RCU guarantees that all readers who read !CSS_DYING complete before reparenting?

Now I'm confused. Is it strictly required to check CSS_DYING under lruvec lock? CSS_DYING is updated outside the lruvec lock anyway?

Quoted 11 lines

memcg_reparent_objcgs() lock child lruvec move child to parent zero child nr_pages unlock child lruvec
lock child lruvec
charge stale delta to child
So it seems lock_batch_lruvec() should be implemented like this:

Quoted 8 lines

static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)

{
    struct mem_cgroup *memcg = lruvec_memcg(lruvec);

    rcu_read_lock();
retry:
    while (memcg && css_is_dying(&memcg->css))
        memcg = parent_mem_cgroup(memcg);

Isn't this loop unnecessary as spin_lock_irq() -> check CSS_DYING -> goto retry does the same thing? (of course, we need to fetch the parent memcg before retry then...)

Quoted 11 lines

    lruvec = mem_cgroup_lruvec(memcg, pgdat);
    spin_lock_irq(&lruvec->lru_lock);
    if (memcg && unlikely(css_is_dying(&memcg->css))) {
        spin_unlock_irq(&lruvec->lru_lock);
        goto retry;
    }

    rcu_read_unlock();

    return lruvec;
}

Thanks!

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Qi Zheng

author reviewer

2026-06-24 07:12 UTC #8 lore raw

Hi Harry,

Quoted 112 lines · Harry Yoo

Quoted 94 lines · Qi Zheng
Hi Harry,
Quoted 91 lines · Harry Yoo
Quoted 1 line · Qi Zheng
Hi Harry,
Hi Qi!
Quoted 13 lines · Harry Yoo
Quoted 12 lines · Qi Zheng
Quoted 8 lines
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock.
The reset_batch_size() later folds those deltas into walk->lruvec under
the lruvec lock.
Ouch.
Quoted 7 lines
Quoted 5 lines
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?
Maybe this should be placed before "To fix it...".
Thanks!
Quoted 40 lines
Quoted 40 lines
Quoted 9 lines
The page table walker can run concurrently with the memcg reparenting path
as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 14 lines
       --> update_batch_size
           --> walk->nr_pages += delta

                                 mem_cgroup_css_offline
                                 --> memcg_reparent_objcgs
                                     --> lock lruvec
                                         lru_gen_reparent_memcg
                                         --> reparent child folios to
parent
                                         unlock lruvec

       lock lruvec
       reset_batch_size
       --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size() doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 9 lines
This will trigger the following warning in lru_gen_exit_memcg():
      VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
                     sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to
that owner.
Quoted 6 lines
Quoted 3 lines
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas to non-dying parent) doesn't work?
Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag.
Right.
Quoted 3 lines
I also considered using mem_cgroup_tryget_online(), but that only prevent
the memcg from being freed. It's doesn't prevent the offlining.
Right.
I think checking CSS_DYING under RCU and grabbing the lruvec of the first non-dying memcg should work (this pattern is already used where we use RCU to guarantee memcgs are not freed).
If we do not observe CSS_DYING flag, it is safe to charge deltas to the lruvec because RCU guarantees that reparenting cannot happen under us.
Quoted 6 lines
Quoted 2 lines
If we do observe CSS_DYING, we can walk up the hierarchy and charge deltas to the first non-dying memcg.
Checking CSS_DYING looks feasible, but the rcu lock alone cannot prevent reparenting. We should recheck CSS_DYING after acquiring the lruvec lock, otherwise we might run into the following race:
Haha, actually, I was thinking of checking CSS_DYING under both RCU and lruvec lock. (because that's the pattern)
Quoted 5 lines
CPU0 reset_batch_size CPU1 memcg teardown
===================== ==================
read !CSS_DYING

Quoted 7 lines

Quoted 1 line
set CSS_DYING
Oh, I thought the entire critical section is covered by RCU. (I see lock_batch_lruvec() you suggested below doesn't do that)
Isn't RCU enough to prevent reparenting because RCU guarantees that all readers who read !CSS_DYING complete before reparenting?

Oh, I think you are right.

I forgot that offlining is executed in the rcu work context.

Let's walk through this again:

cgroup_destroy_locked
--> kill_css_sync

    --> css->flags |= CSS_DYING;                    1)
    kill_css_finish
    --> css_killed_ref_fn
        --> css_killed_work_fn  <-- RCU work !!     2)
            --> offline_css
                --> reparent memcg

So while holding the rcu lock, if CSS_DYING is not observed, css_killed_work_fn() will not be called until rcu_read_unlock().

So lock_batch_lruvec() can be implemented like this:

#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)

{
	struct pglist_data *pgdat = lruvec_pgdat(lruvec);
	struct mem_cgroup *memcg = lruvec_memcg(lruvec);

	rcu_read_lock();

	/*
	 * The memcg can be NULL when the memory controller is disabled.
	 * Otherwise, the caller keeps the memcg owning @lruvec alive.
	 */
	if (!memcg || !css_is_dying(&memcg->css))
		goto lock;

	do {
		memcg = parent_mem_cgroup(memcg);
	} while (memcg && css_is_dying(&memcg->css));
	lruvec = mem_cgroup_lruvec(memcg, pgdat);

lock:

	spin_lock_irq(&lruvec->lru_lock);

	return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
{
	lruvec_lock_irq(lruvec);

	return lruvec;
}

#endif

Does this make sense?

Thanks,
Qi

Quoted 42 lines

Now I'm confused. Is it strictly required to check CSS_DYING under lruvec lock? CSS_DYING is updated outside the lruvec lock anyway?
Quoted 11 lines
memcg_reparent_objcgs() lock child lruvec move child to parent zero child nr_pages unlock child lruvec
lock child lruvec
charge stale delta to child
So it seems lock_batch_lruvec() should be implemented like this:
Quoted 8 lines
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
{
     struct mem_cgroup *memcg = lruvec_memcg(lruvec);

     rcu_read_lock();
retry:
     while (memcg && css_is_dying(&memcg->css))
         memcg = parent_mem_cgroup(memcg);
Isn't this loop unnecessary as spin_lock_irq() -> check CSS_DYING -> goto retry does the same thing? (of course, we need to fetch the parent memcg before retry then...)
Quoted 11 lines
     lruvec = mem_cgroup_lruvec(memcg, pgdat);
     spin_lock_irq(&lruvec->lru_lock);
     if (memcg && unlikely(css_is_dying(&memcg->css))) {
         spin_unlock_irq(&lruvec->lru_lock);
         goto retry;
     }

     rcu_read_unlock();

     return lruvec;
}
Thanks!

Harry Yoo

2026-06-25 04:16 UTC #9 lore raw

Quoted 122 lines · Qi Zheng

Hi Harry,
Quoted 119 lines · Harry Yoo
Quoted 98 lines · Qi Zheng
Hi Harry,
Quoted 95 lines · Harry Yoo
Quoted 1 line · Qi Zheng
Hi Harry,
Hi Qi!
Quoted 13 lines · Harry Yoo
Quoted 12 lines · Qi Zheng
Quoted 8 lines
From: Qi Zheng <[email protected]>
The mglru page table walker batches per-generation size deltas in walk->nr_pages while walking page tables without holding the lruvec lock.
The reset_batch_size() later folds those deltas into walk->lruvec under
the lruvec lock.
Ouch.
Quoted 7 lines
Quoted 5 lines
IIRC the user-visible impact of underestimated nr_pages in MGLRU was premature OOMs because MGLRU does not try to reclaim memory when nr_pages reaches zero, but there are still more pages.
Perhaps worth mentioning in the changelog?
Maybe this should be placed before "To fix it...".
Thanks!
Quoted 43 lines
Quoted 43 lines
Quoted 10 lines
The page table walker can run concurrently with the memcg reparenting
path
as follows:
CPU0 CPU1
==== ====
walk_mm
--> walk_page_range
Quoted 15 lines
       --> update_batch_size
           --> walk->nr_pages += delta

                                 mem_cgroup_css_offline
                                 --> memcg_reparent_objcgs
                                     --> lock lruvec
                                         lru_gen_reparent_memcg
                                         --> reparent child
folios to
parent
                                         unlock lruvec

       lock lruvec
       reset_batch_size
       --> child lrugen->nr_pages += delta
The problem here is that, while grabbing a reference to memcg (via mem_cgroup_iter(), for example) makes sure that the memcg is not freed, it does not prevent offlining happening, and reset_batch_size()
doesn't check whether the lruvec has been reparented, or the lruvec is going to be reparented.
Quoted 9 lines
This will trigger the following warning in lru_gen_exit_memcg():
      VM_WARN_ON_ONCE(memchr_inv(lruvec->lrugen.nr_pages, 0,
                     sizeof(lruvec->lrugen.nr_pages)));
To fix it, add lrugen->reparented to remember the new owner of a reparented lruvec, and make reset_batch_size() charge pending deltas to
that owner.
Quoted 7 lines
Quoted 4 lines
Could you please explain why it is unavoidable to introduce the new field and why checking whether the cgroup is dying (and charging deltas
to non-dying parent) doesn't work?
Peiyang tried doing this [1], but it doesn't work because ss->css_offline() is called before clearing the CSS_ONLINE flag.
Right.
Quoted 3 lines
I also considered using mem_cgroup_tryget_online(), but that only prevent
the memcg from being freed. It's doesn't prevent the offlining.
Right.
I think checking CSS_DYING under RCU and grabbing the lruvec of the first non-dying memcg should work (this pattern is already used where we use RCU to guarantee memcgs are not freed).
If we do not observe CSS_DYING flag, it is safe to charge deltas to the lruvec because RCU guarantees that reparenting cannot happen under us.
Quoted 6 lines
Quoted 2 lines
If we do observe CSS_DYING, we can walk up the hierarchy and charge deltas to the first non-dying memcg.
Checking CSS_DYING looks feasible, but the rcu lock alone cannot prevent reparenting. We should recheck CSS_DYING after acquiring the lruvec lock, otherwise we might run into the following race:
Haha, actually, I was thinking of checking CSS_DYING under both RCU and lruvec lock. (because that's the pattern)
Quoted 6 lines
CPU0 reset_batch_size CPU1 memcg teardown
===================== ==================
read !CSS_DYING
set CSS_DYING
Oh, I thought the entire critical section is covered by RCU. (I see lock_batch_lruvec() you suggested below doesn't do that)

Quoted 6 lines

Quoted 2 lines
Isn't RCU enough to prevent reparenting because RCU guarantees that all readers who read !CSS_DYING complete before reparenting?
Oh, I think you are right.
I forgot that offlining is executed in the rcu work context.

It's confusing :)

Quoted 11 lines

Let's walk through this again:

cgroup_destroy_locked
--> kill_css_sync

    --> css->flags |= CSS_DYING;                    1)
    kill_css_finish
    --> css_killed_ref_fn
        --> css_killed_work_fn  <-- RCU work !!     2)
            --> offline_css
                --> reparent memcg

Quoted 2 lines

So while holding the rcu lock, if CSS_DYING is not observed, css_killed_work_fn() will not be called until rcu_read_unlock().

Right.

Quoted 34 lines

So lock_batch_lruvec() can be implemented like this:

#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)

{
    struct pglist_data *pgdat = lruvec_pgdat(lruvec);
    struct mem_cgroup *memcg = lruvec_memcg(lruvec);

    rcu_read_lock();

    /*
     * The memcg can be NULL when the memory controller is disabled.
     * Otherwise, the caller keeps the memcg owning @lruvec alive.
     */
    if (!memcg || !css_is_dying(&memcg->css))
        goto lock;

    do {
        memcg = parent_mem_cgroup(memcg);
    } while (memcg && css_is_dying(&memcg->css));
    lruvec = mem_cgroup_lruvec(memcg, pgdat);

lock:

    spin_lock_irq(&lruvec->lru_lock);

    return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
{
    lruvec_lock_irq(lruvec);

    return lruvec;
}

Quoted 3 lines

#endif
Does this make sense?

Yes, looks good to me!

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Qi Zheng

author reviewer

2026-06-25 06:11 UTC #10 lore raw

Quoted 1 line · Harry Yoo

[...]

Quoted 33 lines

So lock_batch_lruvec() can be implemented like this:

#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)

{
     struct pglist_data *pgdat = lruvec_pgdat(lruvec);
     struct mem_cgroup *memcg = lruvec_memcg(lruvec);

     rcu_read_lock();

     /*
      * The memcg can be NULL when the memory controller is disabled.
      * Otherwise, the caller keeps the memcg owning @lruvec alive.
      */
     if (!memcg || !css_is_dying(&memcg->css))
         goto lock;

     do {
         memcg = parent_mem_cgroup(memcg);
     } while (memcg && css_is_dying(&memcg->css));
     lruvec = mem_cgroup_lruvec(memcg, pgdat);

lock:

     spin_lock_irq(&lruvec->lru_lock);

     return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec)
{
     lruvec_lock_irq(lruvec);

Quoted 7 lines

Quoted 5 lines
     return lruvec;
}
#endif
Does this make sense?
Yes, looks good to me!

OK, this sync method makes more sense as it doesn't require adding a new lrugen->reparente. I'll go with this method and update v3.

Hi Barry and Baolin, what do you think? Since the sync method has been changed, I will temporarily drop your previous Reviewed-by tags in v3. ;)

Thanks,
Qi

Quoted 1 line

Harry Yoo

2026-06-25 06:32 UTC #11 lore raw

Quoted 41 lines · Qi Zheng

Quoted 1 line · Harry Yoo
[...]
Quoted 37 lines
Quoted 37 lines
So lock_batch_lruvec() can be implemented like this:
#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
     struct pglist_data *pgdat = lruvec_pgdat(lruvec); struct mem_cgroup *memcg = lruvec_memcg(lruvec);
     rcu_read_lock();
     /*
      * The memcg can be NULL when the memory controller is disabled.
      * Otherwise, the caller keeps the memcg owning @lruvec alive. */
     if (!memcg || !css_is_dying(&memcg->css))
         goto lock;
     do {
         memcg = parent_mem_cgroup(memcg); } while (memcg && css_is_dying(&memcg->css)); lruvec = mem_cgroup_lruvec(memcg, pgdat);
lock:
     spin_lock_irq(&lruvec->lru_lock);
     return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
     lruvec_lock_irq(lruvec);
     return lruvec;
}
#endif

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Does this make sense?
Yes, looks good to me!
OK, this sync method makes more sense as it doesn't require adding a new lrugen->reparente. I'll go with this method and update v3.

Thanks!

Just one thing to clarify...

So, when we check something that's updated _before_ grace period (CSS_DYING), RCU is sufficient.

But in folio_lruvec_lock*(), that is not the case because reparenting is performed in the RCU work, under the lruvec lock. So the check needs to be done under RCU and the lruvec lock.

This is quite subtle :D

Quoted 2 lines

Hi Barry and Baolin, what do you think? Since the sync method has been changed, I will temporarily drop your previous Reviewed-by tags in v3. ;)

And hopefully Peiyang would kindly double check v3 still not reproduced on the machine :)

Signature · 2 lines

Cheers,
Harry / Hyeonggon

Qi Zheng

author reviewer

2026-06-25 07:38 UTC #12 lore raw

Quoted 55 lines · Harry Yoo

Quoted 41 lines · Qi Zheng
Quoted 1 line · Harry Yoo
[...]
Quoted 37 lines
Quoted 37 lines
So lock_batch_lruvec() can be implemented like this:
#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
     struct pglist_data *pgdat = lruvec_pgdat(lruvec); struct mem_cgroup *memcg = lruvec_memcg(lruvec);
     rcu_read_lock();
     /*
      * The memcg can be NULL when the memory controller is disabled.
      * Otherwise, the caller keeps the memcg owning @lruvec alive. */
     if (!memcg || !css_is_dying(&memcg->css))
         goto lock;
     do {
         memcg = parent_mem_cgroup(memcg); } while (memcg && css_is_dying(&memcg->css)); lruvec = mem_cgroup_lruvec(memcg, pgdat);
lock:
     spin_lock_irq(&lruvec->lru_lock);
     return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
     lruvec_lock_irq(lruvec);
     return lruvec;
}
#endif
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Does this make sense?
Yes, looks good to me!
OK, this sync method makes more sense as it doesn't require adding a new lrugen->reparente. I'll go with this method and update v3.
Thanks!
Just one thing to clarify...

Quoted 8 lines

So, when we check something that's updated _before_ grace period (CSS_DYING), RCU is sufficient.
But in folio_lruvec_lock*(), that is not the case because reparenting is performed in the RCU work, under the lruvec lock. So the check needs to be done under RCU and the lruvec lock.
This is quite subtle :D

Indeed.

And in theory, the l->nr_items check in lock_list_lru_of_memcg() could also be replaced by the CSS_DYING check.

Quoted 6 lines

Quoted 3 lines
Hi Barry and Baolin, what do you think? Since the sync method has been changed, I will temporarily drop your previous Reviewed-by tags in v3. ;)
And hopefully Peiyang would kindly double check v3 still not reproduced on the machine :)

Yeah!

Quoted 1 line

Peiyang He

2026-06-25 08:09 UTC #13 lore raw

Quoted 73 lines · Qi Zheng

Quoted 55 lines · Harry Yoo
Quoted 41 lines · Qi Zheng
Quoted 1 line · Harry Yoo
[...]
Quoted 37 lines
Quoted 37 lines
So lock_batch_lruvec() can be implemented like this:
#ifdef CONFIG_MEMCG
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
      struct pglist_data *pgdat = lruvec_pgdat(lruvec); struct mem_cgroup *memcg = lruvec_memcg(lruvec);
      rcu_read_lock();
      /*
       * The memcg can be NULL when the memory controller is disabled.
       * Otherwise, the caller keeps the memcg owning @lruvec alive. */
      if (!memcg || !css_is_dying(&memcg->css))
          goto lock;
      do {
          memcg = parent_mem_cgroup(memcg); } while (memcg && css_is_dying(&memcg->css)); lruvec = mem_cgroup_lruvec(memcg, pgdat);
lock:
      spin_lock_irq(&lruvec->lru_lock);
      return lruvec;
}
#else
static struct lruvec *lock_batch_lruvec(struct lruvec *lruvec) {
      lruvec_lock_irq(lruvec);
      return lruvec;
}
#endif
Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Does this make sense?
Yes, looks good to me!
OK, this sync method makes more sense as it doesn't require adding a new lrugen->reparente. I'll go with this method and update v3.
Thanks!
Just one thing to clarify...
Quoted 8 lines
So, when we check something that's updated _before_ grace period (CSS_DYING), RCU is sufficient.
But in folio_lruvec_lock*(), that is not the case because reparenting is performed in the RCU work, under the lruvec lock. So the check needs to be done under RCU and the lruvec lock.
This is quite subtle :D
Indeed.
And in theory, the l->nr_items check in lock_list_lru_of_memcg() could also be replaced by the CSS_DYING check.
Quoted 1 line

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
Hi Barry and Baolin, what do you think? Since the sync method has been changed, I will temporarily drop your previous Reviewed-by tags in v3. ;)
And hopefully Peiyang would kindly double check v3 still not reproduced on the machine :)
Yeah!

No problem! I can help test v3.>

Quoted 3 lines

Quoted 1 line

Barry Song

reviewer

2026-06-25 10:07 UTC #14 lore raw

In reply to Qi Zheng

On Thu, Jun 25, 2026 at 2:11 PM Qi Zheng <[email protected]> wrote:

[...]

Quoted 10 lines

Quoted 4 lines
Quoted 2 lines
Does this make sense?
Yes, looks good to me!
OK, this sync method makes more sense as it doesn't require adding a new lrugen->reparente. I'll go with this method and update v3.
Hi Barry and Baolin, what do you think? Since the sync method has been changed, I will temporarily drop your previous Reviewed-by tags in v3. ;)

Feel free to proceed with the new version and drop my tag :-)

Best Regards
Barry