[PATCH 1/2] mm: swap_cgroup: allocate swap_cgroup map using vcalloc()

[PATCH 1/2] mm: swap_cgroup: allocate swap_cgroup map using vcalloc()

[Roman Gushchin]

Currently swap_cgroup's map is constructed as a vmalloc()'s-based
array of pointers to individual struct pages. This brings an
unnecessary complexity into the code.

This patch turns the swap_cgroup's map into a single space
allocated by vcalloc().

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
---
 mm/swap_cgroup.c | 83 ++++++++----------------------------------------
 1 file changed, 13 insertions(+), 70 deletions(-)

diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
index da1278f0563b..18de498c84a4 100644
--- a/mm/swap_cgroup.c
+++ b/mm/swap_cgroup.c
@@ -6,17 +6,18 @@
 #include <linux/swapops.h> /* depends on mm.h include */
 
 static DEFINE_MUTEX(swap_cgroup_mutex);
+
+struct swap_cgroup {
+	unsigned short		id;
+};
+
 struct swap_cgroup_ctrl {
-	struct page **map;
-	unsigned long length;
+	struct swap_cgroup *map;
 	spinlock_t	lock;
 };
 
 static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
 
-struct swap_cgroup {
-	unsigned short		id;
-};
 #define SC_PER_PAGE	(PAGE_SIZE/sizeof(struct swap_cgroup))
 
 /*
@@ -33,44 +34,10 @@ struct swap_cgroup {
  * TODO: we can push these buffers out to HIGHMEM.
  */
 
-/*
- * allocate buffer for swap_cgroup.
- */
-static int swap_cgroup_prepare(int type)
-{
-	struct page *page;
-	struct swap_cgroup_ctrl *ctrl;
-	unsigned long idx, max;
-
-	ctrl = &swap_cgroup_ctrl[type];
-
-	for (idx = 0; idx < ctrl->length; idx++) {
-		page = alloc_page(GFP_KERNEL | __GFP_ZERO);
-		if (!page)
-			goto not_enough_page;
-		ctrl->map[idx] = page;
-
-		if (!(idx % SWAP_CLUSTER_MAX))
-			cond_resched();
-	}
-	return 0;
-not_enough_page:
-	max = idx;
-	for (idx = 0; idx < max; idx++)
-		__free_page(ctrl->map[idx]);
-
-	return -ENOMEM;
-}
-
 static struct swap_cgroup *__lookup_swap_cgroup(struct swap_cgroup_ctrl *ctrl,
 						pgoff_t offset)
 {
-	struct page *mappage;
-	struct swap_cgroup *sc;
-
-	mappage = ctrl->map[offset / SC_PER_PAGE];
-	sc = page_address(mappage);
-	return sc + offset % SC_PER_PAGE;
+	return &ctrl->map[offset];
 }
 
 static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
