linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
@ 2025-04-03  5:33 Shakeel Butt
  2025-04-03  7:45 ` Michal Hocko
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Shakeel Butt @ 2025-04-03  5:33 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Uladzislau Rezki, Johannes Weiner, Michal Hocko, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

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 <shakeel.butt@linux.dev>
---
 mm/vmalloc.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index 3ed720a787ec..cdae76994488 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -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
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03  5:33 [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates Shakeel Butt
@ 2025-04-03  7:45 ` Michal Hocko
  2025-04-03 11:17 ` Uladzislau Rezki
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Michal Hocko @ 2025-04-03  7:45 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Andrew Morton, Uladzislau Rezki, Johannes Weiner, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

On Wed 02-04-25 22:33:26, Shakeel Butt wrote:
> 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.

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

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/vmalloc.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3ed720a787ec..cdae76994488 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -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
> -- 
> 2.47.1

-- 
Michal Hocko
SUSE Labs


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03  5:33 [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates Shakeel Butt
  2025-04-03  7:45 ` Michal Hocko
@ 2025-04-03 11:17 ` Uladzislau Rezki
  2025-04-03 18:20   ` Shakeel Butt
  2025-04-03 16:47 ` Johannes Weiner
  2025-04-22 15:17 ` Yosry Ahmed
  3 siblings, 1 reply; 10+ messages in thread
From: Uladzislau Rezki @ 2025-04-03 11:17 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Andrew Morton, Uladzislau Rezki, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, linux-mm, cgroups, linux-kernel,
	Meta kernel team

On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> 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 <shakeel.butt@linux.dev>
> ---
>  mm/vmalloc.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3ed720a787ec..cdae76994488 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -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) <urezki@gmail.com>

--
Uladzislau Rezki


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03  5:33 [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates Shakeel Butt
  2025-04-03  7:45 ` Michal Hocko
  2025-04-03 11:17 ` Uladzislau Rezki
@ 2025-04-03 16:47 ` Johannes Weiner
  2025-04-03 18:23   ` Shakeel Butt
  2025-04-22 15:17 ` Yosry Ahmed
  3 siblings, 1 reply; 10+ messages in thread
From: Johannes Weiner @ 2025-04-03 16:47 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Andrew Morton, Uladzislau Rezki, Michal Hocko, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> 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 <shakeel.butt@linux.dev>

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?


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03 11:17 ` Uladzislau Rezki
@ 2025-04-03 18:20   ` Shakeel Butt
  2025-04-04 10:34     ` Uladzislau Rezki
  0 siblings, 1 reply; 10+ messages in thread
From: Shakeel Butt @ 2025-04-03 18:20 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

On Thu, Apr 03, 2025 at 01:17:22PM +0200, Uladzislau Rezki wrote:
> On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> > 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 <shakeel.butt@linux.dev>
> > ---
> >  mm/vmalloc.c | 13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > index 3ed720a787ec..cdae76994488 100644
> > --- a/mm/vmalloc.c
> > +++ b/mm/vmalloc.c
> > @@ -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?
> 

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.


> Yes, the comment is clear, but git blame/log takes time.
> 
> Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Thanks.

> 
> --
> Uladzislau Rezki


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03 16:47 ` Johannes Weiner
@ 2025-04-03 18:23   ` Shakeel Butt
  0 siblings, 0 replies; 10+ messages in thread
From: Shakeel Butt @ 2025-04-03 18:23 UTC (permalink / raw)
  To: Johannes Weiner
  Cc: Andrew Morton, Uladzislau Rezki, Michal Hocko, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

On Thu, Apr 03, 2025 at 12:47:41PM -0400, Johannes Weiner wrote:
> On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> > 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 <shakeel.butt@linux.dev>
> 
> 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).


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03 18:20   ` Shakeel Butt
@ 2025-04-04 10:34     ` Uladzislau Rezki
  2025-04-04 17:44       ` Shakeel Butt
  0 siblings, 1 reply; 10+ messages in thread
From: Uladzislau Rezki @ 2025-04-04 10:34 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Uladzislau Rezki, Andrew Morton, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, linux-mm, cgroups, linux-kernel,
	Meta kernel team

On Thu, Apr 03, 2025 at 11:20:18AM -0700, Shakeel Butt wrote:
> On Thu, Apr 03, 2025 at 01:17:22PM +0200, Uladzislau Rezki wrote:
> > On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> > > 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 <shakeel.butt@linux.dev>
> > > ---
> > >  mm/vmalloc.c | 13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > index 3ed720a787ec..cdae76994488 100644
> > > --- a/mm/vmalloc.c
> > > +++ b/mm/vmalloc.c
> > > @@ -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?
> > 
> 
> 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-04 10:34     ` Uladzislau Rezki
@ 2025-04-04 17:44       ` Shakeel Butt
  2025-04-07  9:59         ` Uladzislau Rezki
  0 siblings, 1 reply; 10+ messages in thread
