On 06.05.21 17:26, Zi Yan wrote:
From: Zi Yan <ziy@xxxxxxxxxx> 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@xxxxxxxxxx> --- 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 ;)
-- Thanks, David / dhildenb
|