diff mbox series

[v4,22/22] mm/zsmalloc: update comments for page->zpdesc changes

Message ID 20240729112534.3416707-23-alexs@kernel.org (mailing list archive)
State New
Headers show
Series mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool | expand

Commit Message

alexs@kernel.org July 29, 2024, 11:25 a.m. UTC 
From: Alex Shi <alexs@kernel.org>

Signed-off-by: Alex Shi <alexs@kernel.org>
---
 mm/zsmalloc.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Comments

Sergey Senozhatsky July 30, 2024, 9:37 a.m. UTC | #1 
On (24/07/29 19:25), alexs@kernel.org wrote:
> 
> From: Alex Shi <alexs@kernel.org>
> 

Usually some simple commit message is still expected.
Alex Shi July 30, 2024, 11:45 a.m. UTC | #2 
On 7/30/24 5:37 PM, Sergey Senozhatsky wrote:
> On (24/07/29 19:25), alexs@kernel.org wrote:
>>
>> From: Alex Shi <alexs@kernel.org>
>>
> 
> Usually some simple commit message is still expected.

Uh, my fault. Just forgive this part, is the following log fine?

    After the page to zpdesc conversion, there still left few comments or
    function named with page not zpdesc, let's update the comments and
    rename function create_page_chain() as create_zpdesc_chain().

Thanks
Alex
Sergey Senozhatsky July 31, 2024, 2:16 a.m. UTC | #3 
On (24/07/30 19:45), Alex Shi wrote:
> On 7/30/24 5:37 PM, Sergey Senozhatsky wrote:
> > On (24/07/29 19:25), alexs@kernel.org wrote:
> >>
> >> From: Alex Shi <alexs@kernel.org>
> >>
> > 
> > Usually some simple commit message is still expected.
> 
> Uh, my fault. Just forgive this part, is the following log fine?
> 
>     After the page to zpdesc conversion, there still left few comments or
>     function named with page not zpdesc, let's update the comments and
>     rename function create_page_chain() as create_zpdesc_chain().

A bit of a different thing, still documentation related tho: do
we want to do something about comments that mention page_lock in
zsmalloc.c?
Alex Shi July 31, 2024, 4:14 a.m. UTC | #4 
On 7/31/24 10:16 AM, Sergey Senozhatsky wrote:
> On (24/07/30 19:45), Alex Shi wrote:
>> On 7/30/24 5:37 PM, Sergey Senozhatsky wrote:
>>> On (24/07/29 19:25), alexs@kernel.org wrote:
>>>>
>>>> From: Alex Shi <alexs@kernel.org>
>>>>
>>>
>>> Usually some simple commit message is still expected.
>>
>> Uh, my fault. Just forgive this part, is the following log fine?
>>
>>     After the page to zpdesc conversion, there still left few comments or
>>     function named with page not zpdesc, let's update the comments and
>>     rename function create_page_chain() as create_zpdesc_chain().
> 
> A bit of a different thing, still documentation related tho: do
> we want to do something about comments that mention page_lock in
> zsmalloc.c?

Good question!

There are some comments mentioned about the page_lock in the file, but missed
in the header of file, so how about the following adding:

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 731055ccef23..eac110edbff0 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -25,6 +25,8 @@
  *
  * Usage of struct zpdesc(page) flags:
  *     PG_private: identifies the first component page
+ *     PG_lock: lock all component pages for a zspage free, serialize with
+ *              migration
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Thanks a lot!
Sergey Senozhatsky Aug. 1, 2024, 3:13 a.m. UTC | #5 
On (24/07/31 12:14), Alex Shi wrote:
> > A bit of a different thing, still documentation related tho: do
> > we want to do something about comments that mention page_lock in
> > zsmalloc.c?
> 
> Good question!
> 
> There are some comments mentioned about the page_lock in the file, but missed
> in the header of file, so how about the following adding:

And e.g. things like

`The page locks trylock_zspage got will be released by __free_zspage.`

