| Message ID | 20210506152623.178731-2-zi.yan@sent.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Memory hotplug/hotremove at subsection size | expand |
On 06.05.21 17:26, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > subsection bitmap was set/cleared when a section is added/removed, but > pfn_to_online_page() uses subsection bitmap to check if the page is > online, which is not accurate. It was working when a whole section is > added/removed during memory hotplug and hotremove. When the following > patches enable memory hotplug and hotremove for subsections, > subsection bitmap needs to be changed during page online/offline time, > otherwise, pfn_to_online_page() will not give right answers. Move the > subsection bitmap manipulation code from section_activate() to > online_mem_sections() and section_deactivate() to > offline_mem_sections(), respectively. > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > mm/sparse.c | 36 +++++++++++++++++++++++++++++++++--- > 1 file changed, 33 insertions(+), 3 deletions(-) > > diff --git a/mm/sparse.c b/mm/sparse.c > index b2ada9dc00cb..7637208b8874 100644 > --- a/mm/sparse.c > +++ b/mm/sparse.c > @@ -606,6 +606,7 @@ void __init sparse_init(void) > > #ifdef CONFIG_MEMORY_HOTPLUG > > +static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages); > /* Mark all memory sections within the pfn range as online */ > void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) > { > @@ -621,9 +622,12 @@ void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) > > ms = __nr_to_section(section_nr); > ms->section_mem_map |= SECTION_IS_ONLINE; > + fill_subsection_map(pfn, min(end_pfn, pfn + PAGES_PER_SECTION) - pfn); > } > } > > +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages); > +static bool is_subsection_map_empty(struct mem_section *ms); > /* Mark all memory sections within the pfn range as offline */ > void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) > { > @@ -641,7 +645,13 @@ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) > continue; > > ms = __nr_to_section(section_nr); > - ms->section_mem_map &= ~SECTION_IS_ONLINE; > + > + if (end_pfn < pfn + PAGES_PER_SECTION) { > + clear_subsection_map(pfn, end_pfn - pfn); > + if (is_subsection_map_empty(ms)) > + ms->section_mem_map &= ~SECTION_IS_ONLINE; > + } else > + ms->section_mem_map &= ~SECTION_IS_ONLINE; > } > } > > @@ -668,6 +678,17 @@ static void free_map_bootmem(struct page *memmap) > vmemmap_free(start, end, NULL); > } > > +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, > + unsigned long nr_pages) > +{ > + DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > + unsigned long *subsection_map = &ms->usage->subsection_map[0]; > + > + subsection_mask_set(map, pfn, nr_pages); > + > + return bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION); > +} > + > static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > { > DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; > @@ -760,6 +781,12 @@ static void free_map_bootmem(struct page *memmap) > } > } > > +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, > + unsigned long nr_pages) > +{ > + return 0; > +} > + > static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) > { > return 0; > @@ -800,7 +827,10 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, > struct page *memmap = NULL; > bool empty; > > - if (clear_subsection_map(pfn, nr_pages)) > + if (WARN((IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) && !ms->usage) || > + subsection_map_intersects(ms, pfn, nr_pages), > + "section already deactivated (%#lx + %ld)\n", > + pfn, nr_pages)) > return; > > empty = is_subsection_map_empty(ms); > @@ -855,7 +885,7 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn, > ms->usage = usage; > } > > - rc = fill_subsection_map(pfn, nr_pages); > + rc = !nr_pages || subsection_map_intersects(ms, pfn, nr_pages); > if (rc) { > if (usage) > ms->usage = NULL; > If I am not missing something, this is completely broken for devmem/ZONE_DEVICE that never onlines pages. But also when memory blocks are never onlined, this would be just wrong. Least thing you would need is a sub-section online map. But glimpsing at patch #2, I'd rather stop right away digging deeper into this series :) I think what would really help is drafting a design of how it all could look like and then first discussing the high-level design, investigating how it could play along with all existing users, existing workloads, and existing use cases. Proposing such changes without a clear picture in mind and a high-level overview might give you some unpleasant reactions from some of the developers around here ;)
On 6 May 2021, at 13:48, David Hildenbrand wrote: > On 06.05.21 17:26, Zi Yan wrote: >> From: Zi Yan <ziy@nvidia.com> >> >> subsection bitmap was set/cleared when a section is added/removed, but >> pfn_to_online_page() uses subsection bitmap to check if the page is >> online, which is not accurate. It was working when a whole section is >> added/removed during memory hotplug and hotremove. When the following >> patches enable memory hotplug and hotremove for subsections, >> subsection bitmap needs to be changed during page online/offline time, >> otherwise, pfn_to_online_page() will not give right answers. Move the >> subsection bitmap manipulation code from section_activate() to >> online_mem_sections() and section_deactivate() to >> offline_mem_sections(), respectively. >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- >> mm/sparse.c | 36 +++++++++++++++++++++++++++++++++--- >> 1 file changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/mm/sparse.c b/mm/sparse.c >> index b2ada9dc00cb..7637208b8874 100644 >> --- a/mm/sparse.c >> +++ b/mm/sparse.c >> @@ -606,6 +606,7 @@ void __init sparse_init(void) >> #ifdef CONFIG_MEMORY_HOTPLUG >> +static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages); >> /* Mark all memory sections within the pfn range as online */ >> void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) >> { >> @@ -621,9 +622,12 @@ void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) >> ms = __nr_to_section(section_nr); >> ms->section_mem_map |= SECTION_IS_ONLINE; >> + fill_subsection_map(pfn, min(end_pfn, pfn + PAGES_PER_SECTION) - pfn); >> } >> } >> +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages); >> +static bool is_subsection_map_empty(struct mem_section *ms); >> /* Mark all memory sections within the pfn range as offline */ >> void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) >> { >> @@ -641,7 +645,13 @@ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) >> continue; >> ms = __nr_to_section(section_nr); >> - ms->section_mem_map &= ~SECTION_IS_ONLINE; >> + >> + if (end_pfn < pfn + PAGES_PER_SECTION) { >> + clear_subsection_map(pfn, end_pfn - pfn); >> + if (is_subsection_map_empty(ms)) >> + ms->section_mem_map &= ~SECTION_IS_ONLINE; >> + } else >> + ms->section_mem_map &= ~SECTION_IS_ONLINE; >> } >> } >> @@ -668,6 +678,17 @@ static void free_map_bootmem(struct page *memmap) >> vmemmap_free(start, end, NULL); >> } >> +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, >> + unsigned long nr_pages) >> +{ >> + DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; >> + unsigned long *subsection_map = &ms->usage->subsection_map[0]; >> + >> + subsection_mask_set(map, pfn, nr_pages); >> + >> + return bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION); >> +} >> + >> static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) >> { >> DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; >> @@ -760,6 +781,12 @@ static void free_map_bootmem(struct page *memmap) >> } >> } >> +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, >> + unsigned long nr_pages) >> +{ >> + return 0; >> +} >> + >> static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) >> { >> return 0; >> @@ -800,7 +827,10 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, >> struct page *memmap = NULL; >> bool empty; >> - if (clear_subsection_map(pfn, nr_pages)) >> + if (WARN((IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) && !ms->usage) || >> + subsection_map_intersects(ms, pfn, nr_pages), >> + "section already deactivated (%#lx + %ld)\n", >> + pfn, nr_pages)) >> return; >> empty = is_subsection_map_empty(ms); >> @@ -855,7 +885,7 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn, >> ms->usage = usage; >> } >> - rc = fill_subsection_map(pfn, nr_pages); >> + rc = !nr_pages || subsection_map_intersects(ms, pfn, nr_pages); >> if (rc) { >> if (usage) >> ms->usage = NULL; >> > > If I am not missing something, this is completely broken for devmem/ZONE_DEVICE that never onlines pages. But also when memory blocks are never onlined, this would be just wrong. Least thing you would need is a sub-section online map. Thanks for pointing this out. I did not know that devmem/ZONE_DEVICE never onlines pages. > > But glimpsing at patch #2, I'd rather stop right away digging deeper into this series :) What is the issue of patch 2, which makes pageblock_order a variable all the time? BTW, patch 2 fixes a bug by exporting pageblock_order, since when HUGETLB_PAGE_SIZE_VARIABLE is set, virtio-mem will not see pageblock_order as a variable, which could happen for PPC_BOOK2S_64 with virtio-men enabled, right? Or is this an invalid combination? > > I think what would really help is drafting a design of how it all could look like and then first discussing the high-level design, investigating how it could play along with all existing users, existing workloads, and existing use cases. Proposing such changes without a clear picture in mind and a high-level overview might give you some unpleasant reactions from some of the developers around here ;) Please see my other email for a high-level design. Also I sent the patchset as a RFC to gather information on users, workloads, use cases I did not know about and I learnt a lot from your replies. :) Feedback is always welcome, but I am not sure why it needs to make people unpleasant. ;) — Best Regards, Yan Zi
>> But glimpsing at patch #2, I'd rather stop right away digging deeper into this series :) > > What is the issue of patch 2, which makes pageblock_order a variable all the time? BTW, patch 2 fixes a bug by exporting pageblock_order, since when HUGETLB_PAGE_SIZE_VARIABLE is set, virtio-mem will not see pageblock_order as a variable, which could happen for PPC_BOOK2S_64 with virtio-men enabled, right? Or is this an invalid combination? virtio_mem is x86_64 only. aarch64 and s390x prototypes are available. If I understood "Make pageblock_order a variable and set it to the max of HUGETLB_PAGE_ORDER, MAX_ORDER - 1" correctly, you're setting the pageblock_order on x86_64 to 4M. That mean's you're no longer grouping for THP but MAX_ORDER - 1, which is not what we want. We want to optimize for THP. Also, that would affect virtio-balloon with free page reporting (report only 4 MiB chunks not 2 MiB chunks). > >> >> I think what would really help is drafting a design of how it all could look like and then first discussing the high-level design, investigating how it could play along with all existing users, existing workloads, and existing use cases. Proposing such changes without a clear picture in mind and a high-level overview might give you some unpleasant reactions from some of the developers around here ;) > > Please see my other email for a high-level design. Also I sent the patchset as a RFC to gather information on users, workloads, use cases I did not know about and I learnt a lot from your replies. :) Feedback is always welcome, but I am not sure why it needs to make people unpleasant. ;) Rather the replies might be unpleasant ;)
diff --git a/mm/sparse.c b/mm/sparse.c index b2ada9dc00cb..7637208b8874 100644 --- a/mm/sparse.c +++ b/mm/sparse.c @@ -606,6 +606,7 @@ void __init sparse_init(void) #ifdef CONFIG_MEMORY_HOTPLUG +static int fill_subsection_map(unsigned long pfn, unsigned long nr_pages); /* Mark all memory sections within the pfn range as online */ void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) { @@ -621,9 +622,12 @@ void online_mem_sections(unsigned long start_pfn, unsigned long end_pfn) ms = __nr_to_section(section_nr); ms->section_mem_map |= SECTION_IS_ONLINE; + fill_subsection_map(pfn, min(end_pfn, pfn + PAGES_PER_SECTION) - pfn); } } +static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages); +static bool is_subsection_map_empty(struct mem_section *ms); /* Mark all memory sections within the pfn range as offline */ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) { @@ -641,7 +645,13 @@ void offline_mem_sections(unsigned long start_pfn, unsigned long end_pfn) continue; ms = __nr_to_section(section_nr); - ms->section_mem_map &= ~SECTION_IS_ONLINE; + + if (end_pfn < pfn + PAGES_PER_SECTION) { + clear_subsection_map(pfn, end_pfn - pfn); + if (is_subsection_map_empty(ms)) + ms->section_mem_map &= ~SECTION_IS_ONLINE; + } else + ms->section_mem_map &= ~SECTION_IS_ONLINE; } } @@ -668,6 +678,17 @@ static void free_map_bootmem(struct page *memmap) vmemmap_free(start, end, NULL); } +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, + unsigned long nr_pages) +{ + DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; + unsigned long *subsection_map = &ms->usage->subsection_map[0]; + + subsection_mask_set(map, pfn, nr_pages); + + return bitmap_intersects(map, subsection_map, SUBSECTIONS_PER_SECTION); +} + static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) { DECLARE_BITMAP(map, SUBSECTIONS_PER_SECTION) = { 0 }; @@ -760,6 +781,12 @@ static void free_map_bootmem(struct page *memmap) } } +static int subsection_map_intersects(struct mem_section *ms, unsigned long pfn, + unsigned long nr_pages) +{ + return 0; +} + static int clear_subsection_map(unsigned long pfn, unsigned long nr_pages) { return 0; @@ -800,7 +827,10 @@ static void section_deactivate(unsigned long pfn, unsigned long nr_pages, struct page *memmap = NULL; bool empty; - if (clear_subsection_map(pfn, nr_pages)) + if (WARN((IS_ENABLED(CONFIG_SPARSEMEM_VMEMMAP) && !ms->usage) || + subsection_map_intersects(ms, pfn, nr_pages), + "section already deactivated (%#lx + %ld)\n", + pfn, nr_pages)) return; empty = is_subsection_map_empty(ms); @@ -855,7 +885,7 @@ static struct page * __meminit section_activate(int nid, unsigned long pfn, ms->usage = usage; } - rc = fill_subsection_map(pfn, nr_pages); + rc = !nr_pages || subsection_map_intersects(ms, pfn, nr_pages); if (rc) { if (usage) ms->usage = NULL;