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

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

[Wei Yang]

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@linux.intel.com>
CC: Dan Williams <dan.j.williams@intel.com>
---
 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);
 
 	ms = __nr_to_section(section_nr);
 	set_section_nid(section_nr, nid);
-- 
2.17.1

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

[David Hildenbrand]

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@linux.intel.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> ---
>  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.

Instead of memmap, there would have to be something like

memmap + (start_pfn - SECTION_ALIGN_DOWN(start_pfn))

If I am not wrong :)

-- 
Thanks,

David / dhildenb

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

[Baoquan He]

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@linux.intel.com>
> > CC: Dan Williams <dan.j.williams@intel.com>
> > ---
> >  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.

> 
> Instead of memmap, there would have to be something like
> 
> memmap + (start_pfn - SECTION_ALIGN_DOWN(start_pfn))
> 
> If I am not wrong :)
> 
> -- 
> Thanks,
> 
> David / dhildenb

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

[David Hildenbrand]

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@linux.intel.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> ---
>>>  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?


-- 
Thanks,

David / dhildenb

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

[Baoquan He]

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@linux.intel.com>
> >>> CC: Dan Williams <dan.j.williams@intel.com>
> >>> ---
> >>>  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.

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@linux.intel.com>
>>>>> CC: Dan Williams <dan.j.williams@intel.com>
>>>>> ---
>>>>>  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@redhat.com>

-- 
Thanks,

David / dhildenb

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

[Wei Yang]

On Thu, Feb 06, 2020 at 03:37:40PM +0100, David Hildenbrand wrote:
>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@linux.intel.com>
>>>>>> CC: Dan Williams <dan.j.williams@intel.com>
>>>>>> ---
>>>>>>  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)

Yes, I am looking into this problem. Actually, there maybe another problem.

Just get my brain refreshed, need some time to dig into.

>
>Acked-by: David Hildenbrand <david@redhat.com>
>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[Baoquan He]

On 02/06/20 at 03:37pm, David Hildenbrand wrote:
> 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@linux.intel.com>
> >>>>> CC: Dan Williams <dan.j.williams@intel.com>
> >>>>> ---
> >>>>>  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)

Yeah, I have the same thought as you. And later Dan's words confirms it
in another threaad.

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

[Wei Yang]

On Thu, Feb 06, 2020 at 09:50:16PM +0800, 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@linux.intel.com>
>> > CC: Dan Williams <dan.j.williams@intel.com>
>> > ---
>> >  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.
>

Just see your reply.

Thanks for your explanation. :-)

>> David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[Wei Yang]

On Thu, Feb 06, 2020 at 02:28:53PM +0100, 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@linux.intel.com>
>> CC: Dan Williams <dan.j.williams@intel.com>
>> ---
>>  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.
>
>Instead of memmap, there would have to be something like
>
>memmap + (start_pfn - SECTION_ALIGN_DOWN(start_pfn))
>
>If I am not wrong :)

Hi, David, Thanks for your comment.

To be hones, I am not familiar with SPARSEMEM_VMEMMAP. Here is my
understanding about section_activate() when SPARSEMEM_VMEMMAP is set.

  section_activate(nid, start_pfn, nr_pages, altmap)
    populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
      __populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
        return pfn_to_page(start_pfn)

So the memmap is the page struct for start_pfn when SPARSEMEM_VMEMMAP is set.

Maybe I missed some critical part?

>
>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[David Hildenbrand]

On 06.02.20 14:57, Wei Yang wrote:
> On Thu, Feb 06, 2020 at 02:28:53PM +0100, 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@linux.intel.com>
>>> CC: Dan Williams <dan.j.williams@intel.com>
>>> ---
>>>  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.
>>
>> Instead of memmap, there would have to be something like
>>
>> memmap + (start_pfn - SECTION_ALIGN_DOWN(start_pfn))
>>
>> If I am not wrong :)
> 
> Hi, David, Thanks for your comment.
> 
> To be hones, I am not familiar with SPARSEMEM_VMEMMAP. Here is my
> understanding about section_activate() when SPARSEMEM_VMEMMAP is set.
> 
>   section_activate(nid, start_pfn, nr_pages, altmap)
>     populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
>       __populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
>         return pfn_to_page(start_pfn)
> 
> So the memmap is the page struct for start_pfn when SPARSEMEM_VMEMMAP is set.
> 
> Maybe I missed some critical part?

I was assuming that memmap is the memmap of the section, not of the
sub-section. (judging from the change in the original patch)

If the right memmap pointer to the sub-section is returned, then we are
fine. Will double check :)

-- 
Thanks,

David / dhildenb

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

[Wei Yang]

On Thu, Feb 06, 2020 at 02:59:50PM +0100, David Hildenbrand wrote:
>On 06.02.20 14:57, Wei Yang wrote:
>> On Thu, Feb 06, 2020 at 02:28:53PM +0100, 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@linux.intel.com>
>>>> CC: Dan Williams <dan.j.williams@intel.com>
>>>> ---
>>>>  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.
>>>
>>> Instead of memmap, there would have to be something like
>>>
>>> memmap + (start_pfn - SECTION_ALIGN_DOWN(start_pfn))
>>>
>>> If I am not wrong :)
>> 
>> Hi, David, Thanks for your comment.
>> 
>> To be hones, I am not familiar with SPARSEMEM_VMEMMAP. Here is my
>> understanding about section_activate() when SPARSEMEM_VMEMMAP is set.
>> 
>>   section_activate(nid, start_pfn, nr_pages, altmap)
>>     populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
>>       __populate_section_mmemap(start_pfn, nr_pages, nid, altmap)
>>         return pfn_to_page(start_pfn)
>> 
>> So the memmap is the page struct for start_pfn when SPARSEMEM_VMEMMAP is set.
>> 
>> Maybe I missed some critical part?
>
>I was assuming that memmap is the memmap of the section, not of the
>sub-section. (judging from the change in the original patch)
>
>If the right memmap pointer to the sub-section is returned, then we are
>fine. Will double check :)
>

Thanks, your comments are valuable :-)

>-- 
>Thanks,
>
>David / dhildenb

-- 
Wei Yang
Help you, Help me

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

[Baoquan He]

On 02/06/20 at 08:53pm, 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.

A good fix, thanks.

Reviewed-by: Baoquan He <bhe@redhat.com>

By the way, the failure trace should be added to log so that people can
know better what happened. And this happened in hot adding side in
SPARSEMEM|!VMEMMAP case, the hot removing failed too in this case, I
will psot patch to fix it right away.

> 
> Fixes: ba72b4c8cf60 ("mm/sparsemem: support sub-section hotplug")
> Signed-off-by: Wei Yang <richardw.yang@linux.intel.com>
> CC: Dan Williams <dan.j.williams@intel.com>
> ---
>  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);
>  
>  	ms = __nr_to_section(section_nr);
>  	set_section_nid(section_nr, nid);
> -- 
> 2.17.1
>