| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <429f4ea1-b94a-455a-a4e7-27e1536c0b7c@gmail.com>
Date: Mon, 1 Jul 2024 11:40:04 +0800
From: Alex Shi <seakeel@...il.com>
To: Hyeonggon Yoo <42.hyeyoo@...il.com>, alexs@...nel.org
Cc: Vitaly Wool <vitaly.wool@...sulko.com>, Miaohe Lin
<linmiaohe@...wei.com>, Andrew Morton <akpm@...ux-foundation.org>,
linux-kernel@...r.kernel.org, linux-mm@...ck.org, minchan@...nel.org,
willy@...radead.org, senozhatsky@...omium.org, david@...hat.com,
Nhat Pham <nphamcs@...il.com>, Yosry Ahmed <yosryahmed@...gle.com>
Subject: Re: [PATCH 01/20] mm/zsmalloc: add zpdesc memory descriptor for
zswap.zpool
Ops,
Sorry for missing Yosry and Nhat. How stupid I am!
Here is the first patch on lore: https://lore.kernel.org/all/20240628031138.429622-1-alexs@kernel.org/
Sorry for this!
Alex
On 7/1/24 11:03 AM, Alex Shi wrote:
>
>
> On 6/30/24 9:45 PM, Hyeonggon Yoo wrote:
>> On Fri, Jun 28, 2024 at 12:06 PM <alexs@...nel.org> wrote:
>>>
>>> From: Alex Shi <alexs@...nel.org>
>>>
>>> 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@...il.com>
>>> Signed-off-by: Alex Shi <alexs@...nel.org>
>>> ---
>>> 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@...nel.org>
>>> + * Hyeonggon Yoo <42.hyeyoo@...il.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: 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@mail.gmail.com/
>
> 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
Powered by blists - more mailing lists