diff mbox series

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

Message ID 20241115190229.676440-1-roman.gushchin@linux.dev (mailing list archive)
State New
Headers show
Series [1/2] mm: swap_cgroup: allocate swap_cgroup map using vcalloc() | expand

Commit Message

Roman Gushchin Nov. 15, 2024, 7:02 p.m. UTC 
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(-)

Comments

Shakeel Butt Nov. 15, 2024, 8:17 p.m. UTC | #1 
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
>
Roman Gushchin Nov. 15, 2024, 10:46 p.m. UTC | #2 
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!
diff mbox series

Patch

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