On 6/30/24 9:45 PM, Hyeonggon Yoo wrote:
> On Fri, Jun 28, 2024 at 12:06 PM <alexs@xxxxxxxxxx> wrote:
>>
>> From: Alex Shi <alexs@xxxxxxxxxx>
>>
>> The 1st patch introduces new memory decriptor zpdesc and rename
>> zspage.first_page to zspage.first_zpdesc, no functional change.
>>
>> Originally-by: Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
>> Signed-off-by: Alex Shi <alexs@xxxxxxxxxx>
>> ---
>> mm/zpdesc.h | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++
>> mm/zsmalloc.c | 19 ++++++++---------
>> 2 files changed, 66 insertions(+), 9 deletions(-)
>> create mode 100644 mm/zpdesc.h
>
> Hi Alex, thanks for your effort in pushing this forward!
>
>> diff --git a/mm/zpdesc.h b/mm/zpdesc.h
>> new file mode 100644
>> index 000000000000..a1ab5ebaa936
>> --- /dev/null
>> +++ b/mm/zpdesc.h
>> @@ -0,0 +1,56 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +/* zpdesc.h: zswap.zpool memory descriptor
>> + *
>> + * Written by Alex Shi <alexs@xxxxxxxxxx>
>> + * Hyeonggon Yoo <42.hyeyoo@xxxxxxxxx>
>> + */
>> +#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: Indirected used by page migration
>
> maybe Indirected -> Indirectly?
Hi Yoo,
Thanks for comments! Yes Indirectly is better. I will update it in next version.
>
>> + * @next: Next zpdesc in a zspage in zsmalloc zpool
>> + * @handle: For huge zspage in zsmalloc zpool
>> + * @zspage: Pointer to zspage in zsmalloc
>> + *
>> + * 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;
>> + unsigned long _zp_pad_1;
>
> for understanding, I think it'd be better to replace _zp_pad_1 with movable ops,
> because mops reuses this 'mapping' field.
Right, 'mops' looks a bit more clear.
>
>> + union {
>> + /* Next zpdescs in a zspage in zsmalloc zpool */
>> + struct zpdesc *next;
>> + /* For huge zspage in zsmalloc zpool */
>> + unsigned long handle;
>> + };
>> + struct zspage *zspage;
>
> There was a discussion with Yosry on including memcg_data on zpdesc
> even if it's not used at the moment.
>
> Maybe you can look at:
> https://lore.kernel.org/linux-mm/CAB=+i9Quz9iP2-Lq=oQfKVVnzPDtOaKMm=hUPbnRg5hRxH+qaA@xxxxxxxxxxxxxx/
Thanks for notice.
The memcg_data isn't used for zpdesc. And I have a bit confusion since Yosry said: "I think to drop memcg_data we need to enlighten the code that ...". So we actually don't need to have this unused member, is this right, Yosry?
>
>> diff --git a/mm/zsmalloc.c b/mm/zsmalloc.c
>> index fec1a39e5bbe..67bb80b7413a 100644
>> --- a/mm/zsmalloc.c
>> +++ b/mm/zsmalloc.c
>> @@ -13,17 +13,17 @@
>>
>> /*
>> * 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
>
> the comment for PG_owner_priv_1 can safely be removed as it's not used
> after commit a41ec880aa7b ("zsmalloc: move huge compressed obj from
> page to zspage")
Right, thanks for info!
>
>> @@ -948,7 +949,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))
>> @@ -1325,7 +1326,7 @@ static unsigned long obj_malloc(struct zs_pool *pool,
>> link->handle = handle;
>> else
>> /* record handle to page->index */
>> - zspage->first_page->index = handle;
>> + zspage->first_zpdesc->handle = handle;
>
> FYI this line seems to conflict with
> bcc6116e39f512 ("mm/zsmalloc: move record_obj() into obj_malloc()")
> on mm-unstable.
yes, a new commit made this conflict. will update this in next version.
Thanks
Alex
>
> Best,
> Hyeonggon
