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

[Baoquan He]

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@xxxxxxxxxxxxxxxxxxx>
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxxxx>
> Cc: Baoquan He <bhe@xxxxxxxxxx>
> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> Cc: Wei Yang <richardw.yang@xxxxxxxxxxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> ---
>  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
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.

Reviewed-by: Baoquan He <bhe@xxxxxxxxxx>

>  		if (err)
>  			break;
> -		pfn += pfns;
> -		nr_pages -= pfns;
>  		cond_resched();
>  	}
>  	vmemmap_populate_print_last();
> -- 
> 2.24.1
>