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@xxxxxxxxx> >> >> 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@xxxxxxxxxx> -- Thanks, David / dhildenb
