Re: [PATCH v2 2/2] mm/memory_hotplug: cleanup __add_pages()

[Baoquan He <bhe@redhat.com>]

On 02/28/20 at 12:14pm, David Hildenbrand wrote:
> 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

Hmm, sometime we do use iterator to loop over only, it's just not so good.
People usually have their own preferred coding style, and try to optimize to
remove the itch in heart, totally understand. I have no strong objection
to this, as long as it gets support from reviewers, it's not a bad
thing.

> 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