From: Shakeel Butt @ 2025-04-04 17:44 UTC (permalink / raw)
  To: Uladzislau Rezki
  Cc: Andrew Morton, Johannes Weiner, Michal Hocko, Roman Gushchin,
	Muchun Song, linux-mm, cgroups, linux-kernel, Meta kernel team

On Fri, Apr 04, 2025 at 12:34:33PM +0200, Uladzislau Rezki wrote:
> On Thu, Apr 03, 2025 at 11:20:18AM -0700, Shakeel Butt wrote:
> > On Thu, Apr 03, 2025 at 01:17:22PM +0200, Uladzislau Rezki wrote:
> > > On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> > > > 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 <shakeel.butt@linux.dev>
> > > > ---
> > > >  mm/vmalloc.c | 13 +++++--------
> > > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > index 3ed720a787ec..cdae76994488 100644
> > > > --- a/mm/vmalloc.c
> > > > +++ b/mm/vmalloc.c
> > > > @@ -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?
> > > 
> > 
> > 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?

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


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

Add comment

Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/vmalloc.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/mm/vmalloc.c b/mm/vmalloc.c
index cdae76994488..bcc90d4357e4 100644
--- a/mm/vmalloc.c
+++ b/mm/vmalloc.c
@@ -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);
-- 
2.47.1



^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-04 17:44       ` Shakeel Butt
@ 2025-04-07  9:59         ` Uladzislau Rezki
  0 siblings, 0 replies; 10+ messages in thread
From: Uladzislau Rezki @ 2025-04-07  9:59 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Uladzislau Rezki, Andrew Morton, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, linux-mm, cgroups, linux-kernel,
	Meta kernel team

On Fri, Apr 04, 2025 at 10:44:05AM -0700, Shakeel Butt wrote:
> On Fri, Apr 04, 2025 at 12:34:33PM +0200, Uladzislau Rezki wrote:
> > On Thu, Apr 03, 2025 at 11:20:18AM -0700, Shakeel Butt wrote:
> > > On Thu, Apr 03, 2025 at 01:17:22PM +0200, Uladzislau Rezki wrote:
> > > > On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> > > > > 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 <shakeel.butt@linux.dev>
> > > > > ---
> > > > >  mm/vmalloc.c | 13 +++++--------
> > > > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> > > > > index 3ed720a787ec..cdae76994488 100644
> > > > > --- a/mm/vmalloc.c
> > > > > +++ b/mm/vmalloc.c
> > > > > @@ -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?
> > > > 
> > > 
> > > 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?
> 
> Ack. Andrew, please squash the following into the patch.
> 
> 
> From 982971062e6bd04feabf4f6a745469cb9bddef03 Mon Sep 17 00:00:00 2001
> From: Shakeel Butt <shakeel.butt@linux.dev>
> Date: Fri, 4 Apr 2025 10:41:52 -0700
> Subject: [PATCH] memcg : simplify MEMCG_VMALLOC updates - fix
> 
> Add comment
> 
> Signed-off-by: Shakeel Butt <shakeel.butt@linux.dev>
> ---
>  mm/vmalloc.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index cdae76994488..bcc90d4357e4 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -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);
> -- 
> 2.47.1
> 
Reviewed-by: Uladzislau Rezki (Sony) <urezki@gmail.com>

Thank you!

--
Uladzislau Rezki


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates
  2025-04-03  5:33 [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates Shakeel Butt
                   ` (2 preceding siblings ...)
  2025-04-03 16:47 ` Johannes Weiner
@ 2025-04-22 15:17 ` Yosry Ahmed
  3 siblings, 0 replies; 10+ messages in thread
From: Yosry Ahmed @ 2025-04-22 15:17 UTC (permalink / raw)
  To: Shakeel Butt
  Cc: Andrew Morton, Uladzislau Rezki, Johannes Weiner, Michal Hocko,
	Roman Gushchin, Muchun Song, linux-mm, cgroups, linux-kernel,
	Meta kernel team

On Wed, Apr 02, 2025 at 10:33:26PM -0700, Shakeel Butt wrote:
> 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 <shakeel.butt@linux.dev>
> ---
>  mm/vmalloc.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/vmalloc.c b/mm/vmalloc.c
> index 3ed720a787ec..cdae76994488 100644
> --- a/mm/vmalloc.c
> +++ b/mm/vmalloc.c
> @@ -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);

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 <yosry.ahmed@linux.dev>


>  		/*
>  		 * 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
> -- 
> 2.47.1
> 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2025-04-22 15:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-04-03  5:33 [PATCH] memcg: vmalloc: simplify MEMCG_VMALLOC updates Shakeel Butt
2025-04-03  7:45 ` Michal Hocko
2025-04-03 11:17 ` Uladzislau Rezki
2025-04-03 18:20   ` Shakeel Butt
2025-04-04 10:34     ` Uladzislau Rezki
2025-04-04 17:44       ` Shakeel Butt
2025-04-07  9:59         ` Uladzislau Rezki
2025-04-03 16:47 ` Johannes Weiner
2025-04-03 18:23   ` Shakeel Butt
2025-04-22 15:17 ` Yosry Ahmed

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).