Re: [RFC PATCH 2/3] mm, compaction: use MIN_COMPACT_COSTLY_PRIORITY everywhere for costly orders

[Vlastimil Babka]

On 7/31/19 10:30 PM, Mike Kravetz wrote:
> On 7/31/19 5:06 AM, Vlastimil Babka wrote:
>> On 7/24/19 7:50 PM, Mike Kravetz wrote:
>>> For PAGE_ALLOC_COSTLY_ORDER allocations,
>>> MIN_COMPACT_COSTLY_PRIORITY is minimum (highest priority).  Other
>>> places in the compaction code key off of MIN_COMPACT_PRIORITY.
>>> Costly order allocations will never get to MIN_COMPACT_PRIORITY.
>>> Therefore, some conditions will never be met for costly order
>>> allocations.
>>> 
>>> This was observed when hugetlb allocations could stall for
>>> minutes or hours when should_compact_retry() would return true
>>> more often then it should.  Specifically, this was in the case
>>> where compact_result was COMPACT_DEFERRED and
>>> COMPACT_PARTIAL_SKIPPED and no progress was being made.
>> 
>> Hmm, the point of MIN_COMPACT_COSTLY_PRIORITY was that costly 
>> allocations will not reach the priority where compaction becomes
>> too expensive. With your patch, they still don't reach that
>> priority value, but are allowed to be thorough anyway, even sooner.
>> That just seems like a wrong way to fix the problem.
> 
> Thanks Vlastimil, here is why I took the approach I did.

Thanks for the explanation.

> I instrumented some of the long stalls.  Here is one common example: 
> should_compact_retry returned true 5000000 consecutive times.
> However, the variable compaction_retries is zero.  We never get to
> the code that increments the compaction_retries count because
> compaction_made_progress is false and compaction_withdrawn is true.
> As suggested earlier, I noted why compaction_withdrawn is true.  Of
> the 5000000 calls, 4921875 were COMPACT_DEFERRED 78125 were
> COMPACT_PARTIAL_SKIPPED Note that 5000000/64(1 <<
> COMPACT_MAX_DEFER_SHIFT) == 78125
> 
> I then started looking into why COMPACT_DEFERRED and
> COMPACT_PARTIAL_SKIPPED were being set/returned so often. 
> COMPACT_DEFERRED is set/returned in try_to_compact_pages.
> Specifically, if (prio > MIN_COMPACT_PRIORITY &&
> compaction_deferred(zone, order)) {

Ah, so I see it now, this is indeed why you get so many
COMPACT_DEFERRED, as prio is always above MIN_COMPACT_PRIORITY.

> rc = max_t(enum compact_result, COMPACT_DEFERRED, rc); continue; } 
> COMPACT_PARTIAL_SKIPPED is set/returned in __compact_finished.
> Specifically, if (compact_scanners_met(cc)) { /* Let the next
> compaction start anew. */ reset_cached_positions(cc->zone);
> 
> /* ... */
> 
> if (cc->direct_compaction) cc->zone->compact_blockskip_flush = true;
> 
> if (cc->whole_zone) return COMPACT_COMPLETE; else return
> COMPACT_PARTIAL_SKIPPED; }
> 
> In both cases, compact_priority being MIN_COMPACT_COSTLY_PRIORITY and
> not being able to go to MIN_COMPACT_PRIORITY caused the
> 'compaction_withdrawn' result to be set/returned.

Hmm, looks like compaction_withdrawn() is just too blunt a test. It
mixes up results where the reaction should be more reclaim
(COMPACT_SKIPPED), and the results that depend on compaction priority
(the rest), and then we should either increase the priority, or fail.

> I do not know the subtleties of the compaction code, but it seems
> like retrying in this manner does not make sense.

I agree it doesn't, if we can't go for MIN_COMPACT_PRIORITY.

>> If should_compact_retry() returns misleading results for costly
>> allocations, then that should be fixed instead?
>> 
>> Alternatively, you might want to say that hugetlb allocations are
>> not like other random costly allocations, because the admin
>> setting nr_hugepages is prepared to take the cost (I thought that
>> was indicated by the __GFP_RETRY_MAYFAIL flag, but seeing all the
>> other users of it, I'm not sure anymore).
> 
> The example above, resulted in a stall of a little over 5 minutes.
> However, I have seen them last for hours.  Sure, the caller (admin
> for hugetlbfs) knows there may be high costs.  But, I think
> minutes/hours to try and allocate a single huge page is too much.  We
> should fail sooner that that.

Sure. We should eliminate the pointless retries in any case, the
question is whether we allow the MIN_COMPACT_PRIORITY over
MIN_COMPACT_COSTLY_PRIORITY.

>> In that case should_compact_retry() could take __GFP_RETRY_MAYFAIL
>> into account and allow MIN_COMPACT_PRIORITY even for costly
>> allocations.
> 
> I'll put something like this together to test.

Could you try testing the patch below instead? It should hopefully
eliminate the stalls. If it makes hugepage allocation give up too early,
we'll know we have to involve __GFP_RETRY_MAYFAIL in allowing the
MIN_COMPACT_PRIORITY priority. Thanks!

----8<----
diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index 9569e7c786d3..b8bfe8d5d2e9 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -129,11 +129,7 @@ static inline bool compaction_failed(enum compact_result result)
 	return false;
 }
 
-/*
- * Compaction  has backed off for some reason. It might be throttling or
- * lock contention. Retrying is still worthwhile.
- */
-static inline bool compaction_withdrawn(enum compact_result result)
+static inline bool compaction_needs_reclaim(enum compact_result result)
 {
 	/*
 	 * Compaction backed off due to watermark checks for order-0
@@ -142,6 +138,15 @@ static inline bool compaction_withdrawn(enum compact_result result)
 	if (result == COMPACT_SKIPPED)
 		return true;
 
+	return false;
+}
+
+/*
+ * Compaction  has backed off for some reason. It might be throttling or
+ * lock contention. Retrying is still worthwhile.
+ */
+static inline bool compaction_withdrawn(enum compact_result result)
+{
 	/*
 	 * If compaction is deferred for high-order allocations, it is
 	 * because sync compaction recently failed. If this is the case
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 272c6de1bf4e..3dfce1f79112 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3965,6 +3965,11 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	if (compaction_failed(compact_result))
 		goto check_priority;
 
+	if (compaction_needs_reclaim(compact_result)) {
+		ret = compaction_zonelist_suitable(ac, order, alloc_flags);
+		goto out;
+	}
+
 	/*
 	 * make sure the compaction wasn't deferred or didn't bail out early
 	 * due to locks contention before we declare that we should give up.
@@ -3972,8 +3977,7 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * compaction.
 	 */
 	if (compaction_withdrawn(compact_result)) {
-		ret = compaction_zonelist_suitable(ac, order, alloc_flags);
-		goto out;
+		goto check_priority;
 	}
 
 	/*