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

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

[Matthew Wilcox (Oracle)]

As promised in
https://lore.kernel.org/linux-mm/Z9LwTOudOlCGny3f@casper.infradead.org/
here are some minor cleanups & prep.  The first depends on next-20250313
and I'd like David & Zi to look closely at that since that code all
moved around recently.  I think it's good, but I've been wrong before.

Matthew Wilcox (Oracle) (5):
  mm: Separate folio_split_memcg() 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 1/5] mm: Separate folio_split_memcg() 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()
just handling the memcg accounting.

Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
---
 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..2b4246dc4284 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(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(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..514db6a5eee7 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(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..102109d0ee87 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(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 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>
---
 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 2b4246dc4284..381c5b565dca 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(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 102109d0ee87..07f0b42fee05 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(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 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>
---
 mm/memcontrol.c | 28 ++++++++++++++++++++++------
 1 file changed, 22 insertions(+), 6 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 07f0b42fee05..93102319409a 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 *first)
+{
+	unsigned long memcg_data = first->memcg_data;
+
+	if (!memcg_data)
+		return NULL;
+
+	VM_BUG_ON_PAGE((memcg_data & OBJEXTS_FLAGS_MASK) != MEMCG_DATA_KMEM,
+			first);
+	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;
 		}
 	}
@@ -3091,16 +3107,16 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
  */
 void split_page_memcg(struct page *first, unsigned order)
 {
-	struct folio *folio = page_folio(first);
+	struct obj_cgroup *objcg = page_objcg(first);
 	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(&first[i], objcg);
 
-	obj_cgroup_get_many(__folio_objcg(folio), nr - 1);
+	obj_cgroup_get_many(objcg, nr - 1);
 }
 
 void folio_split_memcg(struct folio *folio, unsigned old_order,
-- 
2.47.2

[PATCH 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>
---
 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 381c5b565dca..28c251e34cdf 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 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>
---
 mm/memcontrol.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 93102319409a..d12afda6d58c 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 1/5] mm: Separate folio_split_memcg() from split_page_memcg()

[Johannes Weiner]

On Thu, Mar 13, 2025 at 02:58:50PM +0000, 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()
> just handling the memcg accounting.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Makes sense to me.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

One small nit:

> @@ -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(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);
>  }

Since this doesn't do the full memcg split anymore, and there is still
a split_page_memcg() which does, it would be good to reflect this in
the name. E.g. folio_split_memcg_refs().

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

[Johannes Weiner]

On Thu, Mar 13, 2025 at 02:58:51PM +0000, Matthew Wilcox (Oracle) 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>

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

[Johannes Weiner]

On Thu, Mar 13, 2025 at 02:58:52PM +0000, Matthew Wilcox (Oracle) wrote:
> @@ -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 *first)

"first" is odd for that function. "page"?

> @@ -3091,16 +3107,16 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
>   */
>  void split_page_memcg(struct page *first, unsigned order)
>  {
> -	struct folio *folio = page_folio(first);
> +	struct obj_cgroup *objcg = page_objcg(first);
>  	unsigned int i, nr = 1 << order;
>  
> -	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> +	if (!objcg)
>  		return;

mem_cgroup_disabled() is a jump label, so a bit cheaper than the
branch. Can you keep that and do page_objcg() after it?

>  	for (i = 1; i < nr; i++)
> -		folio_page(folio, i)->memcg_data = folio->memcg_data;
> +		page_set_objcg(&first[i], objcg);
>  
> -	obj_cgroup_get_many(__folio_objcg(folio), nr - 1);
> +	obj_cgroup_get_many(objcg, nr - 1);
>  }

This is much nicer indeed. Thanks for doing this.

With the above nits considered,

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

[Johannes Weiner]

On Thu, Mar 13, 2025 at 02:58:53PM +0000, Matthew Wilcox (Oracle) 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>
> ---
>  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 381c5b565dca..28c251e34cdf 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;

I suppose we lose a few DEBUG_VM asserts, but no doubt this function
is unnecessarily complicated.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

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

[Johannes Weiner]

On Thu, Mar 13, 2025 at 02:58:54PM +0000, Matthew Wilcox (Oracle) 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>

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

[Matthew Wilcox]

On Thu, Mar 13, 2025 at 12:01:14PM -0400, Johannes Weiner wrote:
> On Thu, Mar 13, 2025 at 02:58:52PM +0000, Matthew Wilcox (Oracle) wrote:
> > @@ -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 *first)
> 
> "first" is odd for that function. "page"?

I'm trying to indicate that it's the first page of the allocation (head
page is wrong because it's not a compound allocation).  But I can just
call it 'page'.  We do that in enough places ...

> > @@ -3091,16 +3107,16 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
> >   */
> >  void split_page_memcg(struct page *first, unsigned order)
> >  {
> > -	struct folio *folio = page_folio(first);
> > +	struct obj_cgroup *objcg = page_objcg(first);
> >  	unsigned int i, nr = 1 << order;
> >  
> > -	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> > +	if (!objcg)
> >  		return;
> 
> mem_cgroup_disabled() is a jump label, so a bit cheaper than the
> branch. Can you keep that and do page_objcg() after it?

Sure, I can do that.  I didn't think it was really worthwhile given
how common splitting is (very rare) and we've already incurred the
function call overhead.

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

[Matthew Wilcox]

On Thu, Mar 13, 2025 at 12:03:48PM -0400, Johannes Weiner wrote:
> >  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;
> 
> I suppose we lose a few DEBUG_VM asserts, but no doubt this function
> is unnecessarily complicated.

I think we lose two -- folio_test_slab() and
memcg_data & MEMCG_DATA_OBJEXTS, both of which seem pretty low value.
Also, one could argue that asking "is this memory charged" should
return true if we ask it about a slab.  So yeah, I didn't think they
were worth preserving.

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 04:15:38PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 13, 2025 at 12:03:48PM -0400, Johannes Weiner wrote:
> > >  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;
> > 
> > I suppose we lose a few DEBUG_VM asserts, but no doubt this function
> > is unnecessarily complicated.
> 
> I think we lose two -- folio_test_slab() and
> memcg_data & MEMCG_DATA_OBJEXTS, both of which seem pretty low value.
> Also, one could argue that asking "is this memory charged" should
> return true if we ask it about a slab.  So yeah, I didn't think they
> were worth preserving.
> 

With separete struct slab, no one should be calling folio_memcg_charged()
with it, so I think these asserts were unnecessary.

> 

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 02:58:50PM +0000, 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()
> just handling the memcg accounting.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 02:58:51PM +0000, Matthew Wilcox (Oracle) 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: Shakeel Butt <shakeel.butt@linux.dev>

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 02:58:52PM +0000, Matthew Wilcox (Oracle) 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: Shakeel Butt <shakeel.butt@linux.dev>

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 02:58:53PM +0000, Matthew Wilcox (Oracle) 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: Shakeel Butt <shakeel.butt@linux.dev>

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

[Shakeel Butt]

On Thu, Mar 13, 2025 at 02:58:54PM +0000, Matthew Wilcox (Oracle) 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: Shakeel Butt <shakeel.butt@linux.dev>

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

[Zi Yan]

On 13 Mar 2025, at 10:58, 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()
> just handling the memcg accounting.
>
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/memcontrol.h |  7 +++++++
>  mm/huge_memory.c           | 16 ++++------------
>  mm/memcontrol.c            | 17 +++++++++++++----
>  3 files changed, 24 insertions(+), 16 deletions(-)
>
Acked-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

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

[Zi Yan]

On 13 Mar 2025, at 10:58, Matthew Wilcox (Oracle) 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>
> ---
>  include/linux/memcontrol.h |  4 ++--
>  mm/memcontrol.c            | 15 +++++++--------
>  mm/page_alloc.c            |  4 ++--
>  3 files changed, 11 insertions(+), 12 deletions(-)
Acked-by: Zi Yan <ziy@nvidia.com>

Best Regards,
Yan, Zi

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

[Matthew Wilcox]

On Thu, Mar 13, 2025 at 12:01:14PM -0400, Johannes Weiner wrote:
> >  void split_page_memcg(struct page *first, unsigned order)
> >  {
> > -	struct folio *folio = page_folio(first);
> > +	struct obj_cgroup *objcg = page_objcg(first);
> >  	unsigned int i, nr = 1 << order;
> >  
> > -	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> > +	if (!objcg)
> >  		return;
> 
> mem_cgroup_disabled() is a jump label, so a bit cheaper than the
> branch. Can you keep that and do page_objcg() after it?

I think this might be better?

-static struct obj_cgroup *page_objcg(const struct page *first)
+static struct obj_cgroup *page_objcg(const struct page *page)
 {
-       unsigned long memcg_data = first->memcg_data;
+       unsigned long memcg_data = page->memcg_data;

-       if (!memcg_data)
+       if (mem_cgroup_disabled() || !memcg_data)
                return NULL;

I'd expect the compiler to inline page_objcg() into its caller and
optimise away the NULL test if mem_cgroup_disabled().

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

[Johannes Weiner]

On Thu, Mar 13, 2025 at 05:59:15PM +0000, Matthew Wilcox wrote:
> On Thu, Mar 13, 2025 at 12:01:14PM -0400, Johannes Weiner wrote:
> > >  void split_page_memcg(struct page *first, unsigned order)
> > >  {
> > > -	struct folio *folio = page_folio(first);
> > > +	struct obj_cgroup *objcg = page_objcg(first);
> > >  	unsigned int i, nr = 1 << order;
> > >  
> > > -	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> > > +	if (!objcg)
> > >  		return;
> > 
> > mem_cgroup_disabled() is a jump label, so a bit cheaper than the
> > branch. Can you keep that and do page_objcg() after it?
> 
> I think this might be better?
> 
> -static struct obj_cgroup *page_objcg(const struct page *first)
> +static struct obj_cgroup *page_objcg(const struct page *page)
>  {
> -       unsigned long memcg_data = first->memcg_data;
> +       unsigned long memcg_data = page->memcg_data;
> 
> -       if (!memcg_data)
> +       if (mem_cgroup_disabled() || !memcg_data)
>                 return NULL;
> 
> I'd expect the compiler to inline page_objcg() into its caller and
> optimise away the NULL test if mem_cgroup_disabled().

That sounds good to me. Thanks!