[PATCH v2 0/5] Minor memcg cleanups & prep for memdescs

[PATCH v2 0/5] Minor memcg cleanups & prep for memdescs

[Matthew Wilcox (Oracle)]

Separate the handling of accounted folios and GFP_ACCOUNT pages for easier
to understand code.  For more detail, see
https://lore.kernel.org/linux-mm/Z9LwTOudOlCGny3f@casper.infradead.org/

Matthew Wilcox (Oracle) (5):
  mm: Separate folio_split_memcg_refs() from split_page_memcg()
  mm: Simplify split_page_memcg()
  mm: Remove references to folio in split_page_memcg()
  mm: Simplify folio_memcg_charged()
  mm: Remove references to folio in __memcg_kmem_uncharge_page()

 include/linux/memcontrol.h | 15 ++++++---
 mm/huge_memory.c           | 16 +++-------
 mm/memcontrol.c            | 62 ++++++++++++++++++++++++++------------
 mm/page_alloc.c            |  4 +--
 4 files changed, 58 insertions(+), 39 deletions(-)

-- 
2.47.2

[PATCH v2 1/5] mm: Separate folio_split_memcg_refs() from split_page_memcg()

[Matthew Wilcox (Oracle)]

Folios always use memcg_data to refer to the mem_cgroup while pages
allocated with GFP_ACCOUNT have a pointer to the obj_cgroup.  Since the
caller already knows what it has, split the function into two and then
we don't need to check.

Move the assignment of split folio memcg_data to the point where we set
up the other parts of the new folio.  That leaves folio_split_memcg_refs()
just handling the memcg accounting.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Zi Yan <ziy@nvidia.com>
---
 include/linux/memcontrol.h |  7 +++++++
 mm/huge_memory.c           | 16 ++++------------
 mm/memcontrol.c            | 17 +++++++++++++----
 3 files changed, 24 insertions(+), 16 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 57664e2a8fb7..d090089c5497 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1039,6 +1039,8 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
 }
 
 void split_page_memcg(struct page *head, int old_order, int new_order);
+void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
+		unsigned new_order);
 
 static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
 {
@@ -1463,6 +1465,11 @@ static inline void split_page_memcg(struct page *head, int old_order, int new_or
 {
 }
 
+static inline void folio_split_memcg_refs(struct folio *folio,
+		unsigned old_order, unsigned new_order)
+{
+}
+
 static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
 {
 	return 0;
diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 14b1963898a7..3e5ecc8f3d13 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -3394,6 +3394,9 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
 			folio_set_young(new_folio);
 		if (folio_test_idle(folio))
 			folio_set_idle(new_folio);
+#ifdef CONFIG_MEMCG
+		new_folio->memcg_data = folio->memcg_data;
+#endif
 
 		folio_xchg_last_cpupid(new_folio, folio_last_cpupid(folio));
 	}
@@ -3525,18 +3528,7 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
 			}
 		}
 
-		/*
-		 * Reset any memcg data overlay in the tail pages.
-		 * folio_nr_pages() is unreliable until prep_compound_page()
-		 * was called again.
-		 */
-#ifdef NR_PAGES_IN_LARGE_FOLIO
-		folio->_nr_pages = 0;
-#endif
-
-
-		/* complete memcg works before add pages to LRU */
-		split_page_memcg(&folio->page, old_order, split_order);
+		folio_split_memcg_refs(folio, old_order, split_order);
 		split_page_owner(&folio->page, old_order, split_order);
 		pgalloc_tag_split(folio, old_order, split_order);
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 87544df4c3b8..4674d9815a50 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3101,10 +3101,19 @@ void split_page_memcg(struct page *head, int old_order, int new_order)
 	for (i = new_nr; i < old_nr; i += new_nr)
 		folio_page(folio, i)->memcg_data = folio->memcg_data;
 
-	if (folio_memcg_kmem(folio))
-		obj_cgroup_get_many(__folio_objcg(folio), old_nr / new_nr - 1);
-	else
-		css_get_many(&folio_memcg(folio)->css, old_nr / new_nr - 1);
+	obj_cgroup_get_many(__folio_objcg(folio), old_nr / new_nr - 1);
+}
+
+void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
+		unsigned new_order)
+{
+	unsigned new_refs;
+
+	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
+		return;
+
+	new_refs = (1 << (old_order - new_order)) - 1;
+	css_get_many(&__folio_memcg(folio)->css, new_refs);
 }
 
 unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap)
