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 <[email protected]>
CC: Dan Williams <[email protected]>
---
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
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 <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> 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
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 <[email protected]>
> > CC: Dan Williams <[email protected]>
> > ---
> > 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
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 <[email protected]>
>>> CC: Dan Williams <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
>> CC: Dan Williams <[email protected]>
>> ---
>> 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
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 <[email protected]>
>>> CC: Dan Williams <[email protected]>
>>> ---
>>> 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
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 <[email protected]>
> >>> CC: Dan Williams <[email protected]>
> >>> ---
> >>> 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.
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 <[email protected]>
>>>> CC: Dan Williams <[email protected]>
>>>> ---
>>>> 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
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 <[email protected]>
>>>>> CC: Dan Williams <[email protected]>
>>>>> ---
>>>>> 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 <[email protected]>
--
Thanks,
David / dhildenb
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 <[email protected]>
>> > CC: Dan Williams <[email protected]>
>> > ---
>> > 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
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 <[email protected]>
>>>>>> CC: Dan Williams <[email protected]>
>>>>>> ---
>>>>>> 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 <[email protected]>
>
>--
>Thanks,
>
>David / dhildenb
--
Wei Yang
Help you, Help me
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 <[email protected]>
> >>>>> CC: Dan Williams <[email protected]>
> >>>>> ---
> >>>>> 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.
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 <[email protected]>
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 <[email protected]>
> CC: Dan Williams <[email protected]>
> ---
> 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
>