* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-19 12:21 ` Michal Hocko
@ 2017-10-20 2:13 ` Joonsoo Kim
2017-10-20 5:59 ` Michal Hocko
2017-10-20 7:22 ` Xishi Qiu
2017-10-26 13:04 ` Vlastimil Babka
2017-10-26 13:59 ` Michal Hocko
2 siblings, 2 replies; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-20 2:13 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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@suse.com>
> 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.
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().
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 2:13 ` Joonsoo Kim
@ 2017-10-20 5:59 ` Michal Hocko
2017-10-20 6:50 ` Joonsoo Kim
2017-10-20 7:22 ` Xishi Qiu
1 sibling, 1 reply; 31+ messages in thread
From: Michal Hocko @ 2017-10-20 5:59 UTC (permalink / raw)
To: Joonsoo Kim
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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@suse.com>
> > 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.
> 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?
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 5:59 ` Michal Hocko
@ 2017-10-20 6:50 ` Joonsoo Kim
2017-10-20 7:02 ` Michal Hocko
0 siblings, 1 reply; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-20 6:50 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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@suse.com>
> > > 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.
Until now, there is no user calling alloc_contig_range() with partial
pages except CMA allocator but API could support it.
>
> > 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?
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 6:50 ` Joonsoo Kim
@ 2017-10-20 7:02 ` Michal Hocko
2017-10-23 5:23 ` Joonsoo Kim
0 siblings, 1 reply; 31+ messages in thread
From: Michal Hocko @ 2017-10-20 7:02 UTC (permalink / raw)
To: Joonsoo Kim
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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@suse.com>
> > > > 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
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 7:02 ` Michal Hocko
@ 2017-10-23 5:23 ` Joonsoo Kim
2017-10-23 8:10 ` Michal Hocko
0 siblings, 1 reply; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-23 5:23 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Fri, Oct 20, 2017 at 09:02:20AM +0200, Michal Hocko wrote:
> 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@suse.com>
> > > > > 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
No, but, as I mentioned already, this API can be called with less
pages. I know that there is no user with less pages at this moment but
I cannot see any point to reduce this API's capability.
> 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.
It is already used by other component.
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?
> > > > 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.
I think that it can be done easily.
set_migratetype_isolate() {
lock
__set_migratetype_isolate();
unlock
}
set_migratetype_isolate_if_no_unmovable_pages() {
lock
if (has_unmovable_pages())
fail
else
__set_migratetype_isolate()
unlock
}
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>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-23 5:23 ` Joonsoo Kim
@ 2017-10-23 8:10 ` Michal Hocko
2017-10-24 4:44 ` Joonsoo Kim
0 siblings, 1 reply; 31+ messages in thread
From: Michal Hocko @ 2017-10-23 8:10 UTC (permalink / raw)
To: Joonsoo Kim
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Mon 23-10-17 14:23:09, Joonsoo Kim wrote:
> On Fri, Oct 20, 2017 at 09:02:20AM +0200, Michal Hocko wrote:
> > 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@suse.com>
> > > > > > 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
>
> No, but, as I mentioned already, this API can be called with less
> pages. I know that there is no user with less pages at this moment but
> I cannot see any point to reduce this API's capability.
I am still confused. So when exactly would you want to use this api for
MIGRATE_MOVABLE and use a partial MIGRATE_CMA pageblock?
> > 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.
>
> It is already used by other component.
>
> 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.
> > > > > 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.
>
> I think that it can be done easily.
>
> set_migratetype_isolate() {
> lock
> __set_migratetype_isolate();
> unlock
> }
>
> set_migratetype_isolate_if_no_unmovable_pages() {
> lock
> if (has_unmovable_pages())
> fail
> else
> __set_migratetype_isolate()
> unlock
> }
So you are essentially suggesting to split the API for
alloc_contig_range and hotplug users? Care to send a patch? It is not
like I would really love this but I would really like to have this issue
addressed because I really do want all other patches which depend on
this to be merged in the next release cycle.
That being said, I would much rather see MIGRATE_CMA case special cased
than duplicate the already confusing API but I will not insist of
course.
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-23 8:10 ` Michal Hocko
@ 2017-10-24 4:44 ` Joonsoo Kim
2017-10-24 7:44 ` Michal Hocko
2017-10-24 8:12 ` Vlastimil Babka
0 siblings, 2 replies; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-24 4:44 UTC (permalink / raw)
To: Michal Hocko
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Mon, Oct 23, 2017 at 10:10:09AM +0200, Michal Hocko wrote:
> On Mon 23-10-17 14:23:09, Joonsoo Kim wrote:
> > On Fri, Oct 20, 2017 at 09:02:20AM +0200, Michal Hocko wrote:
> > > 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@suse.com>
> > > > > > > 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
> >
> > No, but, as I mentioned already, this API can be called with less
> > pages. I know that there is no user with less pages at this moment but
> > I cannot see any point to reduce this API's capability.
>
> I am still confused. So when exactly would you want to use this api for
> MIGRATE_MOVABLE and use a partial MIGRATE_CMA pageblock?
>
> > > 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.
> >
> > It is already used by other component.
> >
> > 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 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.
> >
> > I think that it can be done easily.
> >
> > set_migratetype_isolate() {
> > lock
> > __set_migratetype_isolate();
> > unlock
> > }
> >
> > set_migratetype_isolate_if_no_unmovable_pages() {
> > lock
> > if (has_unmovable_pages())
> > fail
> > else
> > __set_migratetype_isolate()
> > unlock
> > }
>
> So you are essentially suggesting to split the API for
> alloc_contig_range and hotplug users? Care to send a patch? It is not
> like I would really love this but I would really like to have this issue
> addressed because I really do want all other patches which depend on
> this to be merged in the next release cycle.
>
> That being said, I would much rather see MIGRATE_CMA case special cased
> than duplicate the already confusing API but I will not insist of
> course.
Okay. I atteach the patch. Andrew, could you revert Michal's series
and apply this patch first? Perhaps, Michal will resend his series on
top of this one.
Thanks.
--------------->8-------------------
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-24 4:44 ` Joonsoo Kim
@ 2017-10-24 7:44 ` Michal Hocko
2017-10-24 8:12 ` Vlastimil Babka
1 sibling, 0 replies; 31+ messages in thread
From: Michal Hocko @ 2017-10-24 7:44 UTC (permalink / raw)
To: Joonsoo Kim
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Tue 24-10-17 13:44:23, Joonsoo Kim wrote:
> On Mon, Oct 23, 2017 at 10:10:09AM +0200, Michal Hocko wrote:
[...]
> > 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'.
I am still confused. So when is it safe to isolate a page from the CMA
pageblock for something that is not a CMA allocation request? Don't we
lose a CMA guanratee that way?
[...]
> > That being said, I would much rather see MIGRATE_CMA case special cased
> > than duplicate the already confusing API but I will not insist of
> > course.
>
> Okay. I atteach the patch. Andrew, could you revert Michal's series
> and apply this patch first? Perhaps, Michal will resend his series on
> top of this one.
I am not convinced about this approach but I will not argue about the
patch though. If this is seen as a right way forward, I will rebase
my patches on top.
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-24 4:44 ` Joonsoo Kim
2017-10-24 7:44 ` Michal Hocko
@ 2017-10-24 8:12 ` Vlastimil Babka
2017-10-24 12:25 ` Michal Hocko
2017-10-26 2:47 ` Joonsoo Kim
1 sibling, 2 replies; 31+ messages in thread
From: Vlastimil Babka @ 2017-10-24 8:12 UTC (permalink / raw)
To: Joonsoo Kim, Michal Hocko
Cc: linux-mm, Michael Ellerman, Andrew Morton, KAMEZAWA Hiroyuki,
Reza Arbab, Yasuaki Ishimatsu, qiuxishi, Igor Mammedov,
Vitaly Kuznetsov, LKML
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?
Vlastimil
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-24 8:12 ` Vlastimil Babka
@ 2017-10-24 12:25 ` Michal Hocko
2017-10-26 2:47 ` Joonsoo Kim
1 sibling, 0 replies; 31+ messages in thread
From: Michal Hocko @ 2017-10-24 12:25 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Joonsoo Kim, linux-mm, Michael Ellerman, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-24 8:12 ` Vlastimil Babka
2017-10-24 12:25 ` Michal Hocko
@ 2017-10-26 2:47 ` Joonsoo Kim
2017-10-26 7:41 ` Michal Hocko
1 sibling, 1 reply; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-26 2:47 UTC (permalink / raw)
To: Vlastimil Babka
Cc: Michal Hocko, linux-mm, Michael Ellerman, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-26 2:47 ` Joonsoo Kim
@ 2017-10-26 7:41 ` Michal Hocko
0 siblings, 0 replies; 31+ messages in thread
From: Michal Hocko @ 2017-10-26 7:41 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Vlastimil Babka, linux-mm, Michael Ellerman, Andrew Morton,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
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
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 2:13 ` Joonsoo Kim
2017-10-20 5:59 ` Michal Hocko
@ 2017-10-20 7:22 ` Xishi Qiu
2017-10-20 8:17 ` Michal Hocko
1 sibling, 1 reply; 31+ messages in thread
From: Xishi Qiu @ 2017-10-20 7:22 UTC (permalink / raw)
To: Joonsoo Kim
Cc: Michal Hocko, linux-mm, Michael Ellerman, Vlastimil Babka,
Andrew Morton, KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu,
Igor Mammedov, Vitaly Kuznetsov, LKML
On 2017/10/20 10:13, 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@suse.com>
>> 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.
>
> 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().
>
> Thanks.
>
Hi Joonsoo,
How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()?
Something like the skip_hwpoisoned_pages.
Thanks,
Xishi Qiu
>
> .
>
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 7:22 ` Xishi Qiu
@ 2017-10-20 8:17 ` Michal Hocko
2017-10-23 5:26 ` Joonsoo Kim
0 siblings, 1 reply; 31+ messages in thread
From: Michal Hocko @ 2017-10-20 8:17 UTC (permalink / raw)
To: Xishi Qiu
Cc: Joonsoo Kim, linux-mm, Michael Ellerman, Vlastimil Babka,
Andrew Morton, KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Fri 20-10-17 15:22:14, Xishi Qiu wrote:
> On 2017/10/20 10:13, Joonsoo Kim wrote:
>
> > On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote:
[...]
> >> >From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
> >> From: Michal Hocko <mhocko@suse.com>
> >> 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.
> >
> > 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().
> >
> > Thanks.
> >
>
> Hi Joonsoo,
>
> How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()?
> Something like the skip_hwpoisoned_pages.
I believe this is what Joonsoo was proposing actually. I cannot say I
would like skip_hwpoisoned_pages and adding one more flag is just too
ugly. So I would prefer to have something that would actually make sense
from the semantic POV. If CMA really needs to work with partial CMA
blocks then MIGRATE_CMA as an indicator sounds like the right way to go
to me. But I am not a CMA expert so I might be missing some subtlety
here.
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-20 8:17 ` Michal Hocko
@ 2017-10-23 5:26 ` Joonsoo Kim
0 siblings, 0 replies; 31+ messages in thread
From: Joonsoo Kim @ 2017-10-23 5:26 UTC (permalink / raw)
To: Michal Hocko
Cc: Xishi Qiu, linux-mm, Michael Ellerman, Vlastimil Babka,
Andrew Morton, KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Fri, Oct 20, 2017 at 10:17:00AM +0200, Michal Hocko wrote:
> On Fri 20-10-17 15:22:14, Xishi Qiu wrote:
> > On 2017/10/20 10:13, Joonsoo Kim wrote:
> >
> > > On Thu, Oct 19, 2017 at 02:21:18PM +0200, Michal Hocko wrote:
> [...]
> > >> >From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
> > >> From: Michal Hocko <mhocko@suse.com>
> > >> 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.
> > >
> > > 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().
> > >
> > > Thanks.
> > >
> >
> > Hi Joonsoo,
> >
> > How about add a flag to skip or not has_unmovable_pages() in set_migratetype_isolate()?
> > Something like the skip_hwpoisoned_pages.
>
> I believe this is what Joonsoo was proposing actually. I cannot say I
Yes, I initially suggested this idea but change my mind. Now, I think
that problem is not in has_unmovable_pages() but in
set_migratetype_isolate(). So different solution is needed. See my other
reply to Michal.
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>
^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-19 12:21 ` Michal Hocko
2017-10-20 2:13 ` Joonsoo Kim
@ 2017-10-26 13:04 ` Vlastimil Babka
2017-10-26 13:59 ` Michal Hocko
2 siblings, 0 replies; 31+ messages in thread
From: Vlastimil Babka @ 2017-10-26 13:04 UTC (permalink / raw)
To: Michal Hocko, Joonsoo Kim, Andrew Morton
Cc: linux-mm, Michael Ellerman, KAMEZAWA Hiroyuki, Reza Arbab,
Yasuaki Ishimatsu, qiuxishi, Igor Mammedov, Vitaly Kuznetsov,
LKML
On 10/19/2017 02:21 PM, 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@suse.com>
> 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.
>
> Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
Acked-by: Vlastimil Babka <vbabka@suse.cz>
> ---
> include/linux/page-isolation.h | 2 +-
> mm/page_alloc.c | 12 +++++++++++-
> mm/page_isolation.c | 10 +++++-----
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index d4cd2014fa6f..fa9db0c7b54e 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -30,7 +30,7 @@ static inline bool is_migrate_isolate(int migratetype)
> #endif
>
> bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> - bool skip_hwpoisoned_pages);
> + int migratetype, bool skip_hwpoisoned_pages);
> void set_pageblock_migratetype(struct page *page, int migratetype);
> int move_freepages_block(struct zone *zone, struct page *page,
> int migratetype, int *num_movable);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5b4d85ae445c..259aeb22462f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7344,6 +7344,7 @@ void *__init alloc_large_system_hash(const char *tablename,
> * race condition. So you can't expect this function should be exact.
> */
> bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> + int migratetype,
> bool skip_hwpoisoned_pages)
> {
> unsigned long pfn, iter, found;
> @@ -7356,6 +7357,15 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> if (zone_idx(zone) == ZONE_MOVABLE)
> return false;
>
> + /*
> + * CMA allocations (alloc_contig_range) really need to mark isolate
> + * CMA pageblocks even when they are not movable in fact so consider
> + * them movable here.
> + */
> + if (is_migrate_cma(migratetype) &&
> + is_migrate_cma(get_pageblock_migratetype(page)))
> + return false;
> +
> pfn = page_to_pfn(page);
> for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
> unsigned long check = pfn + iter;
> @@ -7441,7 +7451,7 @@ bool is_pageblock_removable_nolock(struct page *page)
> if (!zone_spans_pfn(zone, pfn))
> return false;
>
> - return !has_unmovable_pages(zone, page, 0, true);
> + return !has_unmovable_pages(zone, page, 0, MIGRATE_MOVABLE, true);
> }
>
> #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 757410d9f758..8616f5332c77 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -14,7 +14,7 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/page_isolation.h>
>
> -static int set_migratetype_isolate(struct page *page,
> +static int set_migratetype_isolate(struct page *page, int migratetype,
> bool skip_hwpoisoned_pages)
> {
> struct zone *zone;
> @@ -51,7 +51,7 @@ static int set_migratetype_isolate(struct page *page,
> * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
> * We just check MOVABLE pages.
> */
> - if (!has_unmovable_pages(zone, page, arg.pages_found,
> + if (!has_unmovable_pages(zone, page, arg.pages_found, migratetype,
> skip_hwpoisoned_pages))
> ret = 0;
>
> @@ -63,14 +63,14 @@ static int set_migratetype_isolate(struct page *page,
> out:
> if (!ret) {
> unsigned long nr_pages;
> - int migratetype = get_pageblock_migratetype(page);
> + int mt = get_pageblock_migratetype(page);
>
> set_pageblock_migratetype(page, MIGRATE_ISOLATE);
> zone->nr_isolate_pageblock++;
> nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE,
> NULL);
>
> - __mod_zone_freepage_state(zone, -nr_pages, migratetype);
> + __mod_zone_freepage_state(zone, -nr_pages, mt);
> }
>
> spin_unlock_irqrestore(&zone->lock, flags);
> @@ -182,7 +182,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> pfn += pageblock_nr_pages) {
> page = __first_valid_page(pfn, pageblock_nr_pages);
> if (page &&
> - set_migratetype_isolate(page, skip_hwpoisoned_pages)) {
> + set_migratetype_isolate(page, migratetype, skip_hwpoisoned_pages)) {
> undo_pfn = pfn;
> goto undo;
> }
>
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread* Re: [PATCH 1/2] mm: drop migrate type checks from has_unmovable_pages
2017-10-19 12:21 ` Michal Hocko
2017-10-20 2:13 ` Joonsoo Kim
2017-10-26 13:04 ` Vlastimil Babka
@ 2017-10-26 13:59 ` Michal Hocko
2 siblings, 0 replies; 31+ messages in thread
From: Michal Hocko @ 2017-10-26 13:59 UTC (permalink / raw)
To: Andrew Morton
Cc: linux-mm, Michael Ellerman, Vlastimil Babka, Joonsoo Kim,
KAMEZAWA Hiroyuki, Reza Arbab, Yasuaki Ishimatsu, qiuxishi,
Igor Mammedov, Vitaly Kuznetsov, LKML
On Thu 19-10-17 14:21:18, Michal Hocko wrote:
[...]
> From 8cbd811d741f5dd93d1b21bb3ef94482a4d0bd32 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> 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.
>
> Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
Andrew,
could you add this one to the bundle as well? After
mm-drop-migrate-type-checks-from-has_unmovable_pages.patch, please.
Joonsoo would like to see a larger change in this area [1] but I think
we need to think those much more through [2] and Joonsoo agreed to take
the simpler patch first [3].
Thanks!
[1] http://lkml.kernel.org/r/20171024044423.GA31424@js1304-P5Q-DELUXE
[2] http://lkml.kernel.org/r/20171024122526.3kmabkcbmj4johli@dhcp22.suse.cz
[3] http://lkml.kernel.org/r/20171026024707.GA11791@js1304-P5Q-DELUXE
> include/linux/page-isolation.h | 2 +-
> mm/page_alloc.c | 12 +++++++++++-
> mm/page_isolation.c | 10 +++++-----
> 3 files changed, 17 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/page-isolation.h b/include/linux/page-isolation.h
> index d4cd2014fa6f..fa9db0c7b54e 100644
> --- a/include/linux/page-isolation.h
> +++ b/include/linux/page-isolation.h
> @@ -30,7 +30,7 @@ static inline bool is_migrate_isolate(int migratetype)
> #endif
>
> bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> - bool skip_hwpoisoned_pages);
> + int migratetype, bool skip_hwpoisoned_pages);
> void set_pageblock_migratetype(struct page *page, int migratetype);
> int move_freepages_block(struct zone *zone, struct page *page,
> int migratetype, int *num_movable);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 5b4d85ae445c..259aeb22462f 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7344,6 +7344,7 @@ void *__init alloc_large_system_hash(const char *tablename,
> * race condition. So you can't expect this function should be exact.
> */
> bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> + int migratetype,
> bool skip_hwpoisoned_pages)
> {
> unsigned long pfn, iter, found;
> @@ -7356,6 +7357,15 @@ bool has_unmovable_pages(struct zone *zone, struct page *page, int count,
> if (zone_idx(zone) == ZONE_MOVABLE)
> return false;
>
> + /*
> + * CMA allocations (alloc_contig_range) really need to mark isolate
> + * CMA pageblocks even when they are not movable in fact so consider
> + * them movable here.
> + */
> + if (is_migrate_cma(migratetype) &&
> + is_migrate_cma(get_pageblock_migratetype(page)))
> + return false;
> +
> pfn = page_to_pfn(page);
> for (found = 0, iter = 0; iter < pageblock_nr_pages; iter++) {
> unsigned long check = pfn + iter;
> @@ -7441,7 +7451,7 @@ bool is_pageblock_removable_nolock(struct page *page)
> if (!zone_spans_pfn(zone, pfn))
> return false;
>
> - return !has_unmovable_pages(zone, page, 0, true);
> + return !has_unmovable_pages(zone, page, 0, MIGRATE_MOVABLE, true);
> }
>
> #if (defined(CONFIG_MEMORY_ISOLATION) && defined(CONFIG_COMPACTION)) || defined(CONFIG_CMA)
> diff --git a/mm/page_isolation.c b/mm/page_isolation.c
> index 757410d9f758..8616f5332c77 100644
> --- a/mm/page_isolation.c
> +++ b/mm/page_isolation.c
> @@ -14,7 +14,7 @@
> #define CREATE_TRACE_POINTS
> #include <trace/events/page_isolation.h>
>
> -static int set_migratetype_isolate(struct page *page,
> +static int set_migratetype_isolate(struct page *page, int migratetype,
> bool skip_hwpoisoned_pages)
> {
> struct zone *zone;
> @@ -51,7 +51,7 @@ static int set_migratetype_isolate(struct page *page,
> * FIXME: Now, memory hotplug doesn't call shrink_slab() by itself.
> * We just check MOVABLE pages.
> */
> - if (!has_unmovable_pages(zone, page, arg.pages_found,
> + if (!has_unmovable_pages(zone, page, arg.pages_found, migratetype,
> skip_hwpoisoned_pages))
> ret = 0;
>
> @@ -63,14 +63,14 @@ static int set_migratetype_isolate(struct page *page,
> out:
> if (!ret) {
> unsigned long nr_pages;
> - int migratetype = get_pageblock_migratetype(page);
> + int mt = get_pageblock_migratetype(page);
>
> set_pageblock_migratetype(page, MIGRATE_ISOLATE);
> zone->nr_isolate_pageblock++;
> nr_pages = move_freepages_block(zone, page, MIGRATE_ISOLATE,
> NULL);
>
> - __mod_zone_freepage_state(zone, -nr_pages, migratetype);
> + __mod_zone_freepage_state(zone, -nr_pages, mt);
> }
>
> spin_unlock_irqrestore(&zone->lock, flags);
> @@ -182,7 +182,7 @@ int start_isolate_page_range(unsigned long start_pfn, unsigned long end_pfn,
> pfn += pageblock_nr_pages) {
> page = __first_valid_page(pfn, pageblock_nr_pages);
> if (page &&
> - set_migratetype_isolate(page, skip_hwpoisoned_pages)) {
> + set_migratetype_isolate(page, migratetype, skip_hwpoisoned_pages)) {
> undo_pfn = pfn;
> goto undo;
> }
> --
> 2.14.2
>
> --
> Michal Hocko
> SUSE Labs
--
Michal Hocko
SUSE Labs
--
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>
^ permalink raw reply [flat|nested] 31+ messages in thread