-- 
2.47.2

[PATCH v2 2/5] mm: Simplify split_page_memcg()

[Matthew Wilcox (Oracle)]

The last argument to split_page_memcg() is now always 0, so remove it,
effectively reverting commit b8791381d7ed.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
Acked-by: Zi Yan <ziy@nvidia.com>
---
 include/linux/memcontrol.h |  4 ++--
 mm/memcontrol.c            | 15 +++++++--------
 mm/page_alloc.c            |  4 ++--
 3 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index d090089c5497..ea28cacfb0d2 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -1038,7 +1038,7 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
 	rcu_read_unlock();
 }
 
-void split_page_memcg(struct page *head, int old_order, int new_order);
+void split_page_memcg(struct page *first, unsigned order);
 void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
 		unsigned new_order);
 
@@ -1461,7 +1461,7 @@ void count_memcg_event_mm(struct mm_struct *mm, enum vm_event_item idx)
 {
 }
 
-static inline void split_page_memcg(struct page *head, int old_order, int new_order)
+static inline void split_page_memcg(struct page *first, unsigned order)
 {
 }
 
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 4674d9815a50..862bb0d5c0f2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3086,22 +3086,21 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
 }
 
 /*
- * Because folio_memcg(head) is not set on tails, set it now.
+ * The objcg is only set on the first page, so transfer it to all the
+ * other pages.
  */
-void split_page_memcg(struct page *head, int old_order, int new_order)
+void split_page_memcg(struct page *first, unsigned order)
 {
-	struct folio *folio = page_folio(head);
-	int i;
-	unsigned int old_nr = 1 << old_order;
-	unsigned int new_nr = 1 << new_order;
+	struct folio *folio = page_folio(first);
+	unsigned int i, nr = 1 << order;
 
 	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
 		return;
 
-	for (i = new_nr; i < old_nr; i += new_nr)
+	for (i = 1; i < nr; i++)
 		folio_page(folio, i)->memcg_data = folio->memcg_data;
 
-	obj_cgroup_get_many(__folio_objcg(folio), old_nr / new_nr - 1);
+	obj_cgroup_get_many(__folio_objcg(folio), nr - 1);
 }
 
 void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 004c4856e71a..987e14d87932 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2813,7 +2813,7 @@ void split_page(struct page *page, unsigned int order)
 		set_page_refcounted(page + i);
 	split_page_owner(page, order, 0);
 	pgalloc_tag_split(page_folio(page), order, 0);
-	split_page_memcg(page, order, 0);
+	split_page_memcg(page, order);
 }
 EXPORT_SYMBOL_GPL(split_page);
 
