Re: [PATCH v2] mm, page_isolation: remove drain_all_pages() in set_migratetype_isolate()

[David Hildenbrand <david@redhat.com>]

On 18.12.18 21:46, Wei Yang wrote:
> Below is a brief call flow for __offline_pages() and
> alloc_contig_range():
> 
>   __offline_pages()/alloc_contig_range()
>       start_isolate_page_range()
>           set_migratetype_isolate()
>               drain_all_pages()
>       drain_all_pages()
> 
> Current logic is: isolate and drain pcp list for each pageblock and
> drain pcp list again. This is not necessary and we could just drain pcp
> list once after isolate this whole range.
> 
> The reason is start_isolate_page_range() will set the migrate type of
> a range to MIGRATE_ISOLATE. After doing so, this range will never be
> allocated from Buddy, neither to a real user nor to pcp list.
> 
> Since drain_all_pages() is zone based, by reduce times of
> drain_all_pages() also reduce some contention on this particular zone.
> 
> Signed-off-by: Wei Yang <richard.weiyang@gmail.com>

Yes, as far as I can see, when a MIGRATE_ISOLATE page gets freed, it
will not go onto the pcp list again.

However, start_isolate_page_range() is also called via
alloc_contig_range(). Are you sure we can effectively drop the
drain_all_pages() for that call path?

> 
> ---
> v2: adjust changelog with MIGRATE_ISOLATE effects for the isolated range
> ---
>  mm/page_isolation.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 43e085608846..f44c0e333bed 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -83,8 +83,6 @@ static int set_migratetype_isolate(struct page *page, int migratetype,
>  	}
>  
>  	spin_unlock_irqrestore(&zone->lock, flags);
> -	if (!ret)
> -		drain_all_pages(zone);
>  	return ret;
>  }
>  
> 


-- 

Thanks,

David / dhildenb