linux-kernel - Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <22e8cac7-2d3d-6099-9875-70e2ea007343@huawei.com>
Date:   Sat, 25 Jun 2022 11:18:09 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     Zach O'Keefe <zokeefe@...gle.com>
CC:     Muchun Song <songmuchun@...edance.com>,
        <akpm@...ux-foundation.org>, <shy828301@...il.com>,
        <willy@...radead.org>, <linux-mm@...ck.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 14/16] mm/huge_memory: fix comment of page_deferred_list

On 2022/6/25 1:09, Zach O'Keefe wrote:
> On 23 Jun 20:26, Miaohe Lin wrote:
>> On 2022/6/23 15:24, Muchun Song wrote:
>>> On Thu, Jun 23, 2022 at 01:06:25AM +0800, Miaohe Lin wrote:
>>>> The current comment is confusing because if global or memcg deferred list
>>>> in the second tail page is occupied by compound_head, why we still use
>>>> page[2].deferred_list here? I think it wants to say that Global or memcg
>>>> deferred list in the first tail page is occupied by compound_mapcount and
>>>> compound_pincount so we use the second tail page's deferred_list instead.
>>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@...wei.com>
>>>> ---
>>>>  include/linux/huge_mm.h | 4 ++--
>>>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>>>> index 12b297f9951d..2e8062b3417a 100644
>>>> --- a/include/linux/huge_mm.h
>>>> +++ b/include/linux/huge_mm.h
>>>> @@ -294,8 +294,8 @@ static inline bool thp_migration_supported(void)
>>>>  static inline struct list_head *page_deferred_list(struct page *page)
>>>>  {
>>>>  	/*
>>>> -	 * Global or memcg deferred list in the second tail pages is
>>>> -	 * occupied by compound_head.
>>>> +	 * Global or memcg deferred list in the first tail page is
>>>> +	 * occupied by compound_mapcount and compound_pincount.
>>>>  	 */
>>>
>>> The structure of "struct page" seems to have told us the information that
>>> we resue the 2nd tail page to be used as deferred_list. I am not sure the
>>
>> Yes, it does.
>>
>>> value of those comments. Maybe better to remove them?
>>
>> IMHO above comment tries to tell us why deferred list in the second tail page is used
>> instead of first tail page. But it should be fine to remove the above comments as they
>> don't seem to provide much info (thought I'm not really sure).
>>
>> Thanks.
>>
> 
> Just a suggestion - feel free to disregard. Maybe we don't need to repeat the
> comments in struct page, but maybe a "see organization of tail pages of compound
> page in "struct page" definition" would at least point new people to where this
> magic 2 comes from.  Maybe an obvious place to check after you're familiar with
> overloading struct page data for compound pages - but IMO it's not obvious for
> newcomers.

This is a good suggestion to me. Thanks!

> 
>>>
>>> Thanks.
>>>
>>>>  	return &page[2].deferred_list;
>>>>  }
>>>> -- 
>>>> 2.23.0
>>>>
>>>>
>>> .
>>>
>>
> .
>