* [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
@ 2023-11-29 10:45 Barry Song
2023-12-06 9:54 ` Baolin Wang
0 siblings, 1 reply; 4+ messages in thread
From: Barry Song @ 2023-11-29 10:45 UTC (permalink / raw)
To: akpm, linux-mm
Cc: david, shikemeng, willy, mgorman, hannes, baolin.wang,
linux-kernel, Barry Song, Zhanyuan Hu
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;
}
}
}
}
In contrast, slow path is skipping unsuitable pageblocks in a decent way.
I don't know if it is an intended design or just an oversight. But
it seems more sensible to skip unsuitable pageblock.
Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
Signed-off-by: Barry Song <v-songbaohua@oppo.com>
---
mm/compaction.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/mm/compaction.c b/mm/compaction.c
index 01ba298739dd..98c485a25614 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc)
cc->total_free_scanned += nr_scanned;
if (!page)
return;
+ /*
+ * Otherwise, we can blindly choose an improper pageblock especially
+ * while using the min mark
+ */
+ if (!suitable_migration_target(cc, page))
+ return;
low_pfn = page_to_pfn(page);
fast_isolate_around(cc, low_pfn);
--
2.34.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-11-29 10:45 [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Barry Song
@ 2023-12-06 9:54 ` Baolin Wang
2023-12-06 10:18 ` Barry Song
0 siblings, 1 reply; 4+ messages in thread
From: Baolin Wang @ 2023-12-06 9:54 UTC (permalink / raw)
To: Barry Song, akpm, linux-mm
Cc: david, shikemeng, willy, mgorman, hannes, linux-kernel,
Barry Song, Zhanyuan Hu
On 11/29/2023 6:45 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;
> }
> }
> }
> }
Yes, the min_pfn can be an unsuitable migration target. But I think we
can just add the suitable_migration_target() validation into 'min_pfn'
case? Since other cases must be suitable target which found from
MIGRATE_MOVABLE free list. Something like below:
diff --git a/mm/compaction.c b/mm/compaction.c
index 01ba298739dd..4e8eb4571909 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct
compact_control *cc)
min(pageblock_end_pfn(min_pfn),
zone_end_pfn(cc->zone)),
cc->zone);
+ if
(!suitable_migration_target(cc, page))
+ page = NULL;
cc->free_pfn = min_pfn;
}
}
By the way, I wonder if this patch can improve the efficiency of
compaction in your test case?
> In contrast, slow path is skipping unsuitable pageblocks in a decent way.
>
> I don't know if it is an intended design or just an oversight. But
> it seems more sensible to skip unsuitable pageblock.
>
> Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> ---
> mm/compaction.c | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 01ba298739dd..98c485a25614 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc)
> cc->total_free_scanned += nr_scanned;
> if (!page)
> return;
> + /*
> + * Otherwise, we can blindly choose an improper pageblock especially
> + * while using the min mark
> + */
> + if (!suitable_migration_target(cc, page))
> + return;
>
> low_pfn = page_to_pfn(page);
> fast_isolate_around(cc, low_pfn);
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-06 9:54 ` Baolin Wang
@ 2023-12-06 10:18 ` Barry Song
2023-12-07 1:50 ` Baolin Wang
0 siblings, 1 reply; 4+ messages in thread
From: Barry Song @ 2023-12-06 10:18 UTC (permalink / raw)
To: Baolin Wang
Cc: akpm, linux-mm, david, shikemeng, willy, mgorman, hannes,
linux-kernel, Barry Song, Zhanyuan Hu
On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> On 11/29/2023 6:45 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;
> > }
> > }
> > }
> > }
>
> Yes, the min_pfn can be an unsuitable migration target. But I think we
> can just add the suitable_migration_target() validation into 'min_pfn'
> case? Since other cases must be suitable target which found from
> MIGRATE_MOVABLE free list. Something like below:
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index 01ba298739dd..4e8eb4571909 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct
> compact_control *cc)
>
> min(pageblock_end_pfn(min_pfn),
>
> zone_end_pfn(cc->zone)),
> cc->zone);
> + if
> (!suitable_migration_target(cc, page))
> + page = NULL;
> cc->free_pfn = min_pfn;
> }
> }
>
yes. this makes more senses.
> By the way, I wonder if this patch can improve the efficiency of
> compaction in your test case?
This happens not quite often. when running 25 machines for
one night, most of them can hit this unexpected code path.
but the frequency isn't many times in one second. it might
be one time in a couple of hours.
so it is very difficult to measure the visible performance impact
in my machines though the affection of choosing the unsuitable
migration_target should be negative.
I feel like it's worth fixing this to at least make the code theoretically
self-explanatory? as it is quite odd unsuitable_migration_target can
be still migration_target?
>
> > In contrast, slow path is skipping unsuitable pageblocks in a decent way.
> >
> > I don't know if it is an intended design or just an oversight. But
> > it seems more sensible to skip unsuitable pageblock.
> >
> > Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
> > Signed-off-by: Barry Song <v-songbaohua@oppo.com>
> > ---
> > mm/compaction.c | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/mm/compaction.c b/mm/compaction.c
> > index 01ba298739dd..98c485a25614 100644
> > --- a/mm/compaction.c
> > +++ b/mm/compaction.c
> > @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc)
> > cc->total_free_scanned += nr_scanned;
> > if (!page)
> > return;
> > + /*
> > + * Otherwise, we can blindly choose an improper pageblock especially
> > + * while using the min mark
> > + */
> > + if (!suitable_migration_target(cc, page))
> > + return;
> >
> > low_pfn = page_to_pfn(page);
> > fast_isolate_around(cc, low_pfn);
Thanks
Barry
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock
2023-12-06 10:18 ` Barry Song
@ 2023-12-07 1:50 ` Baolin Wang
0 siblings, 0 replies; 4+ messages in thread
From: Baolin Wang @ 2023-12-07 1:50 UTC (permalink / raw)
To: Barry Song
Cc: akpm, linux-mm, david, shikemeng, willy, mgorman, hannes,
linux-kernel, Barry Song, Zhanyuan Hu
On 12/6/2023 6:18 PM, Barry Song wrote:
> On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 11/29/2023 6:45 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;
>>> }
>>> }
>>> }
>>> }
>>
>> Yes, the min_pfn can be an unsuitable migration target. But I think we
>> can just add the suitable_migration_target() validation into 'min_pfn'
>> case? Since other cases must be suitable target which found from
>> MIGRATE_MOVABLE free list. Something like below:
>>
>> diff --git a/mm/compaction.c b/mm/compaction.c
>> index 01ba298739dd..4e8eb4571909 100644
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1611,6 +1611,8 @@ static void fast_isolate_freepages(struct
>> compact_control *cc)
>>
>> min(pageblock_end_pfn(min_pfn),
>>
>> zone_end_pfn(cc->zone)),
>> cc->zone);
>> + if
>> (!suitable_migration_target(cc, page))
>> + page = NULL;
>> cc->free_pfn = min_pfn;
>> }
>> }
>>
>
> yes. this makes more senses.
>
>> By the way, I wonder if this patch can improve the efficiency of
>> compaction in your test case?
>
> This happens not quite often. when running 25 machines for
> one night, most of them can hit this unexpected code path.
> but the frequency isn't many times in one second. it might
> be one time in a couple of hours.
>
> so it is very difficult to measure the visible performance impact
> in my machines though the affection of choosing the unsuitable
> migration_target should be negative.
OK. Fair enough.
>
> I feel like it's worth fixing this to at least make the code theoretically
> self-explanatory? as it is quite odd unsuitable_migration_target can
> be still migration_target?
>
>>
>>> In contrast, slow path is skipping unsuitable pageblocks in a decent way.
>>>
>>> I don't know if it is an intended design or just an oversight. But
>>> it seems more sensible to skip unsuitable pageblock.
>>>
>>> Reported-by: Zhanyuan Hu <huzhanyuan@oppo.com>
>>> Signed-off-by: Barry Song <v-songbaohua@oppo.com>
>>> ---
>>> mm/compaction.c | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/mm/compaction.c b/mm/compaction.c
>>> index 01ba298739dd..98c485a25614 100644
>>> --- a/mm/compaction.c
>>> +++ b/mm/compaction.c
>>> @@ -1625,6 +1625,12 @@ static void fast_isolate_freepages(struct compact_control *cc)
>>> cc->total_free_scanned += nr_scanned;
>>> if (!page)
>>> return;
>>> + /*
>>> + * Otherwise, we can blindly choose an improper pageblock especially
>>> + * while using the min mark
>>> + */
>>> + if (!suitable_migration_target(cc, page))
>>> + return;
>>>
>>> low_pfn = page_to_pfn(page);
>>> fast_isolate_around(cc, low_pfn);
>
> Thanks
> Barry
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-12-07 1:50 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-11-29 10:45 [RFC PATCH] mm: compaction: avoid fast_isolate_freepages blindly choose improper pageblock Barry Song
2023-12-06 9:54 ` Baolin Wang
2023-12-06 10:18 ` Barry Song
2023-12-07 1:50 ` Baolin Wang
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 NNTP newsgroup(s).