* [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
@ 2023-12-06 11:00 Barry Song
2023-12-07 1:57 ` Baolin Wang
2023-12-13 13:02 ` Mel Gorman
0 siblings, 2 replies; 5+ messages in thread
From: Barry Song @ 2023-12-06 11:00 UTC (permalink / raw
To: akpm, baolin.wang, linux-mm
Cc: david, hannes, huzhanyuan, linux-kernel, mgorman, shikemeng,
v-songbaohua, willy
Testing shows fast_isolate_freepages can blindly choose an unsuitable
pageblock from time to time particularly while the min mark is used
from XXX path:
if (!page) {
cc->fast_search_fail++;
if (scan_start) {
/*
* Use the highest PFN found above min. If one was
* not found, be pessimistic for direct compaction
* and use the min mark.
*/
if (highest >= min_pfn) {
page = pfn_to_page(highest);
cc->free_pfn = highest;
} else {
if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
page = pageblock_pfn_to_page(min_pfn,
min(pageblock_end_pfn(min_pfn),
zone_end_pfn(cc->zone)),
cc->zone);
cc->free_pfn = min_pfn;
}
}
}
}
The reason is that no code is doing any check on the min_pfn
min_pfn = pageblock_start_pfn(cc->free_pfn - (distance >> 1));
In contrast, slow path of isolate_freepages() is always skipping unsuitable
pageblocks in a decent way.
This issue doesn't happen quite often. When running 25 machines with 16GiB
memory for one night, most of them can hit this unexpected code path.
However the frequency isn't like many times per second. It might be one
time in a couple of hours. Thus, it is very hard to measure the visible
performance impact in my machines though the affection of choosing the
unsuitable migration_target should be negative in theory.
I feel it's still worth fixing this to at least make the code theoretically
self-explanatory as it is quite odd an unsuitable migration_target can be
still migration_target.
Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
v1:
move the fix to the specific min_pfn path with respect to Baolin's comment
rfc:
https://lore.kernel.org/linux-mm/20231129104530.63787-1-v-songbaohua@oppo.com/#t
mm/compaction.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/mm/compaction.c b/mm/compaction.c
index 01ba298739dd..de15a2ef0af5 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1611,6 +1611,9 @@ static void fast_isolate_freepages(struct compact_control *cc)
min(pageblock_end_pfn(min_pfn),
zone_end_pfn(cc->zone)),
cc->zone);
+ if (page && !suitable_migration_target(cc, page))
+ page = NULL;
+
cc->free_pfn = min_pfn;
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-06 11:00 [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Barry Song
@ 2023-12-07 1:57 ` Baolin Wang
2023-12-13 13:02 ` Mel Gorman
1 sibling, 0 replies; 5+ messages in thread
From: Baolin Wang @ 2023-12-07 1:57 UTC (permalink / raw
To: Barry Song, akpm, linux-mm
Cc: david, hannes, huzhanyuan, linux-kernel, mgorman, shikemeng,
v-songbaohua, willy
On 12/6/2023 7:00 PM, Barry Song wrote:
> Testing shows fast_isolate_freepages can blindly choose an unsuitable
> pageblock from time to time particularly while the min mark is used
> from XXX path:
> if (!page) {
> cc->fast_search_fail++;
> if (scan_start) {
> /*
> * Use the highest PFN found above min. If one was
> * not found, be pessimistic for direct compaction
> * and use the min mark.
> */
> if (highest >= min_pfn) {
> page = pfn_to_page(highest);
> cc->free_pfn = highest;
> } else {
> if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
> page = pageblock_pfn_to_page(min_pfn,
> min(pageblock_end_pfn(min_pfn),
> zone_end_pfn(cc->zone)),
> cc->zone);
> cc->free_pfn = min_pfn;
> }
> }
> }
> }
>
> The reason is that no code is doing any check on the min_pfn
> min_pfn = pageblock_start_pfn(cc->free_pfn - (distance >> 1));
>
> In contrast, slow path of isolate_freepages() is always skipping unsuitable
> pageblocks in a decent way.
>
> This issue doesn't happen quite often. When running 25 machines with 16GiB
> memory for one night, most of them can hit this unexpected code path.
> However the frequency isn't like many times per second. It might be one
> time in a couple of hours. Thus, it is very hard to measure the visible
> performance impact in my machines though the affection of choosing the
> unsuitable migration_target should be negative in theory.
>
> I feel it's still worth fixing this to at least make the code theoretically
> self-explanatory as it is quite odd an unsuitable migration_target can be
> still migration_target.
>
> Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
LGTM. Thanks.
Reviewed-by: Baolin Wang <baolin.wang@linux.alibaba.com>
> ---
> v1:
> move the fix to the specific min_pfn path with respect to Baolin's comment
> rfc:
> https://lore.kernel.org/linux-mm/20231129104530.63787-1-v-songbaohua@oppo.com/#t
>
> mm/compaction.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 01ba298739dd..de15a2ef0af5 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1611,6 +1611,9 @@ static void fast_isolate_freepages(struct compact_control *cc)
> min(pageblock_end_pfn(min_pfn),
> zone_end_pfn(cc->zone)),
> cc->zone);
> + if (page && !suitable_migration_target(cc, page))
> + page = NULL;
> +
> cc->free_pfn = min_pfn;
> }
> }
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-06 11:00 [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Barry Song
2023-12-07 1:57 ` Baolin Wang
@ 2023-12-13 13:02 ` Mel Gorman
2023-12-14 1:34 ` Barry Song
1 sibling, 1 reply; 5+ messages in thread
From: Mel Gorman @ 2023-12-13 13:02 UTC (permalink / raw
To: Barry Song
Cc: akpm, baolin.wang, linux-mm, david, hannes, huzhanyuan,
linux-kernel, shikemeng, v-songbaohua, willy
On Thu, Dec 07, 2023 at 12:00:54AM +1300, Barry Song wrote:
> Testing shows fast_isolate_freepages can blindly choose an unsuitable
> pageblock from time to time particularly while the min mark is used
> from XXX path:
> if (!page) {
> cc->fast_search_fail++;
> if (scan_start) {
> /*
> * Use the highest PFN found above min. If one was
> * not found, be pessimistic for direct compaction
> * and use the min mark.
> */
> if (highest >= min_pfn) {
> page = pfn_to_page(highest);
> cc->free_pfn = highest;
> } else {
> if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
> page = pageblock_pfn_to_page(min_pfn,
> min(pageblock_end_pfn(min_pfn),
> zone_end_pfn(cc->zone)),
> cc->zone);
> cc->free_pfn = min_pfn;
> }
> }
> }
> }
>
> The reason is that no code is doing any check on the min_pfn
> min_pfn = pageblock_start_pfn(cc->free_pfn - (distance >> 1));
>
> In contrast, slow path of isolate_freepages() is always skipping unsuitable
> pageblocks in a decent way.
>
> This issue doesn't happen quite often. When running 25 machines with 16GiB
> memory for one night, most of them can hit this unexpected code path.
> However the frequency isn't like many times per second. It might be one
> time in a couple of hours. Thus, it is very hard to measure the visible
> performance impact in my machines though the affection of choosing the
> unsuitable migration_target should be negative in theory.
>
> I feel it's still worth fixing this to at least make the code theoretically
> self-explanatory as it is quite odd an unsuitable migration_target can be
> still migration_target.
>
> Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
Acked-by: Mel Gorman <mgorman@techsingularity.net>
--
Mel Gorman
SUSE Labs
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-13 13:02 ` Mel Gorman
@ 2023-12-14 1:34 ` Barry Song
2023-12-14 22:17 ` Andrew Morton
0 siblings, 1 reply; 5+ messages in thread
From: Barry Song @ 2023-12-14 1:34 UTC (permalink / raw
To: Mel Gorman, akpm
Cc: baolin.wang, linux-mm, david, hannes, huzhanyuan, linux-kernel,
shikemeng, v-songbaohua, willy
On Wed, Dec 13, 2023 at 9:02 PM Mel Gorman <mgorman@techsingularity.net> wrote:
>
> On Thu, Dec 07, 2023 at 12:00:54AM +1300, Barry Song wrote:
> > Testing shows fast_isolate_freepages can blindly choose an unsuitable
> > pageblock from time to time particularly while the min mark is used
> > from XXX path:
> > if (!page) {
> > cc->fast_search_fail++;
> > if (scan_start) {
> > /*
> > * Use the highest PFN found above min. If one was
> > * not found, be pessimistic for direct compaction
> > * and use the min mark.
> > */
> > if (highest >= min_pfn) {
> > page = pfn_to_page(highest);
> > cc->free_pfn = highest;
> > } else {
> > if (cc->direct_compaction && pfn_valid(min_pfn)) { /* XXX */
> > page = pageblock_pfn_to_page(min_pfn,
> > min(pageblock_end_pfn(min_pfn),
> > zone_end_pfn(cc->zone)),
> > cc->zone);
> > cc->free_pfn = min_pfn;
> > }
> > }
> > }
> > }
> >
> > The reason is that no code is doing any check on the min_pfn
> > min_pfn = pageblock_start_pfn(cc->free_pfn - (distance >> 1));
> >
> > In contrast, slow path of isolate_freepages() is always skipping unsuitable
> > pageblocks in a decent way.
> >
> > This issue doesn't happen quite often. When running 25 machines with 16GiB
> > memory for one night, most of them can hit this unexpected code path.
> > However the frequency isn't like many times per second. It might be one
> > time in a couple of hours. Thus, it is very hard to measure the visible
> > performance impact in my machines though the affection of choosing the
> > unsuitable migration_target should be negative in theory.
> >
> > I feel it's still worth fixing this to at least make the code theoretically
> > self-explanatory as it is quite odd an unsuitable migration_target can be
> > still migration_target.
> >
> > Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>
> Acked-by: Mel Gorman <mgorman@techsingularity.net>
Hi Mel,
Thanks!
Hi Andrew,
Given this patch has been in mm-stable,
https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-stable&id=d19b1a1797
does it still have a chance to collect Mel's tag？
>
> --
> Mel Gorman
> SUSE Labs
Thanks
Barry
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-14 1:34 ` Barry Song
@ 2023-12-14 22:17 ` Andrew Morton
0 siblings, 0 replies; 5+ messages in thread
From: Andrew Morton @ 2023-12-14 22:17 UTC (permalink / raw
To: Barry Song
Cc: Mel Gorman, baolin.wang, linux-mm, david, hannes, huzhanyuan,
linux-kernel, shikemeng, v-songbaohua, willy
On Thu, 14 Dec 2023 09:34:35 +0800 Barry Song <21cnbao@gmail.com> wrote:
> > > Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> > > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> >
> > Acked-by: Mel Gorman <mgorman@techsingularity.net>
>
> Hi Mel,
> Thanks!
>
> Hi Andrew,
> Given this patch has been in mm-stable,
> https://git.kernel.org/pub/scm/linux/kernel/git/akpm/mm.git/commit/?h=mm-stable&id=d19b1a1797
> does it still have a chance to collect Mel's tag？
That would require a rebuild of mm-stable, which is also a rebase. I
don't think I've had to do that before - I'll occasionally rebase, but
just to drop things - not a full rebuild from the quilt patches.
If I end up having to do that then I'll somehow try to remember to make
this change.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2023-12-14 22:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-12-06 11:00 [PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Barry Song
2023-12-07 1:57 ` Baolin Wang
2023-12-13 13:02 ` Mel Gorman
2023-12-14 1:34 ` Barry Song
2023-12-14 22:17 ` Andrew Morton
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).