@@ -5033,7 +5033,7 @@ static void *make_alloc_exact(unsigned long addr, unsigned int order,
 
 		split_page_owner(page, order, 0);
 		pgalloc_tag_split(page_folio(page), order, 0);
-		split_page_memcg(page, order, 0);
+		split_page_memcg(page, order);
 		while (page < --last)
 			set_page_refcounted(last);
 
-- 
2.47.2

[PATCH v2 3/5] mm: Remove references to folio in split_page_memcg()

[Matthew Wilcox (Oracle)]

We know that the passed in page is not part of a folio (it's a plain
page allocated with GFP_ACCOUNT), so we should get rid of the misleading
references to folios.

Introduce page_objcg() and page_set_objcg() helpers to make things more
clear.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 30 +++++++++++++++++++++++-------
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 862bb0d5c0f2..9e9027dda78c 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2697,6 +2697,23 @@ static int obj_cgroup_charge_pages(struct obj_cgroup *objcg, gfp_t gfp,
 	return ret;
 }
 
+static struct obj_cgroup *page_objcg(const struct page *page)
+{
+	unsigned long memcg_data = page->memcg_data;
+
+	if (mem_cgroup_disabled() || !memcg_data)
+		return NULL;
+
+	VM_BUG_ON_PAGE((memcg_data & OBJEXTS_FLAGS_MASK) != MEMCG_DATA_KMEM,
+			page);
+	return (struct obj_cgroup *)(memcg_data - MEMCG_DATA_KMEM);
+}
+
+static void page_set_objcg(struct page *page, const struct obj_cgroup *objcg)
+{
+	page->memcg_data = (unsigned long)objcg | MEMCG_DATA_KMEM;
+}
+
 /**
  * __memcg_kmem_charge_page: charge a kmem page to the current memory cgroup
  * @page: page to charge
@@ -2715,8 +2732,7 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
 		ret = obj_cgroup_charge_pages(objcg, gfp, 1 << order);
 		if (!ret) {
 			obj_cgroup_get(objcg);
-			page->memcg_data = (unsigned long)objcg |
-				MEMCG_DATA_KMEM;
+			page_set_objcg(page, objcg);
 			return 0;
 		}
 	}
@@ -3089,18 +3105,18 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
  * The objcg is only set on the first page, so transfer it to all the
  * other pages.
  */
-void split_page_memcg(struct page *first, unsigned order)
+void split_page_memcg(struct page *page, unsigned order)
 {
-	struct folio *folio = page_folio(first);
+	struct obj_cgroup *objcg = page_objcg(page);
 	unsigned int i, nr = 1 << order;
 
-	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
+	if (!objcg)
 		return;
 
 	for (i = 1; i < nr; i++)
-		folio_page(folio, i)->memcg_data = folio->memcg_data;
+		page_set_objcg(&page[i], objcg);
 
-	obj_cgroup_get_many(__folio_objcg(folio), nr - 1);
+	obj_cgroup_get_many(objcg, nr - 1);
 }
 
 void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
-- 
2.47.2

[PATCH v2 4/5] mm: Simplify folio_memcg_charged()

[Matthew Wilcox (Oracle)]

There's no need to check which kind of pointer is in the memcg_data
field, all we actually care about is whether it's zero or not.
Saves 70 bytes in workingset_activation() with the Debian config.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 include/linux/memcontrol.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index ea28cacfb0d2..53364526d877 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -438,9 +438,7 @@ static inline struct mem_cgroup *folio_memcg(struct folio *folio)
  */
 static inline bool folio_memcg_charged(struct folio *folio)
 {
-	if (folio_memcg_kmem(folio))
-		return __folio_objcg(folio) != NULL;
-	return __folio_memcg(folio) != NULL;
+	return folio->memcg_data != 0;
 }
 
 /*
-- 
2.47.2

[PATCH v2 5/5] mm: Remove references to folio in __memcg_kmem_uncharge_page()

[Matthew Wilcox (Oracle)]

This use of folios is misleading because these pages are not part of
a folio.  Remove an unnecessary call to page_folio(), saving 58 bytes
of text in a Debian kernel build.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
Acked-by: Johannes Weiner <hannes@cmpxchg.org>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 9e9027dda78c..76ba1953173a 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2746,16 +2746,14 @@ int __memcg_kmem_charge_page(struct page *page, gfp_t gfp, int order)
  */
 void __memcg_kmem_uncharge_page(struct page *page, int order)
 {
-	struct folio *folio = page_folio(page);
-	struct obj_cgroup *objcg;
+	struct obj_cgroup *objcg = page_objcg(page);
 	unsigned int nr_pages = 1 << order;
 
-	if (!folio_memcg_kmem(folio))
+	if (!objcg)
 		return;
 
-	objcg = __folio_objcg(folio);
 	obj_cgroup_uncharge_pages(objcg, nr_pages);
-	folio->memcg_data = 0;
+	page->memcg_data = 0;
 	obj_cgroup_put(objcg);
 }
 
-- 
2.47.2

Re: [PATCH v2 1/5] mm: Separate folio_split_memcg_refs() from split_page_memcg()

[David Hildenbrand]

On 14.03.25 14:36, Matthew Wilcox (Oracle) wrote:
> Folios always use memcg_data to refer to the mem_cgroup while pages
> allocated with GFP_ACCOUNT have a pointer to the obj_cgroup.  Since the
> caller already knows what it has, split the function into two and then
> we don't need to check.
> 
> Move the assignment of split folio memcg_data to the point where we set
> up the other parts of the new folio.  That leaves folio_split_memcg_refs()
> just handling the memcg accounting.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Acked-by: Zi Yan <ziy@nvidia.com>
> ---
>   include/linux/memcontrol.h |  7 +++++++
>   mm/huge_memory.c           | 16 ++++------------
>   mm/memcontrol.c            | 17 +++++++++++++----
>   3 files changed, 24 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 57664e2a8fb7..d090089c5497 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1039,6 +1039,8 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
>   }
>   
>   void split_page_memcg(struct page *head, int old_order, int new_order);
> +void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
> +		unsigned new_order);
>   
>   static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>   {
> @@ -1463,6 +1465,11 @@ static inline void split_page_memcg(struct page *head, int old_order, int new_or
>   {
>   }
>   
> +static inline void folio_split_memcg_refs(struct folio *folio,
> +		unsigned old_order, unsigned new_order)
> +{
> +}
> +
>   static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>   {
>   	return 0;
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 14b1963898a7..3e5ecc8f3d13 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3394,6 +3394,9 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>   			folio_set_young(new_folio);
>   		if (folio_test_idle(folio))
>   			folio_set_idle(new_folio);
> +#ifdef CONFIG_MEMCG
> +		new_folio->memcg_data = folio->memcg_data;
> +#endif
>   
>   		folio_xchg_last_cpupid(new_folio, folio_last_cpupid(folio));
>   	}
> @@ -3525,18 +3528,7 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>   			}
>   		}
>   
> -		/*
> -		 * Reset any memcg data overlay in the tail pages.
> -		 * folio_nr_pages() is unreliable until prep_compound_page()
> -		 * was called again.
> -		 */
> -#ifdef NR_PAGES_IN_LARGE_FOLIO
> -		folio->_nr_pages = 0;
> -#endif


