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

From: Joonsoo Kim <iamjoonsoo.kim@lge.com>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>,
	linux-mm@kvack.org, Michael Ellerman <mpe@ellerman.id.au>,
	Andrew Morton <akpm@linux-foundation.org>,
	KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>,
	Reza Arbab <arbab@linux.vnet.ibm.com>,
	Yasuaki Ishimatsu <yasu.isimatu@gmail.com>,
	qiuxishi@huawei.com, Igor Mammedov <imammedo@redhat.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
Date: Thu, 26 Oct 2017 11:47:07 +0900
Message-ID: <20171026024707.GA11791@js1304-P5Q-DELUXE> (raw)
In-Reply-To: <fdb6b325-8de8-b809-81eb-c164736d6a58@suse.cz>

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.

> 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.

Thanks.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>