During stress testing, the following situation was observed:
70 root 39 19 0 0 0 R 100.0 0.0 959:29.92 khugepaged
310936 root 20 0 84416 25620 512 R 99.7 1.5 642:37.22 hugealloc
Tracing shows isolate_migratepages_block() endlessly looping over the
first block in the DMA zone:
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
The problem is that the functions tries to test and set the skip bit
once on the block, to avoid skipping on its own skip-set, using
pageblock_aligned() on the pfn as a test. But because this is the DMA
zone which starts at pfn 1, this is never true for the first block,
and the skip bit isn't set or tested at all. As a result,
fast_find_migrateblock() returns the same pageblock over and over.
If the pfn isn't pageblock-aligned, also check if it's the start of
the zone to ensure test-and-set-exactly-once on unaligned ranges.
Thanks to Vlastimil Babka for the help in debugging this.
Fixes: 90ed667c03fe ("Revert "Revert "mm/compaction: fix set skip in fast_find_migrateblock""")
Signed-off-by: Johannes Weiner <[email protected]>
---
mm/compaction.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/mm/compaction.c b/mm/compaction.c
index dbc9f86b1934..eacca2794e47 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -912,11 +912,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
/*
* Check if the pageblock has already been marked skipped.
- * Only the aligned PFN is checked as the caller isolates
+ * Only the first PFN is checked as the caller isolates
* COMPACT_CLUSTER_MAX at a time so the second call must
* not falsely conclude that the block should be skipped.
*/
- if (!valid_page && pageblock_aligned(low_pfn)) {
+ if (!valid_page && (pageblock_aligned(low_pfn) ||
+ low_pfn == cc->zone->zone_start_pfn)) {
if (!isolation_suitable(cc, page)) {
low_pfn = end_pfn;
folio = NULL;
@@ -2002,7 +2003,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
* before making it "skip" so other compaction instances do
* not scan the same block.
*/
- if (pageblock_aligned(low_pfn) &&
+ if ((pageblock_aligned(low_pfn) ||
+ low_pfn == cc->zone->zone_start_pfn) &&
!fast_find_block && !isolation_suitable(cc, page))
continue;
--
2.41.0
On 7/31/23 19:24, Johannes Weiner wrote:
> During stress testing, the following situation was observed:
>
> 70 root 39 19 0 0 0 R 100.0 0.0 959:29.92 khugepaged
> 310936 root 20 0 84416 25620 512 R 99.7 1.5 642:37.22 hugealloc
>
> Tracing shows isolate_migratepages_block() endlessly looping over the
> first block in the DMA zone:
>
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
>
> The problem is that the functions tries to test and set the skip bit
> once on the block, to avoid skipping on its own skip-set, using
> pageblock_aligned() on the pfn as a test. But because this is the DMA
> zone which starts at pfn 1, this is never true for the first block,
> and the skip bit isn't set or tested at all. As a result,
> fast_find_migrateblock() returns the same pageblock over and over.
>
> If the pfn isn't pageblock-aligned, also check if it's the start of
> the zone to ensure test-and-set-exactly-once on unaligned ranges.
>
> Thanks to Vlastimil Babka for the help in debugging this.
>
> Fixes: 90ed667c03fe ("Revert "Revert "mm/compaction: fix set skip in fast_find_migrateblock""")
Yeah I suggested this commit for Fixes: as before the commit (or the
previous, reverted attempt) the skip would be set in
fast_find_migrateblock() so even though the issue of not handling unaligned
zones properly is older, it wouldn't cause an endless loop otherwise. Since
90ed667c03fe is rc1, we don't need stable.
> Signed-off-by: Johannes Weiner <[email protected]>
Reviewed-by: Vlastimil Babka <[email protected]>
> ---
> mm/compaction.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index dbc9f86b1934..eacca2794e47 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -912,11 +912,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>
> /*
> * Check if the pageblock has already been marked skipped.
> - * Only the aligned PFN is checked as the caller isolates
> + * Only the first PFN is checked as the caller isolates
> * COMPACT_CLUSTER_MAX at a time so the second call must
> * not falsely conclude that the block should be skipped.
> */
> - if (!valid_page && pageblock_aligned(low_pfn)) {
> + if (!valid_page && (pageblock_aligned(low_pfn) ||
> + low_pfn == cc->zone->zone_start_pfn)) {
> if (!isolation_suitable(cc, page)) {
> low_pfn = end_pfn;
> folio = NULL;
> @@ -2002,7 +2003,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
> * before making it "skip" so other compaction instances do
> * not scan the same block.
> */
> - if (pageblock_aligned(low_pfn) &&
> + if ((pageblock_aligned(low_pfn) ||
> + low_pfn == cc->zone->zone_start_pfn) &&
> !fast_find_block && !isolation_suitable(cc, page))
> continue;
>
On Mon, Jul 31, 2023 at 01:24:50PM -0400, Johannes Weiner wrote:
> During stress testing, the following situation was observed:
>
> 70 root 39 19 0 0 0 R 100.0 0.0 959:29.92 khugepaged
> 310936 root 20 0 84416 25620 512 R 99.7 1.5 642:37.22 hugealloc
>
> Tracing shows isolate_migratepages_block() endlessly looping over the
> first block in the DMA zone:
>
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
>
> The problem is that the functions tries to test and set the skip bit
> once on the block, to avoid skipping on its own skip-set, using
> pageblock_aligned() on the pfn as a test. But because this is the DMA
> zone which starts at pfn 1, this is never true for the first block,
> and the skip bit isn't set or tested at all. As a result,
> fast_find_migrateblock() returns the same pageblock over and over.
>
> If the pfn isn't pageblock-aligned, also check if it's the start of
> the zone to ensure test-and-set-exactly-once on unaligned ranges.
>
> Thanks to Vlastimil Babka for the help in debugging this.
>
> Fixes: 90ed667c03fe ("Revert "Revert "mm/compaction: fix set skip in fast_find_migrateblock""")
> Signed-off-by: Johannes Weiner <[email protected]>
Acked-by: Mel Gorman <[email protected]>
--
Mel Gorman
SUSE Labs
On 8/1/2023 1:24 AM, Johannes Weiner wrote:
> During stress testing, the following situation was observed:
>
> 70 root 39 19 0 0 0 R 100.0 0.0 959:29.92 khugepaged
> 310936 root 20 0 84416 25620 512 R 99.7 1.5 642:37.22 hugealloc
>
> Tracing shows isolate_migratepages_block() endlessly looping over the
> first block in the DMA zone:
>
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_finished: node=0 zone=DMA order=9 ret=no_suitable_page
> hugealloc-310936 [001] ..... 237297.415718: mm_compaction_isolate_migratepages: range=(0x1 ~ 0x400) nr_scanned=513 nr_taken=0
>
> The problem is that the functions tries to test and set the skip bit
> once on the block, to avoid skipping on its own skip-set, using
> pageblock_aligned() on the pfn as a test. But because this is the DMA
> zone which starts at pfn 1, this is never true for the first block,
> and the skip bit isn't set or tested at all. As a result,
> fast_find_migrateblock() returns the same pageblock over and over.
>
> If the pfn isn't pageblock-aligned, also check if it's the start of
> the zone to ensure test-and-set-exactly-once on unaligned ranges.
>
> Thanks to Vlastimil Babka for the help in debugging this.
>
> Fixes: 90ed667c03fe ("Revert "Revert "mm/compaction: fix set skip in fast_find_migrateblock""")
> Signed-off-by: Johannes Weiner <[email protected]>
Reviewed-by: Baolin Wang <[email protected]>
> ---
> mm/compaction.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/mm/compaction.c b/mm/compaction.c
> index dbc9f86b1934..eacca2794e47 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -912,11 +912,12 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn,
>
> /*
> * Check if the pageblock has already been marked skipped.
> - * Only the aligned PFN is checked as the caller isolates
> + * Only the first PFN is checked as the caller isolates
> * COMPACT_CLUSTER_MAX at a time so the second call must
> * not falsely conclude that the block should be skipped.
> */
> - if (!valid_page && pageblock_aligned(low_pfn)) {
> + if (!valid_page && (pageblock_aligned(low_pfn) ||
> + low_pfn == cc->zone->zone_start_pfn)) {
> if (!isolation_suitable(cc, page)) {
> low_pfn = end_pfn;
> folio = NULL;
> @@ -2002,7 +2003,8 @@ static isolate_migrate_t isolate_migratepages(struct compact_control *cc)
> * before making it "skip" so other compaction instances do
> * not scan the same block.
> */
> - if (pageblock_aligned(low_pfn) &&
> + if ((pageblock_aligned(low_pfn) ||
> + low_pfn == cc->zone->zone_start_pfn) &&
> !fast_find_block && !isolation_suitable(cc, page))
> continue;
>