I remember that we could trigger a warning without that, but I don't 
immediately find where that warning was. IIRC, if we'd split to order-0, 
page[1] would have indicated that it had a memcg set, and something 
bailed out.

Maybe Zi Yan recalls where that check fired.

In any case, if that warning no longer fires this is a very nice cleanup!

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 3/5] mm: Remove references to folio in split_page_memcg()

[David Hildenbrand]

On 14.03.25 14:36, Matthew Wilcox (Oracle) wrote:
> We know that the passed in page is not part of a folio 

It would be great if we would have a way to assert that. ... but I'm 
afraid that has to wait for the memdesc split.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 0/5] Minor memcg cleanups & prep for memdescs

[David Hildenbrand]

On 14.03.25 14:36, Matthew Wilcox (Oracle) wrote:
> Separate the handling of accounted folios and GFP_ACCOUNT pages for easier
> to understand code.  For more detail, see
> https://lore.kernel.org/linux-mm/Z9LwTOudOlCGny3f@casper.infradead.org/

Nothing jumped at me, looking good.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/5] mm: Separate folio_split_memcg_refs() from split_page_memcg()

[Zi Yan]

On 14 Mar 2025, at 17:49, David Hildenbrand wrote:

> On 14.03.25 14:36, Matthew Wilcox (Oracle) wrote:
>> Folios always use memcg_data to refer to the mem_cgroup while pages
>> allocated with GFP_ACCOUNT have a pointer to the obj_cgroup.  Since the
>> caller already knows what it has, split the function into two and then
>> we don't need to check.
>>
>> Move the assignment of split folio memcg_data to the point where we set
>> up the other parts of the new folio.  That leaves folio_split_memcg_refs()
>> just handling the memcg accounting.
>>
>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
>> Acked-by: Zi Yan <ziy@nvidia.com>
>> ---
>>   include/linux/memcontrol.h |  7 +++++++
>>   mm/huge_memory.c           | 16 ++++------------
>>   mm/memcontrol.c            | 17 +++++++++++++----
>>   3 files changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 57664e2a8fb7..d090089c5497 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -1039,6 +1039,8 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
>>   }
>>    void split_page_memcg(struct page *head, int old_order, int new_order);
>> +void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
>> +		unsigned new_order);
>>    static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>>   {
>> @@ -1463,6 +1465,11 @@ static inline void split_page_memcg(struct page *head, int old_order, int new_or
>>   {
>>   }
>>  +static inline void folio_split_memcg_refs(struct folio *folio,
>> +		unsigned old_order, unsigned new_order)
>> +{
>> +}
>> +
>>   static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>>   {
>>   	return 0;
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index 14b1963898a7..3e5ecc8f3d13 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -3394,6 +3394,9 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>   			folio_set_young(new_folio);
>>   		if (folio_test_idle(folio))
>>   			folio_set_idle(new_folio);
>> +#ifdef CONFIG_MEMCG
>> +		new_folio->memcg_data = folio->memcg_data;
>> +#endif
>>    		folio_xchg_last_cpupid(new_folio, folio_last_cpupid(folio));
>>   	}
>> @@ -3525,18 +3528,7 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>   			}
>>   		}
>>  -		/*
>> -		 * Reset any memcg data overlay in the tail pages.
>> -		 * folio_nr_pages() is unreliable until prep_compound_page()
>> -		 * was called again.
>> -		 */
>> -#ifdef NR_PAGES_IN_LARGE_FOLIO
>> -		folio->_nr_pages = 0;
>> -#endif
>
>
> I remember that we could trigger a warning without that, but I don't immediately find where that warning was. IIRC, if we'd split to order-0, page[1] would have indicated that it had a memcg set, and something bailed out.
>
> Maybe Zi Yan recalls where that check fired.

