Re: [PATCH] mm/sparsemem: pfn_to_page is not valid yet on SPARSEMEM

[David Hildenbrand]

On 06.02.20 15:07, Baoquan He wrote:
> On 02/06/20 at 02:55pm, David Hildenbrand wrote:
>> On 06.02.20 14:50, Baoquan He wrote:
>>> On 02/06/20 at 02:28pm, David Hildenbrand wrote:
>>>> On 06.02.20 13:53, Wei Yang wrote:
>>>>> When we use SPARSEMEM instead of SPARSEMEM_VMEMMAP, pfn_to_page()
>>>>> doesn't work before sparse_init_one_section() is called. This leads to a
>>>>> crash when hotplug memory.
>>>>>
>>>>> We should use memmap as it did.
>>>>>
>>>>> Fixes: ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
>>>>> Signed-off-by: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
>>>>> CC: Dan Williams <dan.j.williams@xxxxxxxxx>
>>>>> ---
>>>>>  mm/sparse.c | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>>>> index 5a8599041a2a..2efb24ff8f96 100644
>>>>> --- a/mm/sparse.c
>>>>> +++ b/mm/sparse.c
>>>>> @@ -882,7 +882,7 @@ int __meminit sparse_add_section(int nid, unsigned long start_pfn,
>>>>>  	 * Poison uninitialized struct pages in order to catch invalid flags
>>>>>  	 * combinations.
>>>>>  	 */
>>>>> -	page_init_poison(pfn_to_page(start_pfn), sizeof(struct page) * nr_pages);
>>>>> +	page_init_poison(memmap, sizeof(struct page) * nr_pages);
>>>>
>>>> If you add sub-sections that don't fall onto the start of the section,
>>>>
>>>> pfn_to_page(start_pfn) != memmap
>>>>
>>>> and your patch would break that under SPARSEMEM_VMEMMAP if I am not wrong.
>>>
>>> It returns the pfn_to_page(pfn) from __populate_section_memmap() and
>>> assign to memmap in vmemmap case, how come it breaks anything. Correct
>>> me if I was wrong.
>>
>> I'm sorry, I can't follow :) Can you elaborate?
>>
>> Was your comment targeted at why the old code cannot be broken or why
>> this patch cannot be broken?
> 
> Sorry for the confusion :-) the latter. I mean the returned memmap has been
> at the pfn_to_page(start_pfn) in SPARSEMEM_VMEMMAP case.

Yeah, at least for SPARSEMEM_VMEMMAP it is indeed right. Thanks :)


Now, about SPARSEMEM:

populate_section_memmap() does not care about nr_pages and will allocate
a memmap for the whole section. So, whenever we add sub-sections to a
section, we allocate a new memmap for the whole section. And we do
overwrite the memmap pointer in our section. ( sparse_add_section() )

That makes me assume that sub-section hot-add under SPARSEMEM is either

a) never enabled and only works with SPARSEMEM_VMEMMAP
b) horribly broken

And I think a) applies (looking at pfn_section_valid()). Therefore, we
don't have to care about sub-section hot-add specifics (and I would be
broken already)

Acked-by: David Hildenbrand <david@xxxxxxxxxx>

-- 
Thanks,

David / dhildenb