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

[David Hildenbrand <david@redhat.com>]

On 18.12.18 22:49, Wei Yang wrote:
> 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.

Sorry, I missed that you actually documented the "alloc_contig_range"
scenario in you patch description. My fault!

Acked-by: David Hildenbrand <david@redhat.com>


-- 

Thanks,

David / dhildenb