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

Groups > linux.kernel > #1351745 > unrolled thread

[PATCH] mm, oom: protect !costly allocations some more (was: Re: [PATCH 0/3] OOM detection rework v4)

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-07 17:10 +0100
    Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-08 04:50 +0100
      Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-08 10:10 +0100
        Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-08 10:30 +0100
    Re: [PATCH] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 10:30 +0100
      Re: [PATCH] mm, oom: protect !costly allocations some more Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-08 10:40 +0100
      Re: [PATCH] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-08 10:50 +0100
        Re: [PATCH] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 11:00 +0100
          Re: [PATCH] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-08 11:20 +0100
            Re: [PATCH] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 12:20 +0100
              Re: [PATCH] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-08 13:30 +0100
                Re: [PATCH] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 13:40 +0100
    Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Sergey Senozhatsky <sergey.senozhatsky.work@gmail.com> - 2016-03-08 11:00 +0100
      Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-08 15:00 +0100
    Re: [PATCH] mm, oom: protect !costly allocations some more (was:  Re: [PATCH 0/3] OOM detection rework v4) Hugh Dickins <hughd@google.com> - 2016-03-08 11:40 +0100
    [PATCH 1/3] mm, compaction: change COMPACT_ constants into enum Michal Hocko <mhocko@kernel.org> - 2016-03-08 14:50 +0100
      Re: [PATCH 1/3] mm, compaction: change COMPACT_ constants into enum Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 15:20 +0100
      Re: [PATCH 1/3] mm, compaction: change COMPACT_ constants into enum "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-03-09 05:00 +0100
    [PATCH 3/3] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-08 14:50 +0100
      Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 15:40 +0100
        Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-08 15:50 +0100
          Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 16:10 +0100
      Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-09 12:20 +0100
        Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Vlastimil Babka <vbabka@suse.cz> - 2016-03-09 15:10 +0100
        Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Hugh Dickins <hughd@google.com> - 2016-03-11 13:20 +0100
          Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-11 14:10 +0100
            Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Hugh Dickins <hughd@google.com> - 2016-03-11 20:10 +0100
              Re: [PATCH 3/3] mm, oom: protect !costly allocations some more Michal Hocko <mhocko@kernel.org> - 2016-03-14 17:30 +0100
    [PATCH 0/2] oom rework: high order enahncements Michal Hocko <mhocko@kernel.org> - 2016-03-08 14:50 +0100
      [PATCH 2/3] mm, compaction: cover all compaction mode in compact_zone Michal Hocko <mhocko@kernel.org> - 2016-03-08 14:50 +0100
        Re: [PATCH 2/3] mm, compaction: cover all compaction mode in  compact_zone Vlastimil Babka <vbabka@suse.cz> - 2016-03-08 15:30 +0100
        Re: [PATCH 2/3] mm, compaction: cover all compaction mode in compact_zone "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-03-09 05:00 +0100
    Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Joonsoo Kim <js1304@gmail.com> - 2016-03-08 16:20 +0100
      Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-08 17:10 +0100
        Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Joonsoo Kim <js1304@gmail.com> - 2016-03-08 18:10 +0100
          Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-09 11:50 +0100
            Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Joonsoo Kim <js1304@gmail.com> - 2016-03-11 16:00 +0100
              Re: [PATCH] mm, oom: protect !costly allocations some more (was: Re:  [PATCH 0/3] OOM detection rework v4) Michal Hocko <mhocko@kernel.org> - 2016-03-11 16:30 +0100

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

#1353113 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On Tue 08-03-16 15:34:37, Vlastimil Babka wrote:
> On 03/08/2016 02:42 PM, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> > 
> > should_reclaim_retry will give up retries for higher order allocations
> > if none of the eligible zones has any requested or higher order pages
> > available even if we pass the watermak check for order-0. This is done
> > because there is no guarantee that the reclaimable and currently free
> > pages will form the required order.
> > 
> > This can, however, lead to situations were the high-order request (e.g.
> > order-2 required for the stack allocation during fork) will trigger
> > OOM too early - e.g. after the first reclaim/compaction round. Such a
> > system would have to be highly fragmented and there is no guarantee
> > further reclaim/compaction attempts would help but at least make sure
> > that the compaction was active before we go OOM and keep retrying even
> > if should_reclaim_retry tells us to oom if the last compaction round
> > was either inactive (deferred, skipped or bailed out early due to
> > contention) or it told us to continue.
> > 
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
> > ---
> >  include/linux/compaction.h |  5 +++++
> >  mm/page_alloc.c            | 53 ++++++++++++++++++++++++++++++++--------------
> >  2 files changed, 42 insertions(+), 16 deletions(-)
> > 
> > diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> > index b167801187e7..49e04326dcb8 100644
> > --- a/include/linux/compaction.h
> > +++ b/include/linux/compaction.h
> > @@ -14,6 +14,11 @@ enum compact_result {
> >  	/* compaction should continue to another pageblock */
> >  	COMPACT_CONTINUE,
> >  	/*
> > +	 * whoever is calling compaction should retry because it was either
> > +	 * not active or it tells us there is more work to be done.
> > +	 */
> > +	COMPACT_SHOULD_RETRY = COMPACT_CONTINUE,
> 
> Hmm, I'm not sure about this. AFAIK compact_zone() doesn't ever return
> COMPACT_CONTINUE, and thus try_to_compact_pages() also doesn't. This
> overloading of CONTINUE only applies to compaction_suitable(). But the
> value that should_compact_retry() is testing comes only from
> try_to_compact_pages(). So this is not wrong, but perhaps a bit misleading?

Well the idea was that I wanted to cover all the _possible_ cases where
compaction might want to tell us "please try again even when the last
round wasn't really successful". COMPACT_CONTINUE might not be returned
right now but we can come up with that in the future. It sounds like a
sensible feedback to me. But maybe there would be a better name for such
a feedback. I confess this is a bit oom-rework centric name...

Also I find it better to hide details behind a more generic name.

I am open to suggestions here, of course.
-- 
Michal Hocko
SUSE Labs

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

#1353130 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On 03/08/2016 03:48 PM, Michal Hocko wrote:
> On Tue 08-03-16 15:34:37, Vlastimil Babka wrote:
>>> --- a/include/linux/compaction.h
>>> +++ b/include/linux/compaction.h
>>> @@ -14,6 +14,11 @@ enum compact_result {
>>>  	/* compaction should continue to another pageblock */
>>>  	COMPACT_CONTINUE,
>>>  	/*
>>> +	 * whoever is calling compaction should retry because it was either
>>> +	 * not active or it tells us there is more work to be done.
>>> +	 */
>>> +	COMPACT_SHOULD_RETRY = COMPACT_CONTINUE,
>>
>> Hmm, I'm not sure about this. AFAIK compact_zone() doesn't ever return
>> COMPACT_CONTINUE, and thus try_to_compact_pages() also doesn't. This
>> overloading of CONTINUE only applies to compaction_suitable(). But the
>> value that should_compact_retry() is testing comes only from
>> try_to_compact_pages(). So this is not wrong, but perhaps a bit misleading?
> 
> Well the idea was that I wanted to cover all the _possible_ cases where
> compaction might want to tell us "please try again even when the last
> round wasn't really successful". COMPACT_CONTINUE might not be returned
> right now but we can come up with that in the future. It sounds like a
> sensible feedback to me. But maybe there would be a better name for such
> a feedback. I confess this is a bit oom-rework centric name...

Hmm, I see. But it doesn't really tell use to please try again. That
interpretation is indeed oom-specific. What it's actually telling us is
either a) reclaim and then try again (COMPACT_SKIPPED), b) try again
just to overcome the deferred state (COMPACT_DEFERRED). COMPACT_CONTINUE
says "go ahead", but only from compaction_suitable().
So the attempt a generic name doesn't really work here I'm afraid :/

> Also I find it better to hide details behind a more generic name.
> 
> I am open to suggestions here, of course.
> 

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

#1354020 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

Joonsoo has pointed out that this attempt is still not sufficient
becasuse we might have invoked only a single compaction round which
is might be not enough. I fully agree with that. Here is my take on
that. It is again based on the number of retries loop.

I was also playing with an idea of doing something similar to the
reclaim retry logic:
	if (order) {
		if (compaction_made_progress(compact_result)
			no_compact_progress = 0;
		else if (compaction_failed(compact_result)
			no_compact_progress++;
	}
but it is compaction_failed() part which is not really
straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
compaction_suitable however hide this from compaction users so it
seems like we can never see it.

Maybe we can update the feedback mechanism from the compaction but
retries count seems reasonably easy to understand and pragmatic. If
we cannot form a order page after we tried for N times then it really
doesn't make much sense to continue and we are oom for this order. I am
holding my breath to hear from Hugh on this, though. In case it doesn't
then I would be really interested whether changing MAX_COMPACT_RETRIES
makes any difference.

I haven't preserved Tested-by from Sergey to be on the safe side even
though strictly speaking this should be less prone to high order OOMs
because we clearly retry more times.
---
From 33f08d6eeb0f5eaf1c73c292f070102ddec5878a Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Wed, 9 Mar 2016 10:57:42 +0100
Subject: [PATCH] mm, oom: protect !costly allocations some more

should_reclaim_retry will give up retries for higher order allocations
if none of the eligible zones has any requested or higher order pages
available even if we pass the watermak check for order-0. This is done
because there is no guarantee that the reclaimable and currently free
pages will form the required order.

This can, however, lead to situations were the high-order request (e.g.
order-2 required for the stack allocation during fork) will trigger
OOM too early - e.g. after the first reclaim/compaction round. Such a
system would have to be highly fragmented and there is no guarantee
further reclaim/compaction attempts would help but at least make sure
that the compaction was active before we go OOM and keep retrying even
if should_reclaim_retry tells us to oom if
	- the last compaction round was either inactive (deferred,
	  skipped or bailed out early due to contention) or
	- we haven't completed at least MAX_COMPACT_RETRIES successful
	  (either COMPACT_PARTIAL or COMPACT_COMPLETE) compaction
	  rounds.

The first rule ensures that the very last attempt for compaction
was ignored while the second guarantees that the compaction has done
some work. Multiple retries might be needed to prevent occasional
pigggy packing of other contexts to steal the compacted pages while
the current context manages to retry to allocate them.

If the given number of successful retries is not sufficient for a
reasonable workloads we should focus on the collected compaction
tracepoints data and try to address the issue in the compaction code.
If this is not feasible we can increase the retries limit.

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

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index b167801187e7..7d028ccf440a 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -61,6 +61,12 @@ extern void compaction_defer_reset(struct zone *zone, int order,
 				bool alloc_success);
 extern bool compaction_restarting(struct zone *zone, int order);
 
+static inline bool compaction_made_progress(enum compact_result result)
+{
+	return (compact_result > COMPACT_SKIPPED &&
+				compact_result < COMPACT_NO_SUITABLE_PAGE)
+}
+
 #else
 static inline enum compact_result try_to_compact_pages(gfp_t gfp_mask,
 			unsigned int order, int alloc_flags,
@@ -93,6 +99,10 @@ static inline bool compaction_deferred(struct zone *zone, int order)
 	return true;
 }
 
+static inline bool compaction_made_progress(enum compact_result result)
+{
+	return false;
+}
 #endif /* CONFIG_COMPACTION */
 
 #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4acc0aa1aee0..5f1fc3793836 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2813,34 +2813,33 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
 	return page;
 }
 
+
+/*
+ * Maximum number of compaction retries wit a progress before OOM
+ * killer is consider as the only way to move forward.
+ */
+#define MAX_COMPACT_RETRIES 16
+
 #ifdef CONFIG_COMPACTION
 /* Try memory compaction for high-order allocations before reclaim */
 static struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 		int alloc_flags, const struct alloc_context *ac,
 		enum migrate_mode mode, int *contended_compaction,
-		bool *deferred_compaction)
+		enum compact_result *compact_result)
 {
-	enum compact_result compact_result;
 	struct page *page;
 
 	if (!order)
 		return NULL;
 
 	current->flags |= PF_MEMALLOC;
-	compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
+	*compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
 						mode, contended_compaction);
 	current->flags &= ~PF_MEMALLOC;
 
