| Message ID | 20230418191313.268131-3-hannes@cmpxchg.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: reliable huge page allocator | expand |
On Tue, Apr 18, 2023 at 03:12:49PM -0400, Johannes Weiner wrote: > During stress testing, two deadlock scenarios were observed: > > 1. One GFP_NOFS allocation was sleeping on too_many_isolated(), and > all CPUs were busy with compactors that appeared to be spinning on > buffer locks. > > Give GFP_NOFS compactors additional isolation headroom, the same > way we do during reclaim, to eliminate this deadlock scenario. > > 2. In a more pernicious scenario, the GFP_NOFS allocation was > busy-spinning in compaction, but seemingly never making > progress. Upon closer inspection, memory was dominated by file > pages, which the fs compactor isn't allowed to touch. The remaining > anon pages didn't have the contiguity to satisfy the request. > > Allow GFP_NOFS allocations to bypass watermarks when compaction > failed at the highest priority. > > While these deadlocks were encountered only in tests with the > subsequent patches (which put a lot more demand on compaction), in > theory these problems already exist in the code today. Fix them now. > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> Definitely needs to be split out. > --- > mm/compaction.c | 15 +++++++++++++-- > mm/page_alloc.c | 10 +++++++++- > 2 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/mm/compaction.c b/mm/compaction.c > index 8238e83385a7..84db84e8fd3a 100644 > --- a/mm/compaction.c > +++ b/mm/compaction.c > @@ -745,8 +745,9 @@ isolate_freepages_range(struct compact_control *cc, > } > > /* Similar to reclaim, but different enough that they don't share logic */ > -static bool too_many_isolated(pg_data_t *pgdat) > +static bool too_many_isolated(struct compact_control *cc) > { > + pg_data_t *pgdat = cc->zone->zone_pgdat; > bool too_many; > > unsigned long active, inactive, isolated; > @@ -758,6 +759,16 @@ static bool too_many_isolated(pg_data_t *pgdat) > isolated = node_page_state(pgdat, NR_ISOLATED_FILE) + > node_page_state(pgdat, NR_ISOLATED_ANON); > > + /* > + * GFP_NOFS callers are allowed to isolate more pages, so they > + * won't get blocked by normal direct-reclaimers, forming a > + * circular deadlock. GFP_NOIO won't get here. > + */ > + if (cc->gfp_mask & __GFP_FS) { > + inactive >>= 3; > + active >>= 3; > + } > + This comment needs to explain why GFP_NOFS gets special treatment explaning that a GFP_NOFS context may not be able to migrate pages and why. As a follow-up, if GFP_NOFS cannot deal with the majority of the migration contexts then it should bail out of compaction entirely. The changelog doesn't say why but maybe SYNC_LIGHT is the issue?
On Fri, Apr 21, 2023 at 01:27:43PM +0100, Mel Gorman wrote: > On Tue, Apr 18, 2023 at 03:12:49PM -0400, Johannes Weiner wrote: > > During stress testing, two deadlock scenarios were observed: > > > > 1. One GFP_NOFS allocation was sleeping on too_many_isolated(), and > > all CPUs were busy with compactors that appeared to be spinning on > > buffer locks. > > > > Give GFP_NOFS compactors additional isolation headroom, the same > > way we do during reclaim, to eliminate this deadlock scenario. > > > > 2. In a more pernicious scenario, the GFP_NOFS allocation was > > busy-spinning in compaction, but seemingly never making > > progress. Upon closer inspection, memory was dominated by file > > pages, which the fs compactor isn't allowed to touch. The remaining > > anon pages didn't have the contiguity to satisfy the request. > > > > Allow GFP_NOFS allocations to bypass watermarks when compaction > > failed at the highest priority. > > > > While these deadlocks were encountered only in tests with the > > subsequent patches (which put a lot more demand on compaction), in > > theory these problems already exist in the code today. Fix them now. > > > > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> > > Definitely needs to be split out. Will do. > > mm/compaction.c | 15 +++++++++++++-- > > mm/page_alloc.c | 10 +++++++++- > > 2 files changed, 22 insertions(+), 3 deletions(-) > > > > diff --git a/mm/compaction.c b/mm/compaction.c > > index 8238e83385a7..84db84e8fd3a 100644 > > --- a/mm/compaction.c > > +++ b/mm/compaction.c > > @@ -745,8 +745,9 @@ isolate_freepages_range(struct compact_control *cc, > > } > > > > /* Similar to reclaim, but different enough that they don't share logic */ > > -static bool too_many_isolated(pg_data_t *pgdat) > > +static bool too_many_isolated(struct compact_control *cc) > > { > > + pg_data_t *pgdat = cc->zone->zone_pgdat; > > bool too_many; > > > > unsigned long active, inactive, isolated; > > @@ -758,6 +759,16 @@ static bool too_many_isolated(pg_data_t *pgdat) > > isolated = node_page_state(pgdat, NR_ISOLATED_FILE) + > > node_page_state(pgdat, NR_ISOLATED_ANON); > > > > + /* > > + * GFP_NOFS callers are allowed to isolate more pages, so they > > + * won't get blocked by normal direct-reclaimers, forming a > > + * circular deadlock. GFP_NOIO won't get here. > > + */ > > + if (cc->gfp_mask & __GFP_FS) { > > + inactive >>= 3; > > + active >>= 3; > > + } > > + > > This comment needs to explain why GFP_NOFS gets special treatment > explaning that a GFP_NOFS context may not be able to migrate pages and > why. Fair point, I'll expand on that. > As a follow-up, if GFP_NOFS cannot deal with the majority of the > migration contexts then it should bail out of compaction entirely. The > changelog doesn't say why but maybe SYNC_LIGHT is the issue? It's this condition in isolate_migratepages_block(): /* * Only allow to migrate anonymous pages in GFP_NOFS context * because those do not depend on fs locks. */ if (!(cc->gfp_mask & __GFP_FS) && mapping) goto isolate_fail_put; In terms of bailing even earlier: We do have per-zone file and anon counts that could be consulted. However, the real problem is interleaving of anon and file. Even if only 10% of the zone is anon, it could still be worth trying to compact if they're relatively contiguous. OTOH 50% anon could be uncompactable if every block also contains at least one file. We don't know until we actually scan. I'm hesitant to give allocations premature access to the last reserves. What might work is for NOFS contexts to test if anon is low up front and shortcutting directly to the highest priority (SYNC_FULL). One good faith scan attempt at least before touching the reserves.
diff --git a/mm/compaction.c b/mm/compaction.c index 8238e83385a7..84db84e8fd3a 100644 --- a/mm/compaction.c +++ b/mm/compaction.c @@ -745,8 +745,9 @@ isolate_freepages_range(struct compact_control *cc, } /* Similar to reclaim, but different enough that they don't share logic */ -static bool too_many_isolated(pg_data_t *pgdat) +static bool too_many_isolated(struct compact_control *cc) { + pg_data_t *pgdat = cc->zone->zone_pgdat; bool too_many; unsigned long active, inactive, isolated; @@ -758,6 +759,16 @@ static bool too_many_isolated(pg_data_t *pgdat) isolated = node_page_state(pgdat, NR_ISOLATED_FILE) + node_page_state(pgdat, NR_ISOLATED_ANON); + /* + * GFP_NOFS callers are allowed to isolate more pages, so they + * won't get blocked by normal direct-reclaimers, forming a + * circular deadlock. GFP_NOIO won't get here. + */ + if (cc->gfp_mask & __GFP_FS) { + inactive >>= 3; + active >>= 3; + } + too_many = isolated > (inactive + active) / 2; if (!too_many) wake_throttle_isolated(pgdat); @@ -806,7 +817,7 @@ isolate_migratepages_block(struct compact_control *cc, unsigned long low_pfn, * list by either parallel reclaimers or compaction. If there are, * delay for some time until fewer pages are isolated */ - while (unlikely(too_many_isolated(pgdat))) { + while (unlikely(too_many_isolated(cc))) { /* stop isolation if there are still pages not migrated */ if (cc->nr_migratepages) return -EAGAIN; diff --git a/mm/page_alloc.c b/mm/page_alloc.c index 3bb3484563ed..ac03571e0532 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -4508,8 +4508,16 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order, prep_new_page(page, order, gfp_mask, alloc_flags); /* Try get a page from the freelist if available */ - if (!page) + if (!page) { + /* + * It's possible that the only migration sources are + * file pages, and the GFP_NOFS stack is holding up + * other compactors. Use reserves to avoid deadlock. + */ + if (prio == MIN_COMPACT_PRIORITY && !(gfp_mask & __GFP_FS)) + alloc_flags |= ALLOC_NO_WATERMARKS; page = get_page_from_freelist(gfp_mask, order, alloc_flags, ac); + } if (page) { struct zone *zone = page_zone(page);
During stress testing, two deadlock scenarios were observed: 1. One GFP_NOFS allocation was sleeping on too_many_isolated(), and all CPUs were busy with compactors that appeared to be spinning on buffer locks. Give GFP_NOFS compactors additional isolation headroom, the same way we do during reclaim, to eliminate this deadlock scenario. 2. In a more pernicious scenario, the GFP_NOFS allocation was busy-spinning in compaction, but seemingly never making progress. Upon closer inspection, memory was dominated by file pages, which the fs compactor isn't allowed to touch. The remaining anon pages didn't have the contiguity to satisfy the request. Allow GFP_NOFS allocations to bypass watermarks when compaction failed at the highest priority. While these deadlocks were encountered only in tests with the subsequent patches (which put a lot more demand on compaction), in theory these problems already exist in the code today. Fix them now. Signed-off-by: Johannes Weiner <hannes@cmpxchg.org> --- mm/compaction.c | 15 +++++++++++++-- mm/page_alloc.c | 10 +++++++++- 2 files changed, 22 insertions(+), 3 deletions(-)