Messages in this thread | | | Subject | Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages() | From | David Hildenbrand <> | Date | Fri, 28 Feb 2020 12:14:47 +0100 |
| |
On 28.02.20 11:34, Baoquan He wrote: > On 02/28/20 at 10:58am, David Hildenbrand wrote: >> Let's drop the basically unused section stuff and simplify. The logic >> now matches the logic in __remove_pages(). >> >> Cc: Segher Boessenkool <segher@kernel.crashing.org> >> Cc: Andrew Morton <akpm@linux-foundation.org> >> Cc: Oscar Salvador <osalvador@suse.de> >> Cc: Michal Hocko <mhocko@kernel.org> >> Cc: Baoquan He <bhe@redhat.com> >> Cc: Dan Williams <dan.j.williams@intel.com> >> Cc: Wei Yang <richardw.yang@linux.intel.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> >> --- >> mm/memory_hotplug.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 deletions(-) >> >> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c >> index 8fe7e32dad48..1a00b5a37ef6 100644 >> --- a/mm/memory_hotplug.c >> +++ b/mm/memory_hotplug.c >> @@ -307,8 +307,9 @@ static int check_hotplug_memory_addressable(unsigned long pfn, >> int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, >> struct mhp_restrictions *restrictions) >> { >> + const unsigned long end_pfn = pfn + nr_pages; >> + unsigned long cur_nr_pages; >> int err; >> - unsigned long nr, start_sec, end_sec; >> struct vmem_altmap *altmap = restrictions->altmap; >> >> err = check_hotplug_memory_addressable(pfn, nr_pages); >> @@ -331,18 +332,13 @@ int __ref __add_pages(int nid, unsigned long pfn, unsigned long nr_pages, >> if (err) >> return err; >> >> - start_sec = pfn_to_section_nr(pfn); >> - end_sec = pfn_to_section_nr(pfn + nr_pages - 1); >> - for (nr = start_sec; nr <= end_sec; nr++) { >> - unsigned long pfns; >> - >> - pfns = min(nr_pages, PAGES_PER_SECTION >> - - (pfn & ~PAGE_SECTION_MASK)); >> - err = sparse_add_section(nid, pfn, pfns, altmap); >> + for (; pfn < end_pfn; pfn += cur_nr_pages) { >> + /* Select all remaining pages up to the next section boundary */ >> + cur_nr_pages = min(end_pfn - pfn, >> + SECTION_ALIGN_UP(pfn + 1) - pfn); >> + err = sparse_add_section(nid, pfn, cur_nr_pages, altmap); > > Honestly, I am not a big fan of this kind of code refactoring. The old > code may span seveal more lines or define several several more veriables, > but logic is clear, and no visible defect. It's hard to say how much we
I'm sorry, but iterating over variables and not using a single one in the body is definitely not clean, at least IMHO. Leftover from sub-section hotadd support.
> can benefit from this kind of code simplifying, and reviewing it will take > people more time. While for the code style consistency with > __remove_page(), I would like to see it's merged. My personal opinion.
Thanks!
-- Thanks,
David / dhildenb
| |