linux-kernel - Re: [PATCH] mm/page_alloc: make calling prep_compound

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]

Date:   Wed, 15 Jun 2022 15:44:06 +0800
From:   Miaohe Lin <linmiaohe@...wei.com>
To:     Matthew Wilcox <willy@...radead.org>
CC:     Joao Martins <joao.m.martins@...cle.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        <linux-mm@...ck.org>, <linux-kernel@...r.kernel.org>,
        Dan Williams <dan.j.williams@...el.com>
Subject: Re: [PATCH] mm/page_alloc: make calling prep_compound_head more
 reliable

On 2022/6/14 21:13, Matthew Wilcox wrote:
> On Wed, Jun 08, 2022 at 08:17:35PM +0800, Miaohe Lin wrote:
>> +++ b/mm/page_alloc.c
>> @@ -6771,13 +6771,18 @@ static void __ref memmap_init_compound(struct page *head,
>>                 set_page_count(page, 0);
>>
>>                 /*
>> -                * The first tail page stores compound_mapcount_ptr() and
>> -                * compound_order() and the second tail page stores
>> -                * compound_pincount_ptr(). Call prep_compound_head() after
>> -                * the first and second tail pages have been initialized to
>> -                * not have the data overwritten.
>> +                * The first tail page stores compound_mapcount_ptr(),
>> +                * compound_order() and compound_pincount_ptr(). Call
>> +                * prep_compound_head() after the first tail page have
>> +                * been initialized to not have the data overwritten.
>> +                *
>> +                * Note the idea to make this right after we initialize
>> +                * the offending tail pages is trying to take advantage
>> +                * of the likelihood of those tail struct pages being
>> +                * cached given that we will read them right after in
>> +                * prep_compound_head().
> 
> It's not that we'll read them again, it's that the cacheline will still
> be in cache, and therefore dirty.

Thanks for pointing this out.

> 
> Honestly, I don't think we need this extra explanation in a comment.
> Just change the first paragraph to reflect reality and leave it at that.

Will do it in next version if prep_compound_head is not moved outside loop.

> 
>>                  */
>> -               if (pfn == head_pfn + 2)
>> +               if (unlikely(pfn == head_pfn + 1))
> 
> We definitely don't need the unlikely here.

Could you please give me a more detailed explanation? IIUC, the above if condition
will only meet at a probability of 1/512. So unlikely tells the compiler to do some
optimization around it. Or am I miss something?

Thanks!

> 
>>                         prep_compound_head(head, order);
>>         }
>>  }
>>
>> Or am I miss something?
>>
>> Thanks!
>>
>>> .
>>>
>>
> 
> .
>