| Message ID | 20240729112534.3416707-2-alexs@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/zsmalloc: add zpdesc memory descriptor for zswap.zpool | expand |
On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote: > From: Alex Shi <alexs@kernel.org> 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 :) > 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." > > Originally-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > Signed-off-by: Alex Shi <alexs@kernel.org> > --- > mm/zpdesc.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ > mm/zsmalloc.c | 21 ++++++++-------- > 2 files changed, 76 insertions(+), 11 deletions(-) > create mode 100644 mm/zpdesc.h > > diff --git a/mm/zpdesc.h b/mm/zpdesc.h > new file mode 100644 > index 000000000000..2dbef231f616 > --- /dev/null > +++ b/mm/zpdesc.h > @@ -0,0 +1,66 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* zpdesc.h: zswap.zpool memory descriptor > + * > + * Written by Alex Shi <alexs@kernel.org> > + * Hyeonggon Yoo <42.hyeyoo@gmail.com> > + */ > +#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. > + * 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. > + * 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; 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. > + 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. > +#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 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.
On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote: > +/* > + * 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 > +}; 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.
On 8/3/24 2:52 AM, Vishal Moola wrote: > On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote: >> From: Alex Shi <alexs@kernel.org> > > 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! :) > >> 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." >> >> Originally-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> >> Signed-off-by: Alex Shi <alexs@kernel.org> >> --- >> mm/zpdesc.h | 66 +++++++++++++++++++++++++++++++++++++++++++++++++++ >> mm/zsmalloc.c | 21 ++++++++-------- >> 2 files changed, 76 insertions(+), 11 deletions(-) >> create mode 100644 mm/zpdesc.h >> >> diff --git a/mm/zpdesc.h b/mm/zpdesc.h >> new file mode 100644 >> index 000000000000..2dbef231f616 >> --- /dev/null >> +++ b/mm/zpdesc.h >> @@ -0,0 +1,66 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* zpdesc.h: zswap.zpool memory descriptor >> + * >> + * Written by Alex Shi <alexs@kernel.org> >> + * Hyeonggon Yoo <42.hyeyoo@gmail.com> >> + */ >> +#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. >> + * > > 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. > >> + * 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; > > 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! > >> + 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. > >> +#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 > > 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. 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
On 8/3/24 3:30 AM, Matthew Wilcox wrote: > On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote: >> +/* >> + * 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 >> +}; > > 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; > }; > Yes, we still have a huge gap between this target. > 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 > > 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.
On Mon, Aug 05, 2024 at 12:06:24PM +0800, Alex Shi wrote: > > > On 8/3/24 2:52 AM, Vishal Moola wrote: > > On Mon, Jul 29, 2024 at 07:25:13PM +0800, alexs@kernel.org wrote: > >> From: Alex Shi <alexs@kernel.org> > > > >> + 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 > > > > 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. > > 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 ? 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. > Thanks > Alex >
On 8/9/24 2:21 AM, Vishal Moola wrote: >> 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 ? > 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. Right, will add some comments for this. Thanks!
diff --git a/mm/zpdesc.h b/mm/zpdesc.h new file mode 100644 index 000000000000..2dbef231f616 --- /dev/null +++ b/mm/zpdesc.h @@ -0,0 +1,66 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* zpdesc.h: zswap.zpool memory descriptor + * + * Written by Alex Shi <alexs@kernel.org> + * Hyeonggon Yoo <42.hyeyoo@gmail.com> + */ +#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 diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c index 5d6581ab7c07..a532851025f9 100644 --- a/mm/zsmalloc.c +++ b/mm/zsmalloc.c @@ -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);