Re: [PATCH] mm/page_alloc: make calling prep_compound_head more reliable

[Miaohe Lin]

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!
>>
>>> .
>>>
>>
> 
> .
>