* [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock()
@ 2021-01-23 15:43 Wonhyuk Yang
2021-01-25 16:49 ` Vlastimil Babka
0 siblings, 1 reply; 3+ messages in thread
From: Wonhyuk Yang @ 2021-01-23 15:43 UTC (permalink / raw)
To: Andrew Morton; +Cc: Mel Gorman, Vlastimil Babka, linux-mm, Wonhyuk Yang
In the fast_find_migrateblock(), It iterate freelist to find
proper pageblock. But there are two edge cases. First, if the page
we found is equal to cc->migrate_pfn, it is considered that we
didn't found suitable pageblock. Second, if the loop was terminated
because order is less than PAGE_ALLOC_COSTLY_ORDER, it could
be considered that we found suitable one.
Fixes: 70b44595eafe9 ("mm, compaction: use free lists to quickly locate
a migration source")
Signed-off-by: Wonhyuk Yang <vvghjk1234@gmail.com>
---
mm/compaction.c | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index e5acb9714436..46f49e6b7d1a 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1742,8 +1742,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
distance >>= 2;
high_pfn = pageblock_start_pfn(cc->migrate_pfn + distance);
- for (order = cc->order - 1;
- order >= PAGE_ALLOC_COSTLY_ORDER && pfn == cc->migrate_pfn && nr_scanned < limit;
+ for (order = cc->order - 1, pfn = high_pfn;
+ order >= PAGE_ALLOC_COSTLY_ORDER && pfn == high_pfn && nr_scanned < limit;
order--) {
struct free_area *area = &cc->zone->free_area[order];
struct list_head *freelist;
@@ -1785,7 +1785,6 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
}
if (nr_scanned >= limit) {
- cc->fast_search_fail++;
move_freelist_tail(freelist, freepage);
break;
}
@@ -1799,9 +1798,10 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
* If fast scanning failed then use a cached entry for a page block
* that had free pages as the basis for starting a linear scan.
*/
- if (pfn == cc->migrate_pfn)
+ if (pfn == high_pfn) {
+ cc->fast_search_fail++;
pfn = reinit_migrate_pfn(cc);
-
+ }
return pfn;
}
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock()
2021-01-23 15:43 [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock() Wonhyuk Yang
@ 2021-01-25 16:49 ` Vlastimil Babka
2021-01-26 14:37 ` Wonhyuk Yang
0 siblings, 1 reply; 3+ messages in thread
From: Vlastimil Babka @ 2021-01-25 16:49 UTC (permalink / raw)
To: Wonhyuk Yang, Andrew Morton; +Cc: Mel Gorman, linux-mm
On 1/23/21 4:43 PM, Wonhyuk Yang wrote:
> In the fast_find_migrateblock(), It iterate freelist to find
> proper pageblock. But there are two edge cases. First, if the page
> we found is equal to cc->migrate_pfn, it is considered that we
> didn't found suitable pageblock. Second, if the loop was terminated
> because order is less than PAGE_ALLOC_COSTLY_ORDER, it could
> be considered that we found suitable one.
>
> Fixes: 70b44595eafe9 ("mm, compaction: use free lists to quickly locate
> a migration source")
>
> Signed-off-by: Wonhyuk Yang <vvghjk1234@gmail.com>
Seems correct, although it's quite subtle code and it's been a long time...
Acked-by: Vlastimil Babka <vbabka@suse.cz>
Maybe instead of replacing one magic value of 'pfn' with another
(cc->migrate_pfn with high_pfn) I would just add a "bool found" and use it
appropriately. Would make the function less subtle perhaps.
While reviewing I found more potentially questionable parts, if you're interested:
- if we go through "if (get_pageblock_skip(freepage))... continue;" we are
increasing nr_scanned++; but not checking the limit.
- the "if (list_is_last(freelist, &freepage->lru)) break;" part seems
unneccesary? if we are on the last page and just "continue;" the
list_for_each_entry() iteration should stop anyway.
> ---
> mm/compaction.c | 10 +++++-----
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index e5acb9714436..46f49e6b7d1a 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1742,8 +1742,8 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
> distance >>= 2;
> high_pfn = pageblock_start_pfn(cc->migrate_pfn + distance);
>
> - for (order = cc->order - 1;
> - order >= PAGE_ALLOC_COSTLY_ORDER && pfn == cc->migrate_pfn && nr_scanned < limit;
> + for (order = cc->order - 1, pfn = high_pfn;
> + order >= PAGE_ALLOC_COSTLY_ORDER && pfn == high_pfn && nr_scanned < limit;
> order--) {
> struct free_area *area = &cc->zone->free_area[order];
> struct list_head *freelist;
> @@ -1785,7 +1785,6 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
> }
>
> if (nr_scanned >= limit) {
> - cc->fast_search_fail++;
> move_freelist_tail(freelist, freepage);
> break;
> }
> @@ -1799,9 +1798,10 @@ static unsigned long fast_find_migrateblock(struct compact_control *cc)
> * If fast scanning failed then use a cached entry for a page block
> * that had free pages as the basis for starting a linear scan.
> */
> - if (pfn == cc->migrate_pfn)
> + if (pfn == high_pfn) {
> + cc->fast_search_fail++;
> pfn = reinit_migrate_pfn(cc);
> -
> + }
> return pfn;
> }
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock()
2021-01-25 16:49 ` Vlastimil Babka
@ 2021-01-26 14:37 ` Wonhyuk Yang
0 siblings, 0 replies; 3+ messages in thread
From: Wonhyuk Yang @ 2021-01-26 14:37 UTC (permalink / raw)
To: Vlastimil Babka; +Cc: Andrew Morton, Mel Gorman, linux-mm
On Tue, Jan 26, 2021 at 1:49 AM Vlastimil Babka <vbabka@suse.cz> wrote:
>
> Maybe instead of replacing one magic value of 'pfn' with another
> (cc->migrate_pfn with high_pfn) I would just add a "bool found" and use it
> appropriately. Would make the function less subtle perhaps.
I agree. Using that bool variable will look good.
>
> While reviewing I found more potentially questionable parts, if you're interested:
>
> - if we go through "if (get_pageblock_skip(freepage))... continue;" we are
> increasing nr_scanned++; but not checking the limit.
Yes, you've got a point. I think nr_scanned checking should be first.
>
> - the "if (list_is_last(freelist, &freepage->lru)) break;" part seems
> unneccesary? if we are on the last page and just "continue;" the
> list_for_each_entry() iteration should stop anyway.
>
Yes It seems unnecessary. only if the skipped block was reordered, it
would be required.
Then I will reflect your opinion and send a new version.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-01-26 14:37 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-23 15:43 [PATCH] mm/compactoin: Fix edge case of fast_find_migrateblock() Wonhyuk Yang
2021-01-25 16:49 ` Vlastimil Babka
2021-01-26 14:37 ` Wonhyuk Yang
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.