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

[Michal Hocko]

On Thu 26-10-17 11:47:07, Joonsoo Kim wrote:
> On Tue, Oct 24, 2017 at 10:12:58AM +0200, 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
> 
> This potential problem exists there before Michal's series if the
> migratetype of the target pageblock isn't MIGRATE_MOVABLE or MIGRATE_CMA.
> However, his series enlarges this potential problem surface. It
> would be the problem now even if the migratetype of the target
> pageblock is MIGRATE_MOVABLE.
> 
> > 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
> 
> I'm not very sensitive that which patch is applied first. I can do
> rebase. But, IMHO, correct applying order is my patch first and then
> Michal's series.
> 
> Anyway, Michal, feel free to do what you think correct.

If you do not mind I would rather go with the simple patch first and
then build on top of that. For two reasons 1) it documents the CMA
requirement and 2) there do not seem to be any real users affected by
the issue you are seeing right now. And 3) I really believe
alloc_contig_range needs a deeper thought to be usable in more general
contexts.

> > 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?
> 
> I noticed the problem you mentioned now and, yes, it should be fixed.
> My patch will naturally fixes this issue, too.

I really do not see how.
-- 
Michal Hocko
SUSE Labs