-	switch (compact_result) {
-	case COMPACT_DEFERRED:
-		*deferred_compaction = true;
-		/* fall-through */
-	case COMPACT_SKIPPED:
+	if (*compact_result <= COMPACT_SKIPPED)
 		return NULL;
-	default:
-		break;
-	}
 
 	/*
 	 * At least in one zone compaction wasn't deferred or skipped, so let's
@@ -2870,15 +2869,44 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 
 	return NULL;
 }
+
+static inline bool
+should_compact_retry(unsigned int order, enum compact_result compact_result,
+		     int contended_compaction, int compaction_retries)
+{
+	/*
+	 * !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.
+	 */
+	if (order && order <= PAGE_ALLOC_COSTLY_ORDER) {
+		if (compact_result <= COMPACT_SKIPPED)
+			return true;
+		if (contended_compaction > COMPACT_CONTENDED_NONE)
+			return true;
+		if (compaction_retries <= MAX_COMPACT_RETRIES)
+			return true;
+	}
+
+	return false;
+}
 #else
 static inline struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 		int alloc_flags, const struct alloc_context *ac,
 		enum migrate_mode mode, int *contended_compaction,
-		bool *deferred_compaction)
+		enum compact_result *compact_result)
 {
 	return NULL;
 }
+
+static inline bool
+should_compact_retry(unsigned int order, enum compact_result compact_result,
+		     int contended_compaction, int compaction_retries)
+{
+	return false;
+}
 #endif /* CONFIG_COMPACTION */
 
 /* Perform direct synchronous page reclaim */
@@ -3118,7 +3146,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	int alloc_flags;
 	unsigned long did_some_progress;
 	enum migrate_mode migration_mode = MIGRATE_ASYNC;
-	bool deferred_compaction = false;
+	enum compact_result compact_result;
+	int compaction_retries = 0;
 	int contended_compaction = COMPACT_CONTENDED_NONE;
 	int no_progress_loops = 0;
 
@@ -3227,10 +3256,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
 					migration_mode,
 					&contended_compaction,
-					&deferred_compaction);
+					&compact_result);
 	if (page)
 		goto got_pg;
 
+	if (order && compaction_made_progress(compact_result))
+		compaction_retries++;
+
 	/* Checks for THP-specific high-order allocations */
 	if (is_thp_gfp_mask(gfp_mask)) {
 		/*
@@ -3240,7 +3272,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 		 * to heavily disrupt the system, so we fail the allocation
 		 * instead of entering direct reclaim.
 		 */
-		if (deferred_compaction)
+		if (compact_result == COMPACT_DEFERRED)
 			goto nopage;
 
 		/*
@@ -3294,6 +3326,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 				 did_some_progress > 0, no_progress_loops))
 		goto retry;
 
+	if (should_compact_retry(order, compact_result, contended_compaction,
+				 compaction_retries))
+		goto retry;
+
 	/* Reclaim has failed us, start killing things */
 	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
 	if (page)
@@ -3314,7 +3350,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
 					    ac, migration_mode,
 					    &contended_compaction,
-					    &deferred_compaction);
+					    &compact_result);
 	if (page)
 		goto got_pg;
 nopage:
-- 
2.7.0

-- 
Michal Hocko
SUSE Labs

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

#1354158 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On 03/09/2016 12:11 PM, Michal Hocko wrote:
> Joonsoo has pointed out that this attempt is still not sufficient
> becasuse we might have invoked only a single compaction round which
> is might be not enough. I fully agree with that. Here is my take on
> that. It is again based on the number of retries loop.
> 
> I was also playing with an idea of doing something similar to the
> reclaim retry logic:
> 	if (order) {
> 		if (compaction_made_progress(compact_result)

Progress for compaction would probably mean counting successful
migrations. This would converge towards a definitive false (without
parallel activity) in the current implementation, but probably not for
the proposed redesigns where migration and free scanner initial
positions are not fixed.

> 			no_compact_progress = 0;
> 		else if (compaction_failed(compact_result)
> 			no_compact_progress++;
> 	}
> but it is compaction_failed() part which is not really
> straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
> resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
> compaction_suitable however hide this from compaction users so it
> seems like we can never see it.

Anything other than COMPACT_PARTIAL is "failed" :) But it doesn't itself
hint at whether retrying makes sense or not. Reclaim is simpler in this
sense...

> Maybe we can update the feedback mechanism from the compaction but
> retries count seems reasonably easy to understand and pragmatic. If
> we cannot form a order page after we tried for N times then it really
> doesn't make much sense to continue and we are oom for this order. I am
> holding my breath to hear from Hugh on this, though. In case it doesn't
> then I would be really interested whether changing MAX_COMPACT_RETRIES
> makes any difference.
> 
> I haven't preserved Tested-by from Sergey to be on the safe side even
> though strictly speaking this should be less prone to high order OOMs
> because we clearly retry more times.
> ---
> From 33f08d6eeb0f5eaf1c73c292f070102ddec5878a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Wed, 9 Mar 2016 10:57:42 +0100
> Subject: [PATCH] mm, oom: protect !costly allocations some more
> 
> should_reclaim_retry will give up retries for higher order allocations
> if none of the eligible zones has any requested or higher order pages
> available even if we pass the watermak check for order-0. This is done
> because there is no guarantee that the reclaimable and currently free
> pages will form the required order.
> 
> This can, however, lead to situations were the high-order request (e.g.
> order-2 required for the stack allocation during fork) will trigger
> OOM too early - e.g. after the first reclaim/compaction round. Such a
> system would have to be highly fragmented and there is no guarantee
> further reclaim/compaction attempts would help but at least make sure
> that the compaction was active before we go OOM and keep retrying even
> if should_reclaim_retry tells us to oom if
> 	- the last compaction round was either inactive (deferred,
> 	  skipped or bailed out early due to contention) or
> 	- we haven't completed at least MAX_COMPACT_RETRIES successful
> 	  (either COMPACT_PARTIAL or COMPACT_COMPLETE) compaction
> 	  rounds.
> 
> The first rule ensures that the very last attempt for compaction
> was ignored while the second guarantees that the compaction has done
> some work. Multiple retries might be needed to prevent occasional
> pigggy packing of other contexts to steal the compacted pages while
> the current context manages to retry to allocate them.
> 
> If the given number of successful retries is not sufficient for a
> reasonable workloads we should focus on the collected compaction
> tracepoints data and try to address the issue in the compaction code.
> If this is not feasible we can increase the retries limit.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

Yeah, this could work.
Acked-by: Vlastimil Babka <vbabka@suse.cz>

> ---
>  include/linux/compaction.h | 10 +++++++
>  mm/page_alloc.c            | 68 +++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index b167801187e7..7d028ccf440a 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -61,6 +61,12 @@ extern void compaction_defer_reset(struct zone *zone, int order,
>  				bool alloc_success);
>  extern bool compaction_restarting(struct zone *zone, int order);
>  
> +static inline bool compaction_made_progress(enum compact_result result)
> +{
> +	return (compact_result > COMPACT_SKIPPED &&
> +				compact_result < COMPACT_NO_SUITABLE_PAGE)
> +}
> +
>  #else
>  static inline enum compact_result try_to_compact_pages(gfp_t gfp_mask,
>  			unsigned int order, int alloc_flags,
> @@ -93,6 +99,10 @@ static inline bool compaction_deferred(struct zone *zone, int order)
>  	return true;
>  }
>  
> +static inline bool compaction_made_progress(enum compact_result result)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_COMPACTION */
>  
>  #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4acc0aa1aee0..5f1fc3793836 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2813,34 +2813,33 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	return page;
>  }
>  
> +
> +/*
> + * Maximum number of compaction retries wit a progress before OOM
> + * killer is consider as the only way to move forward.
> + */
> +#define MAX_COMPACT_RETRIES 16
> +
>  #ifdef CONFIG_COMPACTION
>  /* Try memory compaction for high-order allocations before reclaim */
>  static struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  		int alloc_flags, const struct alloc_context *ac,
>  		enum migrate_mode mode, int *contended_compaction,
> -		bool *deferred_compaction)
> +		enum compact_result *compact_result)
>  {
> -	enum compact_result compact_result;
>  	struct page *page;
>  
>  	if (!order)
>  		return NULL;
>  
>  	current->flags |= PF_MEMALLOC;
> -	compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
> +	*compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>  						mode, contended_compaction);
>  	current->flags &= ~PF_MEMALLOC;
>  
> -	switch (compact_result) {
> -	case COMPACT_DEFERRED:
> -		*deferred_compaction = true;
> -		/* fall-through */
> -	case COMPACT_SKIPPED:
> +	if (*compact_result <= COMPACT_SKIPPED)
>  		return NULL;
> -	default:
> -		break;
> -	}
>  
>  	/*
>  	 * At least in one zone compaction wasn't deferred or skipped, so let's
> @@ -2870,15 +2869,44 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  
>  	return NULL;
>  }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> +		     int contended_compaction, int compaction_retries)
> +{
> +	/*
> +	 * !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.
> +	 */
> +	if (order && order <= PAGE_ALLOC_COSTLY_ORDER) {
> +		if (compact_result <= COMPACT_SKIPPED)
> +			return true;
> +		if (contended_compaction > COMPACT_CONTENDED_NONE)
> +			return true;
> +		if (compaction_retries <= MAX_COMPACT_RETRIES)
> +			return true;
> +	}
> +
> +	return false;
> +}
>  #else
>  static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  		int alloc_flags, const struct alloc_context *ac,
>  		enum migrate_mode mode, int *contended_compaction,
> -		bool *deferred_compaction)
> +		enum compact_result *compact_result)
>  {
>  	return NULL;
>  }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> +		     int contended_compaction, int compaction_retries)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_COMPACTION */
>  
>  /* Perform direct synchronous page reclaim */
> @@ -3118,7 +3146,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	int alloc_flags;
>  	unsigned long did_some_progress;
>  	enum migrate_mode migration_mode = MIGRATE_ASYNC;
> -	bool deferred_compaction = false;
> +	enum compact_result compact_result;
> +	int compaction_retries = 0;
>  	int contended_compaction = COMPACT_CONTENDED_NONE;
>  	int no_progress_loops = 0;
>  
> @@ -3227,10 +3256,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
>  					migration_mode,
>  					&contended_compaction,
> -					&deferred_compaction);
> +					&compact_result);
>  	if (page)
>  		goto got_pg;
>  
> +	if (order && compaction_made_progress(compact_result))
> +		compaction_retries++;
> +
>  	/* Checks for THP-specific high-order allocations */
>  	if (is_thp_gfp_mask(gfp_mask)) {
>  		/*
> @@ -3240,7 +3272,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		 * to heavily disrupt the system, so we fail the allocation
>  		 * instead of entering direct reclaim.
>  		 */
> -		if (deferred_compaction)
> +		if (compact_result == COMPACT_DEFERRED)
>  			goto nopage;
>  
>  		/*
> @@ -3294,6 +3326,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  				 did_some_progress > 0, no_progress_loops))
>  		goto retry;
>  
> +	if (should_compact_retry(order, compact_result, contended_compaction,
> +				 compaction_retries))
> +		goto retry;
> +
>  	/* Reclaim has failed us, start killing things */
>  	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
>  	if (page)
> @@ -3314,7 +3350,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
>  					    ac, migration_mode,
>  					    &contended_compaction,
> -					    &deferred_compaction);
> +					    &compact_result);
>  	if (page)
>  		goto got_pg;
>  nopage:
> 

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