@@ -168,32 +135,20 @@ unsigned short lookup_swap_cgroup_id(swp_entry_t ent)
 
 int swap_cgroup_swapon(int type, unsigned long max_pages)
 {
-	void *array;
-	unsigned long length;
+	struct swap_cgroup *map;
 	struct swap_cgroup_ctrl *ctrl;
 
 	if (mem_cgroup_disabled())
 		return 0;
 
-	length = DIV_ROUND_UP(max_pages, SC_PER_PAGE);
-
-	array = vcalloc(length, sizeof(void *));
-	if (!array)
+	map = vcalloc(max_pages, sizeof(struct swap_cgroup));
+	if (!map)
 		goto nomem;
 
 	ctrl = &swap_cgroup_ctrl[type];
 	mutex_lock(&swap_cgroup_mutex);
-	ctrl->length = length;
-	ctrl->map = array;
+	ctrl->map = map;
 	spin_lock_init(&ctrl->lock);
-	if (swap_cgroup_prepare(type)) {
-		/* memory shortage */
-		ctrl->map = NULL;
-		ctrl->length = 0;
-		mutex_unlock(&swap_cgroup_mutex);
-		vfree(array);
-		goto nomem;
-	}
 	mutex_unlock(&swap_cgroup_mutex);
 
 	return 0;
@@ -205,8 +160,7 @@ int swap_cgroup_swapon(int type, unsigned long max_pages)
 
 void swap_cgroup_swapoff(int type)
 {
-	struct page **map;
-	unsigned long i, length;
+	struct swap_cgroup *map;
 	struct swap_cgroup_ctrl *ctrl;
 
 	if (mem_cgroup_disabled())
@@ -215,19 +169,8 @@ void swap_cgroup_swapoff(int type)
 	mutex_lock(&swap_cgroup_mutex);
 	ctrl = &swap_cgroup_ctrl[type];
 	map = ctrl->map;
-	length = ctrl->length;
 	ctrl->map = NULL;
-	ctrl->length = 0;
 	mutex_unlock(&swap_cgroup_mutex);
 
-	if (map) {
-		for (i = 0; i < length; i++) {
-			struct page *page = map[i];
-			if (page)
-				__free_page(page);
-			if (!(i % SWAP_CLUSTER_MAX))
-				cond_resched();
-		}
-		vfree(map);
-	}
+	kvfree(map);
 }
-- 
2.47.0.338.g60cca15819-goog

[PATCH 2/2] mm: swap_cgroup: get rid of __lookup_swap_cgroup()

[Roman Gushchin]

Because swap_cgroup map is now virtually contiguous,
swap_cgroup_record() can be simplified, which eliminates
a need to use __lookup_swap_cgroup().

Now as __lookup_swap_cgroup() is really trivial and is used only once,
it can be inlined.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
---
 mm/swap_cgroup.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/mm/swap_cgroup.c b/mm/swap_cgroup.c
index 18de498c84a4..0db907308c94 100644
--- a/mm/swap_cgroup.c
+++ b/mm/swap_cgroup.c
@@ -33,13 +33,6 @@ static struct swap_cgroup_ctrl swap_cgroup_ctrl[MAX_SWAPFILES];
  *
  * TODO: we can push these buffers out to HIGHMEM.
  */
-
-static struct swap_cgroup *__lookup_swap_cgroup(struct swap_cgroup_ctrl *ctrl,
-						pgoff_t offset)
-{
-	return &ctrl->map[offset];
-}
-
 static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
 					struct swap_cgroup_ctrl **ctrlp)
 {
@@ -49,7 +42,7 @@ static struct swap_cgroup *lookup_swap_cgroup(swp_entry_t ent,
 	ctrl = &swap_cgroup_ctrl[swp_type(ent)];
 	if (ctrlp)
 		*ctrlp = ctrl;
-	return __lookup_swap_cgroup(ctrl, offset);
+	return &ctrl->map[offset];
 }
 
 /**
@@ -104,16 +97,9 @@ unsigned short swap_cgroup_record(swp_entry_t ent, unsigned short id,
 
 	spin_lock_irqsave(&ctrl->lock, flags);
 	old = sc->id;
-	for (;;) {
+	for (; offset < end; offset++, sc++) {
 		VM_BUG_ON(sc->id != old);
 		sc->id = id;
-		offset++;
-		if (offset == end)
-			break;
-		if (offset % SC_PER_PAGE)
-			sc++;
-		else
-			sc = __lookup_swap_cgroup(ctrl, offset);
 	}
 	spin_unlock_irqrestore(&ctrl->lock, flags);
 
-- 
2.47.0.338.g60cca15819-goog

Re: [PATCH 1/2] mm: swap_cgroup: allocate swap_cgroup map using vcalloc()

[Shakeel Butt]

On Fri, Nov 15, 2024 at 07:02:28PM +0000, Roman Gushchin wrote:
> Currently swap_cgroup's map is constructed as a vmalloc()'s-based
> array of pointers to individual struct pages. This brings an
> unnecessary complexity into the code.
> 
> This patch turns the swap_cgroup's map into a single space
> allocated by vcalloc().
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>

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

[...]
> @@ -215,19 +169,8 @@ void swap_cgroup_swapoff(int type)
>  	mutex_lock(&swap_cgroup_mutex);
>  	ctrl = &swap_cgroup_ctrl[type];
>  	map = ctrl->map;
> -	length = ctrl->length;
>  	ctrl->map = NULL;
> -	ctrl->length = 0;
>  	mutex_unlock(&swap_cgroup_mutex);
>  
> -	if (map) {
> -		for (i = 0; i < length; i++) {
> -			struct page *page = map[i];
> -			if (page)
> -				__free_page(page);
> -			if (!(i % SWAP_CLUSTER_MAX))
> -				cond_resched();
> -		}
> -		vfree(map);
> -	}
> +	kvfree(map);

Any reason to use kvfree() instead of just vfree()?

>  }
> -- 
> 2.47.0.338.g60cca15819-goog
> 

Re: [PATCH 2/2] mm: swap_cgroup: get rid of __lookup_swap_cgroup()

[Shakeel Butt]

On Fri, Nov 15, 2024 at 07:02:29PM +0000, Roman Gushchin wrote:
> Because swap_cgroup map is now virtually contiguous,
> swap_cgroup_record() can be simplified, which eliminates
> a need to use __lookup_swap_cgroup().
> 
> Now as __lookup_swap_cgroup() is really trivial and is used only once,
> it can be inlined.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>

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

Re: [PATCH 1/2] mm: swap_cgroup: allocate swap_cgroup map using vcalloc()

[Roman Gushchin]

On Fri, Nov 15, 2024 at 12:17:43PM -0800, Shakeel Butt wrote:
> On Fri, Nov 15, 2024 at 07:02:28PM +0000, Roman Gushchin wrote:
> > Currently swap_cgroup's map is constructed as a vmalloc()'s-based
> > array of pointers to individual struct pages. This brings an
> > unnecessary complexity into the code.
> > 
> > This patch turns the swap_cgroup's map into a single space
> > allocated by vcalloc().
> > 
> > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> 
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Thank you for reviewing the patchset!

> 
> [...]
> > @@ -215,19 +169,8 @@ void swap_cgroup_swapoff(int type)
> >  	mutex_lock(&swap_cgroup_mutex);
> >  	ctrl = &swap_cgroup_ctrl[type];
> >  	map = ctrl->map;
> > -	length = ctrl->length;
> >  	ctrl->map = NULL;
> > -	ctrl->length = 0;
> >  	mutex_unlock(&swap_cgroup_mutex);
> >  
> > -	if (map) {
> > -		for (i = 0; i < length; i++) {
> > -			struct page *page = map[i];
> > -			if (page)
> > -				__free_page(page);
> > -			if (!(i % SWAP_CLUSTER_MAX))
> > -				cond_resched();
> > -		}
> > -		vfree(map);
> > -	}
> > +	kvfree(map);
> 
> Any reason to use kvfree() instead of just vfree()?

No, you're right, vfree() is a better choice here.

Andrew, do you prefer a v2 or it's easier to master a fix-up?

Thank you!