Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1371405 > unrolled thread

[PATCH 00/11] oom detection rework v5

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/11] oom detection rework v5 Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
    [PATCH 10/11] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 10/11] mm, oom: protect !costly allocations some more Andrew Morton <akpm@linux-foundation.org> - 2016-04-06 02:10 +0200
        Re: [PATCH 10/11] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-04-06 11:30 +0200
      Re: [PATCH 10/11] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 16:50 +0200
    [PATCH 08/11] mm, compaction: Simplify __alloc_pages_direct_compact feedback interface Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 08/11] mm, compaction: Simplify  __alloc_pages_direct_compact feedback interface Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 16:00 +0200
    [PATCH 04/11] mm, compaction: cover all compaction mode in compact_zone Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
    [PATCH 05/11] mm, compaction: distinguish COMPACT_DEFERRED from COMPACT_SKIPPED Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 05/11] mm, compaction: distinguish COMPACT_DEFERRED from  COMPACT_SKIPPED Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 13:10 +0200
        Re: [PATCH 05/11] mm, compaction: distinguish COMPACT_DEFERRED from  COMPACT_SKIPPED Michal Hocko <mhocko@kernel.org> - 2016-04-11 13:30 +0200
    [PATCH 07/11] mm, compaction: Update compaction_result ordering Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 07/11] mm, compaction: Update compaction_result ordering Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 14:20 +0200
    [PATCH 02/11] mm: throttle on IO only when there are too many dirty and writeback pages Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
    [PATCH 01/11] mm, oom: rework oom detection Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
    [PATCH 03/11] mm, compaction: change COMPACT_ constants into enum Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
    [PATCH 06/11] mm, compaction: distinguish between full and partial COMPACT_COMPLETE Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 14:20 +0200
        Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Michal Hocko <mhocko@kernel.org> - 2016-04-11 14:50 +0200
          Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 15:00 +0200
            Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Michal Hocko <mhocko@kernel.org> - 2016-04-11 15:30 +0200
              Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 15:50 +0200
                Re: [PATCH 06/11] mm, compaction: distinguish between full and  partial COMPACT_COMPLETE Michal Hocko <mhocko@kernel.org> - 2016-04-11 15:50 +0200
    [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Andrew Morton <akpm@linux-foundation.org> - 2016-04-06 02:00 +0200
        Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Hugh Dickins <hughd@google.com> - 2016-04-06 03:00 +0200
          Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Michal Hocko <mhocko@kernel.org> - 2016-04-06 11:30 +0200
          Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Andrew Morton <akpm@linux-foundation.org> - 2016-04-06 19:50 +0200
      Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 16:40 +0200
        Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Michal Hocko <mhocko@kernel.org> - 2016-04-11 17:20 +0200
          Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Michal Hocko <mhocko@kernel.org> - 2016-04-11 17:40 +0200
          Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Vlastimil Babka <vbabka@suse.cz> - 2016-04-12 14:00 +0200
            Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Michal Hocko <mhocko@kernel.org> - 2016-04-12 14:30 +0200
      Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Michal Hocko <mhocko@kernel.org> - 2016-04-11 18:00 +0200
        [RFC PATCH] mm: use compaction feedback for thp backoff conditions Michal Hocko <mhocko@kernel.org> - 2016-04-11 18:10 +0200
        Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to  helpers Vlastimil Babka <vbabka@suse.cz> - 2016-04-12 14:00 +0200
    [PATCH 11/11] mm: consider compaction feedback also for costly allocation Michal Hocko <mhocko@kernel.org> - 2016-04-05 13:30 +0200
      Re: [PATCH 11/11] mm: consider compaction feedback also for costly  allocation Michal Hocko <mhocko@kernel.org> - 2016-04-05 14:50 +0200
      Re: [PATCH 11/11] mm: consider compaction feedback also for costly  allocation Vlastimil Babka <vbabka@suse.cz> - 2016-04-11 17:10 +0200
    Re: [PATCH 00/11] oom detection rework v5 Michal Hocko <mhocko@kernel.org> - 2016-04-05 14:50 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1375855 — Re: [PATCH 06/11] mm, compaction: distinguish between full and partial COMPACT_COMPLETE

On Mon 11-04-16 14:53:36, Vlastimil Babka wrote:
> On 04/11/2016 02:46 PM, Michal Hocko wrote:
> >>This assumes that migrate scanner at initial position implies also free
> >>scanner at the initial position. That should be true, because migration
> >>scanner is the first to run. But getting the zone->compact_cached_*_pfn is
> >>racy. Worse, zone->compact_cached_migrate_pfn is array distinguishing sync
> >>and async compaction, so it's possible that async compaction has advanced
> >>both its own migrate scanner cached position, and the shared free scanner
> >>cached position, and then sync compaction starts migrate scanner at
> >>start_pfn, but free scanner has already advanced.
> >
> >OK, I see. The whole thing smelled racy but I thought it wouldn't be
> >such a big deal. Even if we raced then only a marginal part of the zone
> >wouldn't be scanned, right? Or is it possible that free_pfn would appear
> >in the middle of the zone because of the race?
> 
> The racy part is negligible but I didn't realize the sync/async migrate
> scanner part until now. So yeah, free_pfn could have got to middle of zone
> when it was in the async mode. But that also means that the async mode
> recently used up all free pages in the second half of the zone. WRT free
> pages isolation, async mode is not trying less than sync, so it shouldn't be
> a considerable missed opportunity if we don't rescan the it, though.

I am not really sure I understand. The primary intention of this patch
is to distinguish where we have scanned basically whole zones from cases
where a new scan started off previous mark and so it was just unlucky to
see only tiny bit of the zone where we would clearly give up too early.
FWIU this shouldn't be the case if we start scanning from the beginning
of the zone even if we raced on the other end of the zone because the
missed part would be negligible. Is that understanding correct?
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1375911 — Re: [PATCH 06/11] mm, compaction: distinguish between full and partial COMPACT_COMPLETE

On 04/11/2016 03:27 PM, Michal Hocko wrote:
> On Mon 11-04-16 14:53:36, Vlastimil Babka wrote:
>> On 04/11/2016 02:46 PM, Michal Hocko wrote:
>>
>> The racy part is negligible but I didn't realize the sync/async migrate
>> scanner part until now. So yeah, free_pfn could have got to middle of zone
>> when it was in the async mode. But that also means that the async mode
>> recently used up all free pages in the second half of the zone. WRT free
>> pages isolation, async mode is not trying less than sync, so it shouldn't be
>> a considerable missed opportunity if we don't rescan the it, though.
>
> I am not really sure I understand. The primary intention of this patch
> is to distinguish where we have scanned basically whole zones from cases
> where a new scan started off previous mark and so it was just unlucky to
> see only tiny bit of the zone where we would clearly give up too early.
> FWIU this shouldn't be the case if we start scanning from the beginning
> of the zone even if we raced on the other end of the zone because the
> missed part would be negligible. Is that understanding correct?

Yes, it should be less unlucky than seeing a tiny bit of the zone. Just 
wanted to point out that you might still not see the whole zone in one 
compaction attempt. E.g. async compaction is first, advances the free 
scanner and caches its position when it bails out due to being 
contended. Then direct reclaim frees some pages behind the cached 
position. Sync compaction attempts starts migration scanner from 
start_pfn, but picks up the cached free scanner pfn. The result is 
missing some free pages and the scanners meeting somewhat earlier than 
they otherwise would. Probably not critical even for OOM decisions, as 
that's also racy anyway.

[toc] | [prev] | [next] | [standalone]

#1375914 — Re: [PATCH 06/11] mm, compaction: distinguish between full and partial COMPACT_COMPLETE

On Mon 11-04-16 15:42:21, Vlastimil Babka wrote:
> On 04/11/2016 03:27 PM, Michal Hocko wrote:
> >On Mon 11-04-16 14:53:36, Vlastimil Babka wrote:
> >>On 04/11/2016 02:46 PM, Michal Hocko wrote:
> >>
> >>The racy part is negligible but I didn't realize the sync/async migrate
> >>scanner part until now. So yeah, free_pfn could have got to middle of zone
> >>when it was in the async mode. But that also means that the async mode
> >>recently used up all free pages in the second half of the zone. WRT free
> >>pages isolation, async mode is not trying less than sync, so it shouldn't be
> >>a considerable missed opportunity if we don't rescan the it, though.
> >
> >I am not really sure I understand. The primary intention of this patch
> >is to distinguish where we have scanned basically whole zones from cases
> >where a new scan started off previous mark and so it was just unlucky to
> >see only tiny bit of the zone where we would clearly give up too early.
> >FWIU this shouldn't be the case if we start scanning from the beginning
> >of the zone even if we raced on the other end of the zone because the
> >missed part would be negligible. Is that understanding correct?
> 
> Yes, it should be less unlucky than seeing a tiny bit of the zone. Just
> wanted to point out that you might still not see the whole zone in one
> compaction attempt. E.g. async compaction is first, advances the free
> scanner and caches its position when it bails out due to being contended.
> Then direct reclaim frees some pages behind the cached position. Sync
> compaction attempts starts migration scanner from start_pfn, but picks up
> the cached free scanner pfn. The result is missing some free pages and the
> scanners meeting somewhat earlier than they otherwise would. Probably not
> critical even for OOM decisions, as that's also racy anyway.

OK, I see now. I agree this shouldn't be critical and thanks for the
clarification.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1371420 — [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

From: Michal Hocko <mhocko@suse.com>

Compaction can provide a wild variation of feedback to the caller. Many
of them are implementation specific and the caller of the compaction
(especially the page allocator) shouldn't be bound to specifics of the
current implementation.

This patch abstracts the feedback into three basic types:
	- compaction_made_progress - compaction was active and made some
	  progress.
	- compaction_failed - compaction failed and further attempts to
	  invoke it would most probably fail and therefore it is not
	  worth retrying
	- compaction_withdrawn - compaction wasn't invoked for an
          implementation specific reasons. In the current implementation
          it means that the compaction was deferred, contended or the
          page scanners met too early without any progress. Retrying is
          still worthwhile.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/compaction.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++
 mm/page_alloc.c            | 25 ++++------------
 2 files changed, 80 insertions(+), 19 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index a7b9091ff349..512db9c3f0ed 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -78,6 +78,70 @@ extern void compaction_defer_reset(struct zone *zone, int order,
 				bool alloc_success);
 extern bool compaction_restarting(struct zone *zone, int order);
 
+/* Compaction has made some progress and retrying makes sense */
+static inline bool compaction_made_progress(enum compact_result result)
+{
+	/*
+	 * Even though this might sound confusing this in fact tells us
+	 * that the compaction successfully isolated and migrated some
+	 * pageblocks.
+	 */
+	if (result == COMPACT_PARTIAL)
+		return true;
+
+	return false;
+}
+
+/* Compaction has failed and it doesn't make much sense to keep retrying. */
+static inline bool compaction_failed(enum compact_result result)
+{
+	/* All zones where scanned completely and still not result. */
+	if (result == COMPACT_COMPLETE)
+		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)
+{
+	/*
+	 * Compaction backed off due to watermark checks for order-0
+	 * so the regular reclaim has to try harder and reclaim something.
+	 */
+	if (result == COMPACT_SKIPPED)
+		return true;
+
+	/*
+	 * If compaction is deferred for high-order allocations, it is
+	 * because sync compaction recently failed. If this is the case
+	 * and the caller requested a THP allocation, we do not want
+	 * to heavily disrupt the system, so we fail the allocation
+	 * instead of entering direct reclaim.
+	 */
+	if (result == COMPACT_DEFERRED)
+		return true;
+
+	/*
+	 * If compaction in async mode encounters contention or blocks higher
+	 * priority task we back off early rather than cause stalls.
+	 */
+	if (result == COMPACT_CONTENDED)
+		return true;
+
+	/*
+	 * Page scanners have met but we haven't scanned full zones so this
+	 * is a back off in fact.
+	 */
+	if (result == COMPACT_PARTIAL_SKIPPED)
+		return true;
+
+	return false;
+}
+
 extern int kcompactd_run(int nid);
 extern void kcompactd_stop(int nid);
 extern void wakeup_kcompactd(pg_data_t *pgdat, int order, int classzone_idx);
@@ -114,6 +178,16 @@ static inline bool compaction_deferred(struct zone *zone, int order)
 	return true;
 }
 
+static inline bool compaction_made_progress(enum compact_result result)
+{
+	return false;
+}
+
+static inline bool compaction_withdrawn(enum compact_result result)
+{
+	return true;
+}
+
 static inline int kcompactd_run(int nid)
 {
 	return 0;
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c37e6d1ad643..c05de84c8157 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3362,25 +3362,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
-	/* Checks for THP-specific high-order allocations */
-	if (is_thp_gfp_mask(gfp_mask)) {
-		/*
-		 * If compaction is deferred for high-order allocations, it is
-		 * because sync compaction recently failed. If this is the case
-		 * and the caller requested a THP allocation, we do not want
-		 * to heavily disrupt the system, so we fail the allocation
-		 * instead of entering direct reclaim.
-		 */
-		if (compact_result == COMPACT_DEFERRED)
-			goto nopage;
-
-		/*
-		 * Compaction is contended so rather back off than cause
-		 * excessive stalls.
-		 */
-		if(compact_result == COMPACT_CONTENDED)
-			goto nopage;
-	}
+	/*
+	 * Checks for THP-specific high-order allocations and back off
+	 * if the the compaction backed off
+	 */
+	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
+		goto nopage;
 
 	/*
 	 * It can become very expensive to allocate transparent hugepages at
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1372082 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Tue,  5 Apr 2016 13:25:31 +0200 Michal Hocko <mhocko@kernel.org> wrote:

> From: Michal Hocko <mhocko@suse.com>
> 
> Compaction can provide a wild variation of feedback to the caller. Many
> of them are implementation specific and the caller of the compaction
> (especially the page allocator) shouldn't be bound to specifics of the
> current implementation.
> 
> This patch abstracts the feedback into three basic types:
> 	- compaction_made_progress - compaction was active and made some
> 	  progress.
> 	- compaction_failed - compaction failed and further attempts to
> 	  invoke it would most probably fail and therefore it is not
> 	  worth retrying
> 	- compaction_withdrawn - compaction wasn't invoked for an
>           implementation specific reasons. In the current implementation
>           it means that the compaction was deferred, contended or the
>           page scanners met too early without any progress. Retrying is
>           still worthwhile.
> 
> ...
>
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3362,25 +3362,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (page)
>  		goto got_pg;
>  
> -	/* Checks for THP-specific high-order allocations */
> -	if (is_thp_gfp_mask(gfp_mask)) {
> -		/*
> -		 * If compaction is deferred for high-order allocations, it is
> -		 * because sync compaction recently failed. If this is the case
> -		 * and the caller requested a THP allocation, we do not want
> -		 * to heavily disrupt the system, so we fail the allocation
> -		 * instead of entering direct reclaim.
> -		 */
> -		if (compact_result == COMPACT_DEFERRED)
> -			goto nopage;
> -
> -		/*
> -		 * Compaction is contended so rather back off than cause
> -		 * excessive stalls.
> -		 */
> -		if(compact_result == COMPACT_CONTENDED)
> -			goto nopage;
> -	}
> +	/*
> +	 * Checks for THP-specific high-order allocations and back off
> +	 * if the the compaction backed off
> +	 */
> +	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> +		goto nopage;

This change smashed into Hugh's "huge tmpfs: shmem_huge_gfpmask and
shmem_recovery_gfpmask".

I ended up doing this:

	/* Checks for THP-specific high-order allocations */
	if (!is_thp_allocation(gfp_mask, order))
		migration_mode = MIGRATE_SYNC_LIGHT;

	/*
	 * Checks for THP-specific high-order allocations and back off
	 * if the the compaction backed off
	 */
	if (is_thp_allocation(gfp_mask) && compaction_withdrawn(compact_result))
		goto nopage;

[toc] | [prev] | [next] | [standalone]

#1372112 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Tue, 5 Apr 2016, Andrew Morton wrote:
> On Tue,  5 Apr 2016 13:25:31 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > -	if (is_thp_gfp_mask(gfp_mask)) {
> > -		/*
> > -		 * If compaction is deferred for high-order allocations, it is
> > -		 * because sync compaction recently failed. If this is the case
> > -		 * and the caller requested a THP allocation, we do not want
> > -		 * to heavily disrupt the system, so we fail the allocation
> > -		 * instead of entering direct reclaim.
> > -		 */
> > -		if (compact_result == COMPACT_DEFERRED)
> > -			goto nopage;
> > -
> > -		/*
> > -		 * Compaction is contended so rather back off than cause
> > -		 * excessive stalls.
> > -		 */
> > -		if(compact_result == COMPACT_CONTENDED)
> > -			goto nopage;
> > -	}
> > +	/*
> > +	 * Checks for THP-specific high-order allocations and back off
> > +	 * if the the compaction backed off
> > +	 */
> > +	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> > +		goto nopage;
> 
> This change smashed into Hugh's "huge tmpfs: shmem_huge_gfpmask and
> shmem_recovery_gfpmask".
> 
> I ended up doing this:
> 
> 	/* Checks for THP-specific high-order allocations */
> 	if (!is_thp_allocation(gfp_mask, order))
> 		migration_mode = MIGRATE_SYNC_LIGHT;
> 
> 	/*
> 	 * Checks for THP-specific high-order allocations and back off
> 	 * if the the compaction backed off
> 	 */
> 	if (is_thp_allocation(gfp_mask) && compaction_withdrawn(compact_result))
> 		goto nopage;

You'll already have found that is_thp_allocation() needs the order too.
But then you had to drop a hunk out of his 10/11 also to fit with mine.

What you've done may be just right, but I haven't had time to digest
Michal's changes yet (and not yet seen what happens to the PF_KTHREAD
distinction), so I think it will probably end up better if you take
his exactly as he tested and posted them, and drop my 30/31 and 31/31
for now - I can resubmit them (or maybe drop 30 altogether) after I've
pondered and tested a little on top of Michal's.

Huge tmpfs got along fine for many months without 30/31 and 31/31: 30
is just for experimentation, and 31 to reduce the compaction stalls we
saw under some loads.  Maybe I'll find that Michal's rework has changed
the balance there anyway, and something else or nothing at all needed.

(The gfp_mask stuff was very confusing, and it's painful for me, how
~__GFP_KSWAPD_RECLAIM gets used as a secret password to say "THP" and
how to angle compaction - or maybe it's all more straightforward now.)

Many thanks for giving us both this quick exposure!

Hugh

[toc] | [prev] | [next] | [standalone]

#1372322 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Tue 05-04-16 17:55:39, Hugh Dickins wrote:
> On Tue, 5 Apr 2016, Andrew Morton wrote:
> > On Tue,  5 Apr 2016 13:25:31 +0200 Michal Hocko <mhocko@kernel.org> wrote:
> > > -	if (is_thp_gfp_mask(gfp_mask)) {
> > > -		/*
> > > -		 * If compaction is deferred for high-order allocations, it is
> > > -		 * because sync compaction recently failed. If this is the case
> > > -		 * and the caller requested a THP allocation, we do not want
> > > -		 * to heavily disrupt the system, so we fail the allocation
> > > -		 * instead of entering direct reclaim.
> > > -		 */
> > > -		if (compact_result == COMPACT_DEFERRED)
> > > -			goto nopage;
> > > -
> > > -		/*
> > > -		 * Compaction is contended so rather back off than cause
> > > -		 * excessive stalls.
> > > -		 */
> > > -		if(compact_result == COMPACT_CONTENDED)
> > > -			goto nopage;
> > > -	}
> > > +	/*
> > > +	 * Checks for THP-specific high-order allocations and back off
> > > +	 * if the the compaction backed off
> > > +	 */
> > > +	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> > > +		goto nopage;
> > 
> > This change smashed into Hugh's "huge tmpfs: shmem_huge_gfpmask and
> > shmem_recovery_gfpmask".
> > 
> > I ended up doing this:
> > 
> > 	/* Checks for THP-specific high-order allocations */
> > 	if (!is_thp_allocation(gfp_mask, order))
> > 		migration_mode = MIGRATE_SYNC_LIGHT;
> > 
> > 	/*
> > 	 * Checks for THP-specific high-order allocations and back off
> > 	 * if the the compaction backed off
> > 	 */
> > 	if (is_thp_allocation(gfp_mask) && compaction_withdrawn(compact_result))
> > 		goto nopage;
> 
> You'll already have found that is_thp_allocation() needs the order too.
> But then you had to drop a hunk out of his 10/11 also to fit with mine.
> 
> What you've done may be just right, but I haven't had time to digest
> Michal's changes yet (and not yet seen what happens to the PF_KTHREAD
> distinction), so I think it will probably end up better if you take
> his exactly as he tested and posted them, and drop my 30/31 and 31/31
> for now

I have only briefly checked your patch30 but I guess the above is
not really necessary. If the request is __GFP_REPEAT (I haven't checked
whether that is the case for shmem) then we promote to MIGRATE_SYNC_LIGHT
as soon as we cannot move on with ASYNC. For !__GFP_REPEAT I did
+       if (is_thp_gfp_mask(gfp_mask) && !(current->flags & PF_KTHREAD))
+               migration_mode = MIGRATE_ASYNC;
+       else
+               migration_mode = MIGRATE_SYNC_LIGHT;
        page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
                                            ac, migration_mode,
                                            &compact_result);

