| Message ID | 20200206125343.9070-1-richardw.yang@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/sparsemem: pfn_to_page is not valid yet on SPARSEMEM | expand |
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 :)
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
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?
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
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 :)
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.
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
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
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>
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
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.
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 >
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);
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(-)