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 <[email protected]>
Signed-off-by: Barry Song <[email protected]>
---
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
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 <[email protected]>
> Signed-off-by: Barry Song <[email protected]>
> ---
> 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);
On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang
<[email protected]> 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 <[email protected]>
> > Signed-off-by: Barry Song <[email protected]>
> > ---
> > 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
On 12/6/2023 6:18 PM, Barry Song wrote:
> On Wed, Dec 6, 2023 at 10:54 PM Baolin Wang
> <[email protected]> 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 <[email protected]>
>>> Signed-off-by: Barry Song <[email protected]>
>>> ---
>>> 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