so you will end up doing SYNC_LIGHT for !is_thp_allocation as well

> - I can resubmit them (or maybe drop 30 altogether) after I've
> pondered and tested a little on top of Michal's.

I guess this would be safer. If it turns out that we need some special
handling I would prefer if that could be done in should_compact_retry.
 
> Huge tmpfs got along fine for many months without 30/31 and 31/31: 30
> is just for experimentation, and 31 to reduce the compaction stalls we
> saw under some loads.  Maybe I'll find that Michal's rework has changed
> the balance there anyway, and something else or nothing at all needed.
> 
> (The gfp_mask stuff was very confusing, and it's painful for me, how
> ~__GFP_KSWAPD_RECLAIM gets used as a secret password to say "THP" and
> how to angle compaction - or maybe it's all more straightforward now.)
> 
> Many thanks for giving us both this quick exposure!

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1372704 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Tue, 5 Apr 2016 17:55:39 -0700 (PDT) Hugh Dickins <hughd@google.com> wrote:

> > I ended up doing this:
> > 
> > 	/* Checks for THP-specific high-order allocations */
> > 	if (!is_thp_allocation(gfp_mask, order))
> > 		migration_mode = MIGRATE_SYNC_LIGHT;
> > 
> > 	/*
> > 	 * Checks for THP-specific high-order allocations and back off
> > 	 * if the the compaction backed off
> > 	 */
> > 	if (is_thp_allocation(gfp_mask) && compaction_withdrawn(compact_result))
> > 		goto nopage;
> 
> You'll already have found that is_thp_allocation() needs the order too.
> But then you had to drop a hunk out of his 10/11 also to fit with mine.
> 
> What you've done may be just right, but I haven't had time to digest
> Michal's changes yet (and not yet seen what happens to the PF_KTHREAD
> distinction), so I think it will probably end up better if you take
> his exactly as he tested and posted them, and drop my 30/31 and 31/31
> for now - I can resubmit them (or maybe drop 30 altogether) after I've
> pondered and tested a little on top of Michal's.
> 
> Huge tmpfs got along fine for many months without 30/31 and 31/31: 30
> is just for experimentation, and 31 to reduce the compaction stalls we
> saw under some loads.  Maybe I'll find that Michal's rework has changed
> the balance there anyway, and something else or nothing at all needed.
> 
> (The gfp_mask stuff was very confusing, and it's painful for me, how
> ~__GFP_KSWAPD_RECLAIM gets used as a secret password to say "THP" and
> how to angle compaction - or maybe it's all more straightforward now.)

OK, thanks.  I dropped
huge-tmpfs-shmem_huge_gfpmask-and-shmem_recovery_gfpmask.patch and
huge-tmpfs-no-kswapd-by-default-on-sync-allocations.patch and restored
Michal's patches.

> Many thanks for giving us both this quick exposure!

I'll push all this into -next later today.

[toc] | [prev] | [next] | [standalone]

#1376025 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On 04/05/2016 01:25 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> Compaction can provide a wild variation of feedback to the caller. Many
> of them are implementation specific and the caller of the compaction
> (especially the page allocator) shouldn't be bound to specifics of the
> current implementation.
>
> This patch abstracts the feedback into three basic types:
> 	- compaction_made_progress - compaction was active and made some
> 	  progress.
> 	- compaction_failed - compaction failed and further attempts to
> 	  invoke it would most probably fail and therefore it is not
> 	  worth retrying
> 	- compaction_withdrawn - compaction wasn't invoked for an
>            implementation specific reasons. In the current implementation
>            it means that the compaction was deferred, contended or the
>            page scanners met too early without any progress. Retrying is
>            still worthwhile.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>   include/linux/compaction.h | 74 ++++++++++++++++++++++++++++++++++++++++++++++
>   mm/page_alloc.c            | 25 ++++------------
>   2 files changed, 80 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index a7b9091ff349..512db9c3f0ed 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -78,6 +78,70 @@ extern void compaction_defer_reset(struct zone *zone, int order,
>   				bool alloc_success);
>   extern bool compaction_restarting(struct zone *zone, int order);
>
> +/* Compaction has made some progress and retrying makes sense */
> +static inline bool compaction_made_progress(enum compact_result result)
> +{
> +	/*
> +	 * Even though this might sound confusing this in fact tells us
> +	 * that the compaction successfully isolated and migrated some
> +	 * pageblocks.
> +	 */
> +	if (result == COMPACT_PARTIAL)
> +		return true;
> +
> +	return false;
> +}
> +
> +/* Compaction has failed and it doesn't make much sense to keep retrying. */
> +static inline bool compaction_failed(enum compact_result result)
> +{
> +	/* All zones where scanned completely and still not result. */

Hmm given that try_to_compact_pages() uses a max() on results, then in 
fact it takes only one zone to get this. Others could have been also 
SKIPPED or DEFERRED. Is that what you want?

> +	if (result == COMPACT_COMPLETE)
> +		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)
> +{
> +	/*
> +	 * Compaction backed off due to watermark checks for order-0
> +	 * so the regular reclaim has to try harder and reclaim something.
> +	 */
> +	if (result == COMPACT_SKIPPED)
> +		return true;
> +
> +	/*
> +	 * If compaction is deferred for high-order allocations, it is
> +	 * because sync compaction recently failed. If this is the case
> +	 * and the caller requested a THP allocation, we do not want
> +	 * to heavily disrupt the system, so we fail the allocation
> +	 * instead of entering direct reclaim.
> +	 */
> +	if (result == COMPACT_DEFERRED)
> +		return true;
> +
> +	/*
> +	 * If compaction in async mode encounters contention or blocks higher
> +	 * priority task we back off early rather than cause stalls.
> +	 */
> +	if (result == COMPACT_CONTENDED)
> +		return true;
> +
> +	/*
> +	 * Page scanners have met but we haven't scanned full zones so this
> +	 * is a back off in fact.
> +	 */
> +	if (result == COMPACT_PARTIAL_SKIPPED)
> +		return true;
> +
> +	return false;
> +}
> +

[...]

> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index c37e6d1ad643..c05de84c8157 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3362,25 +3362,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>   	if (page)
>   		goto got_pg;
>
> -	/* Checks for THP-specific high-order allocations */
> -	if (is_thp_gfp_mask(gfp_mask)) {
> -		/*
> -		 * If compaction is deferred for high-order allocations, it is
> -		 * because sync compaction recently failed. If this is the case
> -		 * and the caller requested a THP allocation, we do not want
> -		 * to heavily disrupt the system, so we fail the allocation
> -		 * instead of entering direct reclaim.
> -		 */
> -		if (compact_result == COMPACT_DEFERRED)
> -			goto nopage;
> -
> -		/*
> -		 * Compaction is contended so rather back off than cause
> -		 * excessive stalls.
> -		 */
> -		if(compact_result == COMPACT_CONTENDED)
> -			goto nopage;
> -	}
> +	/*
> +	 * Checks for THP-specific high-order allocations and back off
> +	 * if the the compaction backed off
> +	 */
> +	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> +		goto nopage;

The change of semantics for THP is not trivial here and should at least 
be discussed in changelog. CONTENDED and DEFERRED is only subset of 
compaction_withdrawn() as seen above. Why is it useful to back off due 
to COMPACT_PARTIAL_SKIPPED (we were just unlucky in our starting 
position), but not due to COMPACT_COMPLETE (we have seen the whole zone 
but failed anyway)? Why back off due to COMPACT_SKIPPED (not enough 
order-0 pages) without trying reclaim at least once, and then another 
async compaction, like before?

>
>   	/*
>   	 * It can become very expensive to allocate transparent hugepages at
>

[toc] | [prev] | [next] | [standalone]

#1376066 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Mon 11-04-16 16:39:21, Vlastimil Babka wrote:
> On 04/05/2016 01:25 PM, Michal Hocko wrote:
[...]
> >+/* Compaction has failed and it doesn't make much sense to keep retrying. */
> >+static inline bool compaction_failed(enum compact_result result)
> >+{
> >+	/* All zones where scanned completely and still not result. */
> 
> Hmm given that try_to_compact_pages() uses a max() on results, then in fact
> it takes only one zone to get this. Others could have been also SKIPPED or
> DEFERRED. Is that what you want?

In short I didn't find any better way and still guarantee a some
guarantee of convergence. COMPACT_COMPLETE means that at least one zone
was completely scanned and led to no result. That zone would be
compact_suitable by definition. If I made DEFERRED or SKIPPED more
priorite (aka higher in the enum) then I could easily end up in a state
where all zones would return COMPACT_COMPLETE and few remaining would
just alternate returning their DEFFERED resp. SKIPPED. So while this
might sound like giving up too early I couldn't come up with anything
more specific that would lead to reliable results.

I am open to any suggestions of course.

[...]
> >--- a/mm/page_alloc.c
> >+++ b/mm/page_alloc.c
> >@@ -3362,25 +3362,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	if (page)
> >  		goto got_pg;
> >
> >-	/* Checks for THP-specific high-order allocations */
> >-	if (is_thp_gfp_mask(gfp_mask)) {
> >-		/*
> >-		 * If compaction is deferred for high-order allocations, it is
> >-		 * because sync compaction recently failed. If this is the case
> >-		 * and the caller requested a THP allocation, we do not want
> >-		 * to heavily disrupt the system, so we fail the allocation
> >-		 * instead of entering direct reclaim.
> >-		 */
> >-		if (compact_result == COMPACT_DEFERRED)
> >-			goto nopage;
> >-
> >-		/*
> >-		 * Compaction is contended so rather back off than cause
> >-		 * excessive stalls.
> >-		 */
> >-		if(compact_result == COMPACT_CONTENDED)
> >-			goto nopage;
> >-	}
> >+	/*
> >+	 * Checks for THP-specific high-order allocations and back off
> >+	 * if the the compaction backed off
> >+	 */
> >+	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> >+		goto nopage;
> 
> The change of semantics for THP is not trivial here and should at least be
> discussed in changelog. CONTENDED and DEFERRED is only subset of
> compaction_withdrawn() as seen above.

True. My main motivation was to get rid of the compaction specific code
from the allocator path as much as possible. I can drop the above hunk
of course but I think we should get rid of these checks and make the
code simpler. To be honest I am not even sure those changes are really
measurable.

> Why is it useful to back off due to
> COMPACT_PARTIAL_SKIPPED (we were just unlucky in our starting position), but
> not due to COMPACT_COMPLETE (we have seen the whole zone but failed anyway)?

OK, that is a good remark. I could change that to:
	if (is_thp_gfp_mask(gfp_mask) &&
		(compaction_withdrawn(compact_result) || compaction_failed(compact_result))

> Why back off due to COMPACT_SKIPPED (not enough order-0 pages) without
> trying reclaim at least once, and then another async compaction, like
> before?

The idea was that COMPACT_SKIPPED wouldn't change after a single reclaim
round most of the time because a zone would have to get above
low_wmark + 1<<9 pages.  So the only situation where it would matter would be if
we had some order-9 pages available hidden by the min wmark and we would
reclaim enough to get above the above gap. I am not sure this is what we
really want in the first place. Increase the reclaim stalls when we are
getting under memory pressure.

Thanks!
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376092 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Mon 11-04-16 17:14:10, Michal Hocko wrote:
> On Mon 11-04-16 16:39:21, Vlastimil Babka wrote:
> > On 04/05/2016 01:25 PM, Michal Hocko wrote:
[...]
> > >+	/*
> > >+	 * Checks for THP-specific high-order allocations and back off
> > >+	 * if the the compaction backed off
> > >+	 */
> > >+	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
> > >+		goto nopage;
> > 
> > The change of semantics for THP is not trivial here and should at least be
> > discussed in changelog. CONTENDED and DEFERRED is only subset of
> > compaction_withdrawn() as seen above.
> 
> True. My main motivation was to get rid of the compaction specific code
> from the allocator path as much as possible. I can drop the above hunk

I was thinking about this some more and will drop the hunk. I would
rather have this patch without side effects as much as possible. A
follow up patch can get rid of the specific checks and use a simpler
ones. I will post -fix patch to Andrew.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376731 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On 04/11/2016 05:14 PM, Michal Hocko wrote:
> On Mon 11-04-16 16:39:21, Vlastimil Babka wrote:
>> On 04/05/2016 01:25 PM, Michal Hocko wrote:
> [...]
>>> +/* Compaction has failed and it doesn't make much sense to keep retrying. */
>>> +static inline bool compaction_failed(enum compact_result result)
>>> +{
>>> +	/* All zones where scanned completely and still not result. */
>>
>> Hmm given that try_to_compact_pages() uses a max() on results, then in fact
>> it takes only one zone to get this. Others could have been also SKIPPED or
>> DEFERRED. Is that what you want?
>
> In short I didn't find any better way and still guarantee a some
> guarantee of convergence. COMPACT_COMPLETE means that at least one zone
> was completely scanned and led to no result. That zone would be
> compact_suitable by definition. If I made DEFERRED or SKIPPED more
> priorite (aka higher in the enum) then I could easily end up in a state
> where all zones would return COMPACT_COMPLETE and few remaining would
> just alternate returning their DEFFERED resp. SKIPPED. So while this
> might sound like giving up too early I couldn't come up with anything
> more specific that would lead to reliable results.
>
> I am open to any suggestions of course.

I guess you would have to track each zone separately and make sure 
you've seen COMPACT_COMPLETE in all of them, although not necessary 
during the same zonelist attempt. But then do the same for reclaim, as 
you would also have to match COMPAT_SKIPPED and inability of reclaim... 
and that gets uglier and uglier, and also against the move to node-based 
reclaim...

So there's a danger that you'll see COMPACT_COMPLETE on a small ZONE_DMA 
early on, before the larger zones even stop being deferred, but I don't 
see an easy solution.

[toc] | [prev] | [next] | [standalone]

#1376749 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On Tue 12-04-16 13:53:47, Vlastimil Babka wrote:
> On 04/11/2016 05:14 PM, Michal Hocko wrote:
> >On Mon 11-04-16 16:39:21, Vlastimil Babka wrote:
> >>On 04/05/2016 01:25 PM, Michal Hocko wrote:
> >[...]
> >>>+/* Compaction has failed and it doesn't make much sense to keep retrying. */
> >>>+static inline bool compaction_failed(enum compact_result result)
> >>>+{
> >>>+	/* All zones where scanned completely and still not result. */
> >>
> >>Hmm given that try_to_compact_pages() uses a max() on results, then in fact
> >>it takes only one zone to get this. Others could have been also SKIPPED or
> >>DEFERRED. Is that what you want?
> >
> >In short I didn't find any better way and still guarantee a some
> >guarantee of convergence. COMPACT_COMPLETE means that at least one zone
> >was completely scanned and led to no result. That zone would be
> >compact_suitable by definition. If I made DEFERRED or SKIPPED more
> >priorite (aka higher in the enum) then I could easily end up in a state
> >where all zones would return COMPACT_COMPLETE and few remaining would
> >just alternate returning their DEFFERED resp. SKIPPED. So while this
> >might sound like giving up too early I couldn't come up with anything
> >more specific that would lead to reliable results.
> >
> >I am open to any suggestions of course.
> 
> I guess you would have to track each zone separately and make sure you've
> seen COMPACT_COMPLETE in all of them, although not necessary during the same
> zonelist attempt. But then do the same for reclaim, as you would also have
> to match COMPAT_SKIPPED and inability of reclaim... and that gets uglier and
> uglier, and also against the move to node-based reclaim...

I think we want to get rid some of these states long term. Or at least
do not defer or skip for small orders that really matter and are nofail
in fact. But I cannot tell I would understand the defer logic enought to
do it right now.

> So there's a danger that you'll see COMPACT_COMPLETE on a small ZONE_DMA
> early on, before the larger zones even stop being deferred, but I don't see
> an easy solution.

ZONE_DMA should back off most of the time due to watermark checks. But
it is true that we might a small zone which is not protected by lowmem
reserves.

I certainly see a lot of room for improving the compaction and this
rework looks like a good motivation.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376125 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

Hi Andrew,
Vlastimil has pointed out[1] that using compaction_withdrawn() for THP
allocations has some non-trivial consequences. While I still think that
the check is OK it is true we shouldn't sneak in a potential behavior
change into something that basically provides an API. So can you fold
the following partial revert into the original patch please?

[1] http://lkml.kernel.org/r/570BB719.2030007@suse.cz

---
From 71ddeee4238e33d67ef07883e73f946a7cc40e73 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 11 Apr 2016 17:38:22 +0200
Subject: [PATCH] ction-abstract-compaction-feedback-to-helpers-fix

Preserve the original thp back off checks to not introduce any
functional changes as per Vlastimil.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 25 +++++++++++++++++++------
 1 file changed, 19 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index c05de84c8157..c37e6d1ad643 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3362,12 +3362,25 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
-	/*
-	 * Checks for THP-specific high-order allocations and back off
-	 * if the the compaction backed off
-	 */
-	if (is_thp_gfp_mask(gfp_mask) && compaction_withdrawn(compact_result))
-		goto nopage;
+	/* Checks for THP-specific high-order allocations */
+	if (is_thp_gfp_mask(gfp_mask)) {
+		/*
+		 * If compaction is deferred for high-order allocations, it is
+		 * because sync compaction recently failed. If this is the case
+		 * and the caller requested a THP allocation, we do not want
+		 * to heavily disrupt the system, so we fail the allocation
+		 * instead of entering direct reclaim.
+		 */
+		if (compact_result == COMPACT_DEFERRED)
+			goto nopage;
+
+		/*
+		 * Compaction is contended so rather back off than cause
+		 * excessive stalls.
+		 */
+		if(compact_result == COMPACT_CONTENDED)
+			goto nopage;
+	}
 
 	/*
 	 * It can become very expensive to allocate transparent hugepages at
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376143 — [RFC PATCH] mm: use compaction feedback for thp backoff conditions

On Mon 11-04-16 17:40:36, Michal Hocko wrote:
> Hi Andrew,
> Vlastimil has pointed out[1] that using compaction_withdrawn() for THP
> allocations has some non-trivial consequences. While I still think that
> the check is OK it is true we shouldn't sneak in a potential behavior
> change into something that basically provides an API. So can you fold
> the following partial revert into the original patch please?
> 
> [1] http://lkml.kernel.org/r/570BB719.2030007@suse.cz

This would be an RFC on top.
---
From 6cfed80ad41f3f1506930b9a3254fe135bf90d4c Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 11 Apr 2016 17:51:28 +0200
Subject: [PATCH] mm: use compaction feedback for thp backoff conditions

THP requests skip the direct reclaim if the compaction is either
deferred or contended to reduce stalls which wouldn't help the
allocation success anyway. These checks are ignoring other potential
feedback modes which we have available now.

It clearly doesn't make much sense to go and reclaim few pages if the
previous compaction has failed.

We can also simplify the check by using compaction_withdrawn which
checks for both COMPACT_CONTENDED and COMPACT_DEFERRED. This check
is however covering more reasons why the compaction was withdrawn.
None of them should be a problem for the THP case though.

It is safe to back of if we see COMPACT_SKIPPED because that means
that compaction_suitable failed and a single round of the reclaim is
unlikely to make any difference here. We would have to be close to
the low watermark to reclaim enough and even then there is no guarantee
that the compaction would make any progress while the direct reclaim
would have caused the stall.

COMPACT_PARTIAL_SKIPPED is slightly different because that means that we
have only seen a part of the zone so a retry would make some sense. But
it would be a compaction retry not a reclaim retry to perform. We are
not doing that and that might indeed lead to situations where THP fails
but this should happen only rarely and it would be really hard to
measure.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 27 ++++++++-------------------
 1 file changed, 8 insertions(+), 19 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 5eae9e0555ed..6d1da0ceaf1e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3420,25 +3420,14 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (order && compaction_made_progress(compact_result))
 		compaction_retries++;
 
-	/* Checks for THP-specific high-order allocations */
-	if (is_thp_gfp_mask(gfp_mask)) {
-		/*
-		 * If compaction is deferred for high-order allocations, it is
-		 * because sync compaction recently failed. If this is the case
-		 * and the caller requested a THP allocation, we do not want
-		 * to heavily disrupt the system, so we fail the allocation
-		 * instead of entering direct reclaim.
-		 */
-		if (compact_result == COMPACT_DEFERRED)
-			goto nopage;
-
-		/*
-		 * Compaction is contended so rather back off than cause
-		 * excessive stalls.
-		 */
-		if(compact_result == COMPACT_CONTENDED)
-			goto nopage;
-	}
+	/*
+	 * Checks for THP-specific high-order allocations and back off
+	 * if the the compaction backed off or failed
+	 */
+	if (is_thp_gfp_mask(gfp_mask) &&
+			(compaction_withdrawn(compact_result) ||
+			 compaction_failed(compact_result)))
+		goto nopage;
 
 	/* Try direct reclaim and then allocating */
 	page = __alloc_pages_direct_reclaim(gfp_mask, order, alloc_flags, ac,
-- 
2.8.0.rc3

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376730 — Re: [PATCH 09/11] mm, compaction: Abstract compaction feedback to helpers

On 04/11/2016 05:40 PM, Michal Hocko wrote:
> Hi Andrew,
> Vlastimil has pointed out[1] that using compaction_withdrawn() for THP
> allocations has some non-trivial consequences. While I still think that
> the check is OK it is true we shouldn't sneak in a potential behavior
> change into something that basically provides an API. So can you fold
> the following partial revert into the original patch please?
>
> [1] http://lkml.kernel.org/r/570BB719.2030007@suse.cz
>
> ---
>  From 71ddeee4238e33d67ef07883e73f946a7cc40e73 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 11 Apr 2016 17:38:22 +0200
> Subject: [PATCH] ction-abstract-compaction-feedback-to-helpers-fix
>
> Preserve the original thp back off checks to not introduce any
> functional changes as per Vlastimil.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Ack, thanks.

[toc] | [prev] | [next] | [standalone]

#1371422 — [PATCH 11/11] mm: consider compaction feedback also for costly allocation

From: Michal Hocko <mhocko@suse.com>

PAGE_ALLOC_COSTLY_ORDER retry logic is mostly handled inside
should_reclaim_retry currently where we decide to not retry after at
least order worth of pages were reclaimed or the watermark check for at
least one zone would succeed after reclaiming all pages if the reclaim
hasn't made any progress. Compaction feedback is mostly ignored and we
just try to make sure that the compaction did at least something before
giving up.

The first condition was added by a41f24ea9fd6 ("page allocator: smarter
retry of costly-order allocations) and it assumed that lumpy reclaim
could have created a page of the sufficient order. Lumpy reclaim,
has been removed quite some time ago so the assumption doesn't hold
anymore. Remove the check for the number of reclaimed pages and rely
on the compaction feedback solely. should_reclaim_retry now only
makes sure that we keep retrying reclaim for high order pages only
if they are hidden by watermaks so order-0 reclaim makes really sense.

should_compact_retry now keeps retrying even for the costly allocations.
The number of retries is reduced wrt. !costly requests because they are
less important and harder to grant and so their pressure shouldn't cause
contention for other requests or cause an over reclaim. We also do not
reset no_progress_loops for costly request to make sure we do not keep
reclaiming too agressively.

This has been tested by running a process which fragments memory:
	- compact memory
	- mmap large portion of the memory (1920M on 2GRAM machine with 2G
	  of swapspace)
	- MADV_DONTNEED single page in PAGE_SIZE*((1UL<<MAX_ORDER)-1)
	  steps until certain amount of memory is freed (250M in my test)
	  and reduce the step to (step / 2) + 1 after reaching the end of
	  the mapping
	- then run a script which populates the page cache 2G (MemTotal)
	  from /dev/zero to a new file
And then tries to allocate
nr_hugepages=$(awk '/MemAvailable/{printf "%d\n", $2/(2*1024)}' /proc/meminfo)
huge pages.

root@test1:~# echo 1 > /proc/sys/vm/overcommit_memory;echo 1 > /proc/sys/vm/compact_memory; ./fragment-mem-and-run /root/alloc_hugepages.sh 1920M 250M
Node 0, zone      DMA     31     28     31     10      2      0      2      1      2      3      1
Node 0, zone    DMA32    437    319    171     50     28     25     20     16     16     14    437

* This is the /proc/buddyinfo after the compaction

Done fragmenting. size=2013265920 freed=262144000
Node 0, zone      DMA    165     48      3      1      2      0      2      2      2      2      0
Node 0, zone    DMA32  35109  14575    185     51     41     12      6      0      0      0      0

* /proc/buddyinfo after memory got fragmented

Executing "/root/alloc_hugepages.sh"
Eating some pagecache
508623+0 records in
508623+0 records out
2083319808 bytes (2.1 GB) copied, 11.7292 s, 178 MB/s
Node 0, zone      DMA      3      5      3      1      2      0      2      2      2      2      0
Node 0, zone    DMA32    111    344    153     20     24     10      3      0      0      0      0

* /proc/buddyinfo after page cache got eaten

Trying to allocate 129
129

* 129 hugepages requested and all of them granted.

Node 0, zone      DMA      3      5      3      1      2      0      2      2      2      2      0
Node 0, zone    DMA32    127     97     30     99     11      6      2      1      4      0      0

* /proc/buddyinfo after hugetlb allocation.

10 runs will behave as follows:
Trying to allocate 130
130
--
Trying to allocate 129
129
--
Trying to allocate 128
128
--
Trying to allocate 129
129
--
Trying to allocate 128
128
--
Trying to allocate 129
129
--
Trying to allocate 132
132
--
Trying to allocate 129
129
--
Trying to allocate 128
128
--
Trying to allocate 129
129

So basically 100% success for all 10 attempts.
Without the patch numbers looked much worse:
Trying to allocate 128
12
--
Trying to allocate 129
14
--
Trying to allocate 129
7
--
Trying to allocate 129
16
--
Trying to allocate 129
30
--
Trying to allocate 129
38
--
Trying to allocate 129
19
--
Trying to allocate 129
37
--
Trying to allocate 129
28
--
Trying to allocate 129
37

Just for completness the base kernel without oom detection rework looks
as follows:
Trying to allocate 127
30
--
Trying to allocate 129
12
--
Trying to allocate 129
52
--
Trying to allocate 128
32
--
Trying to allocate 129
12
--
Trying to allocate 129
10
--
Trying to allocate 129
32
--
Trying to allocate 128
14
--
Trying to allocate 128
16
--
Trying to allocate 129
8

As we can see the success rate is much more volatile and smaller without
this patch. So the patch not only makes the retry logic for costly
requests more sensible the success rate is even higher.

Signed-off-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 63 +++++++++++++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 3d26ab892a7d..90a18ae92849 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3019,6 +3019,8 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
 		     enum migrate_mode *migrate_mode,
 		     int compaction_retries)
 {
+	int max_retries = MAX_COMPACT_RETRIES;
+
 	if (!order)
 		return false;
 
@@ -3036,17 +3038,24 @@ should_compact_retry(unsigned int order, enum compact_result compact_result,
 	}
 
 	/*
-	 * !costly allocations are really important and we have to make sure
-	 * the compaction wasn't deferred or didn't bail out early due to locks
-	 * contention before we go OOM. Still cap the reclaim retry loops with
-	 * progress to prevent from looping forever and potential trashing.
+	 * 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.
 	 */
-	if (order <= PAGE_ALLOC_COSTLY_ORDER) {
-		if (compaction_withdrawn(compact_result))
-			return true;
-		if (compaction_retries <= MAX_COMPACT_RETRIES)
-			return true;
-	}
+	if (compaction_withdrawn(compact_result))
+		return true;
+
+	/*
+	 * !costly requests are much more important than __GFP_REPEAT
+	 * costly ones because they are de facto nofail and invoke OOM
+	 * killer to move on while costly can fail and users are ready
+	 * to cope with that. 1/4 retries is rather arbitrary but we
+	 * would need much more detailed feedback from compaction to
+	 * make a better decision.
+	 */
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
+		max_retries /= 4;
+	if (compaction_retries <= max_retries)
+		return true;
 
 	return false;
 }
@@ -3207,18 +3216,17 @@ static inline bool is_thp_gfp_mask(gfp_t gfp_mask)
  * Checks whether it makes sense to retry the reclaim to make a forward progress
  * for the given allocation request.
  * The reclaim feedback represented by did_some_progress (any progress during
- * the last reclaim round), pages_reclaimed (cumulative number of reclaimed
- * pages) and no_progress_loops (number of reclaim rounds without any progress
- * in a row) is considered as well as the reclaimable pages on the applicable
- * zone list (with a backoff mechanism which is a function of no_progress_loops).
+ * the last reclaim round) and no_progress_loops (number of reclaim rounds without
+ * any progress in a row) is considered as well as the reclaimable pages on the
+ * applicable zone list (with a backoff mechanism which is a function of
+ * no_progress_loops).
  *
  * Returns true if a retry is viable or false to enter the oom path.
  */
 static inline bool
 should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		     struct alloc_context *ac, int alloc_flags,
-		     bool did_some_progress, unsigned long pages_reclaimed,
-		     int no_progress_loops)
+		     bool did_some_progress, int no_progress_loops)
 {
 	struct zone *zone;
 	struct zoneref *z;
@@ -3230,14 +3238,6 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 	if (no_progress_loops > MAX_RECLAIM_RETRIES)
 		return false;
 
-	if (order > PAGE_ALLOC_COSTLY_ORDER) {
-		if (pages_reclaimed >= (1<<order))
-			return false;
-
-		if (did_some_progress)
-			return true;
-	}
-
 	/*
 	 * Keep reclaiming pages while there is a chance this will lead somewhere.
 	 * If none of the target zones can satisfy our allocation request even
@@ -3308,7 +3308,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	bool can_direct_reclaim = gfp_mask & __GFP_DIRECT_RECLAIM;
 	struct page *page = NULL;
 	int alloc_flags;
-	unsigned long pages_reclaimed = 0;
 	unsigned long did_some_progress;
 	enum migrate_mode migration_mode = MIGRATE_ASYNC;
 	enum compact_result compact_result;
@@ -3442,16 +3441,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
 		goto noretry;
 
-	if (did_some_progress) {
+	/*
+	 * Costly allocations might have made a progress but this doesn't mean
+	 * their order will become available due to high fragmentation so
+	 * always increment the no progress counter for them
+	 */
+	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
 		no_progress_loops = 0;
-		pages_reclaimed += did_some_progress;
-	} else {
+	else
 		no_progress_loops++;
-	}
 
 	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
-				 did_some_progress > 0, pages_reclaimed,
-				 no_progress_loops))
+				 did_some_progress > 0, no_progress_loops))
 		goto retry;
 
 	/*
-- 
2.8.0.rc3

[toc] | [prev] | [next] | [standalone]

#1371517 — Re: [PATCH 11/11] mm: consider compaction feedback also for costly allocation

[Multipart message — attachments visible in raw view] — view raw

Attached you can find both the tool to fragment memory and the script it
calls in my testing.

I was executing this as follows (on 2G machine with 2G swap space):
echo 1 > /proc/sys/vm/overcommit_memory
echo 1 > /proc/sys/vm/compact_memory
/root/fragment-mem-and-run /root/alloc_hugepages.sh 1920M 250M
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1376054 — Re: [PATCH 11/11] mm: consider compaction feedback also for costly allocation

On 04/05/2016 01:25 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
>
> PAGE_ALLOC_COSTLY_ORDER retry logic is mostly handled inside
> should_reclaim_retry currently where we decide to not retry after at
> least order worth of pages were reclaimed or the watermark check for at
> least one zone would succeed after reclaiming all pages if the reclaim
> hasn't made any progress. Compaction feedback is mostly ignored and we
> just try to make sure that the compaction did at least something before
> giving up.
>
> The first condition was added by a41f24ea9fd6 ("page allocator: smarter
> retry of costly-order allocations) and it assumed that lumpy reclaim
> could have created a page of the sufficient order. Lumpy reclaim,
> has been removed quite some time ago so the assumption doesn't hold
> anymore. Remove the check for the number of reclaimed pages and rely
> on the compaction feedback solely. should_reclaim_retry now only
> makes sure that we keep retrying reclaim for high order pages only
> if they are hidden by watermaks so order-0 reclaim makes really sense.
>
> should_compact_retry now keeps retrying even for the costly allocations.
> The number of retries is reduced wrt. !costly requests because they are
> less important and harder to grant and so their pressure shouldn't cause
> contention for other requests or cause an over reclaim. We also do not
> reset no_progress_loops for costly request to make sure we do not keep
> reclaiming too agressively.

[...]

> Signed-off-by: Michal Hocko <mhocko@suse.com>

Acked-by: Vlastimil Babka <vbabka@suse.cz>

[toc] | [prev] | [next] | [standalone]

#1371524

One side note. I have promissed to provide some tracepoints which would
help us to see how the new code behaves. I have some basics but still
have to think more about that so I will send some more patches later on.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web