#1355869 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On Wed, 9 Mar 2016, Michal Hocko wrote:
> Joonsoo has pointed out that this attempt is still not sufficient
> becasuse we might have invoked only a single compaction round which
> is might be not enough. I fully agree with that. Here is my take on
> that. It is again based on the number of retries loop.
> 
> I was also playing with an idea of doing something similar to the
> reclaim retry logic:
> 	if (order) {
> 		if (compaction_made_progress(compact_result)
> 			no_compact_progress = 0;
> 		else if (compaction_failed(compact_result)
> 			no_compact_progress++;
> 	}
> but it is compaction_failed() part which is not really
> straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
> resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
> compaction_suitable however hide this from compaction users so it
> seems like we can never see it.
> 
> Maybe we can update the feedback mechanism from the compaction but
> retries count seems reasonably easy to understand and pragmatic. If
> we cannot form a order page after we tried for N times then it really
> doesn't make much sense to continue and we are oom for this order. I am
> holding my breath to hear from Hugh on this, though.

Never a wise strategy.  But I just got around to it tonight.

I do believe you've nailed it with this patch!  Thank you!

I've applied 1/3, 2/3 and this (ah, it became the missing 3/3 later on)
on top of 4.5.0-rc5-mm1 (I think there have been a couple of mmotms since,
but I've not got to them yet): so far it is looking good on all machines.

After a quick go with the simple make -j20 in tmpfs, which survived
a cycle on the laptop, I've switched back to my original tougher load,
and that's going well so far: no sign of any OOMs.  But I've interrupted
on the laptop to report back to you now, then I'll leave it running
overnight.

> In case it doesn't
> then I would be really interested whether changing MAX_COMPACT_RETRIES
> makes any difference.
> 
> I haven't preserved Tested-by from Sergey to be on the safe side even
> though strictly speaking this should be less prone to high order OOMs
> because we clearly retry more times.
> ---
> From 33f08d6eeb0f5eaf1c73c292f070102ddec5878a Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Wed, 9 Mar 2016 10:57:42 +0100
> Subject: [PATCH] mm, oom: protect !costly allocations some more
> 
> should_reclaim_retry will give up retries for higher order allocations
> if none of the eligible zones has any requested or higher order pages
> available even if we pass the watermak check for order-0. This is done
> because there is no guarantee that the reclaimable and currently free
> pages will form the required order.
> 
> This can, however, lead to situations were the high-order request (e.g.
> order-2 required for the stack allocation during fork) will trigger
> OOM too early - e.g. after the first reclaim/compaction round. Such a
> system would have to be highly fragmented and there is no guarantee
> further reclaim/compaction attempts would help but at least make sure
> that the compaction was active before we go OOM and keep retrying even
> if should_reclaim_retry tells us to oom if
> 	- the last compaction round was either inactive (deferred,
> 	  skipped or bailed out early due to contention) or
> 	- we haven't completed at least MAX_COMPACT_RETRIES successful
> 	  (either COMPACT_PARTIAL or COMPACT_COMPLETE) compaction
> 	  rounds.
> 
> The first rule ensures that the very last attempt for compaction
> was ignored while the second guarantees that the compaction has done
> some work. Multiple retries might be needed to prevent occasional
> pigggy packing of other contexts to steal the compacted pages while
> the current context manages to retry to allocate them.
> 
> If the given number of successful retries is not sufficient for a
> reasonable workloads we should focus on the collected compaction
> tracepoints data and try to address the issue in the compaction code.
> If this is not feasible we can increase the retries limit.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/compaction.h | 10 +++++++
>  mm/page_alloc.c            | 68 +++++++++++++++++++++++++++++++++++-----------
>  2 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index b167801187e7..7d028ccf440a 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -61,6 +61,12 @@ extern void compaction_defer_reset(struct zone *zone, int order,
>  				bool alloc_success);
>  extern bool compaction_restarting(struct zone *zone, int order);
>  
> +static inline bool compaction_made_progress(enum compact_result result)
> +{
> +	return (compact_result > COMPACT_SKIPPED &&
> +				compact_result < COMPACT_NO_SUITABLE_PAGE)

That line didn't build at all:

        return result > COMPACT_SKIPPED && result < COMPACT_NO_SUITABLE_PAGE;

> +}
> +
>  #else
>  static inline enum compact_result try_to_compact_pages(gfp_t gfp_mask,
>  			unsigned int order, int alloc_flags,
> @@ -93,6 +99,10 @@ static inline bool compaction_deferred(struct zone *zone, int order)
>  	return true;
>  }
>  
> +static inline bool compaction_made_progress(enum compact_result result)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_COMPACTION */
>  
>  #if defined(CONFIG_COMPACTION) && defined(CONFIG_SYSFS) && defined(CONFIG_NUMA)
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 4acc0aa1aee0..5f1fc3793836 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2813,34 +2813,33 @@ __alloc_pages_may_oom(gfp_t gfp_mask, unsigned int order,
>  	return page;
>  }
>  
> +
> +/*
> + * Maximum number of compaction retries wit a progress before OOM
> + * killer is consider as the only way to move forward.
> + */
> +#define MAX_COMPACT_RETRIES 16
> +
>  #ifdef CONFIG_COMPACTION
>  /* Try memory compaction for high-order allocations before reclaim */
>  static struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  		int alloc_flags, const struct alloc_context *ac,
>  		enum migrate_mode mode, int *contended_compaction,
> -		bool *deferred_compaction)
> +		enum compact_result *compact_result)
>  {
> -	enum compact_result compact_result;
>  	struct page *page;
>  
>  	if (!order)
>  		return NULL;
>  
>  	current->flags |= PF_MEMALLOC;
> -	compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
> +	*compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>  						mode, contended_compaction);
>  	current->flags &= ~PF_MEMALLOC;
>  
> -	switch (compact_result) {
> -	case COMPACT_DEFERRED:
> -		*deferred_compaction = true;
> -		/* fall-through */
> -	case COMPACT_SKIPPED:
> +	if (*compact_result <= COMPACT_SKIPPED)
>  		return NULL;
> -	default:
> -		break;
> -	}
>  
>  	/*
>  	 * At least in one zone compaction wasn't deferred or skipped, so let's
> @@ -2870,15 +2869,44 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  
>  	return NULL;
>  }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> +		     int contended_compaction, int compaction_retries)
> +{
> +	/*
> +	 * !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.
> +	 */
> +	if (order && order <= PAGE_ALLOC_COSTLY_ORDER) {
> +		if (compact_result <= COMPACT_SKIPPED)
> +			return true;
> +		if (contended_compaction > COMPACT_CONTENDED_NONE)
> +			return true;
> +		if (compaction_retries <= MAX_COMPACT_RETRIES)
> +			return true;
> +	}
> +
> +	return false;
> +}
>  #else
>  static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  		int alloc_flags, const struct alloc_context *ac,
>  		enum migrate_mode mode, int *contended_compaction,
> -		bool *deferred_compaction)
> +		enum compact_result *compact_result)
>  {
>  	return NULL;
>  }
> +
> +static inline bool
> +should_compact_retry(unsigned int order, enum compact_result compact_result,
> +		     int contended_compaction, int compaction_retries)
> +{
> +	return false;
> +}
>  #endif /* CONFIG_COMPACTION */
>  
>  /* Perform direct synchronous page reclaim */
> @@ -3118,7 +3146,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	int alloc_flags;
>  	unsigned long did_some_progress;
>  	enum migrate_mode migration_mode = MIGRATE_ASYNC;
> -	bool deferred_compaction = false;
> +	enum compact_result compact_result;
> +	int compaction_retries = 0;
>  	int contended_compaction = COMPACT_CONTENDED_NONE;
>  	int no_progress_loops = 0;
>  
> @@ -3227,10 +3256,13 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
>  					migration_mode,
>  					&contended_compaction,
> -					&deferred_compaction);
> +					&compact_result);
>  	if (page)
>  		goto got_pg;
>  
> +	if (order && compaction_made_progress(compact_result))
> +		compaction_retries++;
> +
>  	/* Checks for THP-specific high-order allocations */
>  	if (is_thp_gfp_mask(gfp_mask)) {
>  		/*
> @@ -3240,7 +3272,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  		 * to heavily disrupt the system, so we fail the allocation
>  		 * instead of entering direct reclaim.
>  		 */
> -		if (deferred_compaction)
> +		if (compact_result == COMPACT_DEFERRED)
>  			goto nopage;
>  
>  		/*
> @@ -3294,6 +3326,10 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  				 did_some_progress > 0, no_progress_loops))
>  		goto retry;
>  
> +	if (should_compact_retry(order, compact_result, contended_compaction,
> +				 compaction_retries))
> +		goto retry;
> +
>  	/* Reclaim has failed us, start killing things */
>  	page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
>  	if (page)
> @@ -3314,7 +3350,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
>  					    ac, migration_mode,
>  					    &contended_compaction,
> -					    &deferred_compaction);
> +					    &compact_result);
>  	if (page)
>  		goto got_pg;
>  nopage:
> -- 
> 2.7.0
> 
> -- 
> Michal Hocko
> SUSE Labs

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

