Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages

[Michal Hocko]

On Fri 20-10-17 15:50:14, Joonsoo Kim wrote:
> On Fri, Oct 20, 2017 at 07:59:22AM +0200, Michal Hocko wrote:
> > On Fri 20-10-17 11:13:29, Joonsoo Kim wrote:
> > > On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote:
> > > > On Thu 19-10-17 10:20:41, Michal Hocko wrote:
> > > > > On Thu 19-10-17 16:33:56, Joonsoo Kim wrote:
> > > > > > On Thu, Oct 19, 2017 at 09:15:03AM +0200, Michal Hocko wrote:
> > > > > > > On Thu 19-10-17 11:51:11, Joonsoo Kim wrote:
> > > > > [...]
> > > > > > > > Hello,
> > > > > > > > 
> > > > > > > > This patch will break the CMA user. As you mentioned, CMA allocation
> > > > > > > > itself isn't migrateable. So, after a single page is allocated through
> > > > > > > > CMA allocation, has_unmovable_pages() will return true for this
> > > > > > > > pageblock. Then, futher CMA allocation request to this pageblock will
> > > > > > > > fail because it requires isolating the pageblock.
> > > > > > > 
> > > > > > > Hmm, does this mean that the CMA allocation path depends on
> > > > > > > has_unmovable_pages to return false here even though the memory is not
> > > > > > > movable? This sounds really strange to me and kind of abuse of this
> > > > > > 
> > > > > > Your understanding is correct. Perhaps, abuse or wrong function name.
> > > > > >
> > > > > > > function. Which path is that? Can we do the migrate type test theres?
> > > > > > 
> > > > > > alloc_contig_range() -> start_isolate_page_range() ->
> > > > > > set_migratetype_isolate() -> has_unmovable_pages()
> > > > > 
> > > > > I see. It seems that the CMA and memory hotplug have a very different
> > > > > view on what should happen during isolation.
> > > > >  
> > > > > > We can add one argument, 'XXX' to set_migratetype_isolate() and change
> > > > > > it to check migrate type rather than has_unmovable_pages() if 'XXX' is
> > > > > > specified.
> > > > > 
> > > > > Can we use the migratetype argument and do the special thing for
> > > > > MIGRATE_CMA? Like the following diff?
> > > > 
> > > > And with the full changelog.
> > > > ---
> > > > >From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
> > > > From: Michal Hocko <mhocko@xxxxxxxx>
> > > > Date: Thu, 19 Oct 2017 14:14:02 +0200
> > > > Subject: [PATCH] mm: distinguish CMA and MOVABLE isolation in
> > > >  has_unmovable_pages
> > > > 
> > > > Joonsoo has noticed that "mm: drop migrate type checks from
> > > > has_unmovable_pages" would break CMA allocator because it relies on
> > > > has_unmovable_pages returning false even for CMA pageblocks which in
> > > > fact don't have to be movable:
> > > > alloc_contig_range
> > > >   start_isolate_page_range
> > > >     set_migratetype_isolate
> > > >       has_unmovable_pages
> > > > 
> > > > This is a result of the code sharing between CMA and memory hotplug
> > > > while each one has a different idea of what has_unmovable_pages should
> > > > return. This is unfortunate but fixing it properly would require a lot
> > > > of code duplication.
> > > > 
> > > > Fix the issue by introducing the requested migrate type argument
> > > > and special case MIGRATE_CMA case where CMA page blocks are handled
> > > > properly. This will work for memory hotplug because it requires
> > > > MIGRATE_MOVABLE.
> > > 
> > > Unfortunately, alloc_contig_range() can be called with
> > > MIGRATE_MOVABLE so this patch cannot perfectly fix the problem.
> > 
> > Yes, alloc_contig_range can be called with MIGRATE_MOVABLE but my
> > understanding is that only CMA allocator really depends on this weird
> > semantic and that does MIGRATE_CMA unconditionally.
> 
> alloc_contig_range() could be called for partial pages in the
> pageblock. With your patch, this case also fails unnecessarilly if the
> other pages in the pageblock is pinned.

Is this really the case for GB pages? Do we really want to mess those
with CMA blocks and make those blocks basically unusable because GB
pages are rarely (if at all migrateable)?

> Until now, there is no user calling alloc_contig_range() with partial
> pages except CMA allocator but API could support it.

I disagree. If this is a CMA thing it should stay that way. The semantic
is quite confusing already, please let's not make it even worse.

> > > I did a more thinking and found that it's strange to check if there is
> > > unmovable page in the pageblock during the set_migratetype_isolate().
> > > set_migratetype_isolate() should be just for setting the migratetype
> > > of the pageblock. Checking other things should be done by another
> > > place, for example, before calling the start_isolate_page_range() in
> > > __offline_pages().
> > 
> > How do we guarantee the atomicity?
> 
> What atomicity do you mean?

Currently we are checking and isolating pages under zone lock. If we
split that we are losing atomicity, aren't we.
-- 
Michal Hocko
SUSE Labs