Re: [PATCH v1 05/11] mm/page_alloc: simplify __offline_isolated_pages()

[Michal Hocko]

On Wed 19-08-20 12:11:51, David Hildenbrand wrote:
> offline_pages() is the only user.  __offline_isolated_pages() never gets
> called with ranges that contain memory holes and we no longer care about
> the return value. Drop the return value handling and all pfn_valid()
> checks.
> 
> Update the documentation.
> 
> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> Cc: Michal Hocko <mhocko@xxxxxxxx>
> Cc: Wei Yang <richard.weiyang@xxxxxxxxxxxxxxxxx>
> Cc: Baoquan He <bhe@xxxxxxxxxx>
> Cc: Pankaj Gupta <pankaj.gupta.linux@xxxxxxxxx>
> Cc: Oscar Salvador <osalvador@xxxxxxx>
> Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>

Acked-by: Michal Hocko <mhocko@xxxxxxxx>

> ---
>  include/linux/memory_hotplug.h |  4 ++--
>  mm/page_alloc.c                | 27 ++++-----------------------
>  2 files changed, 6 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 375515803cd83..0b461691d1a49 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -103,8 +103,8 @@ extern int online_pages(unsigned long pfn, unsigned long nr_pages,
>  			int online_type, int nid);
>  extern struct zone *test_pages_in_a_zone(unsigned long start_pfn,
>  					 unsigned long end_pfn);
> -extern unsigned long __offline_isolated_pages(unsigned long start_pfn,
> -						unsigned long end_pfn);
> +extern void __offline_isolated_pages(unsigned long start_pfn,
> +				     unsigned long end_pfn);
>  
>  typedef void (*online_page_callback_t)(struct page *page, unsigned int order);
>  
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index cf0b25161feae..03f585f95dc60 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -8692,35 +8692,21 @@ void zone_pcp_reset(struct zone *zone)
>  
>  #ifdef CONFIG_MEMORY_HOTREMOVE
>  /*
> - * All pages in the range must be in a single zone and isolated
> - * before calling this.
> + * All pages in the range must be in a single zone, must not contain holes,
> + * must span full sections, and must be isolated before calling this function.
>   */
> -unsigned long
> -__offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
> +void __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  {
> +	unsigned long pfn = start_pfn;
>  	struct page *page;
>  	struct zone *zone;
>  	unsigned int order;
> -	unsigned long pfn;
>  	unsigned long flags;
> -	unsigned long offlined_pages = 0;
> -
> -	/* find the first valid pfn */
> -	for (pfn = start_pfn; pfn < end_pfn; pfn++)
> -		if (pfn_valid(pfn))
> -			break;
> -	if (pfn == end_pfn)
> -		return offlined_pages;
>  
>  	offline_mem_sections(pfn, end_pfn);
>  	zone = page_zone(pfn_to_page(pfn));
>  	spin_lock_irqsave(&zone->lock, flags);
> -	pfn = start_pfn;
>  	while (pfn < end_pfn) {
> -		if (!pfn_valid(pfn)) {
> -			pfn++;
> -			continue;
> -		}
>  		page = pfn_to_page(pfn);
>  		/*
>  		 * The HWPoisoned page may be not in buddy system, and
> @@ -8728,7 +8714,6 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  		 */
>  		if (unlikely(!PageBuddy(page) && PageHWPoison(page))) {
>  			pfn++;
> -			offlined_pages++;
>  			continue;
>  		}
>  		/*
> @@ -8739,20 +8724,16 @@ __offline_isolated_pages(unsigned long start_pfn, unsigned long end_pfn)
>  			BUG_ON(page_count(page));
>  			BUG_ON(PageBuddy(page));
>  			pfn++;
> -			offlined_pages++;
>  			continue;
>  		}
>  
>  		BUG_ON(page_count(page));
>  		BUG_ON(!PageBuddy(page));
>  		order = page_order(page);
> -		offlined_pages += 1 << order;
>  		del_page_from_free_list(page, zone, order);
>  		pfn += (1 << order);
>  	}
>  	spin_unlock_irqrestore(&zone->lock, flags);
> -
> -	return offlined_pages;
>  }
>  #endif
>  
> -- 
> 2.26.2
> 

-- 
Michal Hocko
SUSE Labs