mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool
Subsystems (2)
v4 cover mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool
From: Alex Shi <[email protected]>
According to Metthew's plan, the page descriptor will be replace by a 8 bytes mem_desc on destination purpose.
https://lore.kernel.org/lkml/[email protected]/
Here is a implement on zsmalloc to replace page descriptor by 'zpdesc', which is still overlay on struct page now. but it's a step move forward above destination.
To name the struct zpdesc instead of zsdesc, since there are still 3 zpools under zswap: zbud, z3fold, zsmalloc for now(z3fold maybe removed soon), and we could easyly extend it to other zswap.zpool in needs.
For all zswap.zpools, they are all using single page since often used under memory pressure. So the conversion via folio series helper is better than page's for compound_head check saving.
For now, all zpools are using some page struct members, like page.flags for PG_private/PG_locked. and list_head lru, page.mapping for page migration.
This patachset does not increase the descriptor size nor introduce any functional changes, and could save about 122Kbytes zsmalloc.o size.
Thanks
Alex
---v3->v4:
- rebase on akpm/mm-unstable Jul 21
- fixed a build warning reported by LKP
- Add a comment update for struct page to zpdesc change
v2->v3:
- Fix LKP reported build issue
- Update the Usage of struct zpdesc fields.
- Rebase onto latest mm-unstable commit 2073cda629a4
v1->v2:
- Take Yosry and Yoo's suggestion to add more members in zpdesc,
- Rebase on latest mm-unstable commit 31334cf98dbd
---
Alex Shi (11):
mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool mm/zsmalloc: use zpdesc in trylock_zspage/lock_zspage mm/zsmalloc: convert create_page_chain() and its users to use zpdesc mm/zsmalloc: rename reset_page to reset_zpdesc and use zpdesc in it mm/zsmalloc: convert SetZsPageMovable and remove unused funcs mm/zsmalloc: convert get/set_first_obj_offset() to take zpdesc mm/zsmalloc: introduce __zpdesc_clear_movable mm/zsmalloc: introduce __zpdesc_clear_zsmalloc mm/zsmalloc: introduce __zpdesc_set_zsmalloc() mm/zsmalloc: fix build warning from lkp testing mm/zsmalloc: update comments for page->zpdesc changes
Hyeonggon Yoo (11):
mm/zsmalloc: convert __zs_map_object/__zs_unmap_object to use zpdesc mm/zsmalloc: add and use pfn/zpdesc seeking funcs mm/zsmalloc: convert obj_malloc() to use zpdesc mm/zsmalloc: convert obj_allocated() and related helpers to use zpdesc mm/zsmalloc: convert init_zspage() to use zpdesc mm/zsmalloc: convert obj_to_page() and zs_free() to use zpdesc mm/zsmalloc: add zpdesc_is_isolated/zpdesc_zone helper for
zs_page_migrate
mm/zsmalloc: convert __free_zspage() to use zdsesc mm/zsmalloc: convert location_to_obj() to take zpdesc mm/zsmalloc: convert migrate_zspage() to use zpdesc mm/zsmalloc: convert get_zspage() to take zpdesc
mm/zpdesc.h | 146 ++++++++++++++++ mm/zsmalloc.c | 466 +++++++++++++++++++++++++++----------------------- 2 files changed, 401 insertions(+), 211 deletions(-) create mode 100644 mm/zpdesc.h
Signature · 1 line
2.43.0
PATCH v4 1/22 mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool
From: Alex Shi <[email protected]>
The 1st patch introduces new memory decriptor zpdesc and rename zspage.first_page to zspage.first_zpdesc, no functional change.
We removed PG_owner_priv_1 since it was moved to zspage after commit a41ec880aa7b ("zsmalloc: move huge compressed obj from page to zspage").
And keep the memcg_data member, since as Yosry pointed out:
"When the pages are freed, put_page() -> folio_put() -> __folio_put() will call mem_cgroup_uncharge(). The latter will call folio_memcg() (which reads folio->memcg_data) to figure out if uncharging needs to be done.
There are also other similar code paths that will check folio->memcg_data. It is currently expected to be present for all folios. So until we have custom code paths per-folio type for allocation/freeing/etc, we need to keep folio->memcg_data present and properly initialized."
--- mm/zpdesc.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ mm/zsmalloc.c | 21 ++++++++-------- 2 files changed, 76 insertions(+), 11 deletions(-) create mode 100644 mm/zpdesc.h
mm/zpdesc.h new file +66 -0
@@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* zpdesc.h: zswap.zpool memory descriptor +* +* Written by Alex Shi <[email protected]> +* Hyeonggon Yoo <[email protected]> +*/ +#ifndef __MM_ZPDESC_H__ +#define __MM_ZPDESC_H__ + +/* +* struct zpdesc - Memory descriptor for zpool memory, now is for zsmalloc +* @flags: Page flags, PG_private: identifies the first component page +* @lru: Indirectly used by page migration +* @mops: Used by page migration +* @next: Next zpdesc in a zspage in zsmalloc zpool +* @handle: For huge zspage in zsmalloc zpool +* @zspage: Pointer to zspage in zsmalloc +* @memcg_data: Memory Control Group data. +* +* This struct overlays struct page for now. Do not modify without a good +* understanding of the issues. +*/ +struct zpdesc { +unsigned long flags; +struct list_head lru; +struct movable_operations *mops; +union { +/* Next zpdescs in a zspage in zsmalloc zpool */ +struct zpdesc *next; +/* For huge zspage in zsmalloc zpool */ +unsigned long handle; + }; +struct zspage *zspage; +unsigned long _zp_pad_1; +#ifdef CONFIG_MEMCG +unsigned long memcg_data; +#endif +}; +#define ZPDESC_MATCH (pg, zp) \ + static_assert(offsetof(struct page, pg) == offsetof(struct zpdesc, zp)) + +ZPDESC_MATCH (flags, flags); +ZPDESC_MATCH (lru, lru); +ZPDESC_MATCH (mapping, mops); +ZPDESC_MATCH (index, next); +ZPDESC_MATCH (index, handle); +ZPDESC_MATCH (private, zspage); +#ifdef CONFIG_MEMCG +ZPDESC_MATCH (memcg_data, memcg_data); +#endif +#undef ZPDESC_MATCH +static_assert (sizeof (struct zpdesc ) <=sizeof (struct page )); + +#define zpdesc_page (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct page *)(zp), \ + struct zpdesc *: (struct page *)(zp))) + +#define zpdesc_folio (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct folio *)(zp), \ + struct zpdesc *: (struct folio *)(zp))) + +#define page_zpdesc (p) (_Generic((p), \ + const struct page *: (const struct zpdesc *)(p), \ + struct page *: (struct zpdesc *)(p))) + +#endif
mm/zsmalloc.c +10 -11
@@ -13,20 +13,18 @@/* * Following is how we use various fields and flags of underlying -* struct page(s) to form a zspage. +* struct zpdesc(page) to form a zspage. * -* Usage of struct page fields: -* page->private: points to zspage -* page->index: links together all component pages of a zspage +* Usage of struct zpdesc fields: +* zpdesc->zspage: points to zspage +* zpdesc->next: links together all component pages of a zspage * For the huge page, this is always 0, so we use this field * to store handle. * page->page_type: PG_zsmalloc, lower 16 bit locate the first object * offset in a subpage of a zspage * -* Usage of struct page flags: +* Usage of struct zpdesc(page) flags: * PG_private: identifies the first component page -* PG_owner_priv_1: identifies the huge component page -* */ #define pr_fmt (fmt) KBUILD_MODNAME ": " fmt
@@ -64,6 +62,7 @@#include <linux/pagemap.h> #include <linux/fs.h> #include <linux/local_lock.h> +#include "zpdesc.h" #define ZSPAGE_MAGIC 0x58
@@ -253,7 +252,7 @@ struct zspage { };unsigned int inuse;unsigned int freeobj; -struct page *first_page; +struct zpdesc *first_zpdesc;struct list_head list;/* fullness list */ struct zs_pool *pool;rwlock_t lock;
@@ -448,7 +447,7 @@ static inline void mod_zspage_inuse(struct zspage *zspage, int val)static inline struct page *get_first_page (struct zspage *zspage) { -struct page *first_page = zspage->first_page; +struct page *first_page =zpdesc_page (zspage->first_zpdesc);VM_BUG_ON_PAGE (!is_first_page (first_page), first_page);return first_page;
@@ -948,7 +947,7 @@ static void create_page_chain(struct size_class *class, struct zspage *zspage,set_page_private (page, (unsigned long )zspage); page->index =0 ;if (i ==0 ) { - zspage->first_page = page; + zspage->first_zpdesc =page_zpdesc (page);SetPagePrivate (page);if (unlikely (class->objs_per_zspage ==1 && class->pages_per_zspage ==1 ))
@@ -1324,7 +1323,7 @@ static unsigned long obj_malloc(struct zs_pool *pool, link->handle = handle |OBJ_ALLOCATED_TAG ; else/* record handle to page->index */ - zspage->first_page->index = handle | OBJ_ALLOCATED_TAG; + zspage->first_zpdesc->handle = handle | OBJ_ALLOCATED_TAG;kunmap_atomic (vaddr);mod_zspage_inuse (zspage,1 );
Signature · 1 line
2.43.0
Quoted 1 line · On Mon, Jul 29, 2024 at 07:25:13PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
I've been busy with other things, so I haven't been able to review this until now. Thanks to both you and Hyeonggon for working on this memdesc :)
Quoted 41 lines
The 1st patch introduces new memory decriptor zpdesc and rename zspage.first_page to zspage.first_zpdesc, no functional change.
We removed
PG_owner_priv_1since it was moved to zspage after commita41ec880aa7b("zsmalloc: move huge compressed obj from page to zspage").And keep the
memcg_datamember, since as Yosry pointed out:
"When the pages are freed,put_page()->folio_put()->__folio_put()will callmem_cgroup_uncharge(). The latter will callfolio_memcg()(which readsfolio->memcg_data) to figure out if uncharging needs to be done.There are also other similar code paths that will check
folio->memcg_data. It is currently expected to be present for all folios. So until we have custom code paths per-folio type for allocation/freeing/etc, we need to keepfolio->memcg_datapresent and properly initialized."Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ mm/zsmalloc.c | 21 ++++++++-------- 2 files changed, 76 insertions(+), 11 deletions(-) create mode 100644 mm/zpdesc.hmm/zpdesc.h new file +9
-0@@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* zpdesc.h: zswap.zpool memory descriptor +* +* Written by Alex Shi <[email protected]> +* Hyeonggon Yoo <[email protected]> +*/ +#ifndef __MM_ZPDESC_H__ +#define __MM_ZPDESC_H__ +
mm/zpdesc.h line 19
Quoted 10 lines
mm/zpdesc.h new file +10
-0+/* +* struct zpdesc - Memory descriptor for zpool memory, now is for zsmalloc +* @flags: Page flags, PG_private: identifies the first component page +* @lru: Indirectly used by page migration +* @mops: Used by page migration +* @next: Next zpdesc in a zspage in zsmalloc zpool +* @handle: For huge zspage in zsmalloc zpool +* @zspage: Pointer to zspage in zsmalloc +* @memcg_data: Memory Control Group data. +*
I think its a good idea to include comments for the padding (namely what aliases with it in struct page) here as well. It doesn't hurt, and will make them easier to remove in the future.
Quoted 4 lines
mm/zpdesc.h new file +4
-0+* This struct overlays struct page for now. Do not modify without a good +* understanding of the issues. +*/ +struct zpdesc {
mm/zpdesc.h line 33
Quoted 10 lines
mm/zpdesc.h new file +10
-0+unsigned long flags; +struct list_head lru; +struct movable_operations *mops; +union { +/* Next zpdescs in a zspage in zsmalloc zpool */ +struct zpdesc *next; +/* For huge zspage in zsmalloc zpool */ +unsigned long handle; + }; +struct zspage *zspage;
I like using pointers here, although I think the comments should be more precise about what the purpose of the pointer is. Maybe something like "Points to the zspage this zpdesc is a part of" or something.
mm/zpdesc.h line 38
Quoted 5 lines
mm/zpdesc.h new file +5
-0+unsigned long _zp_pad_1; +#ifdef CONFIG_MEMCG +unsigned long memcg_data; +#endif +};
You should definitely fold your additions to the struct from patch 17 into this patch. It makes it easier to review, and better for anyone looking at the commit log in the future.
Quoted 18 lines
mm/zpdesc.h new file +18
-0+#define ZPDESC_MATCH (pg, zp) \ + static_assert(offsetof(struct page, pg) == offsetof(struct zpdesc, zp)) + +ZPDESC_MATCH (flags, flags); +ZPDESC_MATCH (lru, lru); +ZPDESC_MATCH (mapping, mops); +ZPDESC_MATCH (index, next); +ZPDESC_MATCH (index, handle); +ZPDESC_MATCH (private, zspage); +#ifdef CONFIG_MEMCG +ZPDESC_MATCH (memcg_data, memcg_data); +#endif +#undef ZPDESC_MATCH +static_assert (sizeof (struct zpdesc ) <=sizeof (struct page )); + +#define zpdesc_page (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct page *)(zp), \ + struct zpdesc *: (struct page *)(zp)))
mm/zpdesc.h line 66
Quoted 10 lines
mm/zpdesc.h new file +10
-0+ +#define zpdesc_folio (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct folio *)(zp), \ + struct zpdesc *: (struct folio *)(zp))) + +#define page_zpdesc (p) (_Generic((p), \ + const struct page *: (const struct zpdesc *)(p), \ + struct page *: (struct zpdesc *)(p))) + +#endif
I'm don't think we need both page and folio cast functions for zpdescs. Sticking to pages will probably suffice (and be easiest) since all APIs zsmalloc cares about are already defined.
We can stick to 1 "middle-man" descriptor for zpdescs since zsmalloc uses those pages as space to track zspages and nothing more. We'll likely end up completely removing it from zsmalloc once we can allocate memdescs on their own: It seems most (if not all) of the "indirect" members of zpdesc are used as indicators to the rest of core-mm telling them not to mess with that memory.
Quoted 5 lines · Vishal Moola
Quoted 1 line · On Mon, Jul 29, 2024 at 07:25:13PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
I've been busy with other things, so I haven't been able to review this until now. Thanks to both you and Hyeonggon for working on this memdesc :)
Hi Vishal,
Thank a lot for your comments!
My pleasure! :)
Quoted 67 lines
Quoted 42 lines
The 1st patch introduces new memory decriptor zpdesc and rename zspage.first_page to zspage.first_zpdesc, no functional change.
We removed
PG_owner_priv_1since it was moved to zspage after commita41ec880aa7b("zsmalloc: move huge compressed obj from page to zspage").And keep the
memcg_datamember, since as Yosry pointed out:
"When the pages are freed,put_page()->folio_put()->__folio_put()will callmem_cgroup_uncharge(). The latter will callfolio_memcg()(which readsfolio->memcg_data) to figure out if uncharging needs to be done.There are also other similar code paths that will check
folio->memcg_data. It is currently expected to be present for all folios. So until we have custom code paths per-folio type for allocation/freeing/etc, we need to keepfolio->memcg_datapresent and properly initialized."Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ mm/zsmalloc.c | 21 ++++++++-------- 2 files changed, 76 insertions(+), 11 deletions(-) create mode 100644 mm/zpdesc.hmm/zpdesc.h new file +9
-0@@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* zpdesc.h: zswap.zpool memory descriptor +* +* Written by Alex Shi <[email protected]> +* Hyeonggon Yoo <[email protected]> +*/ +#ifndef __MM_ZPDESC_H__ +#define __MM_ZPDESC_H__ +mm/zpdesc.h line 19
Quoted 10 lines
mm/zpdesc.h new file +10
-0+/* +* struct zpdesc - Memory descriptor for zpool memory, now is for zsmalloc +* @flags: Page flags, PG_private: identifies the first component page +* @lru: Indirectly used by page migration +* @mops: Used by page migration +* @next: Next zpdesc in a zspage in zsmalloc zpool +* @handle: For huge zspage in zsmalloc zpool +* @zspage: Pointer to zspage in zsmalloc +* @memcg_data: Memory Control Group data. +* I think its a good idea to include comments for the padding (namely what aliases with it in struct page) here as well. It doesn't hurt, and will make them easier to remove in the future.
Quoted 10 lines
mm/zpdesc.h new file +10
-0+* This struct overlays struct page for now. Do not modify without a good +* understanding of the issues. +*/ +struct zpdesc { +unsigned long flags; +struct list_head lru; +struct movable_operations *mops; +union { +/* Next zpdescs in a zspage in zsmalloc zpool */ +struct zpdesc *next;
Quoted 8 lines
mm/zpdesc.h line 33
Quoted 4 lines
mm/zpdesc.h new file +4
-0+/* For huge zspage in zsmalloc zpool */ +unsigned long handle; + }; +struct zspage *zspage;I like using pointers here, although I think the comments should be more precise about what the purpose of the pointer is. Maybe something like "Points to the zspage this zpdesc is a part of" or something.
I will change the comments for this member. Thanks!
Quoted 10 lines
mm/zpdesc.h line 38
Quoted 6 lines
mm/zpdesc.h new file +5
-0+unsigned long _zp_pad_1; +#ifdef CONFIG_MEMCG +unsigned long memcg_data; +#endif +};You should definitely fold your additions to the struct from patch 17 into this patch. It makes it easier to review, and better for anyone looking at the commit log in the future.
Thanks! I will move the struct part from patch 17 here.
Quoted 34 lines
Quoted 19 lines
mm/zpdesc.h new file +18
-0+#define ZPDESC_MATCH (pg, zp) \ + static_assert(offsetof(struct page, pg) == offsetof(struct zpdesc, zp)) + +ZPDESC_MATCH (flags, flags); +ZPDESC_MATCH (lru, lru); +ZPDESC_MATCH (mapping, mops); +ZPDESC_MATCH (index, next); +ZPDESC_MATCH (index, handle); +ZPDESC_MATCH (private, zspage); +#ifdef CONFIG_MEMCG +ZPDESC_MATCH (memcg_data, memcg_data); +#endif +#undef ZPDESC_MATCH +static_assert (sizeof (struct zpdesc ) <=sizeof (struct page )); + +#define zpdesc_page (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct page *)(zp), \ + struct zpdesc *: (struct page *)(zp)))mm/zpdesc.h line 66
Quoted 10 lines
mm/zpdesc.h new file +10
-0+ +#define zpdesc_folio (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct folio *)(zp), \ + struct zpdesc *: (struct folio *)(zp))) + +#define page_zpdesc (p) (_Generic((p), \ + const struct page *: (const struct zpdesc *)(p), \ + struct page *: (struct zpdesc *)(p))) + +#endif I'm don't think we need both page and folio cast functions for zpdescs. Sticking to pages will probably suffice (and be easiest) since all APIs zsmalloc cares about are already defined.
Quoted 6 lines
We can stick to 1 "middle-man" descriptor for zpdescs since zsmalloc uses those pages as space to track zspages and nothing more. We'll likely end up completely removing it from zsmalloc once we can allocate memdescs on their own: It seems most (if not all) of the "indirect" members of zpdesc are used as indicators to the rest of core-mm telling them not to mess with that memory.
Yes, that is also my first attempt to skip folio part, but I found we could got 6.3% object size reduced on zsmalloc.o file, from 37.2KB to 34.9KB, if we use folio series lock and folio_get/put functions. That saving come from compound_head check skipping.
So I wrapped them carefully in zpdesc series functions in zpdesc.h file. They should be easy replaced when we use memdescs in the future. Could we keep them a while, or ?
Thanks
Alex
Quoted 45 lines · Alex Shi
Quoted 35 lines · Vishal Moola
Quoted 19 lines · On Mon, Jul 29, 2024 at 07:25:13PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
mm/zpdesc.h new file +17
-0+ static_assert(offsetof(struct page, pg) == offsetof(struct zpdesc, zp)) + +ZPDESC_MATCH (flags, flags); +ZPDESC_MATCH (lru, lru); +ZPDESC_MATCH (mapping, mops); +ZPDESC_MATCH (index, next); +ZPDESC_MATCH (index, handle); +ZPDESC_MATCH (private, zspage); +#ifdef CONFIG_MEMCG +ZPDESC_MATCH (memcg_data, memcg_data); +#endif +#undef ZPDESC_MATCH +static_assert (sizeof (struct zpdesc ) <=sizeof (struct page )); + +#define zpdesc_page (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct page *)(zp), \ + struct zpdesc *: (struct page *)(zp)))mm/zpdesc.h line 66
Quoted 10 lines
mm/zpdesc.h new file +10
-0+ +#define zpdesc_folio (zp) (_Generic((zp), \ + const struct zpdesc *: (const struct folio *)(zp), \ + struct zpdesc *: (struct folio *)(zp))) + +#define page_zpdesc (p) (_Generic((p), \ + const struct page *: (const struct zpdesc *)(p), \ + struct page *: (struct zpdesc *)(p))) + +#endif I'm don't think we need both page and folio cast functions for zpdescs. Sticking to pages will probably suffice (and be easiest) since all APIs zsmalloc cares about are already defined.
Quoted 6 lines
We can stick to 1 "middle-man" descriptor for zpdescs since zsmalloc uses those pages as space to track zspages and nothing more. We'll likely end up completely removing it from zsmalloc once we can allocate memdescs on their own: It seems most (if not all) of the "indirect" members of zpdesc are used as indicators to the rest of core-mm telling them not to mess with that memory.
Quoted 7 lines
Yes, that is also my first attempt to skip folio part, but I found we could got 6.3% object size reduced on zsmalloc.o file, from 37.2KB to 34.9KB, if we use folio series lock and folio_get/put functions. That saving come from
compound_headcheck skipping.
So I wrapped them carefully in zpdesc series functions inzpdesc.hfile. They should be easy replaced when we use memdescs in the future. Could we keep them a while, or ?
IMO, Its alright to keep both pages and folios due to the size reduction. However if we do keep both, it should be clearer that we Want zpdescs to be order-0 pages, and the only reason we have folios is that compound_head() size reduction (and nothing more). I think a comment by the zpdesc_folio() macro will suffice.
Quoted 3 lines
Thanks
Alex
Quoted 8 lines · Vishal Moola
Quoted 3 lines
So I wrapped them carefully in zpdesc series functions in
zpdesc.hfile. They should be easy replaced when we use memdescs in the future. Could we keep them a while, or ?IMO, Its alright to keep both pages and folios due to the size reduction. However if we do keep both, it should be clearer that we Want zpdescs to be order-0 pages, and the only reason we have folios is that
compound_head()size reduction (and nothing more). I think a comment by thezpdesc_folio()macro will suffice.
Right, will add some comments for this.
Thanks!
Quoted 19 lines · On Mon, Jul 29, 2024 at 07:25:13PM +0800, [email protected] wrote:
mm/zpdesc.h new file +19
-0+/* +* struct zpdesc - Memory descriptor for zpool memory, now is for zsmalloc +* @flags: Page flags, PG_private: identifies the first component page +* @lru: Indirectly used by page migration +* @mops: Used by page migration +* @next: Next zpdesc in a zspage in zsmalloc zpool +* @handle: For huge zspage in zsmalloc zpool +* @zspage: Pointer to zspage in zsmalloc +* @memcg_data: Memory Control Group data. +* +* This struct overlays struct page for now. Do not modify without a good +* understanding of the issues. +*/ +struct zpdesc { +unsigned long flags; +struct list_head lru; +struct movable_operations *mops; +union { +/* Next zpdescs in a zspage in zsmalloc zpool */
mm/zpdesc.h line 38
Quoted 10 lines
mm/zpdesc.h new file +10
-0+struct zpdesc *next; +/* For huge zspage in zsmalloc zpool */ +unsigned long handle; + }; +struct zspage *zspage; +unsigned long _zp_pad_1; +#ifdef CONFIG_MEMCG +unsigned long memcg_data; +#endif +};
Before we do a v5, what's the plan for a shrunk struct page? It feels like a lot of what's going on here is just "because we can". But if you actually had to allocate the memory, would you?
That is, if we get to
struct page {
unsigned long memdesc;
};what do you put in the 60 bits of information? Do you allocate a per-page struct zpdesc, and have each one pointing to a zspage? Or do you extend the current contents of zspage to describe the pages allocated to it, and make each struct page point to the zspage?
I don't know your code, so I'm not trying to choose for you. I'm just trying to make sure we're walking in the right direction.
Quoted 35 lines · Matthew Wilcox
Quoted 19 lines · On Mon, Jul 29, 2024 at 07:25:13PM +0800, [email protected] wrote:
mm/zpdesc.h new file +19
-0+/* +* struct zpdesc - Memory descriptor for zpool memory, now is for zsmalloc +* @flags: Page flags, PG_private: identifies the first component page +* @lru: Indirectly used by page migration +* @mops: Used by page migration +* @next: Next zpdesc in a zspage in zsmalloc zpool +* @handle: For huge zspage in zsmalloc zpool +* @zspage: Pointer to zspage in zsmalloc +* @memcg_data: Memory Control Group data. +* +* This struct overlays struct page for now. Do not modify without a good +* understanding of the issues. +*/ +struct zpdesc { +unsigned long flags; +struct list_head lru; +struct movable_operations *mops; +union { +/* Next zpdescs in a zspage in zsmalloc zpool */ mm/zpdesc.h line 38
Quoted 10 lines
mm/zpdesc.h new file +10
-0+struct zpdesc *next; +/* For huge zspage in zsmalloc zpool */ +unsigned long handle; + }; +struct zspage *zspage; +unsigned long _zp_pad_1; +#ifdef CONFIG_MEMCG +unsigned long memcg_data; +#endif +};Before we do a v5, what's the plan for a shrunk struct page? It feels like a lot of what's going on here is just "because we can". But if you actually had to allocate the memory, would you?
Quoted 6 lines
That is, if we get to
struct page { unsigned long memdesc; };
Yes, we still have a huge gap between this target.
Quoted 4 lines
what do you put in the 60 bits of information? Do you allocate a per-page struct zpdesc, and have each one pointing to a zspage? Or do you extend the current contents of zspage to describe the pages allocated to it, and make each struct page point to the zspage?
I am not very clear the way to get there. The easy path for me, I guess, would move struct zpdesc members out to zspage, like 2nd way of your suggestion.
I believe you has much more idea of the ways to memdesc.
Is there some references or detailed info which I could learn from you?
Many thanks!
Alex
Quoted 3 lines
I don't know your code, so I'm not trying to choose for you. I'm just trying to make sure we're walking in the right direction.
PATCH v4 2/22 mm/zsmalloc: use zpdesc in trylock_zspage/lock_zspage
In reply to [email protected]From: Alex Shi <[email protected]>
To use zpdesc in trylock_zspage/lock_zspage funcs, we add couple of helpers:
zpdesc_lock/zpdesc_unlock/zpdesc_trylock/zpdesc_wait_locked and zpdesc_get/zpdesc_put for this purpose.
Here we use the folio series func in guts for 2 reasons, one zswap.zpool only get single page, and use folio could save some compound_head checking; two, folio_put could bypass devmap checking that we don't need.
--- mm/zpdesc.h | 30 ++++++++++++++++++++++++ mm/zsmalloc.c | 64 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 73 insertions(+), 21 deletions(-)
mm/zpdesc.h +30 -0
@@ -63,4 +63,34 @@ static_assert(sizeof(struct zpdesc) <= sizeof(struct page)); const struct page *: (const struct zpdesc *)(p), \ struct page *: (struct zpdesc *)(p))) +static inline void zpdesc_lock(struct zpdesc *zpdesc) +{ +folio_lock (zpdesc_folio (zpdesc )); +} + +static inline bool zpdesc_trylock (struct zpdesc *zpdesc) +{ +return folio_trylock (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_unlock (struct zpdesc *zpdesc) +{ +folio_unlock (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_wait_locked (struct zpdesc *zpdesc) +{ +folio_wait_locked (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_get (struct zpdesc *zpdesc) +{ +folio_get (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_put (struct zpdesc *zpdesc) +{ +folio_put (zpdesc_folio (zpdesc)); +} +#endif
mm/zsmalloc.c +43 -21
119 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v4 02/22] mm/zsmalloc: use zpdesc in trylock_zspage/lock_zspage
Quoted 5 lines · On Mon, Jul 29, 2024 at 07:25:14PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
To use zpdesc in trylock_zspage/lock_zspage funcs, we add couple of helpers:
zpdesc_lock/zpdesc_unlock/zpdesc_trylock/zpdesc_wait_locked and zpdesc_get/zpdesc_put for this purpose.
You should always include the "()" following function names. It just makes everything more readable.
Quoted 28 lines
Here we use the folio series func in guts for 2 reasons, one zswap.zpool only get single page, and use folio could save some
compound_headchecking; two,folio_putcould bypass devmap checking that we don't need.Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 30 ++++++++++++++++++++++++ mm/zsmalloc.c | 64 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 73 insertions(+), 21 deletions(-)mm/zpdesc.h +9
-0@@ -63,4 +63,34 @@ static_assert(sizeof(struct zpdesc) <= sizeof(struct page)); const struct page *: (const struct zpdesc *)(p), \ struct page *: (struct zpdesc *)(p))) +static inline void zpdesc_lock(struct zpdesc *zpdesc) +{ +folio_lock (zpdesc_folio (zpdesc )); +} + +static inline bool zpdesc_trylock (struct zpdesc *zpdesc) +{ +return folio_trylock (zpdesc_folio (zpdesc)); +}
mm/zpdesc.h static_assert(sizeof(struct zpdesc) <= sizeof(struct page)); · line 84
Quoted 10 lines
mm/zpdesc.h +10
-0+ +static inline void zpdesc_unlock (struct zpdesc *zpdesc) +{ +folio_unlock (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_wait_locked (struct zpdesc *zpdesc) +{ +folio_wait_locked (zpdesc_folio (zpdesc)); +}
The more I look at zsmalloc, the more skeptical I get about it "needing" the folio_lock. At a glance it seems like a zspage already has its own lock, and the migration doesn't appear to be truly physical? There's probably something I'm missing... it would make this code a lot simpler to drop many of the folio locks.
Quoted 17 lines
mm/zpdesc.h +11
-0+ +static inline void zpdesc_get (struct zpdesc *zpdesc) +{ +folio_get (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_put (struct zpdesc *zpdesc) +{ +folio_put (zpdesc_folio (zpdesc)); +} +#endif mm/zsmalloc.c +0
-0
mm/zsmalloc.c static __maybe_unused int is_first_page(struct page *page) · line 440
Quoted 8 lines
mm/zsmalloc.c +5
-0@@ -433,13 +433,17 @@ static __maybe_unused int is_first_page(struct page *page)return PagePrivate (page); } +static int is_first_zpdesc (struct zpdesc *zpdesc) +{ +return PagePrivate (zpdesc_page (zpdesc)); +} +
I feel like we might not even need to use the PG_private flag for zpages? It seems to me like its just used for sanity checking. Can zpage->first_page ever not point to the first zpdesc?
For the purpose of introducing the memdesc its fine to continue using it; just some food for thought.
Re: [PATCH v4 02/22] mm/zsmalloc: use zpdesc in trylock_zspage/lock_zspage
Quoted 9 lines · Vishal Moola
Quoted 5 lines · On Mon, Jul 29, 2024 at 07:25:14PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
To use zpdesc in trylock_zspage/lock_zspage funcs, we add couple of helpers:
zpdesc_lock/zpdesc_unlock/zpdesc_trylock/zpdesc_wait_locked and zpdesc_get/zpdesc_put for this purpose.You should always include the "()" following function names. It just makes everything more readable.
Thanks for reminder, I will update the commit log.
Quoted 37 lines
Quoted 37 lines
Here we use the folio series func in guts for 2 reasons, one zswap.zpool only get single page, and use folio could save some
compound_headchecking; two,folio_putcould bypass devmap checking that we don't need.Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 30 ++++++++++++++++++++++++ mm/zsmalloc.c | 64 ++++++++++++++++++++++++++++++++++----------------- 2 files changed, 73 insertions(+), 21 deletions(-)mm/zpdesc.h +17
-0@@ -63,4 +63,34 @@ static_assert(sizeof(struct zpdesc) <= sizeof(struct page)); const struct page *: (const struct zpdesc *)(p), \ struct page *: (struct zpdesc *)(p))) +static inline void zpdesc_lock(struct zpdesc *zpdesc) +{ +folio_lock (zpdesc_folio (zpdesc )); +} + +static inline bool zpdesc_trylock (struct zpdesc *zpdesc) +{ +return folio_trylock (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_unlock (struct zpdesc *zpdesc) +{ +folio_unlock (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_wait_locked (struct zpdesc *zpdesc) +{
Quoted 8 lines
mm/zpdesc.h static_assert(sizeof(struct zpdesc) <= sizeof(struct page)); · line 84
Quoted 2 lines
mm/zpdesc.h +2
-0+folio_wait_locked (zpdesc_folio (zpdesc)); +}The more I look at zsmalloc, the more skeptical I get about it "needing" the
folio_lock. At a glance it seems like a zspage already has its own lock, and the migration doesn't appear to be truly physical? There's probably something I'm missing... it would make this code a lot simpler to drop many of the folio locks.
folio series could save about 6.3% object code... Anyway I don't insist on it. Just want a double confirm, could we keep the code size saving? :)
Quoted 21 lines
Quoted 21 lines
mm/zpdesc.h +11
-0+ +static inline void zpdesc_get (struct zpdesc *zpdesc) +{ +folio_get (zpdesc_folio (zpdesc)); +} + +static inline void zpdesc_put (struct zpdesc *zpdesc) +{ +folio_put (zpdesc_folio (zpdesc)); +} +#endif mm/zsmalloc.c +0
-0@@ -433,13 +433,17 @@ static __maybe_unused int is_first_page(struct page *page)return PagePrivate (page); }
Quoted 9 lines
mm/zsmalloc.c static __maybe_unused int is_first_page(struct page *page) · line 440
Quoted 5 lines
mm/zsmalloc.c +5
-0+static int is_first_zpdesc (struct zpdesc *zpdesc) +{ +return PagePrivate (zpdesc_page (zpdesc)); +} +I feel like we might not even need to use the
PG_privateflag for zpages? It seems to me like its just used for sanity checking. Canzpage->first_pageever not point to the first zpdesc?
Yes, the PG_private is only for sanity checking now. But zspage.first_zpdesc are still used widely and must point to the first subpage.
I believe we could safely remove this page flag, maybe next patchset?
Quoted 3 lines
For the purpose of introducing the memdesc its fine to continue using it; just some food for thought.
Yes.
Thanks a lot! :)
PATCH v4 3/22 mm/zsmalloc: convert __zs_map_object/__zs_unmap_object to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
These two functions take pointer to an array of struct page. Introduce zpdesc_kmap_atomic() and make __zs_{map,unmap}_object() take pointer to an array of zpdesc instead of page.
Add silly type casting when calling them. Casting will be removed late.
--- mm/zsmalloc.c | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-)
mm/zsmalloc.c +13 -8
@@ -243,6 +243,11 @@ struct zs_pool {atomic_t compaction_in_progress; }; +static inline void *zpdesc_kmap_atomic (struct zpdesc *zpdesc) +{ +return kmap_atomic (zpdesc_page (zpdesc)); +} + struct zspage { struct { unsigned int huge:HUGE_BITS;
@@ -1061,7 +1066,7 @@ static inline void __zs_cpu_down(struct mapping_area *area) }static void *__zs_map_object (struct mapping_area *area, -struct page *pages[2 ],int off,int size) +struct zpdesc *zpdescs[2 ],int off,int size) {int sizes[2 ];void *addr;
@@ -1078,10 +1083,10 @@ static void *__zs_map_object(struct mapping_area *area, sizes[1 ] = size - sizes[0 ];/* copy object to per-cpu buffer */ - addr =kmap_atomic (pages[0 ]); + addr =zpdesc_kmap_atomic (zpdescs[0 ]);memcpy (buf, addr + off, sizes[0 ]);kunmap_atomic (addr); - addr =kmap_atomic (pages[1 ]); + addr =zpdesc_kmap_atomic (zpdescs[1 ]);memcpy (buf + sizes[0 ], addr, sizes[1 ]);kunmap_atomic (addr); out:
@@ -1089,7 +1094,7 @@ static void *__zs_map_object(struct mapping_area *area, }static void __zs_unmap_object (struct mapping_area *area, -struct page *pages[2 ],int off,int size) +struct zpdesc *zpdescs[2 ],int off,int size) {int sizes[2 ];void *addr;
@@ -1108,10 +1113,10 @@ static void __zs_unmap_object(struct mapping_area *area, sizes[1 ] = size - sizes[0 ];/* copy per-cpu buffer to object */ - addr =kmap_atomic (pages[0 ]); + addr =zpdesc_kmap_atomic (zpdescs[0 ]);memcpy (addr + off, buf, sizes[0 ]);kunmap_atomic (addr); - addr =kmap_atomic (pages[1 ]); + addr =zpdesc_kmap_atomic (zpdescs[1 ]);memcpy (addr, buf + sizes[0 ], sizes[1 ]);kunmap_atomic (addr);
@@ -1252,7 +1257,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle, pages[1 ] =get_next_page (page);BUG_ON (!pages[1 ]); - ret =__zs_map_object (area, pages, off, class->size); + ret =__zs_map_object (area, (struct zpdesc **)pages, off, class->size); out:if (likely (!ZsHugePage (zspage))) ret +=ZS_HANDLE_SIZE ;
@@ -1287,7 +1292,7 @@ void zs_unmap_object(struct zs_pool *pool, unsigned long handle) pages[1 ] =get_next_page (page);BUG_ON (!pages[1 ]); -__zs_unmap_object (area, pages, off, class->size); +__zs_unmap_object (area, (struct zpdesc **)pages, off, class->size); }local_unlock (&zs_map_area.lock);
Signature · 1 line
2.43.0
Sergey Senozhatsky
maintainerRe: [PATCH v4 03/22] mm/zsmalloc: convert __zs_map_object/__zs_unmap_object to use zpdesc
Quoted 5 lines · On (24/07/29 19:25), [email protected] wrote:
mm/zsmalloc.c +5
-0+static inline void *zpdesc_kmap_atomic (struct zpdesc *zpdesc) +{ +return kmap_atomic (zpdesc_page (zpdesc)); +} +
[..]
mm/zsmalloc.c static void *__zs_map_object(struct mapping_area *area, · line 1091
Quoted 9 lines
mm/zsmalloc.c +2
-2/* copy object to per-cpu buffer */ - addr =kmap_atomic (pages[0 ]); + addr =zpdesc_kmap_atomic (zpdescs[0 ]);memcpy (buf, addr + off, sizes[0 ]);kunmap_atomic (addr); - addr =kmap_atomic (pages[1 ]); + addr =zpdesc_kmap_atomic (zpdescs[1 ]);memcpy (buf + sizes[0 ], addr, sizes[1 ]);kunmap_atomic (addr);
Don't know if kmap_atomic() wrapper buys us anything, but okay.
PATCH v4 4/22 mm/zsmalloc: add and use pfn/zpdesc seeking funcs
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Add pfn_zpdesc conversion, convert obj_to_location() to take zpdesc and also convert its users to use zpdesc.
--- mm/zpdesc.h | 9 +++++++ mm/zsmalloc.c | 75 ++++++++++++++++++++++++++------------------------- 2 files changed, 47 insertions(+), 37 deletions(-)
mm/zpdesc.h +9 -0
@@ -93,4 +93,13 @@ static inline void zpdesc_put(struct zpdesc *zpdesc) folio_put(zpdesc_folio(zpdesc)); } +static inline unsigned long zpdesc_pfn (struct zpdesc *zpdesc) +{ +return page_to_pfn (zpdesc_page (zpdesc)); +} + +static inline struct zpdesc *pfn_zpdesc (unsigned long pfn) +{ +return page_zpdesc (pfn_to_page (pfn)); +}#endif
mm/zsmalloc.c +38 -37
189 lines not on this page · raw message
Signature · 1 line
2.43.0
PATCH v4 5/22 mm/zsmalloc: convert obj_malloc() to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Use get_first_zpdesc/get_next_zpdesc to replace get_first_page/get_next_page. no functional change.
--- mm/zsmalloc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
mm/zsmalloc.c +8 -8
@@ -1322,12 +1322,12 @@ EXPORT_SYMBOL_GPL(zs_huge_class_size);static unsigned long obj_malloc (struct zs_pool *pool,struct zspage *zspage,unsigned long handle) { -int i, nr_page, offset; +int i, nr_zpdesc, offset;unsigned long obj;struct link_free *link;struct size_class *class; -struct page *m_page; +struct zpdesc *m_zpdesc;unsigned long m_offset;void *vaddr;
@@ -1335,14 +1335,14 @@ static unsigned long obj_malloc(struct zs_pool *pool, obj =get_freeobj (zspage); offset = obj * class->size; - nr_page = offset >>PAGE_SHIFT ; + nr_zpdesc = offset >>PAGE_SHIFT ; m_offset =offset_in_page (offset); - m_page =get_first_page (zspage); + m_zpdesc =get_first_zpdesc (zspage); -for (i =0 ; i < nr_page; i++) - m_page =get_next_page (m_page); +for (i =0 ; i < nr_zpdesc; i++) + m_zpdesc =get_next_zpdesc (m_zpdesc); - vaddr =kmap_atomic (m_page); + vaddr =zpdesc_kmap_atomic (m_zpdesc); link = (struct link_free *)vaddr + m_offset /sizeof (*link);set_freeobj (zspage, link->next >>OBJ_TAG_BITS );if (likely (!ZsHugePage (zspage)))
@@ -1355,7 +1355,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,kunmap_atomic (vaddr);mod_zspage_inuse (zspage,1 ); - obj =location_to_obj (m_page, obj); + obj =location_to_obj (zpdesc_page (m_zpdesc), obj);record_obj (handle, obj);return obj;
Signature · 1 line
2.43.0
PATCH v4 6/22 mm/zsmalloc: convert create_page_chain() and its users to use zpdesc
In reply to [email protected]From: Alex Shi <[email protected]>
Introduce a few helper functions for conversion to convert create_page_chain() to use zpdesc, then use zpdesc in replace_sub_page() too.
--- mm/zpdesc.h | 6 +++ mm/zsmalloc.c | 115 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 82 insertions(+), 39 deletions(-)
mm/zpdesc.h +6 -0
@@ -102,4 +102,10 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn) {return page_zpdesc (pfn_to_page (pfn)); } + +static inline void __zpdesc_set_movable (struct zpdesc *zpdesc, +const struct movable_operations *mops) +{ +__SetPageMovable (zpdesc_page (zpdesc), mops); +}#endif
mm/zsmalloc.c +76 -39
182 lines not on this page · raw message
Signature · 1 line
2.43.0
Re: [PATCH v4 06/22] mm/zsmalloc: convert create_page_chain() and its users to use zpdesc
Quoted 4 lines · On Mon, Jul 29, 2024 at 07:25:18PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
Introduce a few helper functions for conversion to convert
create_page_chain()to use zpdesc, then use zpdesc inreplace_sub_page()too.
As a general note, I've been having trouble keeping track of your helper functions throughout your patchset. Things get confusing when helper functions are "add-ons" to patches and are then replaced/rewritten in various subsequent patches - might just be me though.
Quoted 31 lines
Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 6 +++ mm/zsmalloc.c | 115 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 82 insertions(+), 39 deletions(-)mm/zpdesc.h +6
-0@@ -102,4 +102,10 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn) {return page_zpdesc (pfn_to_page (pfn)); } + +static inline void __zpdesc_set_movable (struct zpdesc *zpdesc, +const struct movable_operations *mops) +{ +__SetPageMovable (zpdesc_page (zpdesc), mops); +}#endif mm/zsmalloc.c +1
-0@@ -248,6 +248,41 @@ static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc)return kmap_atomic (zpdesc_page (zpdesc)); } +static inline void zpdesc_set_zspage(struct zpdesc *zpdesc,
mm/zsmalloc.c static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc) · line 261
Quoted 10 lines
mm/zsmalloc.c +10
-0+ struct zspage *zspage) +{ + zpdesc->zspage = zspage; +} + +static inline void zpdesc_set_first (struct zpdesc *zpdesc) +{ +SetPagePrivate (zpdesc_page (zpdesc)); +} +
I'm not a fan of the names above. IMO, naming should follow some semblance of consistency regarding their purpose (or have comments that describe their purpose instead).
At a glance zpdesc_set_zspage() and zpdesc_set_first() sound like they are doing similar things, but I don't think they serve similar purposes?
Quoted 24 lines
mm/zsmalloc.c +24
-0+static inline void zpdesc_inc_zone_page_state (struct zpdesc *zpdesc) +{ +inc_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline void zpdesc_dec_zone_page_state (struct zpdesc *zpdesc) +{ +dec_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline struct zpdesc *alloc_zpdesc (gfp_t gfp) +{ +struct page *page =alloc_page (gfp); + +return page_zpdesc (page); +} + +static inline void free_zpdesc (struct zpdesc *zpdesc) +{ +struct page *page =zpdesc_page (zpdesc); + +__free_page (page); +} +
Re: [PATCH v4 06/22] mm/zsmalloc: convert create_page_chain() and its users to use zpdesc
Quoted 10 lines · Vishal Moola
Quoted 4 lines · On Mon, Jul 29, 2024 at 07:25:18PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
Introduce a few helper functions for conversion to convert
create_page_chain()to use zpdesc, then use zpdesc inreplace_sub_page()too.As a general note, I've been having trouble keeping track of your helper functions throughout your patchset. Things get confusing when helper functions are "add-ons" to patches and are then replaced/rewritten in various subsequent patches - might just be me though.
Right, maybe too much helper doesn't give necessary help.
Quoted 47 lines
Quoted 32 lines
Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 6 +++ mm/zsmalloc.c | 115 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 82 insertions(+), 39 deletions(-)mm/zpdesc.h +6
-0@@ -102,4 +102,10 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn) {return page_zpdesc (pfn_to_page (pfn)); } + +static inline void __zpdesc_set_movable (struct zpdesc *zpdesc, +const struct movable_operations *mops) +{ +__SetPageMovable (zpdesc_page (zpdesc), mops); +}#endif mm/zsmalloc.c +1
-0@@ -248,6 +248,41 @@ static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc)return kmap_atomic (zpdesc_page (zpdesc)); } +static inline void zpdesc_set_zspage(struct zpdesc *zpdesc,mm/zsmalloc.c static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc) · line 261
Quoted 10 lines
mm/zsmalloc.c +10
-0+ struct zspage *zspage) +{ + zpdesc->zspage = zspage; +} + +static inline void zpdesc_set_first (struct zpdesc *zpdesc) +{ +SetPagePrivate (zpdesc_page (zpdesc)); +} +I'm not a fan of the names above. IMO, naming should follow some semblance of consistency regarding their purpose (or have comments that describe their purpose instead).
Quoted 2 lines
At a glance
zpdesc_set_zspage()andzpdesc_set_first()sound like they are doing similar things, but I don't think they serve similar purposes?
zpdesc_set_zspage() only used in one place, a helper maynot needed. Let me remove it. Same thing for the alloc_zpdesc() and free_zpdesc(), they could be merge into using place.
Thanks
Alex
Quoted 26 lines
Quoted 25 lines
mm/zsmalloc.c +24
-0+static inline void zpdesc_inc_zone_page_state (struct zpdesc *zpdesc) +{ +inc_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline void zpdesc_dec_zone_page_state (struct zpdesc *zpdesc) +{ +dec_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline struct zpdesc *alloc_zpdesc (gfp_t gfp) +{ +struct page *page =alloc_page (gfp); + +return page_zpdesc (page); +} + +static inline void free_zpdesc (struct zpdesc *zpdesc) +{ +struct page *page =zpdesc_page (zpdesc); + +__free_page (page); +} +
Re: [PATCH v4 06/22] mm/zsmalloc: convert create_page_chain() and its users to use zpdesc
Quoted 63 lines · Alex Shi
Quoted 10 lines · Vishal Moola
Quoted 4 lines · On Mon, Jul 29, 2024 at 07:25:18PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
Introduce a few helper functions for conversion to convert
create_page_chain()to use zpdesc, then use zpdesc inreplace_sub_page()too.As a general note, I've been having trouble keeping track of your helper functions throughout your patchset. Things get confusing when helper functions are "add-ons" to patches and are then replaced/rewritten in various subsequent patches - might just be me though.
Right, maybe too much helper doesn't give necessary help.
Quoted 47 lines
Quoted 32 lines
Originally-by Hyeonggon Yoo <[email protected]>Signed-off-by Alex Shi <[email protected]>--- mm/zpdesc.h | 6 +++ mm/zsmalloc.c | 115 +++++++++++++++++++++++++++++++++----------------- 2 files changed, 82 insertions(+), 39 deletions(-)mm/zpdesc.h +6
-0@@ -102,4 +102,10 @@ static inline struct zpdesc *pfn_zpdesc(unsigned long pfn) {return page_zpdesc (pfn_to_page (pfn)); } + +static inline void __zpdesc_set_movable (struct zpdesc *zpdesc, +const struct movable_operations *mops) +{ +__SetPageMovable (zpdesc_page (zpdesc), mops); +}#endif mm/zsmalloc.c +1
-0@@ -248,6 +248,41 @@ static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc)return kmap_atomic (zpdesc_page (zpdesc)); } +static inline void zpdesc_set_zspage(struct zpdesc *zpdesc,mm/zsmalloc.c static inline void *zpdesc_kmap_atomic(struct zpdesc *zpdesc) · line 261
Quoted 10 lines
mm/zsmalloc.c +10
-0+ struct zspage *zspage) +{ + zpdesc->zspage = zspage; +} + +static inline void zpdesc_set_first (struct zpdesc *zpdesc) +{ +SetPagePrivate (zpdesc_page (zpdesc)); +} +I'm not a fan of the names above. IMO, naming should follow some semblance of consistency regarding their purpose (or have comments that describe their purpose instead).
Quoted 5 lines
Quoted 2 lines
At a glance
zpdesc_set_zspage()andzpdesc_set_first()sound like they are doing similar things, but I don't think they serve similar purposes?
zpdesc_set_zspage()only used in one place, a helper maynot needed. Let me remove it. Same thing for thealloc_zpdesc()andfree_zpdesc(), they could be merge into using place.
alloc_zpdesc() and free_zpdesc() are fine as is. The helper functions will be useful whenever memdescs can be allocated on their own, so its better to introduce it now.
Quoted 28 lines
Thanks
AlexQuoted 26 lines
Quoted 25 lines
mm/zsmalloc.c +24
-0+static inline void zpdesc_inc_zone_page_state (struct zpdesc *zpdesc) +{ +inc_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline void zpdesc_dec_zone_page_state (struct zpdesc *zpdesc) +{ +dec_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES ); +} + +static inline struct zpdesc *alloc_zpdesc (gfp_t gfp) +{ +struct page *page =alloc_page (gfp); + +return page_zpdesc (page); +} + +static inline void free_zpdesc (struct zpdesc *zpdesc) +{ +struct page *page =zpdesc_page (zpdesc); + +__free_page (page); +} +
Re: [PATCH v4 06/22] mm/zsmalloc: convert create_page_chain() and its users to use zpdesc
Quoted 11 lines · Vishal Moola
Quoted 8 lines
Quoted 6 lines
I'm not a fan of the names above. IMO, naming should follow some semblance of consistency regarding their purpose (or have comments that describe their purpose instead).
At a glance
zpdesc_set_zspage()andzpdesc_set_first()sound like they are doing similar things, but I don't think they serve similar purposes?
zpdesc_set_zspage()only used in one place, a helper maynot needed. Let me remove it. Same thing for thealloc_zpdesc()andfree_zpdesc(), they could be merge into using place.
alloc_zpdesc()andfree_zpdesc()are fine as is. The helper functions will be useful whenever memdescs can be allocated on their own, so its better to introduce it now.
Thanks for suggestion. will recover them in next version, maybe tomorrow if no more comments.
PATCH v4 7/22 mm/zsmalloc: convert obj_allocated() and related helpers to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Convert obj_allocated(), and related helpers to take zpdesc. Also make its callers to cast (struct page *) to (struct zpdesc *) when calling them. The users will be converted gradually as there are many.
--- mm/zsmalloc.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
mm/zsmalloc.c +10 -10
@@ -845,15 +845,15 @@ static unsigned long handle_to_obj(unsigned long handle)return *(unsigned long *)handle; } -static inline bool obj_allocated (struct page *page,void *obj, +static inline bool obj_allocated (struct zpdesc *zpdesc,void *obj,unsigned long *phandle) {unsigned long handle; -struct zspage *zspage =get_zspage (page); +struct zspage *zspage =get_zspage (zpdesc_page (zpdesc));if (unlikely (ZsHugePage (zspage))) { -VM_BUG_ON_PAGE (!is_first_page (page), page); - handle = page->index; +VM_BUG_ON_PAGE (!is_first_zpdesc (zpdesc),zpdesc_page (zpdesc)); + handle = zpdesc->handle; }else handle = *(unsigned long *)obj;
@@ -1603,18 +1603,18 @@ static void zs_object_copy(struct size_class *class, unsigned long dst,* return handle. */ static unsigned long find_alloced_obj (struct size_class *class, -struct page *page,int *obj_idx) +struct zpdesc *zpdesc,int *obj_idx) {unsigned int offset;int index = *obj_idx;unsigned long handle =0 ; -void *addr =kmap_atomic (page); +void *addr =zpdesc_kmap_atomic (zpdesc); - offset =get_first_obj_offset (page); + offset =get_first_obj_offset (zpdesc_page (zpdesc)); offset += class->size * index;while (offset <PAGE_SIZE ) { -if (obj_allocated (page, addr + offset, &handle)) +if (obj_allocated (zpdesc, addr + offset, &handle))break ; offset += class->size;
@@ -1638,7 +1638,7 @@ static void migrate_zspage(struct zs_pool *pool, struct zspage *src_zspage,struct size_class *class = pool->size_class[src_zspage->class];while (1 ) { - handle =find_alloced_obj (class, s_page, &obj_idx); + handle =find_alloced_obj (class,page_zpdesc (s_page), &obj_idx);if (!handle) { s_page =get_next_page (s_page);if (!s_page)
@@ -1871,7 +1871,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,for (addr = s_addr + offset; addr < s_addr +PAGE_SIZE ; addr += class->size) { -if (obj_allocated (page, addr, &handle)) { +if (obj_allocated (page_zpdesc (page), addr, &handle)) { old_obj =handle_to_obj (handle);obj_to_location (old_obj, &dummy, &obj_idx);
Signature · 1 line
2.43.0
PATCH v4 8/22 mm/zsmalloc: convert init_zspage() to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Replace get_first/next_page func series and kmap_atomic to new helper, no functional change.
--- mm/zsmalloc.c | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-)
mm/zsmalloc.c +8 -8
@@ -948,16 +948,16 @@ static void init_zspage(struct size_class *class, struct zspage *zspage) {unsigned int freeobj =1 ;unsigned long off =0 ; -struct page *page =get_first_page (zspage); +struct zpdesc *zpdesc =get_first_zpdesc (zspage); -while (page) { -struct page *next_page; +while (zpdesc) { +struct zpdesc *next_zpdesc;struct link_free *link;void *vaddr; -set_first_obj_offset (page, off); +set_first_obj_offset (zpdesc_page (zpdesc), off); - vaddr =kmap_atomic (page); + vaddr =zpdesc_kmap_atomic (zpdesc); link = (struct link_free *)vaddr + off /sizeof (*link);while ((off += class->size) <PAGE_SIZE ) {
@@ -970,8 +970,8 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)* page, which must point to the first object on the next * page (if present) */ - next_page =get_next_page (page); -if (next_page) { + next_zpdesc =get_next_zpdesc (zpdesc); +if (next_zpdesc) { link->next = freeobj++ <<OBJ_TAG_BITS ; }else {/*
@@ -981,7 +981,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage) link->next =-1UL <<OBJ_TAG_BITS ; }kunmap_atomic (vaddr); - page = next_page; + zpdesc = next_zpdesc; off %=PAGE_SIZE ; }
Signature · 1 line
2.43.0
PATCH v4 9/22 mm/zsmalloc: convert obj_to_page() and zs_free() to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Rename obj_to_page() to obj_to_zpdesc() and also convert it and its user zs_free() to use zpdesc.
--- mm/zsmalloc.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
mm/zsmalloc.c +5 -5
@@ -820,9 +820,9 @@ static void obj_to_location(unsigned long obj, struct zpdesc **zpdesc, *obj_idx = (obj &OBJ_INDEX_MASK ); } -static void obj_to_page (unsigned long obj,struct page **page) +static void obj_to_zpdesc (unsigned long obj,struct zpdesc **zpdesc) { - *page =pfn_to_page (obj >>OBJ_INDEX_BITS ); + *zpdesc =pfn_zpdesc (obj >>OBJ_INDEX_BITS ); }/**
@@ -1496,7 +1496,7 @@ static void obj_free(int class_size, unsigned long obj)void zs_free (struct zs_pool *pool,unsigned long handle) {struct zspage *zspage; -struct page *f_page; +struct zpdesc *f_zpdesc;unsigned long obj;struct size_class *class;int fullness;
@@ -1510,8 +1510,8 @@ void zs_free(struct zs_pool *pool, unsigned long handle)*/ read_lock (&pool->migrate_lock); obj =handle_to_obj (handle); -obj_to_page (obj, &f_page); - zspage =get_zspage (f_page); +obj_to_zpdesc (obj, &f_zpdesc); + zspage =get_zspage (zpdesc_page (f_zpdesc)); class =zspage_class (pool, zspage);spin_lock (&class->lock);read_unlock (&pool->migrate_lock);
Signature · 1 line
2.43.0
PATCH v4 10/22 mm/zsmalloc: add zpdesc_is_isolated/zpdesc_zone helper for zs_page_migrate
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
To convert page to zpdesc in zs_page_migrate(), we added zpdesc_is_isolated/zpdesc_zone helpers. No functional change.
--- mm/zpdesc.h | 11 +++++++++++ mm/zsmalloc.c | 30 ++++++++++++++++-------------- 2 files changed, 27 insertions(+), 14 deletions(-)
mm/zpdesc.h +11 -0
@@ -108,4 +108,15 @@ static inline void __zpdesc_set_movable(struct zpdesc *zpdesc, {__SetPageMovable (zpdesc_page (zpdesc), mops); } + +static inline bool zpdesc_is_isolated (struct zpdesc *zpdesc) +{ +return PageIsolated (zpdesc_page (zpdesc)); +} + +static inline struct zone *zpdesc_zone (struct zpdesc *zpdesc) +{ +return page_zone (zpdesc_page (zpdesc)); +} +#endif
mm/zsmalloc.c +16 -14
@@ -1830,19 +1830,21 @@ static int zs_page_migrate(struct page *newpage, struct page *page,struct size_class *class;struct zspage *zspage;struct zpdesc *dummy; +struct zpdesc *newzpdesc =page_zpdesc (newpage); +struct zpdesc *zpdesc =page_zpdesc (page);void *s_addr, *d_addr, *addr;unsigned int offset;unsigned long handle;unsigned long old_obj, new_obj;unsigned int obj_idx; -VM_BUG_ON_PAGE (!PageIsolated (page), page); +VM_BUG_ON_PAGE (!zpdesc_is_isolated (zpdesc),zpdesc_page (zpdesc));/* We're committed, tell the world that this is a Zsmalloc page. */ -__SetPageZsmalloc (newpage); +__SetPageZsmalloc (zpdesc_page (newzpdesc));/* The page is locked, so this pointer must remain valid */ - zspage =get_zspage (page); + zspage =get_zspage (zpdesc_page (zpdesc)); pool = zspage->pool;/*
@@ -1859,30 +1861,30 @@ static int zs_page_migrate(struct page *newpage, struct page *page,/* the migrate_write_lock protects zpage access via zs_map_object */ migrate_write_lock (zspage); - offset =get_first_obj_offset (page); - s_addr =kmap_atomic (page); + offset =get_first_obj_offset (zpdesc_page (zpdesc)); + s_addr =zpdesc_kmap_atomic (zpdesc);/* * Here, any user cannot access all objects in the zspage so let's move. */ - d_addr =kmap_atomic (newpage); + d_addr =zpdesc_kmap_atomic (newzpdesc);copy_page (d_addr, s_addr);kunmap_atomic (d_addr);for (addr = s_addr + offset; addr < s_addr +PAGE_SIZE ; addr += class->size) { -if (obj_allocated (page_zpdesc (page), addr, &handle)) { +if (obj_allocated (zpdesc, addr, &handle)) { old_obj =handle_to_obj (handle);obj_to_location (old_obj, &dummy, &obj_idx); - new_obj = (unsigned long )location_to_obj (newpage, + new_obj = (unsigned long )location_to_obj (zpdesc_page (newzpdesc), obj_idx);record_obj (handle, new_obj); } }kunmap_atomic (s_addr); -replace_sub_page (class, zspage,page_zpdesc (newpage),page_zpdesc (page)); +replace_sub_page (class, zspage, newzpdesc, zpdesc);/* * Since we complete the data copy and set up new zspage structure, * it's okay to release migration_lock.
@@ -1891,14 +1893,14 @@ static int zs_page_migrate(struct page *newpage, struct page *page,spin_unlock (&class->lock);migrate_write_unlock (zspage); -get_page (newpage); -if (page_zone (newpage) !=page_zone (page)) { -dec_zone_page_state (page,NR_ZSPAGES ); -inc_zone_page_state (newpage,NR_ZSPAGES ); +zpdesc_get (newzpdesc); +if (zpdesc_zone (newzpdesc) !=zpdesc_zone (zpdesc)) { +zpdesc_dec_zone_page_state (zpdesc); +zpdesc_inc_zone_page_state (newzpdesc); }reset_page (page); -put_page (page); +zpdesc_put (zpdesc);return MIGRATEPAGE_SUCCESS ; }
Signature · 1 line
2.43.0
PATCH v4 11/22 mm/zsmalloc: rename reset_page to reset_zpdesc and use zpdesc in it
In reply to [email protected]From: Alex Shi <[email protected]>
zpdesc.zspage matches with page.private, zpdesc.next matches with page.index. They will be reset in reset_page() wich is called prior to free base pages of a zspage.
Use zpdesc to replace page struct and rename it to reset_zpdesc(), few page helper still left since they are used too widely.
--- mm/zsmalloc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)
mm/zsmalloc.c +7 -5
@@ -865,12 +865,14 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,return true; } -static void reset_page (struct page *page) +static void reset_zpdesc (struct zpdesc *zpdesc) { +struct page *page =zpdesc_page (zpdesc); +__ClearPageMovable (page);ClearPagePrivate (page); -set_page_private (page,0 ); - page->index =0 ; + zpdesc->zspage =NULL ; + zpdesc->next =NULL ;reset_first_obj_offset (page);__ClearPageZsmalloc (page); }
@@ -910,7 +912,7 @@ static void __free_zspage(struct zs_pool *pool, struct size_class *class, do { VM_BUG_ON_PAGE(!PageLocked(page), page); next = get_next_page(page); - reset_page(page); + reset_zpdesc(page_zpdesc(page)); unlock_page(page); dec_zone_page_state(page, NR_ZSPAGES); put_page(page);
@@ -1899,7 +1901,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,zpdesc_inc_zone_page_state (newzpdesc); } -reset_page (page); +reset_zpdesc (zpdesc);zpdesc_put (zpdesc);return MIGRATEPAGE_SUCCESS ;
Signature · 1 line
2.43.0
PATCH v4 12/22 mm/zsmalloc: convert __free_zspage() to use zdsesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Introduce zpdesc_is_locked() and convert __free_zspage() to use zpdesc.
--- mm/zpdesc.h | 4 ++++ mm/zsmalloc.c | 20 ++++++++++---------- 2 files changed, 14 insertions(+), 10 deletions(-)
mm/zpdesc.h +4 -0
@@ -119,4 +119,8 @@ static inline struct zone *zpdesc_zone(struct zpdesc *zpdesc) return page_zone(zpdesc_page(zpdesc)); } +static inline bool zpdesc_is_locked (struct zpdesc *zpdesc) +{ +return PageLocked (zpdesc_page (zpdesc)); +}#endif
mm/zsmalloc.c +10 -10
@@ -901,23 +901,23 @@ static int trylock_zspage(struct zspage *zspage)static void __free_zspage (struct zs_pool *pool,struct size_class *class,struct zspage *zspage) { -struct page *page, *next; +struct zpdesc *zpdesc, *next;assert_spin_locked (&class->lock);VM_BUG_ON (get_zspage_inuse (zspage));VM_BUG_ON (zspage->fullness !=ZS_INUSE_RATIO_0 ); - next = page =get_first_page (zspage); + next = zpdesc =get_first_zpdesc (zspage);do { -VM_BUG_ON_PAGE (!PageLocked (page), page); - next =get_next_page (page); -reset_zpdesc (page_zpdesc (page)); -unlock_page (page); -dec_zone_page_state (page,NR_ZSPAGES ); -put_page (page); - page = next; - }while (page !=NULL ); +VM_BUG_ON_PAGE (!zpdesc_is_locked (zpdesc),zpdesc_page (zpdesc)); + next =get_next_zpdesc (zpdesc); +reset_zpdesc (zpdesc); +zpdesc_unlock (zpdesc); +zpdesc_dec_zone_page_state (zpdesc); +zpdesc_put (zpdesc); + zpdesc = next; + }while (zpdesc !=NULL );cache_free_zspage (pool, zspage);
Signature · 1 line
2.43.0
PATCH v4 13/22 mm/zsmalloc: convert location_to_obj() to take zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
As all users of location_to_obj() now use zpdesc, convert location_to_obj() to take zpdesc.
--- mm/zsmalloc.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
mm/zsmalloc.c +6 -7
@@ -826,15 +826,15 @@ static void obj_to_zpdesc(unsigned long obj, struct zpdesc **zpdesc) }/** -* location_to_obj - get obj value encoded from (<page>, <obj_idx>) -* @page: page object resides in zspage +* location_to_obj - get obj value encoded from (<zpdesc>, <obj_idx>) +* @zpdesc: zpdesc object resides in zspage * @obj_idx: object index */ -static unsigned long location_to_obj (struct page *page,unsigned int obj_idx) +static unsigned long location_to_obj (struct zpdesc *zpdesc,unsigned int obj_idx) {unsigned long obj; - obj =page_to_pfn (page) <<OBJ_INDEX_BITS ; + obj =zpdesc_pfn (zpdesc) <<OBJ_INDEX_BITS ; obj |= obj_idx &OBJ_INDEX_MASK ;return obj;
@@ -1392,7 +1392,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,kunmap_atomic (vaddr);mod_zspage_inuse (zspage,1 ); - obj =location_to_obj (zpdesc_page (m_zpdesc), obj); + obj =location_to_obj (m_zpdesc, obj);record_obj (handle, obj);return obj;
@@ -1879,8 +1879,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page, old_obj =handle_to_obj (handle);obj_to_location (old_obj, &dummy, &obj_idx); - new_obj = (unsigned long )location_to_obj (zpdesc_page (newzpdesc), - obj_idx); + new_obj = (unsigned long )location_to_obj (newzpdesc, obj_idx);record_obj (handle, new_obj); } }
Signature · 1 line
2.43.0
PATCH v4 14/22 mm/zsmalloc: convert migrate_zspage() to use zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Use get_first_zpdesc/get_next_zpdesc to replace get_first/next_page. No functional change.
--- mm/zsmalloc.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
mm/zsmalloc.c +4 -4
@@ -1636,14 +1636,14 @@ static void migrate_zspage(struct zs_pool *pool, struct zspage *src_zspage,unsigned long used_obj, free_obj;unsigned long handle;int obj_idx =0 ; -struct page *s_page =get_first_page (src_zspage); +struct zpdesc *s_zpdesc =get_first_zpdesc (src_zspage);struct size_class *class = pool->size_class[src_zspage->class];while (1 ) { - handle =find_alloced_obj (class,page_zpdesc (s_page), &obj_idx); + handle =find_alloced_obj (class, s_zpdesc, &obj_idx);if (!handle) { - s_page =get_next_page (s_page); -if (!s_page) + s_zpdesc =get_next_zpdesc (s_zpdesc); +if (!s_zpdesc)break ; obj_idx =0 ;continue ;
Signature · 1 line
2.43.0
PATCH v4 15/22 mm/zsmalloc: convert get_zspage() to take zpdesc
In reply to [email protected]From: Hyeonggon Yoo <[email protected]>
Now that all users except get_next_page() (which will be removed in later patch) use zpdesc, convert get_zspage() to take zpdesc instead of page.
--- mm/zsmalloc.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)
mm/zsmalloc.c +10 -10
@@ -779,9 +779,9 @@ static int fix_fullness_group(struct size_class *class, struct zspage *zspage)return newfg; } -static struct zspage *get_zspage (struct page *page) +static struct zspage *get_zspage (struct zpdesc *zpdesc) { -struct zspage *zspage = (struct zspage *)page_private (page); +struct zspage *zspage = zpdesc->zspage;BUG_ON (zspage->magic !=ZSPAGE_MAGIC );return zspage;
@@ -789,7 +789,7 @@ static struct zspage *get_zspage(struct page *page)static struct page *get_next_page (struct page *page) { -struct zspage *zspage =get_zspage (page); +struct zspage *zspage =get_zspage (page_zpdesc (page));if (unlikely (ZsHugePage (zspage)))return NULL ;
@@ -799,7 +799,7 @@ static struct page *get_next_page(struct page *page)static struct zpdesc *get_next_zpdesc (struct zpdesc *zpdesc) { -struct zspage *zspage =get_zspage (zpdesc_page (zpdesc)); +struct zspage *zspage =get_zspage (zpdesc);if (unlikely (ZsHugePage (zspage)))return NULL ;
@@ -849,7 +849,7 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj, unsigned long *phandle) {unsigned long handle; -struct zspage *zspage =get_zspage (zpdesc_page (zpdesc)); +struct zspage *zspage =get_zspage (zpdesc);if (unlikely (ZsHugePage (zspage))) {VM_BUG_ON_PAGE (!is_first_zpdesc (zpdesc),zpdesc_page (zpdesc));
@@ -1265,7 +1265,7 @@ void *zs_map_object(struct zs_pool *pool, unsigned long handle,read_lock (&pool->migrate_lock); obj =handle_to_obj (handle);obj_to_location (obj, &zpdesc, &obj_idx); - zspage =get_zspage (zpdesc_page (zpdesc)); + zspage =get_zspage (zpdesc);/* * migration cannot move any zpages in this zspage. Here, class->lock
@@ -1315,7 +1315,7 @@ void zs_unmap_object(struct zs_pool *pool, unsigned long handle) obj =handle_to_obj (handle);obj_to_location (obj, &zpdesc, &obj_idx); - zspage =get_zspage (zpdesc_page (zpdesc)); + zspage =get_zspage (zpdesc); class =zspage_class (pool, zspage); off =offset_in_page (class->size * obj_idx);
@@ -1479,7 +1479,7 @@ static void obj_free(int class_size, unsigned long obj)obj_to_location (obj, &f_zpdesc, &f_objidx); f_offset =offset_in_page (class_size * f_objidx); - zspage =get_zspage (zpdesc_page (f_zpdesc)); + zspage =get_zspage (f_zpdesc); vaddr =zpdesc_kmap_atomic (f_zpdesc); link = (struct link_free *)(vaddr + f_offset);
@@ -1513,7 +1513,7 @@ void zs_free(struct zs_pool *pool, unsigned long handle)read_lock (&pool->migrate_lock); obj =handle_to_obj (handle);obj_to_zpdesc (obj, &f_zpdesc); - zspage =get_zspage (zpdesc_page (f_zpdesc)); + zspage =get_zspage (f_zpdesc); class =zspage_class (pool, zspage);spin_lock (&class->lock);read_unlock (&pool->migrate_lock);
@@ -1846,7 +1846,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,__SetPageZsmalloc (zpdesc_page (newzpdesc));/* The page is locked, so this pointer must remain valid */ - zspage =get_zspage (zpdesc_page (zpdesc)); + zspage =get_zspage (zpdesc); pool = zspage->pool;/*
Signature · 1 line
2.43.0
PATCH v4 16/22 mm/zsmalloc: convert SetZsPageMovable and remove unused funcs
In reply to [email protected]From: Alex Shi <[email protected]>
Convert SetZsPageMovable() to use zpdesc, and then remove unused funcs:get_next_page/get_first_page/is_first_page.
--- mm/zsmalloc.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-)
mm/zsmalloc.c +5 -28
@@ -468,11 +468,6 @@ static DEFINE_PER_CPU(struct mapping_area, zs_map_area) = { .lock = INIT_LOCAL_LOCK(lock), }; -static __maybe_unused int is_first_page (struct page *page) -{ -return PagePrivate (page); -} -static int is_first_zpdesc (struct zpdesc *zpdesc) {return PagePrivate (zpdesc_page (zpdesc));
@@ -489,14 +484,6 @@ static inline void mod_zspage_inuse(struct zspage *zspage, int val) zspage->inuse += val; } -static inline struct page *get_first_page (struct zspage *zspage) -{ -struct page *first_page =zpdesc_page (zspage->first_zpdesc); - -VM_BUG_ON_PAGE (!is_first_page (first_page), first_page); -return first_page; -} -static struct zpdesc *get_first_zpdesc (struct zspage *zspage) {struct zpdesc *first_zpdesc = zspage->first_zpdesc;
@@ -787,16 +774,6 @@ static struct zspage *get_zspage(struct zpdesc *zpdesc)return zspage; } -static struct page *get_next_page (struct page *page) -{ -struct zspage *zspage =get_zspage (page_zpdesc (page)); - -if (unlikely (ZsHugePage (zspage))) -return NULL ; - -return (struct page *)page->index; -} -static struct zpdesc *get_next_zpdesc (struct zpdesc *zpdesc) {struct zspage *zspage =get_zspage (zpdesc);
@@ -1970,13 +1947,13 @@ static void init_deferred_free(struct zs_pool *pool)static void SetZsPageMovable (struct zs_pool *pool,struct zspage *zspage) { -struct page *page =get_first_page (zspage); +struct zpdesc *zpdesc =get_first_zpdesc (zspage);do { -WARN_ON (!trylock_page (page)); -__SetPageMovable (page, &zsmalloc_mops); -unlock_page (page); - }while ((page =get_next_page (page)) !=NULL ); +WARN_ON (!zpdesc_trylock (zpdesc)); +__zpdesc_set_movable (zpdesc, &zsmalloc_mops); +zpdesc_unlock (zpdesc); + }while ((zpdesc =get_next_zpdesc (zpdesc)) !=NULL ); }#else static inline void zs_flush_migration (struct zs_pool *pool) { }
Signature · 1 line
2.43.0
PATCH v4 17/22 mm/zsmalloc: convert get/set_first_obj_offset() to take zpdesc
In reply to [email protected]From: Alex Shi <[email protected]>
Now that all users of get/set_first_obj_offset() are converted to use zpdesc, convert them to take zpdesc.
--- mm/zpdesc.h | 7 ++++++- mm/zsmalloc.c | 36 ++++++++++++++++++------------------ 2 files changed, 24 insertions(+), 19 deletions(-)
mm/zpdesc.h +6 -1
@@ -15,6 +15,8 @@* @next: Next zpdesc in a zspage in zsmalloc zpool * @handle: For huge zspage in zsmalloc zpool * @zspage: Pointer to zspage in zsmalloc +* @first_obj_offset: First object offset in zsmalloc zpool +* @_refcount: Indirectly use by page migration * @memcg_data: Memory Control Group data. * * This struct overlays struct page for now. Do not modify without a good
@@ -31,7 +33,8 @@ struct zpdesc {unsigned long handle; };struct zspage *zspage; -unsigned long _zp_pad_1; +unsigned int first_obj_offset; +atomic_t _refcount;#ifdef CONFIG_MEMCG unsigned long memcg_data;#endif
@@ -45,6 +48,8 @@ ZPDESC_MATCH(mapping, mops);ZPDESC_MATCH (index, next);ZPDESC_MATCH (index, handle);ZPDESC_MATCH (private, zspage); +ZPDESC_MATCH (page_type, first_obj_offset); +ZPDESC_MATCH (_refcount, _refcount);#ifdef CONFIG_MEMCG ZPDESC_MATCH (memcg_data, memcg_data);#endif
mm/zsmalloc.c +18 -18
@@ -20,8 +20,8 @@* zpdesc->next: links together all component pages of a zspage * For the huge page, this is always 0, so we use this field * to store handle. -* page->page_type: PG_zsmalloc, lower 16 bit locate the first object -* offset in a subpage of a zspage +* 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
@@ -494,26 +494,26 @@ static struct zpdesc *get_first_zpdesc(struct zspage *zspage)#define FIRST_OBJ_PAGE_TYPE_MASK 0xffff -static inline void reset_first_obj_offset (struct page *page) +static inline void reset_first_obj_offset (struct zpdesc *zpdesc) { -VM_WARN_ON_ONCE (!PageZsmalloc (page)); - page->page_type |=FIRST_OBJ_PAGE_TYPE_MASK ; +VM_WARN_ON_ONCE (!PageZsmalloc (zpdesc_page (zpdesc))); + zpdesc->first_obj_offset |=FIRST_OBJ_PAGE_TYPE_MASK ; } -static inline unsigned int get_first_obj_offset (struct page *page) +static inline unsigned int get_first_obj_offset (struct zpdesc *zpdesc) { -VM_WARN_ON_ONCE (!PageZsmalloc (page)); -return page->page_type &FIRST_OBJ_PAGE_TYPE_MASK ; +VM_WARN_ON_ONCE (!PageZsmalloc (zpdesc_page (zpdesc))); +return zpdesc->first_obj_offset &FIRST_OBJ_PAGE_TYPE_MASK ; } -static inline void set_first_obj_offset (struct page *page,unsigned int offset) +static inline void set_first_obj_offset (struct zpdesc *zpdesc,unsigned int offset) {/* With 16 bit available, we can support offsets into 64 KiB pages. */ BUILD_BUG_ON (PAGE_SIZE >SZ_64K ); -VM_WARN_ON_ONCE (!PageZsmalloc (page)); +VM_WARN_ON_ONCE (!PageZsmalloc (zpdesc_page (zpdesc)));VM_WARN_ON_ONCE (offset & ~FIRST_OBJ_PAGE_TYPE_MASK ); - page->page_type &= ~FIRST_OBJ_PAGE_TYPE_MASK ; - page->page_type |= offset &FIRST_OBJ_PAGE_TYPE_MASK ; + zpdesc->first_obj_offset &= ~FIRST_OBJ_PAGE_TYPE_MASK ; + zpdesc->first_obj_offset |= offset &FIRST_OBJ_PAGE_TYPE_MASK ; }static inline unsigned int get_freeobj (struct zspage *zspage)
@@ -850,7 +850,7 @@ static void reset_zpdesc(struct zpdesc *zpdesc)ClearPagePrivate (page); zpdesc->zspage =NULL ; zpdesc->next =NULL ; -reset_first_obj_offset (page); +reset_first_obj_offset (zpdesc);__ClearPageZsmalloc (page); }
@@ -934,7 +934,7 @@ static void init_zspage(struct size_class *class, struct zspage *zspage)struct link_free *link;void *vaddr; -set_first_obj_offset (zpdesc_page (zpdesc), off); +set_first_obj_offset (zpdesc, off); vaddr =zpdesc_kmap_atomic (zpdesc); link = (struct link_free *)vaddr + off /sizeof (*link);
@@ -1589,7 +1589,7 @@ static unsigned long find_alloced_obj(struct size_class *class,unsigned long handle =0 ;void *addr =zpdesc_kmap_atomic (zpdesc); - offset =get_first_obj_offset (zpdesc_page (zpdesc)); + offset =get_first_obj_offset (zpdesc); offset += class->size * index;while (offset <PAGE_SIZE ) {
@@ -1784,8 +1784,8 @@ static void replace_sub_page(struct size_class *class, struct zspage *zspage, }while ((zpdesc =get_next_zpdesc (zpdesc)) !=NULL );create_page_chain (class, zspage, zpdescs); - first_obj_offset =get_first_obj_offset (zpdesc_page (oldzpdesc)); -set_first_obj_offset (zpdesc_page (newzpdesc), first_obj_offset); + first_obj_offset =get_first_obj_offset (oldzpdesc); +set_first_obj_offset (newzpdesc, first_obj_offset);if (unlikely (ZsHugePage (zspage))) newzpdesc->handle = oldzpdesc->handle;__zpdesc_set_movable (newzpdesc, &zsmalloc_mops);
@@ -1840,7 +1840,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,/* the migrate_write_lock protects zpage access via zs_map_object */ migrate_write_lock (zspage); - offset =get_first_obj_offset (zpdesc_page (zpdesc)); + offset =get_first_obj_offset (zpdesc); s_addr =zpdesc_kmap_atomic (zpdesc);/*
Signature · 1 line
2.43.0
PATCH v4 18/22 mm/zsmalloc: introduce __zpdesc_clear_movable
In reply to [email protected]From: Alex Shi <[email protected]>
Add a helper __zpdesc_clear_movable() for __ClearPageMovable(), and use it in callers to make code clear.
--- mm/zpdesc.h | 5 +++++ mm/zsmalloc.c | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-)
mm/zpdesc.h +5 -0
@@ -114,6 +114,11 @@ static inline void __zpdesc_set_movable(struct zpdesc *zpdesc,__SetPageMovable (zpdesc_page (zpdesc), mops); } +static inline void __zpdesc_clear_movable (struct zpdesc *zpdesc) +{ +__ClearPageMovable (zpdesc_page (zpdesc)); +} +static inline bool zpdesc_is_isolated (struct zpdesc *zpdesc) {return PageIsolated (zpdesc_page (zpdesc));
mm/zsmalloc.c +1 -1
@@ -846,7 +846,7 @@ static void reset_zpdesc(struct zpdesc *zpdesc) {struct page *page =zpdesc_page (zpdesc); -__ClearPageMovable (page); +__zpdesc_clear_movable (zpdesc);ClearPagePrivate (page); zpdesc->zspage =NULL ; zpdesc->next =NULL ;
Signature · 1 line
2.43.0
Sergey Senozhatsky
maintainerRe: [PATCH v4 18/22] mm/zsmalloc: introduce __zpdesc_clear_movable
On (24/07/29 19:25), [email protected] wrote:
[..]
Quoted 4 lines
mm/zpdesc.h +4
-0+static inline void __zpdesc_clear_movable (struct zpdesc *zpdesc) +{ +__ClearPageMovable (zpdesc_page (zpdesc)); +}
[..]
mm/zsmalloc.c static void reset_zpdesc(struct zpdesc *zpdesc) · line 850
Quoted 7 lines
mm/zsmalloc.c +1
-1@@ -846,7 +846,7 @@ static void reset_zpdesc(struct zpdesc *zpdesc) {struct page *page =zpdesc_page (zpdesc); -__ClearPageMovable (page); +__zpdesc_clear_movable (zpdesc);ClearPagePrivate (page);
Just a quick question, I see that you wrote wrappers for pretty much everything, including SetPagePrivate(), but not for ClearPagePrivate()?
Re: [PATCH v4 18/22] mm/zsmalloc: introduce __zpdesc_clear_movable
Quoted 13 lines · Sergey Senozhatsky
On (24/07/29 19:25), [email protected] wrote:
[..]Quoted 4 lines
mm/zpdesc.h +4
-0+static inline void __zpdesc_clear_movable (struct zpdesc *zpdesc) +{ +__ClearPageMovable (zpdesc_page (zpdesc)); +}[..]
Quoted 4 lines
mm/zsmalloc.c +0
-0@@ -846,7 +846,7 @@ static void reset_zpdesc(struct zpdesc *zpdesc) {struct page *page =zpdesc_page (zpdesc);
Quoted 7 lines
mm/zsmalloc.c static void reset_zpdesc(struct zpdesc *zpdesc) · line 850
Quoted 3 lines
mm/zsmalloc.c +1
-1-__ClearPageMovable (page); +__zpdesc_clear_movable (zpdesc);ClearPagePrivate (page);Just a quick question, I see that you wrote wrappers for pretty much everything, including
SetPagePrivate(), but not forClearPagePrivate()?
Hi Sergey,
Thanks for comment!
Yes, it's better to have one for clear, I'll sent a patch soon.
Alex
PATCH v4 19/22 mm/zsmalloc: introduce __zpdesc_clear_zsmalloc
In reply to [email protected]From: Alex Shi <[email protected]>
Add a helper __zpdesc_clear_zsmalloc() for __ClearPageZsmalloc(), and use it in callers to make code clear.
--- mm/zpdesc.h | 5 +++++ mm/zsmalloc.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-)
mm/zpdesc.h +5 -0
@@ -119,6 +119,11 @@ static inline void __zpdesc_clear_movable(struct zpdesc *zpdesc)__ClearPageMovable (zpdesc_page (zpdesc)); } +static inline void __zpdesc_clear_zsmalloc (struct zpdesc *zpdesc) +{ +__ClearPageZsmalloc (zpdesc_page (zpdesc)); +} +static inline bool zpdesc_is_isolated (struct zpdesc *zpdesc) {return PageIsolated (zpdesc_page (zpdesc));
mm/zsmalloc.c +2 -2
@@ -851,7 +851,7 @@ static void reset_zpdesc(struct zpdesc *zpdesc) zpdesc->zspage =NULL ; zpdesc->next =NULL ;reset_first_obj_offset (zpdesc); -__ClearPageZsmalloc (page); +__zpdesc_clear_zsmalloc (zpdesc); }static int trylock_zspage (struct zspage *zspage)
@@ -1024,7 +1024,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,if (!zpdesc) {while (--i >=0 ) {zpdesc_dec_zone_page_state (zpdescs[i]); -__ClearPageZsmalloc (zpdesc_page (zpdescs[i])); +__zpdesc_clear_zsmalloc (zpdescs[i]);free_zpdesc (zpdescs[i]); }cache_free_zspage (pool, zspage);
Signature · 1 line
2.43.0
PATCH v4 20/22 mm/zsmalloc: introduce __zpdesc_set_zsmalloc()
In reply to [email protected]From: Alex Shi <[email protected]>
Add a helper __zpdesc_set_zsmalloc() for __SetPageZsmalloc(), and use it in callers to make code clear.
--- mm/zpdesc.h | 5 +++++ mm/zsmalloc.c | 4 ++-- 2 files changed, 7 insertions(+), 2 deletions(-)
mm/zpdesc.h +5 -0
@@ -119,6 +119,11 @@ static inline void __zpdesc_clear_movable(struct zpdesc *zpdesc)__ClearPageMovable (zpdesc_page (zpdesc)); } +static inline void __zpdesc_set_zsmalloc (struct zpdesc *zpdesc) +{ +__SetPageZsmalloc (zpdesc_page (zpdesc)); +} +static inline void __zpdesc_clear_zsmalloc (struct zpdesc *zpdesc) {__ClearPageZsmalloc (zpdesc_page (zpdesc));
mm/zsmalloc.c +2 -2
@@ -1030,7 +1030,7 @@ static struct zspage *alloc_zspage(struct zs_pool *pool,cache_free_zspage (pool, zspage);return NULL ; } -__SetPageZsmalloc (zpdesc_page (zpdesc)); +__zpdesc_set_zsmalloc (zpdesc);zpdesc_inc_zone_page_state (zpdesc); zpdescs[i] = zpdesc;
@@ -1820,7 +1820,7 @@ static int zs_page_migrate(struct page *newpage, struct page *page,VM_BUG_ON_PAGE (!zpdesc_is_isolated (zpdesc),zpdesc_page (zpdesc));/* We're committed, tell the world that this is a Zsmalloc page. */ -__SetPageZsmalloc (zpdesc_page (newzpdesc)); +__zpdesc_set_zsmalloc (newzpdesc);/* The page is locked, so this pointer must remain valid */ zspage =get_zspage (zpdesc);
Signature · 1 line
2.43.0
Re: [PATCH v4 20/22] mm/zsmalloc: introduce __zpdesc_set_zsmalloc()
Quoted 4 lines · On Mon, Jul 29, 2024 at 07:25:32PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
Add a helper
__zpdesc_set_zsmalloc()for__SetPageZsmalloc(), and use it in callers to make code clear.
Definitely just fold this into the prior patch. It effectively does the same thing.
Re: [PATCH v4 20/22] mm/zsmalloc: introduce __zpdesc_set_zsmalloc()
Quoted 8 lines · Vishal Moola
Quoted 4 lines · On Mon, Jul 29, 2024 at 07:25:32PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
Add a helper
__zpdesc_set_zsmalloc()for__SetPageZsmalloc(), and use it in callers to make code clear.Definitely just fold this into the prior patch. It effectively does the same thing.
will merge them into one.
Thanks for comments!
PATCH v4 21/22 mm/zsmalloc: fix build warning from lkp testing
In reply to [email protected]From: Alex Shi <[email protected]>
LKP reported the following warning w/o CONFIG_DEBUG_VM:
mm/zsmalloc.c:471:12: warning: function 'is_first_zpdesc' is not needed and will not be emitted [-Wunneeded-internal-declaration]
To remove this warning, better to incline the function is_first_zpdesc
--- mm/zsmalloc.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
mm/zsmalloc.c +1 -1
@@ -468,7 +468,7 @@ static DEFINE_PER_CPU(struct mapping_area, zs_map_area) = { .lock = INIT_LOCAL_LOCK(lock), }; -static int is_first_zpdesc (struct zpdesc *zpdesc) +static inline bool is_first_zpdesc (struct zpdesc *zpdesc) {return PagePrivate (zpdesc_page (zpdesc)); }
Signature · 1 line
2.43.0
Re: [PATCH v4 21/22] mm/zsmalloc: fix build warning from lkp testing
Quoted 6 lines · On Mon, Jul 29, 2024 at 07:25:33PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
LKP reported the following warning w/o
CONFIG_DEBUG_VM:
mm/zsmalloc.c:471:12: warning: function 'is_first_zpdesc' is not needed and will not be emitted [-Wunneeded-internal-declaration]
To remove this warning, better to incline the functionis_first_zpdesc
In future iterations of the series, just fold this into the patch its fixing. It makes reviewing easier.
Re: [PATCH v4 21/22] mm/zsmalloc: fix build warning from lkp testing
Quoted 11 lines · Vishal Moola
Quoted 6 lines · On Mon, Jul 29, 2024 at 07:25:33PM +0800, [email protected] wrote:
From: Alex Shi <[email protected]>
LKP reported the following warning w/o
CONFIG_DEBUG_VM:
mm/zsmalloc.c:471:12: warning: function 'is_first_zpdesc' is not needed and will not be emitted [-Wunneeded-internal-declaration]
To remove this warning, better to incline the functionis_first_zpdescIn future iterations of the series, just fold this into the patch its fixing. It makes reviewing easier.
Yes, thanks for comments!
PATCH v4 22/22 mm/zsmalloc: update comments for page->zpdesc changes
In reply to [email protected]From: Alex Shi <[email protected]>
--- mm/zsmalloc.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)
mm/zsmalloc.c +7 -7
@@ -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)))
Signature · 1 line
2.43.0
Sergey Senozhatsky
maintainerRe: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 3 lines · On (24/07/29 19:25), [email protected] wrote:
From: Alex Shi <[email protected]>
Usually some simple commit message is still expected.
Re: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 6 lines · Sergey Senozhatsky
Quoted 3 lines · On (24/07/29 19:25), [email protected] wrote:
From: Alex Shi <[email protected]>
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
maintainerRe: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 6 lines · Alex Shi
Quoted 5 lines · Sergey Senozhatsky
Quoted 3 lines · On (24/07/29 19:25), [email protected] wrote:
From: Alex Shi <[email protected]>
Quoted 7 lines
Quoted 1 line
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()ascreate_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?
Re: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 11 lines · Sergey Senozhatsky
Quoted 10 lines · Alex Shi
Quoted 6 lines · Sergey Senozhatsky
Quoted 3 lines · On (24/07/29 19:25), [email protected] wrote:
From: Alex Shi <[email protected]>
Usually some simple commit message is still expected.
Uh, my fault. Just forgive this part, is the following log fine?
Quoted 7 lines
Quoted 3 lines
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()ascreate_zpdesc_chain().A bit of a different thing, still documentation related tho: do we want to do something about comments that mention
page_lockinzsmalloc.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:
mm/zsmalloc.c +2 -0
@@ -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
maintainerRe: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 8 lines · Alex Shi
Quoted 3 lines
A bit of a different thing, still documentation related tho: do we want to do something about comments that mention
page_lockinzsmalloc.c?Good question!
There are some comments mentioned about the
page_lockin 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"?
Re: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 10 lines · Sergey Senozhatsky
Quoted 8 lines · Alex Shi
Quoted 3 lines
A bit of a different thing, still documentation related tho: do we want to do something about comments that mention
page_lockinzsmalloc.c?Good question!
There are some comments mentioned about the
page_lockin the file, but missed in the header of file, so how about the following adding:
Quoted 6 lines
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.
Re: [PATCH v4 22/22] mm/zsmalloc: update comments for page->zpdesc changes
Quoted 13 lines · Matthew Wilcox
Quoted 12 lines · Sergey Senozhatsky
Quoted 8 lines · Alex Shi
Quoted 3 lines
A bit of a different thing, still documentation related tho: do we want to do something about comments that mention
page_lockinzsmalloc.c?Good question!
There are some comments mentioned about the
page_lockin the file, but missed in the header of file, so how about the following adding:And e.g. things like
Quoted 7 lines
Quoted 4 lines
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 <[email protected]>
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().
--- mm/zsmalloc.c | 47 ++++++++++++++++++++++++++--------------------- 1 file changed, 26 insertions(+), 21 deletions(-)
mm/zsmalloc.c +26 -21
@@ -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)
Signature · 1 line
2.43.0
PATCH 23/23 mm/zsmalloc: introduce zpdesc_clear_first() helper
In reply to [email protected]From: Alex Shi <[email protected]>
Like the zpdesc_set_first(), introduce zpdesc_clear_first() helper for ClearPagePrivate(), then clean up a 'struct page' usage in reset_zpdesc().
To: [email protected]
To: [email protected]
To: Andrew Morton <[email protected]>
To: Sergey Senozhatsky <[email protected]>
To: Minchan Kim <[email protected]>
--- mm/zsmalloc.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)
mm/zsmalloc.c +6 -3
@@ -259,6 +259,11 @@ static inline void zpdesc_set_first(struct zpdesc *zpdesc)SetPagePrivate (zpdesc_page (zpdesc)); } +static inline void zpdesc_clear_first (struct zpdesc *zpdesc) +{ +ClearPagePrivate (zpdesc_page (zpdesc)); +} +static inline void zpdesc_inc_zone_page_state (struct zpdesc *zpdesc) {inc_zone_page_state (zpdesc_page (zpdesc),NR_ZSPAGES );
@@ -844,10 +849,8 @@ static inline bool obj_allocated(struct zpdesc *zpdesc, void *obj,static void reset_zpdesc (struct zpdesc *zpdesc) { -struct page *page =zpdesc_page (zpdesc); -__zpdesc_clear_movable (zpdesc); -ClearPagePrivate (page); +zpdesc_clear_first (zpdesc); zpdesc->zspage =NULL ; zpdesc->next =NULL ;reset_first_obj_offset (zpdesc);
Signature · 1 line
2.43.0