#1355895 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On Fri 11-03-16 04:17:30, Hugh Dickins wrote:
> On Wed, 9 Mar 2016, Michal Hocko wrote:
> > Joonsoo has pointed out that this attempt is still not sufficient
> > becasuse we might have invoked only a single compaction round which
> > is might be not enough. I fully agree with that. Here is my take on
> > that. It is again based on the number of retries loop.
> > 
> > I was also playing with an idea of doing something similar to the
> > reclaim retry logic:
> > 	if (order) {
> > 		if (compaction_made_progress(compact_result)
> > 			no_compact_progress = 0;
> > 		else if (compaction_failed(compact_result)
> > 			no_compact_progress++;
> > 	}
> > but it is compaction_failed() part which is not really
> > straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
> > resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
> > compaction_suitable however hide this from compaction users so it
> > seems like we can never see it.
> > 
> > Maybe we can update the feedback mechanism from the compaction but
> > retries count seems reasonably easy to understand and pragmatic. If
> > we cannot form a order page after we tried for N times then it really
> > doesn't make much sense to continue and we are oom for this order. I am
> > holding my breath to hear from Hugh on this, though.
> 
> Never a wise strategy.  But I just got around to it tonight.
> 
> I do believe you've nailed it with this patch!  Thank you!

That's a great news! Thanks for testing.

> I've applied 1/3, 2/3 and this (ah, it became the missing 3/3 later on)
> on top of 4.5.0-rc5-mm1 (I think there have been a couple of mmotms since,
> but I've not got to them yet): so far it is looking good on all machines.
> 
> After a quick go with the simple make -j20 in tmpfs, which survived
> a cycle on the laptop, I've switched back to my original tougher load,
> and that's going well so far: no sign of any OOMs.  But I've interrupted
> on the laptop to report back to you now, then I'll leave it running
> overnight.

OK, let's wait for the rest of the tests but I find it really optimistic
considering how easily you could trigger the issue previously. Anyway
I hope for your Tested-by after you are reasonably confident your loads
are behaving well.

[...]
> > diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> > index b167801187e7..7d028ccf440a 100644
> > --- a/include/linux/compaction.h
> > +++ b/include/linux/compaction.h
> > @@ -61,6 +61,12 @@ extern void compaction_defer_reset(struct zone *zone, int order,
> >  				bool alloc_success);
> >  extern bool compaction_restarting(struct zone *zone, int order);
> >  
> > +static inline bool compaction_made_progress(enum compact_result result)
> > +{
> > +	return (compact_result > COMPACT_SKIPPED &&
> > +				compact_result < COMPACT_NO_SUITABLE_PAGE)
> 
> That line didn't build at all:
> 
>         return result > COMPACT_SKIPPED && result < COMPACT_NO_SUITABLE_PAGE;

those last minute changes... Sorry about that. Fixed.

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1356141 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On Fri, 11 Mar 2016, Michal Hocko wrote:
> On Fri 11-03-16 04:17:30, Hugh Dickins wrote:
> > On Wed, 9 Mar 2016, Michal Hocko wrote:
> > > Joonsoo has pointed out that this attempt is still not sufficient
> > > becasuse we might have invoked only a single compaction round which
> > > is might be not enough. I fully agree with that. Here is my take on
> > > that. It is again based on the number of retries loop.
> > > 
> > > I was also playing with an idea of doing something similar to the
> > > reclaim retry logic:
> > > 	if (order) {
> > > 		if (compaction_made_progress(compact_result)
> > > 			no_compact_progress = 0;
> > > 		else if (compaction_failed(compact_result)
> > > 			no_compact_progress++;
> > > 	}
> > > but it is compaction_failed() part which is not really
> > > straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
> > > resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
> > > compaction_suitable however hide this from compaction users so it
> > > seems like we can never see it.
> > > 
> > > Maybe we can update the feedback mechanism from the compaction but
> > > retries count seems reasonably easy to understand and pragmatic. If
> > > we cannot form a order page after we tried for N times then it really
> > > doesn't make much sense to continue and we are oom for this order. I am
> > > holding my breath to hear from Hugh on this, though.
> > 
> > Never a wise strategy.  But I just got around to it tonight.
> > 
> > I do believe you've nailed it with this patch!  Thank you!
> 
> That's a great news! Thanks for testing.
> 
> > I've applied 1/3, 2/3 and this (ah, it became the missing 3/3 later on)
> > on top of 4.5.0-rc5-mm1 (I think there have been a couple of mmotms since,
> > but I've not got to them yet): so far it is looking good on all machines.
> > 
> > After a quick go with the simple make -j20 in tmpfs, which survived
> > a cycle on the laptop, I've switched back to my original tougher load,
> > and that's going well so far: no sign of any OOMs.  But I've interrupted
> > on the laptop to report back to you now, then I'll leave it running
> > overnight.
> 
> OK, let's wait for the rest of the tests but I find it really optimistic
> considering how easily you could trigger the issue previously. Anyway
> I hope for your Tested-by after you are reasonably confident your loads
> are behaving well.

Three have been stably running load for between 6 and 7 hours now,
no problems, looking very good:

Tested-by: Hugh Dickins <hughd@google.com>

I'll be interested to see how my huge tmpfs loads fare with the rework,
but I'm not quite ready to try that today; and any issue there (I've no
reason to suppose that there will be) can be a separate investigation
for me to make at some future date.  It was this order=2 regression
that was holding me back, and I've now no objection to your patches
(though nobody should imagine that I've actually studied them).

Thank you, Michal.

Hugh

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

#1357397 — Re: [PATCH 3/3] mm, oom: protect !costly allocations some more

On Fri 11-03-16 11:08:05, Hugh Dickins wrote:
> On Fri, 11 Mar 2016, Michal Hocko wrote:
> > On Fri 11-03-16 04:17:30, Hugh Dickins wrote:
> > > On Wed, 9 Mar 2016, Michal Hocko wrote:
> > > > Joonsoo has pointed out that this attempt is still not sufficient
> > > > becasuse we might have invoked only a single compaction round which
> > > > is might be not enough. I fully agree with that. Here is my take on
> > > > that. It is again based on the number of retries loop.
> > > > 
> > > > I was also playing with an idea of doing something similar to the
> > > > reclaim retry logic:
> > > > 	if (order) {
> > > > 		if (compaction_made_progress(compact_result)
> > > > 			no_compact_progress = 0;
> > > > 		else if (compaction_failed(compact_result)
> > > > 			no_compact_progress++;
> > > > 	}
> > > > but it is compaction_failed() part which is not really
> > > > straightforward to define. Is it COMPACT_NO_SUITABLE_PAGE
> > > > resp. COMPACT_NOT_SUITABLE_ZONE sufficient? compact_finished and
> > > > compaction_suitable however hide this from compaction users so it
> > > > seems like we can never see it.
> > > > 
> > > > Maybe we can update the feedback mechanism from the compaction but
> > > > retries count seems reasonably easy to understand and pragmatic. If
> > > > we cannot form a order page after we tried for N times then it really
> > > > doesn't make much sense to continue and we are oom for this order. I am
> > > > holding my breath to hear from Hugh on this, though.
> > > 
> > > Never a wise strategy.  But I just got around to it tonight.
> > > 
> > > I do believe you've nailed it with this patch!  Thank you!
> > 
> > That's a great news! Thanks for testing.
> > 
> > > I've applied 1/3, 2/3 and this (ah, it became the missing 3/3 later on)
> > > on top of 4.5.0-rc5-mm1 (I think there have been a couple of mmotms since,
> > > but I've not got to them yet): so far it is looking good on all machines.
> > > 
> > > After a quick go with the simple make -j20 in tmpfs, which survived
> > > a cycle on the laptop, I've switched back to my original tougher load,
> > > and that's going well so far: no sign of any OOMs.  But I've interrupted
> > > on the laptop to report back to you now, then I'll leave it running
> > > overnight.
> > 
> > OK, let's wait for the rest of the tests but I find it really optimistic
> > considering how easily you could trigger the issue previously. Anyway
> > I hope for your Tested-by after you are reasonably confident your loads
> > are behaving well.
> 
> Three have been stably running load for between 6 and 7 hours now,
> no problems, looking very good:
> 
> Tested-by: Hugh Dickins <hughd@google.com>

Thanks!

> I'll be interested to see how my huge tmpfs loads fare with the rework,
> but I'm not quite ready to try that today; and any issue there (I've no
> reason to suppose that there will be) can be a separate investigation
> for me to make at some future date.  It was this order=2 regression
> that was holding me back, and I've now no objection to your patches
> (though nobody should imagine that I've actually studied them).

I still have some work on top pending and I do not want to rush these
changes and target this for 4.7. 4.6 is just too close and I would hate
to push some last minute changes. I think oom_reaper would be large
enough for 4.6 in this area. 

I will post the full series after rc1. Andrew feel free to drop it from
the mmotm tree for now. I would prefer they got all reviewed together
rather than a larger number of fixups.

Thanks Hugh for your testing. I wish I could depend on it less but I've
not been able to reproduce not matter how much I tried.

-- 
Michal Hocko
SUSE Labs

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

#1353053 — [PATCH 0/2] oom rework: high order enahncements

The first two patches are cleanups for the compaction and the second
patch is updated as per Vlastimil's feedback. I didn't add his Acked-by
because I have added COMPACT_SHOULD_RETRY to make the retry logic in
the page allocator more robust for future changes.

Hugh has still reported this is not sufficient but I would prefer to
handle the issue he is seeing in a separate patch once we understand
what is going on there. The second patch sounds like a reasonable
starting point to me.

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

#1353059 — [PATCH 2/3] mm, compaction: cover all compaction mode in compact_zone

From: Michal Hocko <mhocko@suse.com>

the compiler is complaining after "mm, compaction: change COMPACT_
constants into enum"

mm/compaction.c: In function ‘compact_zone’:
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_DEFERRED’ not handled in switch [-Wswitch]
  switch (ret) {
  ^
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_COMPLETE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NO_SUITABLE_PAGE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NOT_SUITABLE_ZONE’ not handled in switch [-Wswitch]
mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_CONTENDED’ not handled in switch [-Wswitch]

compaction_suitable is allowed to return only COMPACT_PARTIAL,
COMPACT_SKIPPED and COMPACT_CONTINUE so other cases are simply
impossible. Put a VM_BUG_ON to catch an impossible return value.

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

diff --git a/mm/compaction.c b/mm/compaction.c
index 0f61f12d82b6..86968d3a04e6 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1347,15 +1347,12 @@ static enum compact_result compact_zone(struct zone *zone, struct compact_contro
 
 	ret = compaction_suitable(zone, cc->order, cc->alloc_flags,
 							cc->classzone_idx);
-	switch (ret) {
-	case COMPACT_PARTIAL:
-	case COMPACT_SKIPPED:
-		/* Compaction is likely to fail */
+	/* Compaction is likely to fail */
+	if (ret == COMPACT_PARTIAL || ret == COMPACT_SKIPPED)
 		return ret;
-	case COMPACT_CONTINUE:
-		/* Fall through to compaction */
-		;
-	}
+
+	/* huh, compaction_suitable is returning something unexpected */
+	VM_BUG_ON(ret != COMPACT_CONTINUE);
 
 	/*
 	 * Clear pageblock skip if there were failures recently and compaction
-- 
2.7.0

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

#1353096 — Re: [PATCH 2/3] mm, compaction: cover all compaction mode in compact_zone

On 03/08/2016 02:42 PM, Michal Hocko wrote:
> From: Michal Hocko <mhocko@suse.com>
> 
> the compiler is complaining after "mm, compaction: change COMPACT_
> constants into enum"

Potentially a squash into that patch then?

> mm/compaction.c: In function ‘compact_zone’:
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_DEFERRED’ not handled in switch [-Wswitch]
>   switch (ret) {
>   ^
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_COMPLETE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NO_SUITABLE_PAGE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NOT_SUITABLE_ZONE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_CONTENDED’ not handled in switch [-Wswitch]
> 
> compaction_suitable is allowed to return only COMPACT_PARTIAL,
> COMPACT_SKIPPED and COMPACT_CONTINUE so other cases are simply
> impossible. Put a VM_BUG_ON to catch an impossible return value.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>

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

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

#1353776 — Re: [PATCH 2/3] mm, compaction: cover all compaction mode in compact_zone

> 
> From: Michal Hocko <mhocko@suse.com>
> 
> the compiler is complaining after "mm, compaction: change COMPACT_
> constants into enum"
> 
> mm/compaction.c: In function ‘compact_zone’:
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_DEFERRED’ not handled in switch [-Wswitch]
>   switch (ret) {
>   ^
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_COMPLETE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NO_SUITABLE_PAGE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_NOT_SUITABLE_ZONE’ not handled in switch [-Wswitch]
> mm/compaction.c:1350:2: warning: enumeration value ‘COMPACT_CONTENDED’ not handled in switch [-Wswitch]
> 
> compaction_suitable is allowed to return only COMPACT_PARTIAL,
> COMPACT_SKIPPED and COMPACT_CONTINUE so other cases are simply
> impossible. Put a VM_BUG_ON to catch an impossible return value.
> 
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---

Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

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

#1353132

2016-03-08 1:08 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Mon 29-02-16 22:02:13, Michal Hocko wrote:
>> Andrew,
>> could you queue this one as well, please? This is more a band aid than a
>> real solution which I will be working on as soon as I am able to
>> reproduce the issue but the patch should help to some degree at least.
>
> Joonsoo wasn't very happy about this approach so let me try a different
> way. What do you think about the following? Hugh, Sergey does it help

I'm still not happy. Just ensuring one compaction run doesn't mean our
best. What's your purpose of OOM rework? From my understanding,
you'd like to trigger OOM kill deterministic and *not prematurely*.
This makes sense.

But, what you did in case of high order allocation is completely different
with original purpose. It may be deterministic but *completely premature*.
There is no way to prevent premature OOM kill. So, I want to ask one more
time. Why OOM kill is better than retry reclaiming when there is reclaimable
page? Deterministic is for what? It ensures something more?

Please see Hugh's latest vmstat. There are plenty of anon pages when
OOM kill happens and it may have enough swap space. Even if
compaction runs and fails, why do we need to kill something
in this case? OOM kill should be a last resort.

Please see Hugh's previous report and OOM dump.

[  796.540791] Mem-Info:
[  796.557378] active_anon:150198 inactive_anon:46022 isolated_anon:32
 active_file:5107 inactive_file:1664 isolated_file:57
 unevictable:3067 dirty:4 writeback:75 unstable:0
 slab_reclaimable:13907 slab_unreclaimable:23236
 mapped:8889 shmem:3171 pagetables:2176 bounce:0
 free:1637 free_pcp:54 free_cma:0
[  796.630465] Node 0 DMA32 free:13904kB min:3940kB low:4944kB
high:5948kB active_anon:588776kB inactive_anon:188816kB
active_file:20432kB inactive_file:6928kB unevictable:12268kB
isolated(anon):128kB isolated(file):8kB present:1046128kB
managed:1004892kB mlocked:12268kB dirty:16kB writeback:1400kB
mapped:35556kB shmem:12684kB slab_reclaimable:55628kB
slab_unreclaimable:92944kB kernel_stack:4448kB pagetables:8604kB
unstable:0kB bounce:0kB free_pcp:296kB local_pcp:164kB free_cma:0kB
writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
[  796.685815] lowmem_reserve[]: 0 0 0
[  796.687390] Node 0 DMA32: 969*4kB (UE) 184*8kB (UME) 167*16kB (UM)
19*32kB (UM) 3*64kB (UM) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
0*4096kB = 8820kB
[  796.729696] Node 0 hugepages_total=0 hugepages_free=0
hugepages_surp=0 hugepages_size=2048kB

See [  796.557378] and [  796.630465].
In this 100 ms time interval, freepage increase a lot and
there are enough high order pages. OOM kill happen later
so freepage would come from reclaim. This shows
that your previous implementation which uses static retry number
causes premature OOM.

This attempt using compaction result looks not different to me.
It would also cause premature OOM kill.

I don't insist endless retry. I just want a more scientific criteria
that prevents
premature OOM kill. I'm really tire to say same thing again and again.
Am I missing something? This is the situation that I totally misunderstand
something? Please let me know.

Note: your current implementation doesn't consider which zone is compacted.
If DMA zone which easily fail to make high order page is compacted,
your implementation will not do retry. It also looks not our best.

Thanks.

> for your load? I have tested it with the Hugh's load and there was no
> major difference from the previous testing so at least nothing has blown
> up as I am not able to reproduce the issue here.
>
> Other changes in the compaction are still needed but I would like to not
> depend on them right now.
> ---
> From 0974f127e8eb7fe53e65f3a8b398db57effe9755 Mon Sep 17 00:00:00 2001
> From: Michal Hocko <mhocko@suse.com>
> Date: Mon, 7 Mar 2016 15:30:37 +0100
> Subject: [PATCH] mm, oom: protect !costly allocations some more
>
> should_reclaim_retry will give up retries for higher order allocations
> if none of the eligible zones has any requested or higher order pages
> available even if we pass the watermak check for order-0. This is done
> because there is no guarantee that the reclaimable and currently free
> pages will form the required order.
>
> This can, however, lead to situations were the high-order request (e.g.
> order-2 required for the stack allocation during fork) will trigger
> OOM too early - e.g. after the first reclaim/compaction round. Such a
> system would have to be highly fragmented and there is no guarantee
> further reclaim/compaction attempts would help but at least make sure
> that the compaction was active before we go OOM and keep retrying even
> if should_reclaim_retry tells us to oom if the last compaction round
> was either inactive (deferred, skipped or bailed out early due to
> contention) or it told us to continue.
>
> Additionally define COMPACT_NONE which reflects cases where the
> compaction is completely disabled.
>
> Signed-off-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/compaction.h |  2 ++
>  mm/page_alloc.c            | 41 ++++++++++++++++++++++++-----------------
>  2 files changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index 4cd4ddf64cc7..a4cec4a03f7d 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -1,6 +1,8 @@
>  #ifndef _LINUX_COMPACTION_H
>  #define _LINUX_COMPACTION_H
>
> +/* compaction disabled */
> +#define COMPACT_NONE           -1
>  /* Return values for compact_zone() and try_to_compact_pages() */
>  /* compaction didn't start as it was deferred due to past failures */
>  #define COMPACT_DEFERRED       0
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 269a04f20927..f89e3cbfdf90 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -2819,28 +2819,22 @@ static struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>                 int alloc_flags, const struct alloc_context *ac,
>                 enum migrate_mode mode, int *contended_compaction,
> -               bool *deferred_compaction)
> +               unsigned long *compact_result)
>  {
> -       unsigned long compact_result;
>         struct page *page;
>
> -       if (!order)
> +       if (!order) {
> +               *compact_result = COMPACT_NONE;
>                 return NULL;
> +       }
>
>         current->flags |= PF_MEMALLOC;
> -       compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
> +       *compact_result = try_to_compact_pages(gfp_mask, order, alloc_flags, ac,
>                                                 mode, contended_compaction);
>         current->flags &= ~PF_MEMALLOC;
>
> -       switch (compact_result) {
> -       case COMPACT_DEFERRED:
> -               *deferred_compaction = true;
> -               /* fall-through */
> -       case COMPACT_SKIPPED:
> +       if (*compact_result <= COMPACT_SKIPPED)
>                 return NULL;
> -       default:
> -               break;
> -       }
>
>         /*
>          * At least in one zone compaction wasn't deferred or skipped, so let's
> @@ -2875,8 +2869,9 @@ static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>                 int alloc_flags, const struct alloc_context *ac,
>                 enum migrate_mode mode, int *contended_compaction,
> -               bool *deferred_compaction)
> +               unsigned long *compact_result)
>  {
> +       *compact_result = COMPACT_NONE;
>         return NULL;
>  }
>  #endif /* CONFIG_COMPACTION */
> @@ -3118,7 +3113,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>         int alloc_flags;
>         unsigned long did_some_progress;
>         enum migrate_mode migration_mode = MIGRATE_ASYNC;
> -       bool deferred_compaction = false;
> +       unsigned long compact_result;
>         int contended_compaction = COMPACT_CONTENDED_NONE;
>         int no_progress_loops = 0;
>
> @@ -3227,7 +3222,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>         page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags, ac,
>                                         migration_mode,
>                                         &contended_compaction,
> -                                       &deferred_compaction);
> +                                       &compact_result);
>         if (page)
>                 goto got_pg;
>
> @@ -3240,7 +3235,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>                  * to heavily disrupt the system, so we fail the allocation
>                  * instead of entering direct reclaim.
>                  */
> -               if (deferred_compaction)
> +               if (compact_result == COMPACT_DEFERRED)
>                         goto nopage;
>
>                 /*
> @@ -3294,6 +3289,18 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>                                  did_some_progress > 0, no_progress_loops))
>                 goto retry;
>
> +       /*
> +        * !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.
> +        */
> +       if (order && order <= PAGE_ALLOC_COSTLY_ORDER) {
> +               if (compact_result <= COMPACT_CONTINUE)
> +                       goto retry;
> +               if (contended_compaction > COMPACT_CONTENDED_NONE)
> +                       goto retry;
> +       }
> +
>         /* Reclaim has failed us, start killing things */
>         page = __alloc_pages_may_oom(gfp_mask, order, ac, &did_some_progress);
>         if (page)
> @@ -3314,7 +3321,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>         page = __alloc_pages_direct_compact(gfp_mask, order, alloc_flags,
>                                             ac, migration_mode,
>                                             &contended_compaction,
> -                                           &deferred_compaction);
> +                                           &compact_result);
>         if (page)
>                 goto got_pg;
>  nopage:
> --
> 2.7.0
>
> --
> Michal Hocko
> SUSE Labs

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

#1353179

On Wed 09-03-16 00:19:03, Joonsoo Kim wrote:
> 2016-03-08 1:08 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Mon 29-02-16 22:02:13, Michal Hocko wrote:
> >> Andrew,
> >> could you queue this one as well, please? This is more a band aid than a
> >> real solution which I will be working on as soon as I am able to
> >> reproduce the issue but the patch should help to some degree at least.
> >
> > Joonsoo wasn't very happy about this approach so let me try a different
> > way. What do you think about the following? Hugh, Sergey does it help
> 
> I'm still not happy. Just ensuring one compaction run doesn't mean our
> best.

OK, let me think about it some more.

> What's your purpose of OOM rework? From my understanding,
> you'd like to trigger OOM kill deterministic and *not prematurely*.
> This makes sense.

Well this is a bit awkward because we do not have any proper definition
of what prematurely actually means. We do not know whether something
changes and decides to free some memory right after we made the decision.
We also do not know whether reclaiming some more memory would help
because we might be trashing over few remaining pages so there would be
still some progress, albeit small, progress. The system would be
basically unusable and the OOM killer would be a large relief. What I
want to achieve is to have a clear definition of _when_ we fire and do
not fire _often_ to be impractical. There are loads where the new
implementation behaved slightly better (see the cover for my tests) and
there surely be some where this will be worse. I want this to be
reasonably good. I am not claiming we are there yet and the interaction
with the compaction seems like it needs some work, no question about
that.

> But, what you did in case of high order allocation is completely different
> with original purpose. It may be deterministic but *completely premature*.
> There is no way to prevent premature OOM kill. So, I want to ask one more
> time. Why OOM kill is better than retry reclaiming when there is reclaimable
> page? Deterministic is for what? It ensures something more?

yes, If we keep reclaiming we can soon start trashing or over reclaim
too much which would hurt more processes. If you invoke the OOM killer
instead then chances are that you will release a lot of memory at once
and that would help to reconcile the memory pressure as well as free
some page blocks which couldn't have been compacted before and not
affect potentially many processes. The effect would be reduced to a
single process. If we had a proper trashing detection feedback we could
do much more clever decisions of course.

But back to the !costly OOMs. Once your system is fragmented so heavily
that there are no free blocks that would satisfy !costly request then
something has gone terribly wrong and we should fix it. To me it sounds
like we do not care about those requests early enough and only start
carying after we hit the wall. Maybe kcompactd can help us in this
regards.

> Please see Hugh's latest vmstat. There are plenty of anon pages when
> OOM kill happens and it may have enough swap space. Even if
> compaction runs and fails, why do we need to kill something
> in this case? OOM kill should be a last resort.

Well this would be the case even if we were trashing over swap.
Refaulting the swapped out memory all over again...

> Please see Hugh's previous report and OOM dump.
> 
> [  796.540791] Mem-Info:
> [  796.557378] active_anon:150198 inactive_anon:46022 isolated_anon:32
>  active_file:5107 inactive_file:1664 isolated_file:57
>  unevictable:3067 dirty:4 writeback:75 unstable:0
>  slab_reclaimable:13907 slab_unreclaimable:23236
>  mapped:8889 shmem:3171 pagetables:2176 bounce:0
>  free:1637 free_pcp:54 free_cma:0
> [  796.630465] Node 0 DMA32 free:13904kB min:3940kB low:4944kB
> high:5948kB active_anon:588776kB inactive_anon:188816kB
> active_file:20432kB inactive_file:6928kB unevictable:12268kB
> isolated(anon):128kB isolated(file):8kB present:1046128kB
> managed:1004892kB mlocked:12268kB dirty:16kB writeback:1400kB
> mapped:35556kB shmem:12684kB slab_reclaimable:55628kB
> slab_unreclaimable:92944kB kernel_stack:4448kB pagetables:8604kB
> unstable:0kB bounce:0kB free_pcp:296kB local_pcp:164kB free_cma:0kB
> writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
> [  796.685815] lowmem_reserve[]: 0 0 0
> [  796.687390] Node 0 DMA32: 969*4kB (UE) 184*8kB (UME) 167*16kB (UM)
> 19*32kB (UM) 3*64kB (UM) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
> 0*4096kB = 8820kB
> [  796.729696] Node 0 hugepages_total=0 hugepages_free=0
> hugepages_surp=0 hugepages_size=2048kB
> 
> See [  796.557378] and [  796.630465].
> In this 100 ms time interval, freepage increase a lot and
> there are enough high order pages. OOM kill happen later
> so freepage would come from reclaim. This shows
> that your previous implementation which uses static retry number
> causes premature OOM.

Or simply one of the gcc simply exitted and freed up a memory which is
more likely. As I've tried to explain in other email, we cannot prevent
from those races. We simply do not have a crystal ball. All we know is
that at the time we checked the watermarks the last time there were
simply no eligible high order pages available.

> This attempt using compaction result looks not different to me.
> It would also cause premature OOM kill.
> 
> I don't insist endless retry. I just want a more scientific criteria
> that prevents premature OOM kill.

That is exactly what I try to achive here. Right now we are relying on
zone_reclaimable heuristic. That relies that some pages are freed (and
reset NR_PAGES_SCANNED) while we are scanning. With a stream of order-0
pages this is basically unbounded. What I am trying to achieve here
is to base the decision on the feedback. The first attempt was to use
the reclaim feedback. This turned out to be not sufficient for higher
orders because compaction can deffer and skip if we are close to
watermarks which is really surprising to me. So now I've tried to make
sure that we do not hit this path. I agree we can do better but there
always will be a moment to simply give up. Whatever that moment will
be we can still find loads which could theoretically go on for little
more and survive.

> I'm really tire to say same thing again and again.
> Am I missing something? This is the situation that I totally misunderstand
> something? Please let me know.
> 
> Note: your current implementation doesn't consider which zone is compacted.
> If DMA zone which easily fail to make high order page is compacted,
> your implementation will not do retry. It also looks not our best.

Why are we even consider DMA zone when we cannot ever allocate from this
zone?
-- 
Michal Hocko
SUSE Labs

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

#1353245

2016-03-09 1:05 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 09-03-16 00:19:03, Joonsoo Kim wrote:
>> 2016-03-08 1:08 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> > On Mon 29-02-16 22:02:13, Michal Hocko wrote:
>> >> Andrew,
>> >> could you queue this one as well, please? This is more a band aid than a
>> >> real solution which I will be working on as soon as I am able to
>> >> reproduce the issue but the patch should help to some degree at least.
>> >
>> > Joonsoo wasn't very happy about this approach so let me try a different
>> > way. What do you think about the following? Hugh, Sergey does it help
>>
>> I'm still not happy. Just ensuring one compaction run doesn't mean our
>> best.
>
> OK, let me think about it some more.
>
>> What's your purpose of OOM rework? From my understanding,
>> you'd like to trigger OOM kill deterministic and *not prematurely*.
>> This makes sense.
>
> Well this is a bit awkward because we do not have any proper definition
> of what prematurely actually means. We do not know whether something

If we don't have proper definition to it, please define it first. We
need to improve
the situation toward the clear goal. Just certain number of retry which has no
base doesn't make any sense.

> changes and decides to free some memory right after we made the decision.
> We also do not know whether reclaiming some more memory would help
> because we might be trashing over few remaining pages so there would be
> still some progress, albeit small, progress. The system would be
> basically unusable and the OOM killer would be a large relief. What I
> want to achieve is to have a clear definition of _when_ we fire and do

If we have no clear definition about premature, what's the meaning of
a clear definition of _when_? It would just mean random time.

> not fire _often_ to be impractical. There are loads where the new
> implementation behaved slightly better (see the cover for my tests) and
> there surely be some where this will be worse. I want this to be
> reasonably good. I am not claiming we are there yet and the interaction
> with the compaction seems like it needs some work, no question about
> that.
>
>> But, what you did in case of high order allocation is completely different
>> with original purpose. It may be deterministic but *completely premature*.
>> There is no way to prevent premature OOM kill. So, I want to ask one more
>> time. Why OOM kill is better than retry reclaiming when there is reclaimable
>> page? Deterministic is for what? It ensures something more?
>
> yes, If we keep reclaiming we can soon start trashing or over reclaim
> too much which would hurt more processes. If you invoke the OOM killer
> instead then chances are that you will release a lot of memory at once
> and that would help to reconcile the memory pressure as well as free
> some page blocks which couldn't have been compacted before and not
> affect potentially many processes. The effect would be reduced to a
> single process. If we had a proper trashing detection feedback we could
> do much more clever decisions of course.

It looks like you did it for performance reason. You'd better think again about
effect of OOM kill. We don't have enough knowledge about user space program
architecture and killing one important process could lead to whole
system unusable. Moreover, OOM kill could cause important data loss so
should be avoided as much as possible. Performance reason cannot
justify OOM kill.

>
> But back to the !costly OOMs. Once your system is fragmented so heavily
> that there are no free blocks that would satisfy !costly request then
> something has gone terribly wrong and we should fix it. To me it sounds
> like we do not care about those requests early enough and only start
> carying after we hit the wall. Maybe kcompactd can help us in this
> regards.

Yes, but, it's another issue. In any situation, !costly OOM should not happen
prematurely.

>> Please see Hugh's latest vmstat. There are plenty of anon pages when
>> OOM kill happens and it may have enough swap space. Even if
>> compaction runs and fails, why do we need to kill something
>> in this case? OOM kill should be a last resort.
>
> Well this would be the case even if we were trashing over swap.
> Refaulting the swapped out memory all over again...

If thrashing is a main obstacle to decide proper OOM point,
we need to invent a way to handle thrashing or invent reasonable metric
which isn't affected by thrashing.

>> Please see Hugh's previous report and OOM dump.
>>
>> [  796.540791] Mem-Info:
>> [  796.557378] active_anon:150198 inactive_anon:46022 isolated_anon:32
>>  active_file:5107 inactive_file:1664 isolated_file:57
>>  unevictable:3067 dirty:4 writeback:75 unstable:0
>>  slab_reclaimable:13907 slab_unreclaimable:23236
>>  mapped:8889 shmem:3171 pagetables:2176 bounce:0
>>  free:1637 free_pcp:54 free_cma:0
>> [  796.630465] Node 0 DMA32 free:13904kB min:3940kB low:4944kB
>> high:5948kB active_anon:588776kB inactive_anon:188816kB
>> active_file:20432kB inactive_file:6928kB unevictable:12268kB
>> isolated(anon):128kB isolated(file):8kB present:1046128kB
>> managed:1004892kB mlocked:12268kB dirty:16kB writeback:1400kB
>> mapped:35556kB shmem:12684kB slab_reclaimable:55628kB
>> slab_unreclaimable:92944kB kernel_stack:4448kB pagetables:8604kB
>> unstable:0kB bounce:0kB free_pcp:296kB local_pcp:164kB free_cma:0kB
>> writeback_tmp:0kB pages_scanned:0 all_unreclaimable? no
>> [  796.685815] lowmem_reserve[]: 0 0 0
>> [  796.687390] Node 0 DMA32: 969*4kB (UE) 184*8kB (UME) 167*16kB (UM)
>> 19*32kB (UM) 3*64kB (UM) 0*128kB 0*256kB 0*512kB 0*1024kB 0*2048kB
>> 0*4096kB = 8820kB
>> [  796.729696] Node 0 hugepages_total=0 hugepages_free=0
>> hugepages_surp=0 hugepages_size=2048kB
>>
>> See [  796.557378] and [  796.630465].
>> In this 100 ms time interval, freepage increase a lot and
>> there are enough high order pages. OOM kill happen later
>> so freepage would come from reclaim. This shows
>> that your previous implementation which uses static retry number
>> causes premature OOM.
>
> Or simply one of the gcc simply exitted and freed up a memory which is

It doesn't matter where free memory comes from. If free memory increases
due to gcc exit, it implies that we can reclaim some memory from it. There is
no reason to trigger OOM in this case.

> more likely. As I've tried to explain in other email, we cannot prevent
> from those races. We simply do not have a crystal ball. All we know is
> that at the time we checked the watermarks the last time there were
> simply no eligible high order pages available.
>
>> This attempt using compaction result looks not different to me.
>> It would also cause premature OOM kill.
>>
>> I don't insist endless retry. I just want a more scientific criteria
>> that prevents premature OOM kill.
>
> That is exactly what I try to achive here. Right now we are relying on
> zone_reclaimable heuristic. That relies that some pages are freed (and
> reset NR_PAGES_SCANNED) while we are scanning. With a stream of order-0
> pages this is basically unbounded. What I am trying to achieve here
> is to base the decision on the feedback. The first attempt was to use
> the reclaim feedback. This turned out to be not sufficient for higher
> orders because compaction can deffer and skip if we are close to
> watermarks which is really surprising to me. So now I've tried to make
> sure that we do not hit this path. I agree we can do better but there
> always will be a moment to simply give up. Whatever that moment will
> be we can still find loads which could theoretically go on for little
> more and survive.

Problem is that, to me, current implementation looks really simple
give up. Maybe, precise definition about premature would be helpful here.
Without it, it would be just subjective.

>
>> I'm really tire to say same thing again and again.
>> Am I missing something? This is the situation that I totally misunderstand
>> something? Please let me know.
>>
>> Note: your current implementation doesn't consider which zone is compacted.
>> If DMA zone which easily fail to make high order page is compacted,
>> your implementation will not do retry. It also looks not our best.
>
> Why are we even consider DMA zone when we cannot ever allocate from this
> zone?

This is just an example. It could be ZONE_NORMAL and something else. If
we don't try all zones to compact, it's reasonable point to trigger OOM?

Thanks.

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

#1353976

On Wed 09-03-16 02:03:59, Joonsoo Kim wrote:
> 2016-03-09 1:05 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 09-03-16 00:19:03, Joonsoo Kim wrote:
> >> 2016-03-08 1:08 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> >> > On Mon 29-02-16 22:02:13, Michal Hocko wrote:
> >> >> Andrew,
> >> >> could you queue this one as well, please? This is more a band aid than a
> >> >> real solution which I will be working on as soon as I am able to
> >> >> reproduce the issue but the patch should help to some degree at least.
> >> >
> >> > Joonsoo wasn't very happy about this approach so let me try a different
> >> > way. What do you think about the following? Hugh, Sergey does it help
> >>
> >> I'm still not happy. Just ensuring one compaction run doesn't mean our
> >> best.
> >
> > OK, let me think about it some more.
> >
> >> What's your purpose of OOM rework? From my understanding,
> >> you'd like to trigger OOM kill deterministic and *not prematurely*.
> >> This makes sense.
> >
> > Well this is a bit awkward because we do not have any proper definition
> > of what prematurely actually means. We do not know whether something
> 
> If we don't have proper definition to it, please define it first.

OK, I should have probably said that _there_is_no_proper_definition_...
This will always be about heuristics as the clear cut can be pretty
subjective and what some load might see as unreasonable retries others
might see as insufficient. Our ultimate goal is to behave reasonable for
reasonable workloads. I am somehow skeptical about formulating this
into a single equation...

> We need to improve the situation toward the clear goal. Just certain
> number of retry which has no base doesn't make any sense.

Certain number of retries is what we already have right now. And that
certain number is hard to define even though it looks as simple as

NR_PAGES_SCANNED < 6*zone_reclaimable_pages && no_reclaimable_pages

because this is highly fragile when there are only few pages freed
regularly but not sufficient to get us out of the loop... I am trying
to formulate those retries somehow more deterministically considering
the feedback _and_ an estimate about the feasibility of future
reclaim/compaction. I admit that my attempts at compaction part have
been far from ideal so far. Partially because I missed many aspects
how it works.

[...]
> > not fire _often_ to be impractical. There are loads where the new
> > implementation behaved slightly better (see the cover for my tests) and
> > there surely be some where this will be worse. I want this to be
> > reasonably good. I am not claiming we are there yet and the interaction
> > with the compaction seems like it needs some work, no question about
> > that.
> >
> >> But, what you did in case of high order allocation is completely different
> >> with original purpose. It may be deterministic but *completely premature*.
> >> There is no way to prevent premature OOM kill. So, I want to ask one more
> >> time. Why OOM kill is better than retry reclaiming when there is reclaimable
> >> page? Deterministic is for what? It ensures something more?
> >
> > yes, If we keep reclaiming we can soon start trashing or over reclaim
> > too much which would hurt more processes. If you invoke the OOM killer
> > instead then chances are that you will release a lot of memory at once
> > and that would help to reconcile the memory pressure as well as free
> > some page blocks which couldn't have been compacted before and not
> > affect potentially many processes. The effect would be reduced to a
> > single process. If we had a proper trashing detection feedback we could
> > do much more clever decisions of course.
> 
> It looks like you did it for performance reason. You'd better think again about
> effect of OOM kill. We don't have enough knowledge about user space program
> architecture and killing one important process could lead to whole
> system unusable. Moreover, OOM kill could cause important data loss so
> should be avoided as much as possible. Performance reason cannot
> justify OOM kill.

No I am not talking about performance. I am talking about the system
healthiness as whole.

> > But back to the !costly OOMs. Once your system is fragmented so heavily
> > that there are no free blocks that would satisfy !costly request then
> > something has gone terribly wrong and we should fix it. To me it sounds
> > like we do not care about those requests early enough and only start
> > carying after we hit the wall. Maybe kcompactd can help us in this
> > regards.
> 
> Yes, but, it's another issue. In any situation, !costly OOM should not happen
> prematurely.

I fully agree and I guess we also agree on the assumption that we
shouldn't retry endlessly. So let's focus on what the OOM convergence
criteria should look like. I have another proposal which I will send as
a reply to the previous one.

> >> Please see Hugh's latest vmstat. There are plenty of anon pages when
> >> OOM kill happens and it may have enough swap space. Even if
> >> compaction runs and fails, why do we need to kill something
> >> in this case? OOM kill should be a last resort.
> >
> > Well this would be the case even if we were trashing over swap.
> > Refaulting the swapped out memory all over again...
> 
> If thrashing is a main obstacle to decide proper OOM point,
> we need to invent a way to handle thrashing or invent reasonable metric
> which isn't affected by thrashing.

Great, you are welcome to come up with one. But more seriously, isn't
the retries limiting a way to reduce the chances of threshing? It might
be not the ideal one because it doesn't work 100% but can we simply come
up with the one which works that reliable. This is a hard problem which
we haven't been able to solve for ages.

[...]

Thanks!
-- 
Michal Hocko
SUSE Labs

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

#1355966

2016-03-09 19:41 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> On Wed 09-03-16 02:03:59, Joonsoo Kim wrote:
>> 2016-03-09 1:05 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> > On Wed 09-03-16 00:19:03, Joonsoo Kim wrote:
>> >> 2016-03-08 1:08 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
>> >> > On Mon 29-02-16 22:02:13, Michal Hocko wrote:
>> >> >> Andrew,
>> >> >> could you queue this one as well, please? This is more a band aid than a
>> >> >> real solution which I will be working on as soon as I am able to
>> >> >> reproduce the issue but the patch should help to some degree at least.
>> >> >
>> >> > Joonsoo wasn't very happy about this approach so let me try a different
>> >> > way. What do you think about the following? Hugh, Sergey does it help
>> >>
>> >> I'm still not happy. Just ensuring one compaction run doesn't mean our
>> >> best.
>> >
>> > OK, let me think about it some more.
>> >
>> >> What's your purpose of OOM rework? From my understanding,
>> >> you'd like to trigger OOM kill deterministic and *not prematurely*.
>> >> This makes sense.
>> >
>> > Well this is a bit awkward because we do not have any proper definition
>> > of what prematurely actually means. We do not know whether something
>>
>> If we don't have proper definition to it, please define it first.
>
> OK, I should have probably said that _there_is_no_proper_definition_...
> This will always be about heuristics as the clear cut can be pretty
> subjective and what some load might see as unreasonable retries others
> might see as insufficient. Our ultimate goal is to behave reasonable for
> reasonable workloads. I am somehow skeptical about formulating this
> into a single equation...

I don't want a theoretically perfect definition. We need something that
can be used for judging further changes. So, how can you judge that
reasonable behave for reasonable workload? What's your criteria?
If someone complains 16 retries is too small and the other complains
16 retries is too big, what's your decision in this case?

If you decide to increase number of retry in this case, when can we
stop that increasing? If someone complains again that XX is too small
then do you continue to increase it?

For me, for order 0 case, reasonable part is watermark checking with
available (free + reclaimable) memory. It shows that we've done
our best so it doesn't matter that how many times we retry.

But, for high order case, there is no *feasible* estimation. Watermark
check as you did here isn't feasible because high order freepage
problem usually happen when there are enough but fragmented freepages.
It would be always failed. Without feasible estimation, N retry can't
show anything.

Your logic here is just like below.

"We've tried N times reclaim/compaction and failed. It is proved that
there is no possibility to make high order page. We should trigger OOM now."

Is it true that there is no possibility to make high order page in this case?
Can you be sure?

If someone who get OOM complains regression, can you persuade him
by above logic?

I don't think so. This is why I ask you to make proper definition on
term *premature* here.

>> We need to improve the situation toward the clear goal. Just certain
>> number of retry which has no base doesn't make any sense.
>
> Certain number of retries is what we already have right now. And that
> certain number is hard to define even though it looks as simple as
>
> NR_PAGES_SCANNED < 6*zone_reclaimable_pages && no_reclaimable_pages
>
> because this is highly fragile when there are only few pages freed
> regularly but not sufficient to get us out of the loop... I am trying
> to formulate those retries somehow more deterministically considering
> the feedback _and_ an estimate about the feasibility of future
> reclaim/compaction. I admit that my attempts at compaction part have
> been far from ideal so far. Partially because I missed many aspects
> how it works.
> [...]
>> > not fire _often_ to be impractical. There are loads where the new
>> > implementation behaved slightly better (see the cover for my tests) and
>> > there surely be some where this will be worse. I want this to be
>> > reasonably good. I am not claiming we are there yet and the interaction
>> > with the compaction seems like it needs some work, no question about
>> > that.
>> >
>> >> But, what you did in case of high order allocation is completely different
>> >> with original purpose. It may be deterministic but *completely premature*.
>> >> There is no way to prevent premature OOM kill. So, I want to ask one more
>> >> time. Why OOM kill is better than retry reclaiming when there is reclaimable
>> >> page? Deterministic is for what? It ensures something more?
>> >
>> > yes, If we keep reclaiming we can soon start trashing or over reclaim
>> > too much which would hurt more processes. If you invoke the OOM killer
>> > instead then chances are that you will release a lot of memory at once
>> > and that would help to reconcile the memory pressure as well as free
>> > some page blocks which couldn't have been compacted before and not
>> > affect potentially many processes. The effect would be reduced to a
>> > single process. If we had a proper trashing detection feedback we could
>> > do much more clever decisions of course.
>>
>> It looks like you did it for performance reason. You'd better think again about
>> effect of OOM kill. We don't have enough knowledge about user space program
>> architecture and killing one important process could lead to whole
>> system unusable. Moreover, OOM kill could cause important data loss so
>> should be avoided as much as possible. Performance reason cannot
>> justify OOM kill.
>
> No I am not talking about performance. I am talking about the system
> healthiness as whole.

So, do you think that more frequent OOM kill is healthier than other ways?

>> > But back to the !costly OOMs. Once your system is fragmented so heavily
>> > that there are no free blocks that would satisfy !costly request then
>> > something has gone terribly wrong and we should fix it. To me it sounds
>> > like we do not care about those requests early enough and only start
>> > carying after we hit the wall. Maybe kcompactd can help us in this
>> > regards.
>>
>> Yes, but, it's another issue. In any situation, !costly OOM should not happen
>> prematurely.
>
> I fully agree and I guess we also agree on the assumption that we
> shouldn't retry endlessly. So let's focus on what the OOM convergence
> criteria should look like. I have another proposal which I will send as
> a reply to the previous one.

That's also insufficient to me. It just add one more brute force retry
for compaction
without any reasonable estimation.

>> >> Please see Hugh's latest vmstat. There are plenty of anon pages when
>> >> OOM kill happens and it may have enough swap space. Even if
>> >> compaction runs and fails, why do we need to kill something
>> >> in this case? OOM kill should be a last resort.
>> >
>> > Well this would be the case even if we were trashing over swap.
>> > Refaulting the swapped out memory all over again...
>>
>> If thrashing is a main obstacle to decide proper OOM point,
>> we need to invent a way to handle thrashing or invent reasonable metric
>> which isn't affected by thrashing.
>
> Great, you are welcome to come up with one. But more seriously, isn't

For example, we can collect how many pages reclaimed and compare
it with reclaimable pages at start. If number of reclaimed pages
exceed it, we can think that we've tried to reclaim all reclaimable pages
at least once and can go next step such as OOM.

Thanks.

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

#1355982

On Fri 11-03-16 23:53:18, Joonsoo Kim wrote:
> 2016-03-09 19:41 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> > On Wed 09-03-16 02:03:59, Joonsoo Kim wrote:
> >> 2016-03-09 1:05 GMT+09:00 Michal Hocko <mhocko@kernel.org>:
> >> > On Wed 09-03-16 00:19:03, Joonsoo Kim wrote:
[...]
> >> >> What's your purpose of OOM rework? From my understanding,
> >> >> you'd like to trigger OOM kill deterministic and *not prematurely*.
> >> >> This makes sense.
> >> >
> >> > Well this is a bit awkward because we do not have any proper definition
> >> > of what prematurely actually means. We do not know whether something
> >>
> >> If we don't have proper definition to it, please define it first.
> >
> > OK, I should have probably said that _there_is_no_proper_definition_...
> > This will always be about heuristics as the clear cut can be pretty
> > subjective and what some load might see as unreasonable retries others
> > might see as insufficient. Our ultimate goal is to behave reasonable for
> > reasonable workloads. I am somehow skeptical about formulating this
> > into a single equation...
> 
> I don't want a theoretically perfect definition. We need something that
> can be used for judging further changes. So, how can you judge that
> reasonable behave for reasonable workload? What's your criteria?
> If someone complains 16 retries is too small and the other complains
> 16 retries is too big, what's your decision in this case?

The number of retries is the implementation detail. What matters,
really, is whether we can argue about the particular load and why it
should resp. shouldn't trigger the OOM killer. We can use our
tracepoints to have a look and judge the overall progress or lack of it
and see if we could do better. It is not the number of retries to tweak
first. It is the reclaim/compaction to be made more reliable.  Tweaking
the retries would be just the very last resort. If we can see that
compaction doesn't form the high order pages in a sufficient pace we
should find out why.

> If you decide to increase number of retry in this case, when can we
> stop that increasing? If someone complains again that XX is too small
> then do you continue to increase it?
> 
> For me, for order 0 case, reasonable part is watermark checking with
> available (free + reclaimable) memory. It shows that we've done
> our best so it doesn't matter that how many times we retry.
> 
> But, for high order case, there is no *feasible* estimation. Watermark
> check as you did here isn't feasible because high order freepage
> problem usually happen when there are enough but fragmented freepages.
> It would be always failed. Without feasible estimation, N retry can't
> show anything.

That's why I have done compaction retry loop independent on it in the
last patch.

> Your logic here is just like below.
> 
> "We've tried N times reclaim/compaction and failed. It is proved that
> there is no possibility to make high order page. We should trigger OOM now."

Have you seen the last patch where I make sure that the compaction had
to report _success_ at least N times to declare the OOM? I think we can
be reasonably sure that keep compacting again and again without any
bound doesn't make much sense when that doesn't lead to a requested
order page.

> Is it true that there is no possibility to make high order page in this case?
> Can you be sure?

The thing I am trying to tell you, and I seem to fail here, is that you
simply cannot be sure. Full stop. We might be staggering on the edge of the
cliff and fall or be lucky and end up on the safe side.

> If someone who get OOM complains regression, can you persuade him
> by above logic?

This really depends on the particular load of course.

> I don't think so. This is why I ask you to make proper definition on
> term *premature* here.

Sigh. And what if that particular reporter doesn't agree with my
"proper" definition because it doesn't suite the workload of the
interest? I mean, anything we end up doing is highly subjective and
it's been like that since ever OOM was introduced.

[...]
> >> It looks like you did it for performance reason. You'd better think again about
> >> effect of OOM kill. We don't have enough knowledge about user space program
> >> architecture and killing one important process could lead to whole
> >> system unusable. Moreover, OOM kill could cause important data loss so
> >> should be avoided as much as possible. Performance reason cannot
> >> justify OOM kill.
> >
> > No I am not talking about performance. I am talking about the system
> > healthiness as whole.
> 
> So, do you think that more frequent OOM kill is healthier than other ways?

I didn't say so. And except for the Hugh's testcase I haven't seen the
rework would cause that. As per the last testing result it seems that
this particular case has been fixed. If you believe that you can see
other cases than I am more than happy to look at them.

> >> > But back to the !costly OOMs. Once your system is fragmented so heavily
> >> > that there are no free blocks that would satisfy !costly request then
> >> > something has gone terribly wrong and we should fix it. To me it sounds
> >> > like we do not care about those requests early enough and only start
> >> > carying after we hit the wall. Maybe kcompactd can help us in this
> >> > regards.
> >>
> >> Yes, but, it's another issue. In any situation, !costly OOM should not happen
> >> prematurely.
> >
> > I fully agree and I guess we also agree on the assumption that we
> > shouldn't retry endlessly. So let's focus on what the OOM convergence
> > criteria should look like. I have another proposal which I will send as
> > a reply to the previous one.
> 
> That's also insufficient to me. It just add one more brute force retry
> for compaction
> without any reasonable estimation.

The compaction absolutely lacks any useful feedback mechanism. If we
ever grow one I am more than happy to make the estimate better.

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web