| Message ID | 20190809125701.3316-4-david@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/memory_hotplug: online_pages() cleanups | expand |
On Fri 09-08-19 14:57:00, David Hildenbrand wrote: > move_pfn_range_to_zone() will set all pages to PG_reserved via > memmap_init_zone(). The only way a page could no longer be reserved > would be if a MEM_GOING_ONLINE notifier would clear PG_reserved - which > is not done (the online_page callback is used for that purpose by > e.g., Hyper-V instead). walk_system_ram_range() will never call > online_pages_range() with duplicate PFNs, so drop the PageReserved() check. > > Simplify the handling, as online_pages always corresponds to nr_pages. > There is no need for online_pages_blocks() anymore. This would be easier to review if split up into two patches. One that only performs cleanup without any other changes and the PageReserved check. I like the check going away and we should get rid of the dependency on the Reserved bit completely. Other than that I find the start_pfn and pfn being used both for iteration each a different way really confusing and I cannot convince myself it is even correct because I didn't bother to look deeper as I simply think that the order manipulation from the previous is just making things worse at this moment. If the problem is even real then it can be done on top instead with some real example of the memory layout that breaks. > Cc: Andrew Morton <akpm@linux-foundation.org> > Cc: Oscar Salvador <osalvador@suse.de> > Cc: Michal Hocko <mhocko@suse.com> > Cc: Pavel Tatashin <pasha.tatashin@soleen.com> > Cc: Dan Williams <dan.j.williams@intel.com> > Signed-off-by: David Hildenbrand <david@redhat.com> > --- > mm/memory_hotplug.c | 42 ++++++++++++++++++------------------------ > 1 file changed, 18 insertions(+), 24 deletions(-) > > diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c > index 2abd938c8c45..87f85597a19e 100644 > --- a/mm/memory_hotplug.c > +++ b/mm/memory_hotplug.c > @@ -632,37 +632,31 @@ static void generic_online_page(struct page *page, unsigned int order) > #endif > } > > -static int online_pages_blocks(unsigned long start, unsigned long nr_pages) > +static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, > + void *arg) > { > - unsigned long end = start + nr_pages; > - int order, onlined_pages = 0; > + const unsigned long end_pfn = start_pfn + nr_pages; > + unsigned long pfn; > + int order; > > - while (start < end) { > - order = min(MAX_ORDER - 1, > - get_order(PFN_PHYS(end) - PFN_PHYS(start))); > + /* > + * Online the pages. The callback might decide to keep some pages > + * PG_reserved (to add them to the buddy later), but we still account > + * them as being online/belonging to this zone ("present"). > + */ > + for (pfn = start_pfn; pfn < end_pfn; pfn += 1ul << order) { > + order = min(MAX_ORDER - 1, get_order(PFN_PHYS(end_pfn - pfn))); > /* make sure the PFN is aligned and we don't exceed the range */ > - while (!IS_ALIGNED(start, 1ul << order) || > - (1ul << order) > end - start) > + while (!IS_ALIGNED(start_pfn, 1ul << order) || > + (1ul << order) > end_pfn - pfn) > order--; > - (*online_page_callback)(pfn_to_page(start), order); > - > - onlined_pages += (1UL << order); > - start += (1UL << order); > + (*online_page_callback)(pfn_to_page(pfn), order); > } > - return onlined_pages; > -} > - > -static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, > - void *arg) > -{ > - unsigned long onlined_pages = *(unsigned long *)arg; > - > - if (PageReserved(pfn_to_page(start_pfn))) > - onlined_pages += online_pages_blocks(start_pfn, nr_pages); > > - online_mem_sections(start_pfn, start_pfn + nr_pages); > + /* mark all involved sections as online */ > + online_mem_sections(start_pfn, end_pfn); > > - *(unsigned long *)arg = onlined_pages; > + *(unsigned long *)arg += nr_pages; > return 0; > } > > -- > 2.21.0
On 14.08.19 16:19, Michal Hocko wrote: > On Fri 09-08-19 14:57:00, David Hildenbrand wrote: >> move_pfn_range_to_zone() will set all pages to PG_reserved via >> memmap_init_zone(). The only way a page could no longer be reserved >> would be if a MEM_GOING_ONLINE notifier would clear PG_reserved - which >> is not done (the online_page callback is used for that purpose by >> e.g., Hyper-V instead). walk_system_ram_range() will never call >> online_pages_range() with duplicate PFNs, so drop the PageReserved() check. >> >> Simplify the handling, as online_pages always corresponds to nr_pages. >> There is no need for online_pages_blocks() anymore. > > This would be easier to review if split up into two patches. One that > only performs cleanup without any other changes and the PageReserved > check. I like the check going away and we should get rid of the > dependency on the Reserved bit completely. Yes, makes sense. > > Other than that I find the start_pfn and pfn being used both for > iteration each a different way really confusing and I cannot convince > myself it is even correct because I didn't bother to look deeper as > I simply think that the order manipulation from the previous is just > making things worse at this moment. If the problem is even real then it > can be done on top instead with some real example of the memory layout > that breaks. Yes, I'll rework this. Only "pfn" should be used in the loop (it seems to be correct but confusing I agree).
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c index 2abd938c8c45..87f85597a19e 100644 --- a/mm/memory_hotplug.c +++ b/mm/memory_hotplug.c @@ -632,37 +632,31 @@ static void generic_online_page(struct page *page, unsigned int order) #endif } -static int online_pages_blocks(unsigned long start, unsigned long nr_pages) +static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, + void *arg) { - unsigned long end = start + nr_pages; - int order, onlined_pages = 0; + const unsigned long end_pfn = start_pfn + nr_pages; + unsigned long pfn; + int order; - while (start < end) { - order = min(MAX_ORDER - 1, - get_order(PFN_PHYS(end) - PFN_PHYS(start))); + /* + * Online the pages. The callback might decide to keep some pages + * PG_reserved (to add them to the buddy later), but we still account + * them as being online/belonging to this zone ("present"). + */ + for (pfn = start_pfn; pfn < end_pfn; pfn += 1ul << order) { + order = min(MAX_ORDER - 1, get_order(PFN_PHYS(end_pfn - pfn))); /* make sure the PFN is aligned and we don't exceed the range */ - while (!IS_ALIGNED(start, 1ul << order) || - (1ul << order) > end - start) + while (!IS_ALIGNED(start_pfn, 1ul << order) || + (1ul << order) > end_pfn - pfn) order--; - (*online_page_callback)(pfn_to_page(start), order); - - onlined_pages += (1UL << order); - start += (1UL << order); + (*online_page_callback)(pfn_to_page(pfn), order); } - return onlined_pages; -} - -static int online_pages_range(unsigned long start_pfn, unsigned long nr_pages, - void *arg) -{ - unsigned long onlined_pages = *(unsigned long *)arg; - - if (PageReserved(pfn_to_page(start_pfn))) - onlined_pages += online_pages_blocks(start_pfn, nr_pages); - online_mem_sections(start_pfn, start_pfn + nr_pages); + /* mark all involved sections as online */ + online_mem_sections(start_pfn, end_pfn); - *(unsigned long *)arg = onlined_pages; + *(unsigned long *)arg += nr_pages; return 0; }
move_pfn_range_to_zone() will set all pages to PG_reserved via memmap_init_zone(). The only way a page could no longer be reserved would be if a MEM_GOING_ONLINE notifier would clear PG_reserved - which is not done (the online_page callback is used for that purpose by e.g., Hyper-V instead). walk_system_ram_range() will never call online_pages_range() with duplicate PFNs, so drop the PageReserved() check. Simplify the handling, as online_pages always corresponds to nr_pages. There is no need for online_pages_blocks() anymore. Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Oscar Salvador <osalvador@suse.de> Cc: Michal Hocko <mhocko@suse.com> Cc: Pavel Tatashin <pasha.tatashin@soleen.com> Cc: Dan Williams <dan.j.williams@intel.com> Signed-off-by: David Hildenbrand <david@redhat.com> --- mm/memory_hotplug.c | 42 ++++++++++++++++++------------------------ 1 file changed, 18 insertions(+), 24 deletions(-)