Re: [RFC PATCH 02/26] mm: compaction: avoid GFP_NOFS deadlocks

[Johannes Weiner <hannes@cmpxchg.org>]

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.