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

[Michal Hocko <mhocko@suse.com>]

On Wed 19-12-18 13:53:07, Wei Yang wrote:
> On Wed, Dec 19, 2018 at 10:57:19AM +0100, Oscar Salvador wrote:
> >On Wed, Dec 19, 2018 at 10:51:10AM +0100, Michal Hocko wrote:
> >> On Wed 19-12-18 04:46:56, 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.
> >> 
> >> But it is important to note that those pages still can be allocated from
> >> the pcp lists until we do drain_all_pages().
> >
> >I had the same fear, but then I saw that move_freepages_block()->move_freepages() moves
> >the pages to a new list:
> >
> ><--
> >list_move(&page->lru,
> >			  &zone->free_area[order].free_list[migratetype]);
> >-->
> >
> >
> >But looking at it again, I see that this is only for BuddyPages, so I guess
> >that pcp-pages do not really get unlinked, so we could still allocate them.
> 
> Well, I think you are right. But with this in mind, current code looks
> buggy.
> 
> Between has_unmovable_pages() and drain_all_pages(), others still could
> allocate pages on pcp list, right? This means we thought we have
> isolated the range, but not.

THere is no guarantee in that regards and I believe there is also no
demand for such a strong semantic. Or I do not see it at least. 
-- 
Michal Hocko
SUSE Labs