The error I encountered is different. When I rebase my folio_split()
on top of David’s mapcount patchset, my original patch used folio_nr_pages()
after memcg split. Since memcg overlays with _nr_pages, when splitting
to order-0, folio->_nr_page is overwritten with memcg_data, causing
folio_nr_pages() to return a bogus value. With Matthew’s this patch,
memcg_data of page[1] is written inside __split_folio_to_order(),
so in theory __split_folio_to_order() can call folio_nr_pages() like
my original patch.

For folio->_nr_pages = 0, I suppose it is trying to suppress any
page[1]->memcg_data != NULL check in the following code. But I could
not find any.

>
> In any case, if that warning no longer fires this is a very nice cleanup!

Yeah, if we see any warning on memcg later, we know how to fix it. :)


Best Regards,
Yan, Zi

Re: [PATCH v2 1/5] mm: Separate folio_split_memcg_refs() from split_page_memcg()

[David Hildenbrand]

On 15.03.25 00:15, Zi Yan wrote:
> On 14 Mar 2025, at 17:49, David Hildenbrand wrote:
> 
>> On 14.03.25 14:36, Matthew Wilcox (Oracle) wrote:
>>> Folios always use memcg_data to refer to the mem_cgroup while pages
>>> allocated with GFP_ACCOUNT have a pointer to the obj_cgroup.  Since the
>>> caller already knows what it has, split the function into two and then
>>> we don't need to check.
>>>
>>> Move the assignment of split folio memcg_data to the point where we set
>>> up the other parts of the new folio.  That leaves folio_split_memcg_refs()
>>> just handling the memcg accounting.
>>>
>>> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
>>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>>> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
>>> Acked-by: Zi Yan <ziy@nvidia.com>
>>> ---
>>>    include/linux/memcontrol.h |  7 +++++++
>>>    mm/huge_memory.c           | 16 ++++------------
>>>    mm/memcontrol.c            | 17 +++++++++++++----
>>>    3 files changed, 24 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>>> index 57664e2a8fb7..d090089c5497 100644
>>> --- a/include/linux/memcontrol.h
>>> +++ b/include/linux/memcontrol.h
>>> @@ -1039,6 +1039,8 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
>>>    }
>>>     void split_page_memcg(struct page *head, int old_order, int new_order);
>>> +void folio_split_memcg_refs(struct folio *folio, unsigned old_order,
>>> +		unsigned new_order);
>>>     static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>>>    {
>>> @@ -1463,6 +1465,11 @@ static inline void split_page_memcg(struct page *head, int old_order, int new_or
>>>    {
>>>    }
>>>   +static inline void folio_split_memcg_refs(struct folio *folio,
>>> +		unsigned old_order, unsigned new_order)
>>> +{
>>> +}
>>> +
>>>    static inline u64 cgroup_id_from_mm(struct mm_struct *mm)
>>>    {
>>>    	return 0;
>>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>>> index 14b1963898a7..3e5ecc8f3d13 100644
>>> --- a/mm/huge_memory.c
>>> +++ b/mm/huge_memory.c
>>> @@ -3394,6 +3394,9 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>>>    			folio_set_young(new_folio);
>>>    		if (folio_test_idle(folio))
>>>    			folio_set_idle(new_folio);
>>> +#ifdef CONFIG_MEMCG
>>> +		new_folio->memcg_data = folio->memcg_data;
>>> +#endif
>>>     		folio_xchg_last_cpupid(new_folio, folio_last_cpupid(folio));
>>>    	}
>>> @@ -3525,18 +3528,7 @@ static int __split_unmapped_folio(struct folio *folio, int new_order,
>>>    			}
>>>    		}
>>>   -		/*
>>> -		 * Reset any memcg data overlay in the tail pages.
>>> -		 * folio_nr_pages() is unreliable until prep_compound_page()
>>> -		 * was called again.
>>> -		 */
>>> -#ifdef NR_PAGES_IN_LARGE_FOLIO
>>> -		folio->_nr_pages = 0;
>>> -#endif
>>
>>
>> I remember that we could trigger a warning without that, but I don't immediately find where that warning was. IIRC, if we'd split to order-0, page[1] would have indicated that it had a memcg set, and something bailed out.
>>
>> Maybe Zi Yan recalls where that check fired.
> 
> The error I encountered is different. When I rebase my folio_split()
> on top of David’s mapcount patchset, my original patch used folio_nr_pages()
> after memcg split. Since memcg overlays with _nr_pages, when splitting
> to order-0, folio->_nr_page is overwritten with memcg_data, causing
> folio_nr_pages() to return a bogus value. With Matthew’s this patch,
> memcg_data of page[1] is written inside __split_folio_to_order(),
> so in theory __split_folio_to_order() can call folio_nr_pages() like
> my original patch.

Ah, my memory comes back. Right, I fixed that by passing "old_order" 
instead when rebasing on your series and ...

> 
> For folio->_nr_pages = 0, I suppose it is trying to suppress any
> page[1]->memcg_data != NULL check in the following code. But I could
> not find any.

... that was likely an issue I ran into before rebasing on your code, 
where I had to effectively clear page[1]->memcg_data.

So all good, clearing folio->_nr_pages can be dropped.

-- 
Cheers,

David / dhildenb

Re: [PATCH v2 1/5] mm: Separate folio_split_memcg_refs() from split_page_memcg()

[Roman Gushchin]

On Fri, Mar 14, 2025 at 01:36:11PM +0000, Matthew Wilcox wrote:
> Folios always use memcg_data to refer to the mem_cgroup while pages
> allocated with GFP_ACCOUNT have a pointer to the obj_cgroup.  Since the
> caller already knows what it has, split the function into two and then
> we don't need to check.
> 
> Move the assignment of split folio memcg_data to the point where we set
> up the other parts of the new folio.  That leaves folio_split_memcg_refs()
> just handling the memcg accounting.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Acked-by: Zi Yan <ziy@nvidia.com>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Re: [PATCH v2 2/5] mm: Simplify split_page_memcg()

[Roman Gushchin]

On Fri, Mar 14, 2025 at 01:36:12PM +0000, Matthew Wilcox wrote:
> The last argument to split_page_memcg() is now always 0, so remove it,
> effectively reverting commit b8791381d7ed.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
> Acked-by: Zi Yan <ziy@nvidia.com>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Re: [PATCH v2 3/5] mm: Remove references to folio in split_page_memcg()

[Roman Gushchin]

On Fri, Mar 14, 2025 at 01:36:13PM +0000, Matthew Wilcox wrote:
> We know that the passed in page is not part of a folio (it's a plain
> page allocated with GFP_ACCOUNT), so we should get rid of the misleading
> references to folios.
> 
> Introduce page_objcg() and page_set_objcg() helpers to make things more
> clear.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!

Re: [PATCH v2 4/5] mm: Simplify folio_memcg_charged()

[Roman Gushchin]

On Fri, Mar 14, 2025 at 01:36:14PM +0000, Matthew Wilcox wrote:
> There's no need to check which kind of pointer is in the memcg_data
> field, all we actually care about is whether it's zero or not.
> Saves 70 bytes in workingset_activation() with the Debian config.

:)

> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Re: [PATCH v2 5/5] mm: Remove references to folio in __memcg_kmem_uncharge_page()

[Roman Gushchin]

On Fri, Mar 14, 2025 at 01:36:15PM +0000, Matthew Wilcox wrote:
> This use of folios is misleading because these pages are not part of
> a folio.  Remove an unnecessary call to page_folio(), saving 58 bytes
> of text in a Debian kernel build.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Roman Gushchin <roman.gushchin@linux.dev>

Thanks!