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

[Wei Yang <richard.weiyang@gmail.com>]

On Tue, Dec 18, 2018 at 10:14:25PM +0100, David Hildenbrand wrote:
>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?
>

alloc_contig_range() does following now:

   - isolate page range
   - do reclaim and migration
   - drain lru
   - drain pcp list

If step 2 fails, it will not drain lru and pcp list.

I don't see we have to drain pcp list before step 2. And after this
change, it will save some effort if step 2 fails.

>> 
>> ---
>> 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

-- 
Wei Yang
Help you, Help me