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

[Joao Martins <joao.m.martins@oracle.com>]

On 6/7/22 19:32, Andrew Morton wrote:
> 
> Let's cc Joao.
> 
> On Tue, 7 Jun 2022 22:41:57 +0800 Miaohe Lin <linmiaohe@huawei.com> wrote:
> 
>> compound_pincount_ptr is stored at first tail page instead of second tail
>> page now.
> 
> "now"?  Some identifiable commit did this?
> 

I think this was in:

commit5232c63f46fd ("mm: Make compound_pincount always available")

>> And if it or some other field changes again in the future, data
>> overwritten might happen. Calling prep_compound_head() outside the loop
>> to prevent such possible issue. No functional change intended.
>>
>> ...
>>
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -6772,17 +6772,8 @@ static void __ref memmap_init_compound(struct page *head,
>>  		__init_zone_device_page(page, pfn, zone_idx, nid, pgmap);
>>  		prep_compound_tail(head, pfn - head_pfn);
>>  		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.
>> -		 */
>> -		if (pfn == head_pfn + 2)
>> -			prep_compound_head(head, order);
>>  	}
>> +	prep_compound_head(head, order);
>>  }
>>  
>>  void __ref memmap_init_zone_device(struct zone *zone,
> 

memmap_init_compound() is only called in pmem case.

The idea to make this /right after/ we initialize the offending tail pages has
to do with @altmap case wheere struct pages are placed in PMEM and thus taking
advantage of the likelyhood of those tail struct pages being cached given that
we will read them right after in prep_compound_head().

I agree with the general sentiment of making this 'more resilient' to compound
pages structure changes by moving prep_compound_head() after all tail pages are
initialized, although I need to express a concern about this making altmap possibly
being affected or regressed. Considering on 2M compound pages we will access first and
second tail pages /after/ initializing 32768 struct pages, or after touching/initializing
256K struct pages.