Should this (and the rest) spell "zpdesc locks" or something? Or do
we still want to refer to it as "page lock"?
Matthew Wilcox Aug. 1, 2024, 3:35 a.m. UTC | #6 
On Thu, Aug 01, 2024 at 12:13:04PM +0900, Sergey Senozhatsky wrote:
> On (24/07/31 12:14), Alex Shi wrote:
> > > A bit of a different thing, still documentation related tho: do
> > > we want to do something about comments that mention page_lock in
> > > zsmalloc.c?
> > 
> > Good question!
> > 
> > There are some comments mentioned about the page_lock in the file, but missed
> > in the header of file, so how about the following adding:
> 
> And e.g. things like
> 
> `The page locks trylock_zspage got will be released by __free_zspage.`
> 
> Should this (and the rest) spell "zpdesc locks" or something? Or do
> we still want to refer to it as "page lock"?

pages do not have locks.  folios have locks.  zpdesc sounds like it has
a lock too.
Alex Shi Aug. 1, 2024, 8:06 a.m. UTC | #7 
On 8/1/24 11:35 AM, Matthew Wilcox wrote:
> On Thu, Aug 01, 2024 at 12:13:04PM +0900, Sergey Senozhatsky wrote:
>> On (24/07/31 12:14), Alex Shi wrote:
>>>> A bit of a different thing, still documentation related tho: do
>>>> we want to do something about comments that mention page_lock in
>>>> zsmalloc.c?
>>>
>>> Good question!
>>>
>>> There are some comments mentioned about the page_lock in the file, but missed
>>> in the header of file, so how about the following adding:
>>
>> And e.g. things like
>>
>> `The page locks trylock_zspage got will be released by __free_zspage.`
>>
>> Should this (and the rest) spell "zpdesc locks" or something? Or do
>> we still want to refer to it as "page lock"?
> 
> pages do not have locks.  folios have locks.  zpdesc sounds like it has
> a lock too.

Thanks willy and Sergey's suggestion! If I understand right, we'd better to update
all subpages calling in the file by zpdesc? 
Yes that's a bit more fit the code. So, is the following new patch fine?

=========
From 6699da8d62a22e9cba4ee4452b2805fc66920395 Mon Sep 17 00:00:00 2001
From: Alex Shi <alexs@kernel.org>
Date: Mon, 8 Jul 2024 20:26:20 +0800
Subject: [PATCH] mm/zsmalloc: update comments for page->zpdesc changes

Thanks for Sergey and Willy's suggestion!
After the page to zpdesc conversion, there still left few comments or
function named with page not zpdesc, let's update the comments and
rename function create_page_chain() as create_zpdesc_chain().

Signed-off-by: Alex Shi <alexs@kernel.org>
---
 mm/zsmalloc.c | 47 ++++++++++++++++++++++++++---------------------
 1 file changed, 26 insertions(+), 21 deletions(-)

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 1543a339b7f4..490cecea72f6 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -17,14 +17,16 @@
  *
  * Usage of struct zpdesc fields:
  *	zpdesc->zspage: points to zspage
- *	zpdesc->next: links together all component pages of a zspage
+ *	zpdesc->next: links together all component zpdescs of a zspage
  *		For the huge page, this is always 0, so we use this field
  *		to store handle.
  *	zpdesc->first_obj_offset: PG_zsmalloc, lower 16 bit locate the first
  *		object offset in a subpage of a zspage
  *
  * Usage of struct zpdesc(page) flags:
- *	PG_private: identifies the first component page
+ *	PG_private: identifies the first component zpdesc
+ *	PG_lock: lock all component zpdescs for a zspage free, serialize with
+ *		 migration
  */
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
@@ -191,7 +193,10 @@ struct size_class {
 	 */
 	int size;
 	int objs_per_zspage;
-	/* Number of PAGE_SIZE sized pages to combine to form a 'zspage' */
+	/*
+	 * Number of PAGE_SIZE sized zpdescs/pages to combine to
+	 * form a 'zspage'
+	 */
 	int pages_per_zspage;
 
 	unsigned int index;
@@ -913,7 +918,7 @@ static void free_zspage(struct zs_pool *pool, struct size_class *class,
 
 	/*
 	 * Since zs_free couldn't be sleepable, this function cannot call
-	 * lock_page. The page locks trylock_zspage got will be released
+	 * lock_page. The zpdesc locks trylock_zspage got will be released
 	 * by __free_zspage.
 	 */
 	if (!trylock_zspage(zspage)) {
@@ -970,7 +975,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)
 	set_freeobj(zspage, 0);
 }
 
-static void create_page_chain(struct size_class *class, struct zspage *zspage,
+static void create_zpdesc_chain(struct size_class *class, struct zspage *zspage,
 				struct zpdesc *zpdescs[])
 {
 	int i;
@@ -979,9 +984,9 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,
 	int nr_zpdescs = class->pages_per_zspage;
 
 	/*
-	 * Allocate individual pages and link them together as:
-	 * 1. all pages are linked together using zpdesc->next
-	 * 2. each sub-page point to zspage using zpdesc->zspage
+	 * Allocate individual zpdescs and link them together as:
+	 * 1. all zpdescs are linked together using zpdesc->next
+	 * 2. each sub-zpdesc point to zspage using zpdesc->zspage
 	 *
 	 * we set PG_private to identify the first zpdesc (i.e. no other zpdesc
 	 * has this flag set).
@@ -1039,7 +1044,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,
 		zpdescs[i] = zpdesc;
 	}
 
-	create_page_chain(class, zspage, zpdescs);
+	create_zpdesc_chain(class, zspage, zpdescs);
 	init_zspage(class, zspage);
 	zspage->pool = pool;
 	zspage->class = class->index;
@@ -1366,7 +1371,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,
 		/* record handle in the header of allocated chunk */
 		link->handle = handle | OBJ_ALLOCATED_TAG;
 	else
-		/* record handle to page->index */
+		/* record handle to zpdesc->handle */
 		zspage->first_zpdesc->handle = handle | OBJ_ALLOCATED_TAG;
 
 	kunmap_atomic(vaddr);
@@ -1699,19 +1704,19 @@ static int putback_zspage(struct size_class *class, struct zspage *zspage)
 #ifdef CONFIG_COMPACTION
 /*
  * To prevent zspage destroy during migration, zspage freeing should
- * hold locks of all pages in the zspage.
+ * hold locks of all component zpdesc in the zspage.
  */
 static void lock_zspage(struct zspage *zspage)
 {
 	struct zpdesc *curr_zpdesc, *zpdesc;
 
 	/*
-	 * Pages we haven't locked yet can be migrated off the list while we're
+	 * Zpdesc we haven't locked yet can be migrated off the list while we're
 	 * trying to lock them, so we need to be careful and only attempt to
-	 * lock each page under migrate_read_lock(). Otherwise, the page we lock
-	 * may no longer belong to the zspage. This means that we may wait for
-	 * the wrong page to unlock, so we must take a reference to the page
-	 * prior to waiting for it to unlock outside migrate_read_lock().
+	 * lock each zpdesc under migrate_read_lock(). Otherwise, the zpdesc we
+	 * lock may no longer belong to the zspage. This means that we may wait
+	 * for the wrong zpdesc to unlock, so we must take a reference to the
+	 * zpdesc prior to waiting for it to unlock outside migrate_read_lock().
 	 */
 	while (1) {
 		migrate_read_lock(zspage);
@@ -1786,7 +1791,7 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage,
 		idx++;
 	} while ((zpdesc = get_next_zpdesc(zpdesc)) != NULL);
 
-	create_page_chain(class, zspage, zpdescs);
+	create_zpdesc_chain(class, zspage, zpdescs);
 	first_obj_offset = get_first_obj_offset(oldzpdesc);
 	set_first_obj_offset(newzpdesc, first_obj_offset);
 	if (unlikely(ZsHugePage(zspage)))
@@ -1797,8 +1802,8 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage,
 static bool zs_page_isolate(struct page *page, isolate_mode_t mode)
 {
 	/*
-	 * Page is locked so zspage couldn't be destroyed. For detail, look at
-	 * lock_zspage in free_zspage.
+	 * Page/zpdesc is locked so zspage couldn't be destroyed. For detail,
+	 * look at lock_zspage in free_zspage.
 	 */
 	VM_BUG_ON_PAGE(PageIsolated(page), page);
 
@@ -1825,7 +1830,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,
 	/* We're committed, tell the world that this is a Zsmalloc page. */
 	__zpdesc_set_zsmalloc(newzpdesc);
 
-	/* The page is locked, so this pointer must remain valid */
+	/* The zpdesc/page is locked, so this pointer must remain valid */
 	zspage = get_zspage(zpdesc);
 	pool = zspage->pool;
 
@@ -1898,7 +1903,7 @@ static const struct movable_operations zsmalloc_mops = {
 };
 
 /*
- * Caller should hold page_lock of all pages in the zspage
+ * Caller should hold zpdesc locks of all in the zspage
  * In here, we cannot use zspage meta data.
  */
 static void async_free_zspage(struct work_struct *work)
diff mbox series

Patch

diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
index 64e523ae71f8..50ce4a3b8279 100644
--- a/mm/zsmalloc.c
+++ b/mm/zsmalloc.c
@@ -967,7 +967,7 @@  static void init_zspage(struct size_class *class, struct zspage *zspage)
 	set_freeobj(zspage, 0);
 }
 
-static void create_page_chain(struct size_class *class, struct zspage *zspage,
+static void create_zpdesc_chain(struct size_class *class, struct zspage *zspage,
 				struct zpdesc *zpdescs[])
 {
 	int i;
@@ -976,9 +976,9 @@  static void create_page_chain(struct size_class *class, struct zspage *zspage,
 	int nr_zpdescs = class->pages_per_zspage;
 
 	/*
-	 * Allocate individual pages and link them together as:
-	 * 1. all pages are linked together using zpdesc->next
-	 * 2. each sub-page point to zspage using zpdesc->zspage
+	 * Allocate individual zpdescs and link them together as:
+	 * 1. all zpdescs are linked together using zpdesc->next
+	 * 2. each sub-zpdesc point to zspage using zpdesc->zspage
 	 *
 	 * we set PG_private to identify the first zpdesc (i.e. no other zpdesc
 	 * has this flag set).
@@ -1036,7 +1036,7 @@  static struct zspage *alloc_zspage(struct zs_pool *pool,
 		zpdescs[i] = zpdesc;
 	}
 
-	create_page_chain(class, zspage, zpdescs);
+	create_zpdesc_chain(class, zspage, zpdescs);
 	init_zspage(class, zspage);
 	zspage->pool = pool;
 	zspage->class = class->index;
@@ -1363,7 +1363,7 @@  static unsigned long obj_malloc(struct zs_pool *pool,
 		/* record handle in the header of allocated chunk */
 		link->handle = handle | OBJ_ALLOCATED_TAG;
 	else
-		/* record handle to page->index */
+		/* record handle to zpdesc->handle */
 		zspage->first_zpdesc->handle = handle | OBJ_ALLOCATED_TAG;
 
 	kunmap_atomic(vaddr);
@@ -1783,7 +1783,7 @@  static void replace_sub_page(struct size_class *class, struct zspage *zspage,
 		idx++;
 	} while ((zpdesc = get_next_zpdesc(zpdesc)) != NULL);
 
-	create_page_chain(class, zspage, zpdescs);
+	create_zpdesc_chain(class, zspage, zpdescs);
 	first_obj_offset = get_first_obj_offset(oldzpdesc);
 	set_first_obj_offset(newzpdesc, first_obj_offset);
 	if (unlikely(ZsHugePage(zspage)))