memcg: vmalloc: simplify MEMCG_VMALLOC updates

PATCH In review Shakeel Butt 5 participants 10 messages Apr 3, 2025 – Apr 22, 2025

Subsystems (3)

Memory management cgroup vmalloc

lore

Share & tools

lore

Shakeel Butt

author

2025-04-03 05:33 UTC #0 lore raw

PATCH 1/1 memcg: vmalloc: simplify MEMCG_VMALLOC updates

The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

mm/vmalloc.c +5 -8

index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 	for (i = 0; i < vm->nr_pages; i++) {
 		struct page *page = vm->pages[i];
 
 		BUG_ON(!page);
-		if (!(vm->flags & VM_MAP_PUT_PAGES))
-			mod_memcg_page_state(page, MEMCG_VMALLOC, -1);
 		/*
 		 * High-order allocs for huge vmallocs are split, so
 		 * can be freed as an array of order-0 allocations

@@ -3671,12 +3671,9 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
-	if (gfp_mask & __GFP_ACCOUNT) {
-		int i;
-
-		for (i = 0; i < area->nr_pages; i++)
-			mod_memcg_page_state(area->pages[i], MEMCG_VMALLOC, 1);
-	}
+	if (gfp_mask & __GFP_ACCOUNT && area->nr_pages)
+		mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
+				     area->nr_pages);
 
 	/*
 	 * If not enough pages were obtained to accomplish an

Signature · 1 line

2.47.1

Michal Hocko

Acked-by on v1 1/1

2025-04-03 07:45 UTC #1 lore raw

Quoted 6 lines · Shakeel Butt

The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.

I do not rememeber why this was done on page by page but I suspect originally we could have mixed more memcgs on one vm.

The patch makes sense.

Quoted 1 line

Signed-off-by Shakeel Butt <[email protected]>

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 41 lines

---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

mm/vmalloc.c +5 -8

index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 	for (i = 0; i < vm->nr_pages; i++) {
 		struct page *page = vm->pages[i];
 
 		BUG_ON(!page);
-		if (!(vm->flags & VM_MAP_PUT_PAGES))
-			mod_memcg_page_state(page, MEMCG_VMALLOC, -1);
 		/*
 		 * High-order allocs for huge vmallocs are split, so
 		 * can be freed as an array of order-0 allocations

@@ -3671,12 +3671,9 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
-	if (gfp_mask & __GFP_ACCOUNT) {
-		int i;
-
-		for (i = 0; i < area->nr_pages; i++)
-			mod_memcg_page_state(area->pages[i], MEMCG_VMALLOC, 1);
-	}
+	if (gfp_mask & __GFP_ACCOUNT && area->nr_pages)
+		mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
+				     area->nr_pages);
 
 	/*
 	 * If not enough pages were obtained to accomplish an

Signature · 1 line

2.47.1

Signature · 2 lines

Michal Hocko
SUSE Labs

Uladzislau Rezki

maintainer Reviewed-by on v1 1/1

2025-04-03 11:17 UTC #2 lore raw

In reply to Shakeel Butt

Quoted 17 lines · Shakeel Butt

The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644

mm/vmalloc.c void vfree(const void *addr) · line 3375

Quoted 6 lines

mm/vmalloc.c +2 -0

@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);

Could you please add a comment stating that the first page should be modified?

Yes, the comment is clear, but git blame/log takes time.

Reviewed-by Uladzislau Rezki (Sony) <[email protected]>

--
Uladzislau Rezki

Shakeel Butt

author

2025-04-03 18:20 UTC #3 lore raw

Quoted 19 lines · Uladzislau Rezki

Quoted 18 lines · Shakeel Butt
The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 

Quoted 8 lines

mm/vmalloc.c void vfree(const void *addr) · line 3377
Quoted 5 lines
mm/vmalloc.c +2 -0
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 
Could you please add a comment stating that the first page should be modified?

Sorry, what do you mean by first page should be modified? mod_memcg_page_state() will not modify the page but extract memcg from it and modify its vmalloc stat.

Quoted 3 lines

Yes, the comment is clear, but git blame/log takes time.
Reviewed-by Uladzislau Rezki (Sony) <[email protected]>

Thanks.

Quoted 3 lines

--
Uladzislau Rezki

Uladzislau Rezki

maintainer

2025-04-04 10:34 UTC #4 lore raw

Quoted 29 lines · Shakeel Butt

Quoted 19 lines · Uladzislau Rezki
Quoted 18 lines · Shakeel Butt
The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
Quoted 8 lines
mm/vmalloc.c void vfree(const void *addr) · line 3377
Quoted 5 lines
mm/vmalloc.c +2 -0
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 
Could you please add a comment stating that the first page should be modified?

Quoted 4 lines

Sorry, what do you mean by first page should be modified? mod_memcg_page_state() will not modify the page but extract memcg from it and modify its vmalloc stat.

I meant what you wrote in the commit message. A mod_memcg_page_state() can be invoked only on a first page within a mapped range, because the rest is anyway is associated with the same mem_cgroup struct.

Just add a comment that we do not need to check all pages. Can you add it?

--
Uladzislau Rezki

Shakeel Butt

author

2025-04-04 17:44 UTC #5 lore raw

Quoted 34 lines · Uladzislau Rezki

Quoted 29 lines · Shakeel Butt
Quoted 19 lines · Uladzislau Rezki
Quoted 18 lines · Shakeel Butt
The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
Quoted 8 lines
mm/vmalloc.c void vfree(const void *addr) · line 3375
Quoted 5 lines
mm/vmalloc.c +2 -0
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 
Could you please add a comment stating that the first page should be modified?
Quoted 4 lines
Sorry, what do you mean by first page should be modified? mod_memcg_page_state() will not modify the page but extract memcg from it and modify its vmalloc stat.

Quoted 5 lines

I meant what you wrote in the commit message. A mod_memcg_page_state() can be invoked only on a first page within a mapped range, because the rest is anyway is associated with the same mem_cgroup struct.
Just add a comment that we do not need to check all pages. Can you add it?

Ack. Andrew, please squash the following into the patch.

From 982971062e6bd04feabf4f6a745469cb9bddef03 Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Fri, 4 Apr 2025 10:41:52 -0700
Subject: [PATCH] memcg : simplify MEMCG_VMALLOC updates - fix

Add comment

Signed-off-by Shakeel Butt <[email protected]>

---
 mm/vmalloc.c | 2 ++
 1 file changed, 2 insertions(+)

mm/vmalloc.c +2 -0

index cdae76994488..bcc90d4357e4 100644
@@ -3370,6 +3370,7 @@ void vfree(const void *addr)
 
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	/* All pages of vm should be charged to same memcg, so use first one. */
 	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
 		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 	for (i = 0; i < vm->nr_pages; i++) {

@@ -3671,6 +3672,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
+	/* All pages of vm should be charged to same memcg, so use first one. */
 	if (gfp_mask & __GFP_ACCOUNT && area->nr_pages)
 		mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
 				     area->nr_pages);

Signature · 1 line

2.47.1

Uladzislau Rezki

maintainer Reviewed-by

2025-04-07 09:59 UTC #6 lore raw

Quoted 79 lines · Shakeel Butt

Quoted 34 lines · Uladzislau Rezki
Quoted 29 lines · Shakeel Butt
Quoted 19 lines · Uladzislau Rezki
Quoted 18 lines · Shakeel Butt
The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 
Quoted 8 lines
mm/vmalloc.c void vfree(const void *addr) · line 3375
Quoted 5 lines
mm/vmalloc.c +2 -0
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 
Could you please add a comment stating that the first page should be modified?
Quoted 4 lines
Sorry, what do you mean by first page should be modified? mod_memcg_page_state() will not modify the page but extract memcg from it and modify its vmalloc stat.
Quoted 5 lines
I meant what you wrote in the commit message. A mod_memcg_page_state() can be invoked only on a first page within a mapped range, because the rest is anyway is associated with the same mem_cgroup struct.
Just add a comment that we do not need to check all pages. Can you add it?
Ack. Andrew, please squash the following into the patch.
From 982971062e6bd04feabf4f6a745469cb9bddef03 Mon Sep 17 00:00:00 2001
From: Shakeel Butt <[email protected]>
Date: Fri, 4 Apr 2025 10:41:52 -0700
Subject: [PATCH] memcg : simplify MEMCG_VMALLOC updates - fix
Add comment
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 2 ++
 1 file changed, 2 insertions(+)
mm/vmalloc.c +2 -0
index cdae76994488..bcc90d4357e4 100644
@@ -3370,6 +3370,7 @@ void vfree(const void *addr)
 
 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	/* All pages of vm should be charged to same memcg, so use first one. */
 	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
 		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 	for (i = 0; i < vm->nr_pages; i++) {
@@ -3671,6 +3672,7 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
+	/* All pages of vm should be charged to same memcg, so use first one. */
 	if (gfp_mask & __GFP_ACCOUNT && area->nr_pages)
 		mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
 				     area->nr_pages);
Signature · 1 line
2.47.1

Reviewed-by Uladzislau Rezki (Sony) <[email protected]>

Thank you!

--
Uladzislau Rezki

Johannes Weiner

maintainer

2025-04-03 16:47 UTC #7 lore raw

In reply to Shakeel Butt

Quoted 8 lines · Shakeel Butt

The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>

It's definitely pointless to handle each page with the stat being per-cgroup only. But I do wonder why it's not a regular vmstat item.

There is no real reason it *should* be a private memcg stat, is there?

Shakeel Butt

author

2025-04-03 18:23 UTC #8 lore raw

Quoted 8 lines · Johannes Weiner

Quoted 7 lines · Shakeel Butt
The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.

Quoted 6 lines

Quoted 1 line
Signed-off-by Shakeel Butt <[email protected]>
It's definitely pointless to handle each page with the stat being per-cgroup only. But I do wonder why it's not a regular vmstat item.
There is no real reason it *should* be a private memcg stat, is there?

Yes, it can be a regular vmstat item (enum node_stat_item). However then we have go over each page as node_stat_item are per-node and vmalloc region can have pages from different nodes (I think but let me check).

Yosry Ahmed

Reviewed-by on v1 1/1

2025-04-22 15:17 UTC #9 lore raw

In reply to Shakeel Butt

Quoted 18 lines · Shakeel Butt

The vmalloc region can either be charged to a single memcg or none. At the moment kernel traverses all the pages backing the vmalloc region to update the MEMCG_VMALLOC stat. However there is no need to look at all the pages as all those pages will be charged to a single memcg or none. Simplify the MEMCG_VMALLOC update by just looking at the first page of the vmalloc region.
Signed-off-by Shakeel Butt <[email protected]>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
mm/vmalloc.c +0 -0
index 3ed720a787ec..cdae76994488 100644
@@ -3370,12 +3370,12 @@ void vfree(const void *addr)
 

mm/vmalloc.c void vfree(const void *addr) · line 3378

Quoted 10 lines

mm/vmalloc.c +2 -2

 	if (unlikely(vm->flags & VM_FLUSH_RESET_PERMS))
 		vm_reset_perms(vm);
+	if (vm->nr_pages && !(vm->flags & VM_MAP_PUT_PAGES))
+		mod_memcg_page_state(vm->pages[0], MEMCG_VMALLOC, -vm->nr_pages);
 	for (i = 0; i < vm->nr_pages; i++) {
 		struct page *page = vm->pages[i];
 
 		BUG_ON(!page);
-		if (!(vm->flags & VM_MAP_PUT_PAGES))
-			mod_memcg_page_state(page, MEMCG_VMALLOC, -1);

We can add a debug check here (and/or in the vmalloc path) to check that all pages are indeed charged to the same memcg.

Regardless, this change makes sense:

Reviewed-by Yosry Ahmed <[email protected]>

Quoted 23 lines

mm/vmalloc.c +3 -6

 		/*
 		 * High-order allocs for huge vmallocs are split, so
 		 * can be freed as an array of order-0 allocations

@@ -3671,12 +3671,9 @@ static void *__vmalloc_area_node(struct vm_struct *area, gfp_t gfp_mask,
 		node, page_order, nr_small_pages, area->pages);
 
 	atomic_long_add(area->nr_pages, &nr_vmalloc_pages);
-	if (gfp_mask & __GFP_ACCOUNT) {
-		int i;
-
-		for (i = 0; i < area->nr_pages; i++)
-			mod_memcg_page_state(area->pages[i], MEMCG_VMALLOC, 1);
-	}
+	if (gfp_mask & __GFP_ACCOUNT && area->nr_pages)
+		mod_memcg_page_state(area->pages[0], MEMCG_VMALLOC,
+				     area->nr_pages);
 
 	/*
 	 * If not enough pages were obtained to accomplish an

Signature · 1 line

2.47.1