Date:   Tue, 24 Oct 2017 14:25:26 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Vlastimil Babka <vbabka@...e.cz>
Cc:     Joonsoo Kim <iamjoonsoo.kim@....com>, linux-mm@...ck.org,
        Michael Ellerman <mpe@...erman.id.au>,
        Andrew Morton <akpm@...ux-foundation.org>,
        KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
        Reza Arbab <arbab@...ux.vnet.ibm.com>,
        Yasuaki Ishimatsu <yasu.isimatu@...il.com>,
        qiuxishi@...wei.com, Igor Mammedov <imammedo@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages

On Tue 24-10-17 10:12:58, Vlastimil Babka wrote:
> On 10/24/2017 06:44 AM, Joonsoo Kim wrote:
> >>> I'm not sure what is the confusing semantic you mentioned. I think
> >>> that set_migratetype_isolate() has confusing semantic and should be
> >>> fixed since making the pageblock isolated doesn't need to check if
> >>> there is unmovable page or not. Do you think that
> >>> set_migratetype_isolate() need to check it? If so, why?
> >>
> >> My intuitive understanding of set_migratetype_isolate is that it either
> >> suceeds and that means that the given pfn range can be isolated for the
> >> given type of allocation (be it movable or cma). No new pages will be
> >> allocated from this range to allow converging into a free range in a
> >> finit amount of time. At least this is how the hotplug code would like
> >> to use it and I suppose that the alloc_contig_range would like to
> >> guarantee the same to not rely on a fixed amount of migration attempts.
> > 
> > Yes, alloc_contig_range() also want to guarantee the similar thing.
> > Major difference between them is 'given pfn range'. memory hotplug
> > works by pageblock unit but alloc_contig_range() doesn't.
> > alloc_contig_range() works by the page unit. However, there is no easy
> > way to isolate individual page so it uses pageblock isolation
> > regardless of 'given pfn range'. In this case, checking movability of
> > all pages on the pageblock would cause the problem as I mentioned
> > before.
> 
> I couldn't look too closely yet, but do I understand correctly that the
> *potential* problem (because as you say there are no such
> alloc_contig_range callers) you are describing is not newly introduced
> by Michal's series? Then his patch fixing the introduced regression
> should be enough for now, and further improvements could be posted on
> top, and not vice versa? Please don't take it wrong, I agree the current
> state is a bit of a mess and improvements are welcome. Also it seems to
> me that Michal is right, and there's nothing preventing
> alloc_contig_range() to allocate from CMA pageblocks for non-CMA
> purposes (likely not movable), and that should be also fixed?

OK, it seems I understand Joonsoo's concern more now. And I agree with
Vlastimil, that it is better to plug the immediate regression with a
minimal patch and discuss general improvements of the pfn based
allocator separatelly. There are more things to clear up there,
including the proper API (alloc_contig_range is just too low level for
anybody to use) as well as the MIGRATE_* flags usage (e.g. I am not
really sure GB pages usage of MIGRATE_MOVABLE is really correct).
alloc_contig_range looks like an internal CMA function which has been
(ab)used for a different purpose to me rather than a well thought
through interface. MAP_CONTIG discussion has shown some interest in
an API for large allocations so I _believe_ we should think that through
befire we grow more unexpected users.

I am definitely willing to help there.

Is that something you would agree with Joonsoo?
-- 
Michal Hocko
SUSE Labs

Powered by blists - more mailing lists