All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-06 13:52 ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	David Rientjes, Joonsoo Kim, Mel Gorman, Michal Hocko,
	Rik van Riel

After several people reported OOM's for order-2 allocations in 4.7 due to
Michal Hocko's OOM rework, he reverted the part that considered compaction
feedback [1] in the decisions to retry reclaim/compaction. This was to provide
a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an almost
complete solution that instead improved compaction reliability.

This series completes the mmotm solution and reintroduces the compaction
feedback into OOM decisions. The first two patches restore the state of mmotm
before the temporary solution was merged, the last patch should be the missing
piece for reliability. The third patch restricts the hardened compaction to
non-costly orders, since costly orders don't result in OOMs in the first place.

Some preliminary testing suggested that this approach should work, but I would
like to ask all who experienced the regression to please retest this. You will
need to apply this series on top of tag mmotm-2016-08-31-16-06 from the mmotm
git tree [2]. Thanks in advance!

[1] http://marc.info/?i=20160822093249.GA14916%40dhcp22.suse.cz%3E
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git

Vlastimil Babka (4):
  Revert "mm, oom: prevent premature OOM killer invocation for high
    order request"
  mm, compaction: more reliably increase direct compaction priority
  mm, compaction: restrict full priority to non-costly orders
  mm, compaction: make full priority ignore pageblock suitability

 include/linux/compaction.h |  1 +
 mm/compaction.c            | 11 ++++++---
 mm/internal.h              |  1 +
 mm/page_alloc.c            | 58 ++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 66 insertions(+), 5 deletions(-)

-- 
2.9.3

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-06 13:52 ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	David Rientjes, Joonsoo Kim, Mel Gorman, Michal Hocko,
	Rik van Riel

After several people reported OOM's for order-2 allocations in 4.7 due to
Michal Hocko's OOM rework, he reverted the part that considered compaction
feedback [1] in the decisions to retry reclaim/compaction. This was to provide
a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an almost
complete solution that instead improved compaction reliability.

This series completes the mmotm solution and reintroduces the compaction
feedback into OOM decisions. The first two patches restore the state of mmotm
before the temporary solution was merged, the last patch should be the missing
piece for reliability. The third patch restricts the hardened compaction to
non-costly orders, since costly orders don't result in OOMs in the first place.

Some preliminary testing suggested that this approach should work, but I would
like to ask all who experienced the regression to please retest this. You will
need to apply this series on top of tag mmotm-2016-08-31-16-06 from the mmotm
git tree [2]. Thanks in advance!

[1] http://marc.info/?i=20160822093249.GA14916%40dhcp22.suse.cz%3E
[2] git://git.kernel.org/pub/scm/linux/kernel/git/mhocko/mm.git

Vlastimil Babka (4):
  Revert "mm, oom: prevent premature OOM killer invocation for high
    order request"
  mm, compaction: more reliably increase direct compaction priority
  mm, compaction: restrict full priority to non-costly orders
  mm, compaction: make full priority ignore pageblock suitability

 include/linux/compaction.h |  1 +
 mm/compaction.c            | 11 ++++++---
 mm/internal.h              |  1 +
 mm/page_alloc.c            | 58 ++++++++++++++++++++++++++++++++++++++++++++--
 4 files changed, 66 insertions(+), 5 deletions(-)

-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* [PATCH 1/4] Revert "mm, oom: prevent premature OOM killer invocation for high order request"
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-06 13:52   ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

Commit 6b4e3181d7bd ("mm, oom: prevent premature OOM killer invocation for high
order request") was intended as a quick fix of OOM regressions for 4.8 and
stable 4.7.x kernels. For a better long-term solution, we still want to
consider compaction feedback, which should be possible after some more
improvements in the following patches.

This reverts commit 6b4e3181d7bd5ca5ab6f45929e4a5ffa7ab4ab7f.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ee3997859f14..1df7694f4ec7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3158,6 +3158,54 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
+static inline bool
+should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
+		     enum compact_result compact_result,
+		     enum compact_priority *compact_priority,
+		     int compaction_retries)
+{
+	int max_retries = MAX_COMPACT_RETRIES;
+
+	if (!order)
+		return false;
+
+	/*
+	 * compaction considers all the zone as desperately out of memory
+	 * so it doesn't really make much sense to retry except when the
+	 * failure could be caused by insufficient priority
+	 */
+	if (compaction_failed(compact_result)) {
+		if (*compact_priority > MIN_COMPACT_PRIORITY) {
+			(*compact_priority)--;
+			return true;
+		}
+		return false;
+	}
+
+	/*
+	 * make sure the compaction wasn't deferred or didn't bail out early
+	 * due to locks contention before we declare that we should give up.
+	 * But do not retry if the given zonelist is not suitable for
+	 * compaction.
+	 */
+	if (compaction_withdrawn(compact_result))
+		return compaction_zonelist_suitable(ac, order, alloc_flags);
+
+	/*
+	 * !costly requests are much more important than __GFP_REPEAT
+	 * costly ones because they are de facto nofail and invoke OOM
+	 * killer to move on while costly can fail and users are ready
+	 * to cope with that. 1/4 retries is rather arbitrary but we
+	 * would need much more detailed feedback from compaction to
+	 * make a better decision.
+	 */
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
+		max_retries /= 4;
+	if (compaction_retries <= max_retries)
+		return true;
+
+	return false;
+}
 #else
 static inline struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
@@ -3168,8 +3216,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
-#endif /* CONFIG_COMPACTION */
-
 static inline bool
 should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
 		     enum compact_result compact_result,
@@ -3196,6 +3242,7 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 	}
 	return false;
 }
+#endif /* CONFIG_COMPACTION */
 
 /* Perform direct synchronous page reclaim */
 static int
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 1/4] Revert "mm, oom: prevent premature OOM killer invocation for high order request"
@ 2016-09-06 13:52   ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

Commit 6b4e3181d7bd ("mm, oom: prevent premature OOM killer invocation for high
order request") was intended as a quick fix of OOM regressions for 4.8 and
stable 4.7.x kernels. For a better long-term solution, we still want to
consider compaction feedback, which should be possible after some more
improvements in the following patches.

This reverts commit 6b4e3181d7bd5ca5ab6f45929e4a5ffa7ab4ab7f.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index ee3997859f14..1df7694f4ec7 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3158,6 +3158,54 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
+static inline bool
+should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
+		     enum compact_result compact_result,
+		     enum compact_priority *compact_priority,
+		     int compaction_retries)
+{
+	int max_retries = MAX_COMPACT_RETRIES;
+
+	if (!order)
+		return false;
+
+	/*
+	 * compaction considers all the zone as desperately out of memory
+	 * so it doesn't really make much sense to retry except when the
+	 * failure could be caused by insufficient priority
+	 */
+	if (compaction_failed(compact_result)) {
+		if (*compact_priority > MIN_COMPACT_PRIORITY) {
+			(*compact_priority)--;
+			return true;
+		}
+		return false;
+	}
+
+	/*
+	 * make sure the compaction wasn't deferred or didn't bail out early
+	 * due to locks contention before we declare that we should give up.
+	 * But do not retry if the given zonelist is not suitable for
+	 * compaction.
+	 */
+	if (compaction_withdrawn(compact_result))
+		return compaction_zonelist_suitable(ac, order, alloc_flags);
+
+	/*
+	 * !costly requests are much more important than __GFP_REPEAT
+	 * costly ones because they are de facto nofail and invoke OOM
+	 * killer to move on while costly can fail and users are ready
+	 * to cope with that. 1/4 retries is rather arbitrary but we
+	 * would need much more detailed feedback from compaction to
+	 * make a better decision.
+	 */
+	if (order > PAGE_ALLOC_COSTLY_ORDER)
+		max_retries /= 4;
+	if (compaction_retries <= max_retries)
+		return true;
+
+	return false;
+}
 #else
 static inline struct page *
 __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
@@ -3168,8 +3216,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
 	return NULL;
 }
 
-#endif /* CONFIG_COMPACTION */
-
 static inline bool
 should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
 		     enum compact_result compact_result,
@@ -3196,6 +3242,7 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
 	}
 	return false;
 }
+#endif /* CONFIG_COMPACTION */
 
 /* Perform direct synchronous page reclaim */
 static int
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-06 13:52   ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

During reclaim/compaction loop, compaction priority can be increased by the
should_compact_retry() function, but the current code is not optimal. Priority
is only increased when compaction_failed() is true, which means that compaction
has scanned the whole zone. This may not happen even after multiple attempts
with a lower priority due to parallel activity, so we might needlessly
struggle on the lower priorities and possibly run out of compaction retry
attempts in the process.

After this patch we are guaranteed at least one attempt at the highest
compaction priority even if we exhaust all retries at the lower priorities.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/page_alloc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1df7694f4ec7..f8bed910e3cf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3174,13 +3174,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * so it doesn't really make much sense to retry except when the
 	 * failure could be caused by insufficient priority
 	 */
-	if (compaction_failed(compact_result)) {
-		if (*compact_priority > MIN_COMPACT_PRIORITY) {
-			(*compact_priority)--;
-			return true;
-		}
-		return false;
-	}
+	if (compaction_failed(compact_result))
+		goto check_priority;
 
 	/*
 	 * make sure the compaction wasn't deferred or didn't bail out early
@@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	if (compaction_retries <= max_retries)
 		return true;
 
+	/*
+	 * Make sure there is at least one attempt at the highest priority
+	 * if we exhausted all retries at the lower priorities
+	 */
+check_priority:
+	if (*compact_priority > MIN_COMPACT_PRIORITY) {
+		(*compact_priority)--;
+		return true;
+	}
 	return false;
 }
 #else
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-06 13:52   ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

During reclaim/compaction loop, compaction priority can be increased by the
should_compact_retry() function, but the current code is not optimal. Priority
is only increased when compaction_failed() is true, which means that compaction
has scanned the whole zone. This may not happen even after multiple attempts
with a lower priority due to parallel activity, so we might needlessly
struggle on the lower priorities and possibly run out of compaction retry
attempts in the process.

After this patch we are guaranteed at least one attempt at the highest
compaction priority even if we exhaust all retries at the lower priorities.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/page_alloc.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 1df7694f4ec7..f8bed910e3cf 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3174,13 +3174,8 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * so it doesn't really make much sense to retry except when the
 	 * failure could be caused by insufficient priority
 	 */
-	if (compaction_failed(compact_result)) {
-		if (*compact_priority > MIN_COMPACT_PRIORITY) {
-			(*compact_priority)--;
-			return true;
-		}
-		return false;
-	}
+	if (compaction_failed(compact_result))
+		goto check_priority;
 
 	/*
 	 * make sure the compaction wasn't deferred or didn't bail out early
@@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	if (compaction_retries <= max_retries)
 		return true;
 
+	/*
+	 * Make sure there is at least one attempt at the highest priority
+	 * if we exhausted all retries at the lower priorities
+	 */
+check_priority:
+	if (*compact_priority > MIN_COMPACT_PRIORITY) {
+		(*compact_priority)--;
+		return true;
+	}
 	return false;
 }
 #else
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 3/4] mm, compaction: restrict full priority to non-costly orders
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-06 13:52   ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

The new ultimate compaction priority disables some heuristics, which may result
in excessive cost. This is fine for non-costly orders where we want to try hard
before resulting for OOM, but might be disruptive for costly orders which do
not trigger OOM and should generally have some fallback. Thus, we disable the
full priority for costly orders.

Suggested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 include/linux/compaction.h | 1 +
 mm/page_alloc.c            | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index 585d55cb0dc0..0d8415820fc3 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -9,6 +9,7 @@ enum compact_priority {
 	COMPACT_PRIO_SYNC_FULL,
 	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
 	COMPACT_PRIO_SYNC_LIGHT,
+	MIN_COMPACT_COSTLY_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
 	DEF_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
 	COMPACT_PRIO_ASYNC,
 	INIT_COMPACT_PRIORITY = COMPACT_PRIO_ASYNC
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f8bed910e3cf..ff60a2837c58 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3165,6 +3165,7 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 		     int compaction_retries)
 {
 	int max_retries = MAX_COMPACT_RETRIES;
+	int min_priority;
 
 	if (!order)
 		return false;
@@ -3204,7 +3205,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * if we exhausted all retries at the lower priorities
 	 */
 check_priority:
-	if (*compact_priority > MIN_COMPACT_PRIORITY) {
+	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
+			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
+	if (*compact_priority > min_priority) {
 		(*compact_priority)--;
 		return true;
 	}
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 3/4] mm, compaction: restrict full priority to non-costly orders
@ 2016-09-06 13:52   ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

The new ultimate compaction priority disables some heuristics, which may result
in excessive cost. This is fine for non-costly orders where we want to try hard
before resulting for OOM, but might be disruptive for costly orders which do
not trigger OOM and should generally have some fallback. Thus, we disable the
full priority for costly orders.

Suggested-by: Michal Hocko <mhocko@kernel.org>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 include/linux/compaction.h | 1 +
 mm/page_alloc.c            | 5 ++++-
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index 585d55cb0dc0..0d8415820fc3 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -9,6 +9,7 @@ enum compact_priority {
 	COMPACT_PRIO_SYNC_FULL,
 	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
 	COMPACT_PRIO_SYNC_LIGHT,
+	MIN_COMPACT_COSTLY_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
 	DEF_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
 	COMPACT_PRIO_ASYNC,
 	INIT_COMPACT_PRIORITY = COMPACT_PRIO_ASYNC
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f8bed910e3cf..ff60a2837c58 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3165,6 +3165,7 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 		     int compaction_retries)
 {
 	int max_retries = MAX_COMPACT_RETRIES;
+	int min_priority;
 
 	if (!order)
 		return false;
@@ -3204,7 +3205,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 * if we exhausted all retries at the lower priorities
 	 */
 check_priority:
-	if (*compact_priority > MIN_COMPACT_PRIORITY) {
+	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
+			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
+	if (*compact_priority > min_priority) {
 		(*compact_priority)--;
 		return true;
 	}
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 4/4] mm, compaction: make full priority ignore pageblock suitability
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-06 13:52   ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

Several people have reported premature OOMs for order-2 allocations (stack)
due to OOM rework in 4.7. In the scenario (parallel kernel build and dd writing
to two drives) many pageblocks get marked as Unmovable and compaction free
scanner struggles to isolate free pages. Joonsoo Kim pointed out that the free
scanner skips pageblocks that are not movable to prevent filling them and
forcing non-movable allocations to fallback to other pageblocks. Such heuristic
makes sense to help prevent long-term fragmentation, but premature OOMs are
relatively more urgent problem. As a compromise, this patch disables the
heuristic only for the ultimate compaction priority.

Reported-by: Ralf-Peter Rohbeck <Ralf-Peter.Rohbeck@quantum.com>
Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Reported-by: Olaf Hering <olaf@aepfle.de>
Suggested-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 11 ++++++++---
 mm/internal.h   |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 29f6c49dc9c2..86d4d0bbfc7c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -997,8 +997,12 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 #ifdef CONFIG_COMPACTION
 
 /* Returns true if the page is within a block suitable for migration to */
-static bool suitable_migration_target(struct page *page)
+static bool suitable_migration_target(struct compact_control *cc,
+							struct page *page)
 {
+	if (cc->ignore_block_suitable)
+		return true;
+
 	/* If the page is a large free page, then disallow migration */
 	if (PageBuddy(page)) {
 		/*
@@ -1083,7 +1087,7 @@ static void isolate_freepages(struct compact_control *cc)
 			continue;
 
 		/* Check the block is suitable for migration */
-		if (!suitable_migration_target(page))
+		if (!suitable_migration_target(cc, page))
 			continue;
 
 		/* If isolation recently failed, do not retry */
@@ -1656,7 +1660,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
 		.classzone_idx = classzone_idx,
 		.direct_compaction = true,
 		.whole_zone = (prio == MIN_COMPACT_PRIORITY),
-		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY)
+		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY),
+		.ignore_block_suitable = (prio == MIN_COMPACT_PRIORITY)
 	};
 	INIT_LIST_HEAD(&cc.freepages);
 	INIT_LIST_HEAD(&cc.migratepages);
diff --git a/mm/internal.h b/mm/internal.h
index 5214bf8e3171..537ac9951f5f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -178,6 +178,7 @@ struct compact_control {
 	unsigned long last_migrated_pfn;/* Not yet flushed page being freed */
 	enum migrate_mode mode;		/* Async or sync migration mode */
 	bool ignore_skip_hint;		/* Scan blocks even if marked skip */
+	bool ignore_block_suitable;	/* Scan blocks considered unsuitable */
 	bool direct_compaction;		/* False from kcompactd or /proc/... */
 	bool whole_zone;		/* Whole zone should/has been scanned */
 	int order;			/* order a direct compactor needs */
-- 
2.9.3

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* [PATCH 4/4] mm, compaction: make full priority ignore pageblock suitability
@ 2016-09-06 13:52   ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-06 13:52 UTC (permalink / raw)
  To: Michal Hocko, Andrew Morton, Arkadiusz Miskiewicz,
	Ralf-Peter Rohbeck, Olaf Hering
  Cc: linux-kernel, Linus Torvalds, linux-mm, Vlastimil Babka,
	Mel Gorman, Joonsoo Kim, David Rientjes, Rik van Riel,
	Michal Hocko

Several people have reported premature OOMs for order-2 allocations (stack)
due to OOM rework in 4.7. In the scenario (parallel kernel build and dd writing
to two drives) many pageblocks get marked as Unmovable and compaction free
scanner struggles to isolate free pages. Joonsoo Kim pointed out that the free
scanner skips pageblocks that are not movable to prevent filling them and
forcing non-movable allocations to fallback to other pageblocks. Such heuristic
makes sense to help prevent long-term fragmentation, but premature OOMs are
relatively more urgent problem. As a compromise, this patch disables the
heuristic only for the ultimate compaction priority.

Reported-by: Ralf-Peter Rohbeck <Ralf-Peter.Rohbeck@quantum.com>
Reported-by: Arkadiusz Miskiewicz <a.miskiewicz@gmail.com>
Reported-by: Olaf Hering <olaf@aepfle.de>
Suggested-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
Cc: Michal Hocko <mhocko@kernel.org>
Cc: Mel Gorman <mgorman@techsingularity.net>
Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Cc: David Rientjes <rientjes@google.com>
Cc: Rik van Riel <riel@redhat.com>
---
 mm/compaction.c | 11 ++++++++---
 mm/internal.h   |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index 29f6c49dc9c2..86d4d0bbfc7c 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -997,8 +997,12 @@ isolate_migratepages_range(struct compact_control *cc, unsigned long start_pfn,
 #ifdef CONFIG_COMPACTION
 
 /* Returns true if the page is within a block suitable for migration to */
-static bool suitable_migration_target(struct page *page)
+static bool suitable_migration_target(struct compact_control *cc,
+							struct page *page)
 {
+	if (cc->ignore_block_suitable)
+		return true;
+
 	/* If the page is a large free page, then disallow migration */
 	if (PageBuddy(page)) {
 		/*
@@ -1083,7 +1087,7 @@ static void isolate_freepages(struct compact_control *cc)
 			continue;
 
 		/* Check the block is suitable for migration */
-		if (!suitable_migration_target(page))
+		if (!suitable_migration_target(cc, page))
 			continue;
 
 		/* If isolation recently failed, do not retry */
@@ -1656,7 +1660,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
 		.classzone_idx = classzone_idx,
 		.direct_compaction = true,
 		.whole_zone = (prio == MIN_COMPACT_PRIORITY),
-		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY)
+		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY),
+		.ignore_block_suitable = (prio == MIN_COMPACT_PRIORITY)
 	};
 	INIT_LIST_HEAD(&cc.freepages);
 	INIT_LIST_HEAD(&cc.migratepages);
diff --git a/mm/internal.h b/mm/internal.h
index 5214bf8e3171..537ac9951f5f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -178,6 +178,7 @@ struct compact_control {
 	unsigned long last_migrated_pfn;/* Not yet flushed page being freed */
 	enum migrate_mode mode;		/* Async or sync migration mode */
 	bool ignore_skip_hint;		/* Scan blocks even if marked skip */
+	bool ignore_block_suitable;	/* Scan blocks considered unsuitable */
 	bool direct_compaction;		/* False from kcompactd or /proc/... */
 	bool whole_zone;		/* Whole zone should/has been scanned */
 	int order;			/* order a direct compactor needs */
-- 
2.9.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-15 18:51   ` Arkadiusz Miskiewicz
  -1 siblings, 0 replies; 48+ messages in thread
From: Arkadiusz Miskiewicz @ 2016-09-15 18:51 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Michal Hocko, Andrew Morton, Ralf-Peter Rohbeck, Olaf Hering,
	linux-kernel, Linus Torvalds, linux-mm, David Rientjes,
	Joonsoo Kim, Mel Gorman, Michal Hocko, Rik van Riel

On Tuesday 06 of September 2016, Vlastimil Babka wrote:
> After several people reported OOM's for order-2 allocations in 4.7 due to
> Michal Hocko's OOM rework, he reverted the part that considered compaction
> feedback [1] in the decisions to retry reclaim/compaction. This was to
> provide a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an
> almost complete solution that instead improved compaction reliability.
> 
> This series completes the mmotm solution and reintroduces the compaction
> feedback into OOM decisions. The first two patches restore the state of
> mmotm before the temporary solution was merged, the last patch should be
> the missing piece for reliability. The third patch restricts the hardened
> compaction to non-costly orders, since costly orders don't result in OOMs
> in the first place.
> 
> Some preliminary testing suggested that this approach should work, but I
> would like to ask all who experienced the regression to please retest
> this. You will need to apply this series on top of tag
> mmotm-2016-08-31-16-06 from the mmotm git tree [2]. Thanks in advance!

My "rm -rf copyX; cp -al org copyX" test x10 in parallel worked without any 
OOM.


-- 
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-15 18:51   ` Arkadiusz Miskiewicz
  0 siblings, 0 replies; 48+ messages in thread
From: Arkadiusz Miskiewicz @ 2016-09-15 18:51 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Michal Hocko, Andrew Morton, Ralf-Peter Rohbeck, Olaf Hering,
	linux-kernel, Linus Torvalds, linux-mm, David Rientjes,
	Joonsoo Kim, Mel Gorman, Michal Hocko, Rik van Riel

On Tuesday 06 of September 2016, Vlastimil Babka wrote:
> After several people reported OOM's for order-2 allocations in 4.7 due to
> Michal Hocko's OOM rework, he reverted the part that considered compaction
> feedback [1] in the decisions to retry reclaim/compaction. This was to
> provide a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an
> almost complete solution that instead improved compaction reliability.
> 
> This series completes the mmotm solution and reintroduces the compaction
> feedback into OOM decisions. The first two patches restore the state of
> mmotm before the temporary solution was merged, the last patch should be
> the missing piece for reliability. The third patch restricts the hardened
> compaction to non-costly orders, since costly orders don't result in OOMs
> in the first place.
> 
> Some preliminary testing suggested that this approach should work, but I
> would like to ask all who experienced the regression to please retest
> this. You will need to apply this series on top of tag
> mmotm-2016-08-31-16-06 from the mmotm git tree [2]. Thanks in advance!

My "rm -rf copyX; cp -al org copyX" test x10 in parallel worked without any 
OOM.


-- 
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 1/4] Revert "mm, oom: prevent premature OOM killer invocation for high order request"
  2016-09-06 13:52   ` Vlastimil Babka
@ 2016-09-21 17:04     ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:04 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:55, Vlastimil Babka wrote:
> Commit 6b4e3181d7bd ("mm, oom: prevent premature OOM killer invocation for high
> order request") was intended as a quick fix of OOM regressions for 4.8 and
> stable 4.7.x kernels. For a better long-term solution, we still want to
> consider compaction feedback, which should be possible after some more
> improvements in the following patches.
> 
> This reverts commit 6b4e3181d7bd5ca5ab6f45929e4a5ffa7ab4ab7f.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>

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

> ---
>  mm/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ee3997859f14..1df7694f4ec7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3158,6 +3158,54 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> +static inline bool
> +should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> +		     enum compact_result compact_result,
> +		     enum compact_priority *compact_priority,
> +		     int compaction_retries)
> +{
> +	int max_retries = MAX_COMPACT_RETRIES;
> +
> +	if (!order)
> +		return false;
> +
> +	/*
> +	 * compaction considers all the zone as desperately out of memory
> +	 * so it doesn't really make much sense to retry except when the
> +	 * failure could be caused by insufficient priority
> +	 */
> +	if (compaction_failed(compact_result)) {
> +		if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +			(*compact_priority)--;
> +			return true;
> +		}
> +		return false;
> +	}
> +
> +	/*
> +	 * make sure the compaction wasn't deferred or didn't bail out early
> +	 * due to locks contention before we declare that we should give up.
> +	 * But do not retry if the given zonelist is not suitable for
> +	 * compaction.
> +	 */
> +	if (compaction_withdrawn(compact_result))
> +		return compaction_zonelist_suitable(ac, order, alloc_flags);
> +
> +	/*
> +	 * !costly requests are much more important than __GFP_REPEAT
> +	 * costly ones because they are de facto nofail and invoke OOM
> +	 * killer to move on while costly can fail and users are ready
> +	 * to cope with that. 1/4 retries is rather arbitrary but we
> +	 * would need much more detailed feedback from compaction to
> +	 * make a better decision.
> +	 */
> +	if (order > PAGE_ALLOC_COSTLY_ORDER)
> +		max_retries /= 4;
> +	if (compaction_retries <= max_retries)
> +		return true;
> +
> +	return false;
> +}
>  #else
>  static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> @@ -3168,8 +3216,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> -#endif /* CONFIG_COMPACTION */
> -
>  static inline bool
>  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
>  		     enum compact_result compact_result,
> @@ -3196,6 +3242,7 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>  	}
>  	return false;
>  }
> +#endif /* CONFIG_COMPACTION */
>  
>  /* Perform direct synchronous page reclaim */
>  static int
> -- 
> 2.9.3

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 1/4] Revert "mm, oom: prevent premature OOM killer invocation for high order request"
@ 2016-09-21 17:04     ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:04 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:55, Vlastimil Babka wrote:
> Commit 6b4e3181d7bd ("mm, oom: prevent premature OOM killer invocation for high
> order request") was intended as a quick fix of OOM regressions for 4.8 and
> stable 4.7.x kernels. For a better long-term solution, we still want to
> consider compaction feedback, which should be possible after some more
> improvements in the following patches.
> 
> This reverts commit 6b4e3181d7bd5ca5ab6f45929e4a5ffa7ab4ab7f.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>

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

> ---
>  mm/page_alloc.c | 51 +++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 49 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index ee3997859f14..1df7694f4ec7 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3158,6 +3158,54 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> +static inline bool
> +should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> +		     enum compact_result compact_result,
> +		     enum compact_priority *compact_priority,
> +		     int compaction_retries)
> +{
> +	int max_retries = MAX_COMPACT_RETRIES;
> +
> +	if (!order)
> +		return false;
> +
> +	/*
> +	 * compaction considers all the zone as desperately out of memory
> +	 * so it doesn't really make much sense to retry except when the
> +	 * failure could be caused by insufficient priority
> +	 */
> +	if (compaction_failed(compact_result)) {
> +		if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +			(*compact_priority)--;
> +			return true;
> +		}
> +		return false;
> +	}
> +
> +	/*
> +	 * make sure the compaction wasn't deferred or didn't bail out early
> +	 * due to locks contention before we declare that we should give up.
> +	 * But do not retry if the given zonelist is not suitable for
> +	 * compaction.
> +	 */
> +	if (compaction_withdrawn(compact_result))
> +		return compaction_zonelist_suitable(ac, order, alloc_flags);
> +
> +	/*
> +	 * !costly requests are much more important than __GFP_REPEAT
> +	 * costly ones because they are de facto nofail and invoke OOM
> +	 * killer to move on while costly can fail and users are ready
> +	 * to cope with that. 1/4 retries is rather arbitrary but we
> +	 * would need much more detailed feedback from compaction to
> +	 * make a better decision.
> +	 */
> +	if (order > PAGE_ALLOC_COSTLY_ORDER)
> +		max_retries /= 4;
> +	if (compaction_retries <= max_retries)
> +		return true;
> +
> +	return false;
> +}
>  #else
>  static inline struct page *
>  __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
> @@ -3168,8 +3216,6 @@ __alloc_pages_direct_compact(gfp_t gfp_mask, unsigned int order,
>  	return NULL;
>  }
>  
> -#endif /* CONFIG_COMPACTION */
> -
>  static inline bool
>  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
>  		     enum compact_result compact_result,
> @@ -3196,6 +3242,7 @@ should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_fla
>  	}
>  	return false;
>  }
> +#endif /* CONFIG_COMPACTION */
>  
>  /* Perform direct synchronous page reclaim */
>  static int
> -- 
> 2.9.3

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-06 13:52   ` Vlastimil Babka
@ 2016-09-21 17:13     ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:13 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:56, Vlastimil Babka wrote:
[...]
> @@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	if (compaction_retries <= max_retries)
>  		return true;
>  
> +	/*
> +	 * Make sure there is at least one attempt at the highest priority
> +	 * if we exhausted all retries at the lower priorities
> +	 */
> +check_priority:
> +	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +		(*compact_priority)--;
> +		return true;

Don't we want to reset compaction_retries here? Otherwise we can consume
all retries on the lower priorities.

Other than that it looks good to me. With that you can add
Acked-by: Michal Hocko <mhocko@suse.com>

> +	}
>  	return false;
>  }
>  #else
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-21 17:13     ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:13 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:56, Vlastimil Babka wrote:
[...]
> @@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	if (compaction_retries <= max_retries)
>  		return true;
>  
> +	/*
> +	 * Make sure there is at least one attempt at the highest priority
> +	 * if we exhausted all retries at the lower priorities
> +	 */
> +check_priority:
> +	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +		(*compact_priority)--;
> +		return true;

Don't we want to reset compaction_retries here? Otherwise we can consume
all retries on the lower priorities.

Other than that it looks good to me. With that you can add
Acked-by: Michal Hocko <mhocko@suse.com>

> +	}
>  	return false;
>  }
>  #else
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/4] mm, compaction: restrict full priority to non-costly orders
  2016-09-06 13:52   ` Vlastimil Babka
@ 2016-09-21 17:15     ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:15 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:57, Vlastimil Babka wrote:
> The new ultimate compaction priority disables some heuristics, which may result
> in excessive cost. This is fine for non-costly orders where we want to try hard
> before resulting for OOM, but might be disruptive for costly orders which do
> not trigger OOM and should generally have some fallback. Thus, we disable the
> full priority for costly orders.
> 
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>

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

> ---
>  include/linux/compaction.h | 1 +
>  mm/page_alloc.c            | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index 585d55cb0dc0..0d8415820fc3 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -9,6 +9,7 @@ enum compact_priority {
>  	COMPACT_PRIO_SYNC_FULL,
>  	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
>  	COMPACT_PRIO_SYNC_LIGHT,
> +	MIN_COMPACT_COSTLY_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
>  	DEF_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
>  	COMPACT_PRIO_ASYNC,
>  	INIT_COMPACT_PRIORITY = COMPACT_PRIO_ASYNC
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f8bed910e3cf..ff60a2837c58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3165,6 +3165,7 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  		     int compaction_retries)
>  {
>  	int max_retries = MAX_COMPACT_RETRIES;
> +	int min_priority;
>  
>  	if (!order)
>  		return false;
> @@ -3204,7 +3205,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	 * if we exhausted all retries at the lower priorities
>  	 */
>  check_priority:
> -	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
> +			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
> +	if (*compact_priority > min_priority) {
>  		(*compact_priority)--;
>  		return true;
>  	}
> -- 
> 2.9.3

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 3/4] mm, compaction: restrict full priority to non-costly orders
@ 2016-09-21 17:15     ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:15 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Tue 06-09-16 15:52:57, Vlastimil Babka wrote:
> The new ultimate compaction priority disables some heuristics, which may result
> in excessive cost. This is fine for non-costly orders where we want to try hard
> before resulting for OOM, but might be disruptive for costly orders which do
> not trigger OOM and should generally have some fallback. Thus, we disable the
> full priority for costly orders.
> 
> Suggested-by: Michal Hocko <mhocko@kernel.org>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Cc: Michal Hocko <mhocko@kernel.org>
> Cc: Mel Gorman <mgorman@techsingularity.net>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Rik van Riel <riel@redhat.com>

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

> ---
>  include/linux/compaction.h | 1 +
>  mm/page_alloc.c            | 5 ++++-
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index 585d55cb0dc0..0d8415820fc3 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -9,6 +9,7 @@ enum compact_priority {
>  	COMPACT_PRIO_SYNC_FULL,
>  	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
>  	COMPACT_PRIO_SYNC_LIGHT,
> +	MIN_COMPACT_COSTLY_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
>  	DEF_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_LIGHT,
>  	COMPACT_PRIO_ASYNC,
>  	INIT_COMPACT_PRIORITY = COMPACT_PRIO_ASYNC
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f8bed910e3cf..ff60a2837c58 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3165,6 +3165,7 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  		     int compaction_retries)
>  {
>  	int max_retries = MAX_COMPACT_RETRIES;
> +	int min_priority;
>  
>  	if (!order)
>  		return false;
> @@ -3204,7 +3205,9 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	 * if we exhausted all retries at the lower priorities
>  	 */
>  check_priority:
> -	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> +	min_priority = (order > PAGE_ALLOC_COSTLY_ORDER) ?
> +			MIN_COMPACT_COSTLY_PRIORITY : MIN_COMPACT_PRIORITY;
> +	if (*compact_priority > min_priority) {
>  		(*compact_priority)--;
>  		return true;
>  	}
> -- 
> 2.9.3

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-06 13:52 ` Vlastimil Babka
@ 2016-09-21 17:18   ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:18 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> After several people reported OOM's for order-2 allocations in 4.7 due to
> Michal Hocko's OOM rework, he reverted the part that considered compaction
> feedback [1] in the decisions to retry reclaim/compaction. This was to provide
> a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an almost
> complete solution that instead improved compaction reliability.
> 
> This series completes the mmotm solution and reintroduces the compaction
> feedback into OOM decisions. The first two patches restore the state of mmotm
> before the temporary solution was merged, the last patch should be the missing
> piece for reliability. The third patch restricts the hardened compaction to
> non-costly orders, since costly orders don't result in OOMs in the first place.
> 
> Some preliminary testing suggested that this approach should work, but I would
> like to ask all who experienced the regression to please retest this. You will
> need to apply this series on top of tag mmotm-2016-08-31-16-06 from the mmotm
> git tree [2]. Thanks in advance!

We still do not ignore fragindex in the full priority. This part has
always been quite unclear to me so I cannot really tell whether that
makes any difference or not but just to be on the safe side I would
preffer to have _all_ the shortcuts out of the way in the highest
priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
so keep retrying but still a complication to understand the workflow.

What do you think?
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-21 17:18   ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-21 17:18 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> After several people reported OOM's for order-2 allocations in 4.7 due to
> Michal Hocko's OOM rework, he reverted the part that considered compaction
> feedback [1] in the decisions to retry reclaim/compaction. This was to provide
> a fix quickly for 4.8 rc and 4.7 stable series, while mmotm had an almost
> complete solution that instead improved compaction reliability.
> 
> This series completes the mmotm solution and reintroduces the compaction
> feedback into OOM decisions. The first two patches restore the state of mmotm
> before the temporary solution was merged, the last patch should be the missing
> piece for reliability. The third patch restricts the hardened compaction to
> non-costly orders, since costly orders don't result in OOMs in the first place.
> 
> Some preliminary testing suggested that this approach should work, but I would
> like to ask all who experienced the regression to please retest this. You will
> need to apply this series on top of tag mmotm-2016-08-31-16-06 from the mmotm
> git tree [2]. Thanks in advance!

We still do not ignore fragindex in the full priority. This part has
always been quite unclear to me so I cannot really tell whether that
makes any difference or not but just to be on the safe side I would
preffer to have _all_ the shortcuts out of the way in the highest
priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
so keep retrying but still a complication to understand the workflow.

What do you think?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-21 17:13     ` Michal Hocko
@ 2016-09-22 12:51       ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 12:51 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/21/2016 07:13 PM, Michal Hocko wrote:
> On Tue 06-09-16 15:52:56, Vlastimil Babka wrote:
> [...]
>> @@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>>  	if (compaction_retries <= max_retries)
>>  		return true;
>>  
>> +	/*
>> +	 * Make sure there is at least one attempt at the highest priority
>> +	 * if we exhausted all retries at the lower priorities
>> +	 */
>> +check_priority:
>> +	if (*compact_priority > MIN_COMPACT_PRIORITY) {
>> +		(*compact_priority)--;
>> +		return true;
> 
> Don't we want to reset compaction_retries here? Otherwise we can consume
> all retries on the lower priorities.

Good point, patch-fix below.

> Other than that it looks good to me. With that you can add
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
 
>> +	}
>>  	return false;
>>  }
>>  #else
> 

----8<----
>From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 22 Sep 2016 13:54:32 +0200
Subject: [PATCH] mm, compaction: more reliably increase direct compaction
 priority-fix

When increasing the compaction priority, also reset retries. Otherwise we can
consume all retries on the lower priorities.

Suggested-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f8bed910e3cf..82fdb690ac62 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3162,7 +3162,7 @@ static inline bool
 should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 		     enum compact_result compact_result,
 		     enum compact_priority *compact_priority,
-		     int compaction_retries)
+		     int *compaction_retries)
 {
 	int max_retries = MAX_COMPACT_RETRIES;
 
@@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 */
 	if (order > PAGE_ALLOC_COSTLY_ORDER)
 		max_retries /= 4;
-	if (compaction_retries <= max_retries)
+	if (*compaction_retries <= max_retries)
 		return true;
 
 	/*
-	 * Make sure there is at least one attempt at the highest priority
-	 * if we exhausted all retries at the lower priorities
+	 * Make sure there are attempts at the highest priority if we exhausted
+	 * all retries or failed at the lower priorities.
 	 */
 check_priority:
 	if (*compact_priority > MIN_COMPACT_PRIORITY) {
 		(*compact_priority)--;
+		*compaction_retries = 0;
 		return true;
 	}
 	return false;
@@ -3224,7 +3225,7 @@ static inline bool
 should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
 		     enum compact_result compact_result,
 		     enum compact_priority *compact_priority,
-		     int compaction_retries)
+		     int *compaction_retries)
 {
 	struct zone *zone;
 	struct zoneref *z;
@@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (did_some_progress > 0 &&
 			should_compact_retry(ac, order, alloc_flags,
 				compact_result, &compact_priority,
-				compaction_retries))
+				&compaction_retries))
 		goto retry;
 
 	/* Reclaim has failed us, start killing things */
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-22 12:51       ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 12:51 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/21/2016 07:13 PM, Michal Hocko wrote:
> On Tue 06-09-16 15:52:56, Vlastimil Babka wrote:
> [...]
>> @@ -3204,6 +3199,15 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>>  	if (compaction_retries <= max_retries)
>>  		return true;
>>  
>> +	/*
>> +	 * Make sure there is at least one attempt at the highest priority
>> +	 * if we exhausted all retries at the lower priorities
>> +	 */
>> +check_priority:
>> +	if (*compact_priority > MIN_COMPACT_PRIORITY) {
>> +		(*compact_priority)--;
>> +		return true;
> 
> Don't we want to reset compaction_retries here? Otherwise we can consume
> all retries on the lower priorities.

Good point, patch-fix below.

> Other than that it looks good to me. With that you can add
> Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!
 
>> +	}
>>  	return false;
>>  }
>>  #else
> 

----8<----

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-22 12:51       ` Vlastimil Babka
@ 2016-09-22 14:08         ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-22 14:08 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 13:54:32 +0200
> Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>  priority-fix
> 
> When increasing the compaction priority, also reset retries. Otherwise we can
> consume all retries on the lower priorities.

OK, this is an improvement. I am just thinking that we might want to
pull
	if (order && compaction_made_progress(compact_result))
		compaction_retries++;

into should_compact_retry as well. I've had it there originally because
it was in line with no_progress_loops but now that we have compaction
priorities it would fit into retry logic better. As a plus it would
count only those compaction rounds where we we didn't have to rely on
the compaction retry logic. What do you think?

> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Anyway
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f8bed910e3cf..82fdb690ac62 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3162,7 +3162,7 @@ static inline bool
>  should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  		     enum compact_result compact_result,
>  		     enum compact_priority *compact_priority,
> -		     int compaction_retries)
> +		     int *compaction_retries)
>  {
>  	int max_retries = MAX_COMPACT_RETRIES;
>  
> @@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	 */
>  	if (order > PAGE_ALLOC_COSTLY_ORDER)
>  		max_retries /= 4;
> -	if (compaction_retries <= max_retries)
> +	if (*compaction_retries <= max_retries)
>  		return true;
>  
>  	/*
> -	 * Make sure there is at least one attempt at the highest priority
> -	 * if we exhausted all retries at the lower priorities
> +	 * Make sure there are attempts at the highest priority if we exhausted
> +	 * all retries or failed at the lower priorities.
>  	 */
>  check_priority:
>  	if (*compact_priority > MIN_COMPACT_PRIORITY) {
>  		(*compact_priority)--;
> +		*compaction_retries = 0;
>  		return true;
>  	}
>  	return false;
> @@ -3224,7 +3225,7 @@ static inline bool
>  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
>  		     enum compact_result compact_result,
>  		     enum compact_priority *compact_priority,
> -		     int compaction_retries)
> +		     int *compaction_retries)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
> @@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (did_some_progress > 0 &&
>  			should_compact_retry(ac, order, alloc_flags,
>  				compact_result, &compact_priority,
> -				compaction_retries))
> +				&compaction_retries))
>  		goto retry;
>  
>  	/* Reclaim has failed us, start killing things */
> -- 
> 2.10.0
> 

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-22 14:08         ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-22 14:08 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 13:54:32 +0200
> Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>  priority-fix
> 
> When increasing the compaction priority, also reset retries. Otherwise we can
> consume all retries on the lower priorities.

OK, this is an improvement. I am just thinking that we might want to
pull
	if (order && compaction_made_progress(compact_result))
		compaction_retries++;

into should_compact_retry as well. I've had it there originally because
it was in line with no_progress_loops but now that we have compaction
priorities it would fit into retry logic better. As a plus it would
count only those compaction rounds where we we didn't have to rely on
the compaction retry logic. What do you think?

> Suggested-by: Michal Hocko <mhocko@suse.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

Anyway
Acked-by: Michal Hocko <mhocko@suse.com>

> ---
>  mm/page_alloc.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index f8bed910e3cf..82fdb690ac62 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3162,7 +3162,7 @@ static inline bool
>  should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  		     enum compact_result compact_result,
>  		     enum compact_priority *compact_priority,
> -		     int compaction_retries)
> +		     int *compaction_retries)
>  {
>  	int max_retries = MAX_COMPACT_RETRIES;
>  
> @@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
>  	 */
>  	if (order > PAGE_ALLOC_COSTLY_ORDER)
>  		max_retries /= 4;
> -	if (compaction_retries <= max_retries)
> +	if (*compaction_retries <= max_retries)
>  		return true;
>  
>  	/*
> -	 * Make sure there is at least one attempt at the highest priority
> -	 * if we exhausted all retries at the lower priorities
> +	 * Make sure there are attempts at the highest priority if we exhausted
> +	 * all retries or failed at the lower priorities.
>  	 */
>  check_priority:
>  	if (*compact_priority > MIN_COMPACT_PRIORITY) {
>  		(*compact_priority)--;
> +		*compaction_retries = 0;
>  		return true;
>  	}
>  	return false;
> @@ -3224,7 +3225,7 @@ static inline bool
>  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
>  		     enum compact_result compact_result,
>  		     enum compact_priority *compact_priority,
> -		     int compaction_retries)
> +		     int *compaction_retries)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
> @@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (did_some_progress > 0 &&
>  			should_compact_retry(ac, order, alloc_flags,
>  				compact_result, &compact_priority,
> -				compaction_retries))
> +				&compaction_retries))
>  		goto retry;
>  
>  	/* Reclaim has failed us, start killing things */
> -- 
> 2.10.0
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-22 14:08         ` Michal Hocko
@ 2016-09-22 14:52           ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-22 14:52 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Thu 22-09-16 16:08:21, Michal Hocko wrote:
> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> > From: Vlastimil Babka <vbabka@suse.cz>
> > Date: Thu, 22 Sep 2016 13:54:32 +0200
> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
> >  priority-fix
> > 
> > When increasing the compaction priority, also reset retries. Otherwise we can
> > consume all retries on the lower priorities.
> 
> OK, this is an improvement. I am just thinking that we might want to
> pull
> 	if (order && compaction_made_progress(compact_result))
> 		compaction_retries++;
> 
> into should_compact_retry as well. I've had it there originally because
> it was in line with no_progress_loops but now that we have compaction
> priorities it would fit into retry logic better. As a plus it would
> count only those compaction rounds where we we didn't have to rely on
                                                 did that should be

> the compaction retry logic. What do you think?
> 
> > Suggested-by: Michal Hocko <mhocko@suse.com>
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Anyway
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> > ---
> >  mm/page_alloc.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index f8bed910e3cf..82fdb690ac62 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3162,7 +3162,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	int max_retries = MAX_COMPACT_RETRIES;
> >  
> > @@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  	 */
> >  	if (order > PAGE_ALLOC_COSTLY_ORDER)
> >  		max_retries /= 4;
> > -	if (compaction_retries <= max_retries)
> > +	if (*compaction_retries <= max_retries)
> >  		return true;
> >  
> >  	/*
> > -	 * Make sure there is at least one attempt at the highest priority
> > -	 * if we exhausted all retries at the lower priorities
> > +	 * Make sure there are attempts at the highest priority if we exhausted
> > +	 * all retries or failed at the lower priorities.
> >  	 */
> >  check_priority:
> >  	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> >  		(*compact_priority)--;
> > +		*compaction_retries = 0;
> >  		return true;
> >  	}
> >  	return false;
> > @@ -3224,7 +3225,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	struct zone *zone;
> >  	struct zoneref *z;
> > @@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	if (did_some_progress > 0 &&
> >  			should_compact_retry(ac, order, alloc_flags,
> >  				compact_result, &compact_priority,
> > -				compaction_retries))
> > +				&compaction_retries))
> >  		goto retry;
> >  
> >  	/* Reclaim has failed us, start killing things */
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-22 14:52           ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-22 14:52 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On Thu 22-09-16 16:08:21, Michal Hocko wrote:
> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> > From: Vlastimil Babka <vbabka@suse.cz>
> > Date: Thu, 22 Sep 2016 13:54:32 +0200
> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
> >  priority-fix
> > 
> > When increasing the compaction priority, also reset retries. Otherwise we can
> > consume all retries on the lower priorities.
> 
> OK, this is an improvement. I am just thinking that we might want to
> pull
> 	if (order && compaction_made_progress(compact_result))
> 		compaction_retries++;
> 
> into should_compact_retry as well. I've had it there originally because
> it was in line with no_progress_loops but now that we have compaction
> priorities it would fit into retry logic better. As a plus it would
> count only those compaction rounds where we we didn't have to rely on
                                                 did that should be

> the compaction retry logic. What do you think?
> 
> > Suggested-by: Michal Hocko <mhocko@suse.com>
> > Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> 
> Anyway
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> > ---
> >  mm/page_alloc.c | 13 +++++++------
> >  1 file changed, 7 insertions(+), 6 deletions(-)
> > 
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index f8bed910e3cf..82fdb690ac62 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -3162,7 +3162,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	int max_retries = MAX_COMPACT_RETRIES;
> >  
> > @@ -3196,16 +3196,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
> >  	 */
> >  	if (order > PAGE_ALLOC_COSTLY_ORDER)
> >  		max_retries /= 4;
> > -	if (compaction_retries <= max_retries)
> > +	if (*compaction_retries <= max_retries)
> >  		return true;
> >  
> >  	/*
> > -	 * Make sure there is at least one attempt at the highest priority
> > -	 * if we exhausted all retries at the lower priorities
> > +	 * Make sure there are attempts at the highest priority if we exhausted
> > +	 * all retries or failed at the lower priorities.
> >  	 */
> >  check_priority:
> >  	if (*compact_priority > MIN_COMPACT_PRIORITY) {
> >  		(*compact_priority)--;
> > +		*compaction_retries = 0;
> >  		return true;
> >  	}
> >  	return false;
> > @@ -3224,7 +3225,7 @@ static inline bool
> >  should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
> >  		     enum compact_result compact_result,
> >  		     enum compact_priority *compact_priority,
> > -		     int compaction_retries)
> > +		     int *compaction_retries)
> >  {
> >  	struct zone *zone;
> >  	struct zoneref *z;
> > @@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
> >  	if (did_some_progress > 0 &&
> >  			should_compact_retry(ac, order, alloc_flags,
> >  				compact_result, &compact_priority,
> > -				compaction_retries))
> > +				&compaction_retries))
> >  		goto retry;
> >  
> >  	/* Reclaim has failed us, start killing things */
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-22 14:52           ` Michal Hocko
@ 2016-09-22 14:59             ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 14:59 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/22/2016 04:52 PM, Michal Hocko wrote:
> On Thu 22-09-16 16:08:21, Michal Hocko wrote:
>> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
>> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
>> > From: Vlastimil Babka <vbabka@suse.cz>
>> > Date: Thu, 22 Sep 2016 13:54:32 +0200
>> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>> >  priority-fix
>> > 
>> > When increasing the compaction priority, also reset retries. Otherwise we can
>> > consume all retries on the lower priorities.
>> 
>> OK, this is an improvement. I am just thinking that we might want to
>> pull
>> 	if (order && compaction_made_progress(compact_result))
>> 		compaction_retries++;
>> 
>> into should_compact_retry as well. I've had it there originally because
>> it was in line with no_progress_loops but now that we have compaction
>> priorities it would fit into retry logic better. As a plus it would
>> count only those compaction rounds where we we didn't have to rely on
>                                                  did that should be
> 
>> the compaction retry logic. What do you think?

Makes sense.

-----8<-----
>From f775ec4be05a21c78a718a382c13132dedd5e2a4 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 22 Sep 2016 13:54:32 +0200
Subject: [PATCH] mm, compaction: more reliably increase direct compaction
 priority-fix-v2

When increasing the compaction priority, also reset retries. Otherwise we can
consume all retries on the lower priorities. Also pull the retries increment
into should_compact_retry() so it counts only the rounds where we actually
rely on it.

Suggested-by: Michal Hocko <mhocko@suse.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/page_alloc.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index f8bed910e3cf..582820080601 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3162,13 +3162,16 @@ static inline bool
 should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 		     enum compact_result compact_result,
 		     enum compact_priority *compact_priority,
-		     int compaction_retries)
+		     int *compaction_retries)
 {
 	int max_retries = MAX_COMPACT_RETRIES;
 
 	if (!order)
 		return false;
 
+	if (compaction_made_progress(compact_result))
+		(*compaction_retries)++;
+
 	/*
 	 * compaction considers all the zone as desperately out of memory
 	 * so it doesn't really make much sense to retry except when the
@@ -3196,16 +3199,17 @@ should_compact_retry(struct alloc_context *ac, int order, int alloc_flags,
 	 */
 	if (order > PAGE_ALLOC_COSTLY_ORDER)
 		max_retries /= 4;
-	if (compaction_retries <= max_retries)
+	if (*compaction_retries <= max_retries)
 		return true;
 
 	/*
-	 * Make sure there is at least one attempt at the highest priority
-	 * if we exhausted all retries at the lower priorities
+	 * Make sure there are attempts at the highest priority if we exhausted
+	 * all retries or failed at the lower priorities.
 	 */
 check_priority:
 	if (*compact_priority > MIN_COMPACT_PRIORITY) {
 		(*compact_priority)--;
+		*compaction_retries = 0;
 		return true;
 	}
 	return false;
@@ -3224,7 +3228,7 @@ static inline bool
 should_compact_retry(struct alloc_context *ac, unsigned int order, int alloc_flags,
 		     enum compact_result compact_result,
 		     enum compact_priority *compact_priority,
-		     int compaction_retries)
+		     int *compaction_retries)
 {
 	struct zone *zone;
 	struct zoneref *z;
@@ -3626,9 +3630,6 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (page)
 		goto got_pg;
 
-	if (order && compaction_made_progress(compact_result))
-		compaction_retries++;
-
 	/* Do not loop if specifically requested */
 	if (gfp_mask & __GFP_NORETRY)
 		goto nopage;
@@ -3663,7 +3664,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (did_some_progress > 0 &&
 			should_compact_retry(ac, order, alloc_flags,
 				compact_result, &compact_priority,
-				compaction_retries))
+				&compaction_retries))
 		goto retry;
 
 	/* Reclaim has failed us, start killing things */
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-22 14:59             ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 14:59 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/22/2016 04:52 PM, Michal Hocko wrote:
> On Thu 22-09-16 16:08:21, Michal Hocko wrote:
>> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
>> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
>> > From: Vlastimil Babka <vbabka@suse.cz>
>> > Date: Thu, 22 Sep 2016 13:54:32 +0200
>> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>> >  priority-fix
>> > 
>> > When increasing the compaction priority, also reset retries. Otherwise we can
>> > consume all retries on the lower priorities.
>> 
>> OK, this is an improvement. I am just thinking that we might want to
>> pull
>> 	if (order && compaction_made_progress(compact_result))
>> 		compaction_retries++;
>> 
>> into should_compact_retry as well. I've had it there originally because
>> it was in line with no_progress_loops but now that we have compaction
>> priorities it would fit into retry logic better. As a plus it would
>> count only those compaction rounds where we we didn't have to rely on
>                                                  did that should be
> 
>> the compaction retry logic. What do you think?

Makes sense.

-----8<-----

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-22 14:52           ` Michal Hocko
@ 2016-09-22 15:06             ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 15:06 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/22/2016 04:52 PM, Michal Hocko wrote:
> On Thu 22-09-16 16:08:21, Michal Hocko wrote:
>> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
>> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
>> > From: Vlastimil Babka <vbabka@suse.cz>
>> > Date: Thu, 22 Sep 2016 13:54:32 +0200
>> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>> >  priority-fix
>> > 
>> > When increasing the compaction priority, also reset retries. Otherwise we can
>> > consume all retries on the lower priorities.
>> 
>> OK, this is an improvement. I am just thinking that we might want to
>> pull
>> 	if (order && compaction_made_progress(compact_result))
>> 		compaction_retries++;
>> 
>> into should_compact_retry as well. I've had it there originally because
>> it was in line with no_progress_loops but now that we have compaction
>> priorities it would fit into retry logic better. As a plus it would
>> count only those compaction rounds where we we didn't have to rely on
>                                                  did that should be
> 
>> the compaction retry logic. What do you think?

In that case I would also add this for consistency?

----8<----
>From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 22 Sep 2016 17:02:37 +0200
Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
 should_reclaim_retry()

The should_reclaim_retry() makes decisions based on no_progress_loops, so it
makes sense to also update the counter there. It will be also consistent with
should_compact_retry() and compaction_retries. No functional change.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/page_alloc.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 582820080601..a01359ab3ed6 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 static inline bool
 should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		     struct alloc_context *ac, int alloc_flags,
-		     bool did_some_progress, int no_progress_loops)
+		     bool did_some_progress, int *no_progress_loops)
 {
 	struct zone *zone;
 	struct zoneref *z;
 
 	/*
+	 * Costly allocations might have made a progress but this doesn't mean
+	 * their order will become available due to high fragmentation so
+	 * always increment the no progress counter for them
+	 */
+	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
+		no_progress_loops = 0;
+	else
+		no_progress_loops++;
+
+	/*
 	 * Make sure we converge to OOM if we cannot make any progress
 	 * several times in the row.
 	 */
-	if (no_progress_loops > MAX_RECLAIM_RETRIES)
+	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
 		return false;
 
 	/*
@@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		unsigned long reclaimable;
 
 		available = reclaimable = zone_reclaimable_pages(zone);
-		available -= DIV_ROUND_UP(no_progress_loops * available,
+		available -= DIV_ROUND_UP((*no_progress_loops) * available,
 					  MAX_RECLAIM_RETRIES);
 		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
 
@@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
 		goto nopage;
 
-	/*
-	 * Costly allocations might have made a progress but this doesn't mean
-	 * their order will become available due to high fragmentation so
-	 * always increment the no progress counter for them
-	 */
-	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
-		no_progress_loops = 0;
-	else
-		no_progress_loops++;
-
 	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
-				 did_some_progress > 0, no_progress_loops))
+				 did_some_progress > 0, &no_progress_loops))
 		goto retry;
 
 	/*
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-22 15:06             ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 15:06 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm, Mel Gorman,
	Joonsoo Kim, David Rientjes, Rik van Riel

On 09/22/2016 04:52 PM, Michal Hocko wrote:
> On Thu 22-09-16 16:08:21, Michal Hocko wrote:
>> On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
>> > >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
>> > From: Vlastimil Babka <vbabka@suse.cz>
>> > Date: Thu, 22 Sep 2016 13:54:32 +0200
>> > Subject: [PATCH] mm, compaction: more reliably increase direct compaction
>> >  priority-fix
>> > 
>> > When increasing the compaction priority, also reset retries. Otherwise we can
>> > consume all retries on the lower priorities.
>> 
>> OK, this is an improvement. I am just thinking that we might want to
>> pull
>> 	if (order && compaction_made_progress(compact_result))
>> 		compaction_retries++;
>> 
>> into should_compact_retry as well. I've had it there originally because
>> it was in line with no_progress_loops but now that we have compaction
>> priorities it would fit into retry logic better. As a plus it would
>> count only those compaction rounds where we we didn't have to rely on
>                                                  did that should be
> 
>> the compaction retry logic. What do you think?

In that case I would also add this for consistency?

----8<----

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-21 17:18   ` Michal Hocko
@ 2016-09-22 15:18     ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 15:18 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On 09/21/2016 07:18 PM, Michal Hocko wrote:
> On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> 
> We still do not ignore fragindex in the full priority. This part has
> always been quite unclear to me so I cannot really tell whether that
> makes any difference or not but just to be on the safe side I would
> preffer to have _all_ the shortcuts out of the way in the highest
> priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
> so keep retrying but still a complication to understand the workflow.
> 
> What do you think?
 
I was thinking that this shouldn't be a problem on non-costly orders and default
extfrag_threshold. But better be safe. Moreover I think the issue is much more
dangerous for compact_zonelist_suitable() as explained below.

----8<----
>From 0e6cb251aa6e3b1be7deff315c0238c4d478f22e Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 22 Sep 2016 15:33:57 +0200
Subject: [PATCH] mm, compaction: ignore fragindex on highest direct compaction
 priority

Fragmentation index check in compaction_suitable() should be the last heuristic
that we allow on the highest compaction priority. Since that's a potential
premature OOM, disable it too. Even more problematic is its usage from
compaction_zonelist_suitable() -> __compaction_suitable() where we check the
order-0 watermark against free plus available-for-reclaim pages, but the
fragindex considers only truly free pages. Thus we can get a result close to 0
indicating failure do to lack of memory, and wrongly decide that compaction
won't be suitable even after reclaim. The solution is to skip the fragindex
check also in this context, regardless of priority.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 include/linux/compaction.h |  5 +++--
 mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
 mm/internal.h              |  1 +
 mm/vmscan.c                |  6 ++++--
 4 files changed, 31 insertions(+), 25 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index 0d8415820fc3..3ccf13d57651 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -97,7 +97,8 @@ extern enum compact_result try_to_compact_pages(gfp_t gfp_mask,
 		const struct alloc_context *ac, enum compact_priority prio);
 extern void reset_isolation_suitable(pg_data_t *pgdat);
 extern enum compact_result compaction_suitable(struct zone *zone, int order,
-		unsigned int alloc_flags, int classzone_idx);
+		unsigned int alloc_flags, int classzone_idx,
+		bool check_fragindex);
 
 extern void defer_compaction(struct zone *zone, int order);
 extern bool compaction_deferred(struct zone *zone, int order);
@@ -183,7 +184,7 @@ static inline void reset_isolation_suitable(pg_data_t *pgdat)
 }
 
 static inline enum compact_result compaction_suitable(struct zone *zone, int order,
-					int alloc_flags, int classzone_idx)
+		int alloc_flags, int classzone_idx, bool check_fragindex)
 {
 	return COMPACT_SKIPPED;
 }
diff --git a/mm/compaction.c b/mm/compaction.c
index 86d4d0bbfc7c..ae6a115f37b2 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1379,7 +1379,6 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
 					int classzone_idx,
 					unsigned long wmark_target)
 {
-	int fragindex;
 	unsigned long watermark;
 
 	if (is_via_compact_memory(order))
@@ -1415,6 +1414,18 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
 						ALLOC_CMA, wmark_target))
 		return COMPACT_SKIPPED;
 
+	return COMPACT_CONTINUE;
+}
+
+enum compact_result compaction_suitable(struct zone *zone, int order,
+					unsigned int alloc_flags,
+					int classzone_idx, bool check_fragindex)
+{
+	enum compact_result ret;
+	int fragindex;
+
+	ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx,
+			zone_page_state(zone, NR_FREE_PAGES));
 	/*
 	 * fragmentation index determines if allocation failures are due to
 	 * low memory or external fragmentation
@@ -1426,21 +1437,12 @@ static enum compact_result __compaction_suitable(struct zone *zone, int order,
 	 *
 	 * Only compact if a failure would be due to fragmentation.
 	 */
-	fragindex = fragmentation_index(zone, order);
-	if (fragindex >= 0 && fragindex <= sysctl_extfrag_threshold)
-		return COMPACT_NOT_SUITABLE_ZONE;
-
-	return COMPACT_CONTINUE;
-}
-
-enum compact_result compaction_suitable(struct zone *zone, int order,
-					unsigned int alloc_flags,
-					int classzone_idx)
-{
-	enum compact_result ret;
+	if (ret == COMPACT_CONTINUE && check_fragindex) {
+		fragindex = fragmentation_index(zone, order);
+		if (fragindex >= 0 && fragindex <= sysctl_extfrag_threshold)
+			ret = COMPACT_NOT_SUITABLE_ZONE;
+	}
 
-	ret = __compaction_suitable(zone, order, alloc_flags, classzone_idx,
-				    zone_page_state(zone, NR_FREE_PAGES));
 	trace_mm_compaction_suitable(zone, order, ret);
 	if (ret == COMPACT_NOT_SUITABLE_ZONE)
 		ret = COMPACT_SKIPPED;
@@ -1473,8 +1475,7 @@ bool compaction_zonelist_suitable(struct alloc_context *ac, int order,
 		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
 		compact_result = __compaction_suitable(zone, order, alloc_flags,
 				ac_classzone_idx(ac), available);
-		if (compact_result != COMPACT_SKIPPED &&
-				compact_result != COMPACT_NOT_SUITABLE_ZONE)
+		if (compact_result != COMPACT_SKIPPED)
 			return true;
 	}
 
@@ -1490,7 +1491,7 @@ static enum compact_result compact_zone(struct zone *zone, struct compact_contro
 	const bool sync = cc->mode != MIGRATE_ASYNC;
 
 	ret = compaction_suitable(zone, cc->order, cc->alloc_flags,
-							cc->classzone_idx);
+				cc->classzone_idx, !cc->ignore_fragindex);
 	/* Compaction is likely to fail */
 	if (ret == COMPACT_SUCCESS || ret == COMPACT_SKIPPED)
 		return ret;
@@ -1661,7 +1662,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
 		.direct_compaction = true,
 		.whole_zone = (prio == MIN_COMPACT_PRIORITY),
 		.ignore_skip_hint = (prio == MIN_COMPACT_PRIORITY),
-		.ignore_block_suitable = (prio == MIN_COMPACT_PRIORITY)
+		.ignore_block_suitable = (prio == MIN_COMPACT_PRIORITY),
+		.ignore_fragindex = (prio == MIN_COMPACT_PRIORITY)
 	};
 	INIT_LIST_HEAD(&cc.freepages);
 	INIT_LIST_HEAD(&cc.migratepages);
@@ -1869,7 +1871,7 @@ static bool kcompactd_node_suitable(pg_data_t *pgdat)
 			continue;
 
 		if (compaction_suitable(zone, pgdat->kcompactd_max_order, 0,
-					classzone_idx) == COMPACT_CONTINUE)
+				classzone_idx, true) == COMPACT_CONTINUE)
 			return true;
 	}
 
@@ -1905,7 +1907,7 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		if (compaction_deferred(zone, cc.order))
 			continue;
 
-		if (compaction_suitable(zone, cc.order, 0, zoneid) !=
+		if (compaction_suitable(zone, cc.order, 0, zoneid, true) !=
 							COMPACT_CONTINUE)
 			continue;
 
diff --git a/mm/internal.h b/mm/internal.h
index 537ac9951f5f..f18adf559e28 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -179,6 +179,7 @@ struct compact_control {
 	enum migrate_mode mode;		/* Async or sync migration mode */
 	bool ignore_skip_hint;		/* Scan blocks even if marked skip */
 	bool ignore_block_suitable;	/* Scan blocks considered unsuitable */
+	bool ignore_fragindex;		/* Ignore fragmentation index */
 	bool direct_compaction;		/* False from kcompactd or /proc/... */
 	bool whole_zone;		/* Whole zone should/has been scanned */
 	int order;			/* order a direct compactor needs */
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 55943a284082..08f16893cb2b 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2511,7 +2511,8 @@ static inline bool should_continue_reclaim(struct pglist_data *pgdat,
 		if (!managed_zone(zone))
 			continue;
 
-		switch (compaction_suitable(zone, sc->order, 0, sc->reclaim_idx)) {
+		switch (compaction_suitable(zone, sc->order, 0,
+						sc->reclaim_idx, true)) {
 		case COMPACT_SUCCESS:
 		case COMPACT_CONTINUE:
 			return false;
@@ -2624,7 +2625,8 @@ static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
 	unsigned long watermark;
 	enum compact_result suitable;
 
-	suitable = compaction_suitable(zone, sc->order, 0, sc->reclaim_idx);
+	suitable = compaction_suitable(zone, sc->order, 0, sc->reclaim_idx,
+									true);
 	if (suitable == COMPACT_SUCCESS)
 		/* Allocation should succeed already. Don't reclaim. */
 		return true;
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-22 15:18     ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-22 15:18 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On 09/21/2016 07:18 PM, Michal Hocko wrote:
> On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> 
> We still do not ignore fragindex in the full priority. This part has
> always been quite unclear to me so I cannot really tell whether that
> makes any difference or not but just to be on the safe side I would
> preffer to have _all_ the shortcuts out of the way in the highest
> priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
> so keep retrying but still a complication to understand the workflow.
> 
> What do you think?
 
I was thinking that this shouldn't be a problem on non-costly orders and default
extfrag_threshold. But better be safe. Moreover I think the issue is much more
dangerous for compact_zonelist_suitable() as explained below.

----8<----

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-22 15:06             ` Vlastimil Babka
@ 2016-09-23  4:04               ` Hillf Danton
  -1 siblings, 0 replies; 48+ messages in thread
From: Hillf Danton @ 2016-09-23  4:04 UTC (permalink / raw)
  To: 'Vlastimil Babka', 'Michal Hocko'
  Cc: 'Andrew Morton', 'Arkadiusz Miskiewicz',
	'Ralf-Peter Rohbeck', 'Olaf Hering',
	linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

> 
> ----8<----
> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 17:02:37 +0200
> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>  should_reclaim_retry()
> 
> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> makes sense to also update the counter there. It will be also consistent with
> should_compact_retry() and compaction_retries. No functional change.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/page_alloc.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 582820080601..a01359ab3ed6 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  static inline bool
>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		     struct alloc_context *ac, int alloc_flags,
> -		     bool did_some_progress, int no_progress_loops)
> +		     bool did_some_progress, int *no_progress_loops)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
> 
>  	/*
> +	 * Costly allocations might have made a progress but this doesn't mean
> +	 * their order will become available due to high fragmentation so
> +	 * always increment the no progress counter for them
> +	 */
> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> +		no_progress_loops = 0;

s/no/*no/
> +	else
> +		no_progress_loops++;

s/no_progress_loops/(*no_progress_loops)/

With that feel free to add
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

> +
> +	/*
>  	 * Make sure we converge to OOM if we cannot make any progress
>  	 * several times in the row.
>  	 */
> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>  		return false;
> 
>  	/*
> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		unsigned long reclaimable;
> 
>  		available = reclaimable = zone_reclaimable_pages(zone);
> -		available -= DIV_ROUND_UP(no_progress_loops * available,
> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>  					  MAX_RECLAIM_RETRIES);
>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> 
> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>  		goto nopage;
> 
> -	/*
> -	 * Costly allocations might have made a progress but this doesn't mean
> -	 * their order will become available due to high fragmentation so
> -	 * always increment the no progress counter for them
> -	 */
> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> -		no_progress_loops = 0;
> -	else
> -		no_progress_loops++;
> -
>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
> -				 did_some_progress > 0, no_progress_loops))
> +				 did_some_progress > 0, &no_progress_loops))
>  		goto retry;
> 
>  	/*
> --
> 2.10.0

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-23  4:04               ` Hillf Danton
  0 siblings, 0 replies; 48+ messages in thread
From: Hillf Danton @ 2016-09-23  4:04 UTC (permalink / raw)
  To: 'Vlastimil Babka', 'Michal Hocko'
  Cc: 'Andrew Morton', 'Arkadiusz Miskiewicz',
	'Ralf-Peter Rohbeck', 'Olaf Hering',
	linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

> 
> ----8<----
> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 17:02:37 +0200
> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>  should_reclaim_retry()
> 
> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> makes sense to also update the counter there. It will be also consistent with
> should_compact_retry() and compaction_retries. No functional change.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/page_alloc.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 582820080601..a01359ab3ed6 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  static inline bool
>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		     struct alloc_context *ac, int alloc_flags,
> -		     bool did_some_progress, int no_progress_loops)
> +		     bool did_some_progress, int *no_progress_loops)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
> 
>  	/*
> +	 * Costly allocations might have made a progress but this doesn't mean
> +	 * their order will become available due to high fragmentation so
> +	 * always increment the no progress counter for them
> +	 */
> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> +		no_progress_loops = 0;

s/no/*no/
> +	else
> +		no_progress_loops++;

s/no_progress_loops/(*no_progress_loops)/

With that feel free to add
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

> +
> +	/*
>  	 * Make sure we converge to OOM if we cannot make any progress
>  	 * several times in the row.
>  	 */
> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>  		return false;
> 
>  	/*
> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		unsigned long reclaimable;
> 
>  		available = reclaimable = zone_reclaimable_pages(zone);
> -		available -= DIV_ROUND_UP(no_progress_loops * available,
> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>  					  MAX_RECLAIM_RETRIES);
>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
> 
> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>  		goto nopage;
> 
> -	/*
> -	 * Costly allocations might have made a progress but this doesn't mean
> -	 * their order will become available due to high fragmentation so
> -	 * always increment the no progress counter for them
> -	 */
> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> -		no_progress_loops = 0;
> -	else
> -		no_progress_loops++;
> -
>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
> -				 did_some_progress > 0, no_progress_loops))
> +				 did_some_progress > 0, &no_progress_loops))
>  		goto retry;
> 
>  	/*
> --
> 2.10.0

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-23  4:04               ` Hillf Danton
@ 2016-09-23  6:55                 ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23  6:55 UTC (permalink / raw)
  To: Hillf Danton, 'Michal Hocko'
  Cc: 'Andrew Morton', 'Arkadiusz Miskiewicz',
	'Ralf-Peter Rohbeck', 'Olaf Hering',
	linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On 09/23/2016 06:04 AM, Hillf Danton wrote:
>>
>> ----8<----
>> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Thu, 22 Sep 2016 17:02:37 +0200
>> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>>  should_reclaim_retry()
>>
>> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
>> makes sense to also update the counter there. It will be also consistent with
>> should_compact_retry() and compaction_retries. No functional change.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>>  mm/page_alloc.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 582820080601..a01359ab3ed6 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>>  static inline bool
>>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		     struct alloc_context *ac, int alloc_flags,
>> -		     bool did_some_progress, int no_progress_loops)
>> +		     bool did_some_progress, int *no_progress_loops)
>>  {
>>  	struct zone *zone;
>>  	struct zoneref *z;
>>
>>  	/*
>> +	 * Costly allocations might have made a progress but this doesn't mean
>> +	 * their order will become available due to high fragmentation so
>> +	 * always increment the no progress counter for them
>> +	 */
>> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> +		no_progress_loops = 0;
> 
> s/no/*no/
>> +	else
>> +		no_progress_loops++;
> 
> s/no_progress_loops/(*no_progress_loops)/

Crap, thanks. I'm asking our gcc guy about possible warnings for this,
and some past mistake I've seen which would be *no_progress_loops++.
 
> With that feel free to add
> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

Thanks!

----8<----
>From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 22 Sep 2016 17:02:37 +0200
Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
 should_reclaim_retry()

The should_reclaim_retry() makes decisions based on no_progress_loops, so it
makes sense to also update the counter there. It will be also consistent with
should_compact_retry() and compaction_retries. No functional change.

[hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
---
 mm/page_alloc.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 582820080601..6039ff40452c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
 static inline bool
 should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		     struct alloc_context *ac, int alloc_flags,
-		     bool did_some_progress, int no_progress_loops)
+		     bool did_some_progress, int *no_progress_loops)
 {
 	struct zone *zone;
 	struct zoneref *z;
 
 	/*
+	 * Costly allocations might have made a progress but this doesn't mean
+	 * their order will become available due to high fragmentation so
+	 * always increment the no progress counter for them
+	 */
+	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
+		*no_progress_loops = 0;
+	else
+		(*no_progress_loops)++;
+
+	/*
 	 * Make sure we converge to OOM if we cannot make any progress
 	 * several times in the row.
 	 */
-	if (no_progress_loops > MAX_RECLAIM_RETRIES)
+	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
 		return false;
 
 	/*
@@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
 		unsigned long reclaimable;
 
 		available = reclaimable = zone_reclaimable_pages(zone);
-		available -= DIV_ROUND_UP(no_progress_loops * available,
+		available -= DIV_ROUND_UP((*no_progress_loops) * available,
 					  MAX_RECLAIM_RETRIES);
 		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
 
@@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
 	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
 		goto nopage;
 
-	/*
-	 * Costly allocations might have made a progress but this doesn't mean
-	 * their order will become available due to high fragmentation so
-	 * always increment the no progress counter for them
-	 */
-	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
-		no_progress_loops = 0;
-	else
-		no_progress_loops++;
-
 	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
-				 did_some_progress > 0, no_progress_loops))
+				 did_some_progress > 0, &no_progress_loops))
 		goto retry;
 
 	/*
-- 
2.10.0

^ permalink raw reply related	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-23  6:55                 ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23  6:55 UTC (permalink / raw)
  To: Hillf Danton, 'Michal Hocko'
  Cc: 'Andrew Morton', 'Arkadiusz Miskiewicz',
	'Ralf-Peter Rohbeck', 'Olaf Hering',
	linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On 09/23/2016 06:04 AM, Hillf Danton wrote:
>>
>> ----8<----
>> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Thu, 22 Sep 2016 17:02:37 +0200
>> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>>  should_reclaim_retry()
>>
>> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
>> makes sense to also update the counter there. It will be also consistent with
>> should_compact_retry() and compaction_retries. No functional change.
>>
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> ---
>>  mm/page_alloc.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 582820080601..a01359ab3ed6 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>>  static inline bool
>>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		     struct alloc_context *ac, int alloc_flags,
>> -		     bool did_some_progress, int no_progress_loops)
>> +		     bool did_some_progress, int *no_progress_loops)
>>  {
>>  	struct zone *zone;
>>  	struct zoneref *z;
>>
>>  	/*
>> +	 * Costly allocations might have made a progress but this doesn't mean
>> +	 * their order will become available due to high fragmentation so
>> +	 * always increment the no progress counter for them
>> +	 */
>> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> +		no_progress_loops = 0;
> 
> s/no/*no/
>> +	else
>> +		no_progress_loops++;
> 
> s/no_progress_loops/(*no_progress_loops)/

Crap, thanks. I'm asking our gcc guy about possible warnings for this,
and some past mistake I've seen which would be *no_progress_loops++.
 
> With that feel free to add
> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

Thanks!

----8<----

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-23  6:55                 ` Vlastimil Babka
@ 2016-09-23  8:23                   ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23  8:23 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
[...]
> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 17:02:37 +0200
> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>  should_reclaim_retry()
> 
> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> makes sense to also update the counter there. It will be also consistent with
> should_compact_retry() and compaction_retries. No functional change.
> 
> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

OK, this looks reasonable to me. Could you post both patches in a
separate thread please? They shouldn't be really needed to mitigate the
pre-mature oom killer issues. Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/page_alloc.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 582820080601..6039ff40452c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  static inline bool
>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		     struct alloc_context *ac, int alloc_flags,
> -		     bool did_some_progress, int no_progress_loops)
> +		     bool did_some_progress, int *no_progress_loops)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
>  
>  	/*
> +	 * Costly allocations might have made a progress but this doesn't mean
> +	 * their order will become available due to high fragmentation so
> +	 * always increment the no progress counter for them
> +	 */
> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> +		*no_progress_loops = 0;
> +	else
> +		(*no_progress_loops)++;
> +
> +	/*
>  	 * Make sure we converge to OOM if we cannot make any progress
>  	 * several times in the row.
>  	 */
> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>  		return false;
>  
>  	/*
> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		unsigned long reclaimable;
>  
>  		available = reclaimable = zone_reclaimable_pages(zone);
> -		available -= DIV_ROUND_UP(no_progress_loops * available,
> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>  					  MAX_RECLAIM_RETRIES);
>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>  
> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>  		goto nopage;
>  
> -	/*
> -	 * Costly allocations might have made a progress but this doesn't mean
> -	 * their order will become available due to high fragmentation so
> -	 * always increment the no progress counter for them
> -	 */
> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> -		no_progress_loops = 0;
> -	else
> -		no_progress_loops++;
> -
>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
> -				 did_some_progress > 0, no_progress_loops))
> +				 did_some_progress > 0, &no_progress_loops))
>  		goto retry;
>  
>  	/*
> -- 
> 2.10.0
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-23  8:23                   ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23  8:23 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
[...]
> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 17:02:37 +0200
> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>  should_reclaim_retry()
> 
> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> makes sense to also update the counter there. It will be also consistent with
> should_compact_retry() and compaction_retries. No functional change.
> 
> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>

OK, this looks reasonable to me. Could you post both patches in a
separate thread please? They shouldn't be really needed to mitigate the
pre-mature oom killer issues. Feel free to add
Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  mm/page_alloc.c | 28 ++++++++++++++--------------
>  1 file changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 582820080601..6039ff40452c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>  static inline bool
>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		     struct alloc_context *ac, int alloc_flags,
> -		     bool did_some_progress, int no_progress_loops)
> +		     bool did_some_progress, int *no_progress_loops)
>  {
>  	struct zone *zone;
>  	struct zoneref *z;
>  
>  	/*
> +	 * Costly allocations might have made a progress but this doesn't mean
> +	 * their order will become available due to high fragmentation so
> +	 * always increment the no progress counter for them
> +	 */
> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> +		*no_progress_loops = 0;
> +	else
> +		(*no_progress_loops)++;
> +
> +	/*
>  	 * Make sure we converge to OOM if we cannot make any progress
>  	 * several times in the row.
>  	 */
> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>  		return false;
>  
>  	/*
> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>  		unsigned long reclaimable;
>  
>  		available = reclaimable = zone_reclaimable_pages(zone);
> -		available -= DIV_ROUND_UP(no_progress_loops * available,
> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>  					  MAX_RECLAIM_RETRIES);
>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>  
> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>  		goto nopage;
>  
> -	/*
> -	 * Costly allocations might have made a progress but this doesn't mean
> -	 * their order will become available due to high fragmentation so
> -	 * always increment the no progress counter for them
> -	 */
> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
> -		no_progress_loops = 0;
> -	else
> -		no_progress_loops++;
> -
>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
> -				 did_some_progress > 0, no_progress_loops))
> +				 did_some_progress > 0, &no_progress_loops))
>  		goto retry;
>  
>  	/*
> -- 
> 2.10.0
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-22 15:18     ` Vlastimil Babka
@ 2016-09-23  8:26       ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23  8:26 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Thu 22-09-16 17:18:48, Vlastimil Babka wrote:
> On 09/21/2016 07:18 PM, Michal Hocko wrote:
> > On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> > 
> > We still do not ignore fragindex in the full priority. This part has
> > always been quite unclear to me so I cannot really tell whether that
> > makes any difference or not but just to be on the safe side I would
> > preffer to have _all_ the shortcuts out of the way in the highest
> > priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
> > so keep retrying but still a complication to understand the workflow.
> > 
> > What do you think?
>  
> I was thinking that this shouldn't be a problem on non-costly orders and default
> extfrag_threshold. But better be safe. Moreover I think the issue is much more
> dangerous for compact_zonelist_suitable() as explained below.
> 
> ----8<----
> >From 0e6cb251aa6e3b1be7deff315c0238c4d478f22e Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 15:33:57 +0200
> Subject: [PATCH] mm, compaction: ignore fragindex on highest direct compaction
>  priority
> 
> Fragmentation index check in compaction_suitable() should be the last heuristic
> that we allow on the highest compaction priority. Since that's a potential
> premature OOM, disable it too. Even more problematic is its usage from
> compaction_zonelist_suitable() -> __compaction_suitable() where we check the
> order-0 watermark against free plus available-for-reclaim pages, but the
> fragindex considers only truly free pages. Thus we can get a result close to 0
> indicating failure do to lack of memory, and wrongly decide that compaction
> won't be suitable even after reclaim. The solution is to skip the fragindex
> check also in this context, regardless of priority.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/compaction.h |  5 +++--
>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
>  mm/internal.h              |  1 +
>  mm/vmscan.c                |  6 ++++--
>  4 files changed, 31 insertions(+), 25 deletions(-)

This is much more code churn than I expected. I was thiking about it
some more and I am really wondering whether it actually make any sense
to check the fragidx for !costly orders. Wouldn't it be much simpler to
just put it out of the way for those regardless of the compaction
priority. In other words does this check makes any measurable difference
for !costly orders?
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-23  8:26       ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23  8:26 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Thu 22-09-16 17:18:48, Vlastimil Babka wrote:
> On 09/21/2016 07:18 PM, Michal Hocko wrote:
> > On Tue 06-09-16 15:52:54, Vlastimil Babka wrote:
> > 
> > We still do not ignore fragindex in the full priority. This part has
> > always been quite unclear to me so I cannot really tell whether that
> > makes any difference or not but just to be on the safe side I would
> > preffer to have _all_ the shortcuts out of the way in the highest
> > priority. It is true that this will cause COMPACT_NOT_SUITABLE_ZONE
> > so keep retrying but still a complication to understand the workflow.
> > 
> > What do you think?
>  
> I was thinking that this shouldn't be a problem on non-costly orders and default
> extfrag_threshold. But better be safe. Moreover I think the issue is much more
> dangerous for compact_zonelist_suitable() as explained below.
> 
> ----8<----
> >From 0e6cb251aa6e3b1be7deff315c0238c4d478f22e Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka <vbabka@suse.cz>
> Date: Thu, 22 Sep 2016 15:33:57 +0200
> Subject: [PATCH] mm, compaction: ignore fragindex on highest direct compaction
>  priority
> 
> Fragmentation index check in compaction_suitable() should be the last heuristic
> that we allow on the highest compaction priority. Since that's a potential
> premature OOM, disable it too. Even more problematic is its usage from
> compaction_zonelist_suitable() -> __compaction_suitable() where we check the
> order-0 watermark against free plus available-for-reclaim pages, but the
> fragindex considers only truly free pages. Thus we can get a result close to 0
> indicating failure do to lack of memory, and wrongly decide that compaction
> won't be suitable even after reclaim. The solution is to skip the fragindex
> check also in this context, regardless of priority.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  include/linux/compaction.h |  5 +++--
>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
>  mm/internal.h              |  1 +
>  mm/vmscan.c                |  6 ++++--
>  4 files changed, 31 insertions(+), 25 deletions(-)

This is much more code churn than I expected. I was thiking about it
some more and I am really wondering whether it actually make any sense
to check the fragidx for !costly orders. Wouldn't it be much simpler to
just put it out of the way for those regardless of the compaction
priority. In other words does this check makes any measurable difference
for !costly orders?
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-23  8:23                   ` Michal Hocko
@ 2016-09-23 10:47                     ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23 10:47 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On 09/23/2016 10:23 AM, Michal Hocko wrote:
> On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
> [...]
>> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Thu, 22 Sep 2016 17:02:37 +0200
>> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>>  should_reclaim_retry()
>>
>> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
>> makes sense to also update the counter there. It will be also consistent with
>> should_compact_retry() and compaction_retries. No functional change.
>>
>> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
> 
> OK, this looks reasonable to me. Could you post both patches in a

Both? I would argue that [1] might be relevant because it resets the
number of retries. Only the should_reclaim_retry() cleanup is not
stricly needed.

[1] http://lkml.kernel.org/r/<deec7319-2976-6d34-ab7b-afbb3f6c32f8@suse.cz>

> separate thread please? They shouldn't be really needed to mitigate the
> pre-mature oom killer issues. Feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
>> ---
>>  mm/page_alloc.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 582820080601..6039ff40452c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>>  static inline bool
>>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		     struct alloc_context *ac, int alloc_flags,
>> -		     bool did_some_progress, int no_progress_loops)
>> +		     bool did_some_progress, int *no_progress_loops)
>>  {
>>  	struct zone *zone;
>>  	struct zoneref *z;
>>  
>>  	/*
>> +	 * Costly allocations might have made a progress but this doesn't mean
>> +	 * their order will become available due to high fragmentation so
>> +	 * always increment the no progress counter for them
>> +	 */
>> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> +		*no_progress_loops = 0;
>> +	else
>> +		(*no_progress_loops)++;
>> +
>> +	/*
>>  	 * Make sure we converge to OOM if we cannot make any progress
>>  	 * several times in the row.
>>  	 */
>> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
>> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>>  		return false;
>>  
>>  	/*
>> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		unsigned long reclaimable;
>>  
>>  		available = reclaimable = zone_reclaimable_pages(zone);
>> -		available -= DIV_ROUND_UP(no_progress_loops * available,
>> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>>  					  MAX_RECLAIM_RETRIES);
>>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>>  
>> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>>  		goto nopage;
>>  
>> -	/*
>> -	 * Costly allocations might have made a progress but this doesn't mean
>> -	 * their order will become available due to high fragmentation so
>> -	 * always increment the no progress counter for them
>> -	 */
>> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> -		no_progress_loops = 0;
>> -	else
>> -		no_progress_loops++;
>> -
>>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
>> -				 did_some_progress > 0, no_progress_loops))
>> +				 did_some_progress > 0, &no_progress_loops))
>>  		goto retry;
>>  
>>  	/*
>> -- 
>> 2.10.0
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-23 10:47                     ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23 10:47 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On 09/23/2016 10:23 AM, Michal Hocko wrote:
> On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
> [...]
>> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka <vbabka@suse.cz>
>> Date: Thu, 22 Sep 2016 17:02:37 +0200
>> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
>>  should_reclaim_retry()
>>
>> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
>> makes sense to also update the counter there. It will be also consistent with
>> should_compact_retry() and compaction_retries. No functional change.
>>
>> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
>> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
>> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
> 
> OK, this looks reasonable to me. Could you post both patches in a

Both? I would argue that [1] might be relevant because it resets the
number of retries. Only the should_reclaim_retry() cleanup is not
stricly needed.

[1] http://lkml.kernel.org/r/<deec7319-2976-6d34-ab7b-afbb3f6c32f8@suse.cz>

> separate thread please? They shouldn't be really needed to mitigate the
> pre-mature oom killer issues. Feel free to add
> Acked-by: Michal Hocko <mhocko@suse.com>
> 
> Thanks!
> 
>> ---
>>  mm/page_alloc.c | 28 ++++++++++++++--------------
>>  1 file changed, 14 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 582820080601..6039ff40452c 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -3401,16 +3401,26 @@ bool gfp_pfmemalloc_allowed(gfp_t gfp_mask)
>>  static inline bool
>>  should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		     struct alloc_context *ac, int alloc_flags,
>> -		     bool did_some_progress, int no_progress_loops)
>> +		     bool did_some_progress, int *no_progress_loops)
>>  {
>>  	struct zone *zone;
>>  	struct zoneref *z;
>>  
>>  	/*
>> +	 * Costly allocations might have made a progress but this doesn't mean
>> +	 * their order will become available due to high fragmentation so
>> +	 * always increment the no progress counter for them
>> +	 */
>> +	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> +		*no_progress_loops = 0;
>> +	else
>> +		(*no_progress_loops)++;
>> +
>> +	/*
>>  	 * Make sure we converge to OOM if we cannot make any progress
>>  	 * several times in the row.
>>  	 */
>> -	if (no_progress_loops > MAX_RECLAIM_RETRIES)
>> +	if (*no_progress_loops > MAX_RECLAIM_RETRIES)
>>  		return false;
>>  
>>  	/*
>> @@ -3425,7 +3435,7 @@ should_reclaim_retry(gfp_t gfp_mask, unsigned order,
>>  		unsigned long reclaimable;
>>  
>>  		available = reclaimable = zone_reclaimable_pages(zone);
>> -		available -= DIV_ROUND_UP(no_progress_loops * available,
>> +		available -= DIV_ROUND_UP((*no_progress_loops) * available,
>>  					  MAX_RECLAIM_RETRIES);
>>  		available += zone_page_state_snapshot(zone, NR_FREE_PAGES);
>>  
>> @@ -3641,18 +3651,8 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order,
>>  	if (order > PAGE_ALLOC_COSTLY_ORDER && !(gfp_mask & __GFP_REPEAT))
>>  		goto nopage;
>>  
>> -	/*
>> -	 * Costly allocations might have made a progress but this doesn't mean
>> -	 * their order will become available due to high fragmentation so
>> -	 * always increment the no progress counter for them
>> -	 */
>> -	if (did_some_progress && order <= PAGE_ALLOC_COSTLY_ORDER)
>> -		no_progress_loops = 0;
>> -	else
>> -		no_progress_loops++;
>> -
>>  	if (should_reclaim_retry(gfp_mask, order, ac, alloc_flags,
>> -				 did_some_progress > 0, no_progress_loops))
>> +				 did_some_progress > 0, &no_progress_loops))
>>  		goto retry;
>>  
>>  	/*
>> -- 
>> 2.10.0
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majordomo@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-23  8:26       ` Michal Hocko
@ 2016-09-23 10:55         ` Vlastimil Babka
  -1 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23 10:55 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On 09/23/2016 10:26 AM, Michal Hocko wrote:
>>  include/linux/compaction.h |  5 +++--
>>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
>>  mm/internal.h              |  1 +
>>  mm/vmscan.c                |  6 ++++--
>>  4 files changed, 31 insertions(+), 25 deletions(-)
> 
> This is much more code churn than I expected. I was thiking about it
> some more and I am really wondering whether it actually make any sense
> to check the fragidx for !costly orders. Wouldn't it be much simpler to
> just put it out of the way for those regardless of the compaction
> priority. In other words does this check makes any measurable difference
> for !costly orders?

I've did some stress tests and sampling
/sys/kernel/debug/extfrag/extfrag_index once per second. The lowest
value I've got for order-2 was 0.705. The default threshold is 0.5, so
this would still result in compaction considered as suitable.

But it's sampling so I might not got to the interesting moments, most of
the time it was -1.000 which means the page should be just available.
Also we would be changing behavior for the user-controlled
vm.extfrag_threshold, so I'm not entirely sure about that.

I could probably reduce the churn so that compaction_suitable() doesn't
need a new parameter. We could just skip compaction_suitable() check
from compact_zone() on the highest priority, and go on even without
sufficient free page gap?

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-23 10:55         ` Vlastimil Babka
  0 siblings, 0 replies; 48+ messages in thread
From: Vlastimil Babka @ 2016-09-23 10:55 UTC (permalink / raw)
  To: Michal Hocko
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On 09/23/2016 10:26 AM, Michal Hocko wrote:
>>  include/linux/compaction.h |  5 +++--
>>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
>>  mm/internal.h              |  1 +
>>  mm/vmscan.c                |  6 ++++--
>>  4 files changed, 31 insertions(+), 25 deletions(-)
> 
> This is much more code churn than I expected. I was thiking about it
> some more and I am really wondering whether it actually make any sense
> to check the fragidx for !costly orders. Wouldn't it be much simpler to
> just put it out of the way for those regardless of the compaction
> priority. In other words does this check makes any measurable difference
> for !costly orders?

I've did some stress tests and sampling
/sys/kernel/debug/extfrag/extfrag_index once per second. The lowest
value I've got for order-2 was 0.705. The default threshold is 0.5, so
this would still result in compaction considered as suitable.

But it's sampling so I might not got to the interesting moments, most of
the time it was -1.000 which means the page should be just available.
Also we would be changing behavior for the user-controlled
vm.extfrag_threshold, so I'm not entirely sure about that.

I could probably reduce the churn so that compaction_suitable() doesn't
need a new parameter. We could just skip compaction_suitable() check
from compact_zone() on the highest priority, and go on even without
sufficient free page gap?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
  2016-09-23 10:47                     ` Vlastimil Babka
@ 2016-09-23 12:06                       ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23 12:06 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On Fri 23-09-16 12:47:23, Vlastimil Babka wrote:
> On 09/23/2016 10:23 AM, Michal Hocko wrote:
> > On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
> > [...]
> >> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> >> From: Vlastimil Babka <vbabka@suse.cz>
> >> Date: Thu, 22 Sep 2016 17:02:37 +0200
> >> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
> >>  should_reclaim_retry()
> >>
> >> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> >> makes sense to also update the counter there. It will be also consistent with
> >> should_compact_retry() and compaction_retries. No functional change.
> >>
> >> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> >> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
> > 
> > OK, this looks reasonable to me. Could you post both patches in a
> 
> Both? I would argue that [1] might be relevant because it resets the
> number of retries. Only the should_reclaim_retry() cleanup is not
> stricly needed.

Even if it is needed which I am not really sure about it would be
easier to track than in the middle of another thread.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
@ 2016-09-23 12:06                       ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23 12:06 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Hillf Danton, 'Andrew Morton',
	'Arkadiusz Miskiewicz', 'Ralf-Peter Rohbeck',
	'Olaf Hering', linux-kernel, 'Linus Torvalds',
	linux-mm, 'Mel Gorman', 'Joonsoo Kim',
	'David Rientjes', 'Rik van Riel'

On Fri 23-09-16 12:47:23, Vlastimil Babka wrote:
> On 09/23/2016 10:23 AM, Michal Hocko wrote:
> > On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
> > [...]
> >> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> >> From: Vlastimil Babka <vbabka@suse.cz>
> >> Date: Thu, 22 Sep 2016 17:02:37 +0200
> >> Subject: [PATCH] mm, page_alloc: pull no_progress_loops update to
> >>  should_reclaim_retry()
> >>
> >> The should_reclaim_retry() makes decisions based on no_progress_loops, so it
> >> makes sense to also update the counter there. It will be also consistent with
> >> should_compact_retry() and compaction_retries. No functional change.
> >>
> >> [hillf.zj@alibaba-inc.com: fix missing pointer dereferences]
> >> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> >> Acked-by: Hillf Danton <hillf.zj@alibaba-inc.com>
> > 
> > OK, this looks reasonable to me. Could you post both patches in a
> 
> Both? I would argue that [1] might be relevant because it resets the
> number of retries. Only the should_reclaim_retry() cleanup is not
> stricly needed.

Even if it is needed which I am not really sure about it would be
easier to track than in the middle of another thread.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
  2016-09-23 10:55         ` Vlastimil Babka
@ 2016-09-23 12:09           ` Michal Hocko
  -1 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23 12:09 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Fri 23-09-16 12:55:23, Vlastimil Babka wrote:
> On 09/23/2016 10:26 AM, Michal Hocko wrote:
> >>  include/linux/compaction.h |  5 +++--
> >>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
> >>  mm/internal.h              |  1 +
> >>  mm/vmscan.c                |  6 ++++--
> >>  4 files changed, 31 insertions(+), 25 deletions(-)
> > 
> > This is much more code churn than I expected. I was thiking about it
> > some more and I am really wondering whether it actually make any sense
> > to check the fragidx for !costly orders. Wouldn't it be much simpler to
> > just put it out of the way for those regardless of the compaction
> > priority. In other words does this check makes any measurable difference
> > for !costly orders?
> 
> I've did some stress tests and sampling
> /sys/kernel/debug/extfrag/extfrag_index once per second. The lowest
> value I've got for order-2 was 0.705. The default threshold is 0.5, so
> this would still result in compaction considered as suitable.
> 
> But it's sampling so I might not got to the interesting moments, most of
> the time it was -1.000 which means the page should be just available.
> Also we would be changing behavior for the user-controlled
> vm.extfrag_threshold, so I'm not entirely sure about that.

Does anybody depend on that or even use it out there? I strongly suspect
this is one of those dark corners people even do not know they exist...

> I could probably reduce the churn so that compaction_suitable() doesn't
> need a new parameter. We could just skip compaction_suitable() check
> from compact_zone() on the highest priority, and go on even without
> sufficient free page gap?

Whatever makes the code easier to understand. Please do not take me
wrong I do not want to push back on this too hard I just always love to
get rid of an obscure heuristic which even might not matter. And as your
testing suggests this might really be the case for !costly orders AFAIU.
-- 
Michal Hocko
SUSE Labs

^ permalink raw reply	[flat|nested] 48+ messages in thread

* Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions
@ 2016-09-23 12:09           ` Michal Hocko
  0 siblings, 0 replies; 48+ messages in thread
From: Michal Hocko @ 2016-09-23 12:09 UTC (permalink / raw)
  To: Vlastimil Babka
  Cc: Andrew Morton, Arkadiusz Miskiewicz, Ralf-Peter Rohbeck,
	Olaf Hering, linux-kernel, Linus Torvalds, linux-mm,
	David Rientjes, Joonsoo Kim, Mel Gorman, Rik van Riel

On Fri 23-09-16 12:55:23, Vlastimil Babka wrote:
> On 09/23/2016 10:26 AM, Michal Hocko wrote:
> >>  include/linux/compaction.h |  5 +++--
> >>  mm/compaction.c            | 44 +++++++++++++++++++++++---------------------
> >>  mm/internal.h              |  1 +
> >>  mm/vmscan.c                |  6 ++++--
> >>  4 files changed, 31 insertions(+), 25 deletions(-)
> > 
> > This is much more code churn than I expected. I was thiking about it
> > some more and I am really wondering whether it actually make any sense
> > to check the fragidx for !costly orders. Wouldn't it be much simpler to
> > just put it out of the way for those regardless of the compaction
> > priority. In other words does this check makes any measurable difference
> > for !costly orders?
> 
> I've did some stress tests and sampling
> /sys/kernel/debug/extfrag/extfrag_index once per second. The lowest
> value I've got for order-2 was 0.705. The default threshold is 0.5, so
> this would still result in compaction considered as suitable.
> 
> But it's sampling so I might not got to the interesting moments, most of
> the time it was -1.000 which means the page should be just available.
> Also we would be changing behavior for the user-controlled
> vm.extfrag_threshold, so I'm not entirely sure about that.

Does anybody depend on that or even use it out there? I strongly suspect
this is one of those dark corners people even do not know they exist...

> I could probably reduce the churn so that compaction_suitable() doesn't
> need a new parameter. We could just skip compaction_suitable() check
> from compact_zone() on the highest priority, and go on even without
> sufficient free page gap?

Whatever makes the code easier to understand. Please do not take me
wrong I do not want to push back on this too hard I just always love to
get rid of an obscure heuristic which even might not matter. And as your
testing suggests this might really be the case for !costly orders AFAIU.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 48+ messages in thread

end of thread, other threads:[~2016-09-23 12:10 UTC | newest]

Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-09-06 13:52 [PATCH 0/4] reintroduce compaction feedback for OOM decisions Vlastimil Babka
2016-09-06 13:52 ` Vlastimil Babka
2016-09-06 13:52 ` [PATCH 1/4] Revert "mm, oom: prevent premature OOM killer invocation for high order request" Vlastimil Babka
2016-09-06 13:52   ` Vlastimil Babka
2016-09-21 17:04   ` Michal Hocko
2016-09-21 17:04     ` Michal Hocko
2016-09-06 13:52 ` [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority Vlastimil Babka
2016-09-06 13:52   ` Vlastimil Babka
2016-09-21 17:13   ` Michal Hocko
2016-09-21 17:13     ` Michal Hocko
2016-09-22 12:51     ` Vlastimil Babka
2016-09-22 12:51       ` Vlastimil Babka
2016-09-22 14:08       ` Michal Hocko
2016-09-22 14:08         ` Michal Hocko
2016-09-22 14:52         ` Michal Hocko
2016-09-22 14:52           ` Michal Hocko
2016-09-22 14:59           ` Vlastimil Babka
2016-09-22 14:59             ` Vlastimil Babka
2016-09-22 15:06           ` Vlastimil Babka
2016-09-22 15:06             ` Vlastimil Babka
2016-09-23  4:04             ` Hillf Danton
2016-09-23  4:04               ` Hillf Danton
2016-09-23  6:55               ` Vlastimil Babka
2016-09-23  6:55                 ` Vlastimil Babka
2016-09-23  8:23                 ` Michal Hocko
2016-09-23  8:23                   ` Michal Hocko
2016-09-23 10:47                   ` Vlastimil Babka
2016-09-23 10:47                     ` Vlastimil Babka
2016-09-23 12:06                     ` Michal Hocko
2016-09-23 12:06                       ` Michal Hocko
2016-09-06 13:52 ` [PATCH 3/4] mm, compaction: restrict full priority to non-costly orders Vlastimil Babka
2016-09-06 13:52   ` Vlastimil Babka
2016-09-21 17:15   ` Michal Hocko
2016-09-21 17:15     ` Michal Hocko
2016-09-06 13:52 ` [PATCH 4/4] mm, compaction: make full priority ignore pageblock suitability Vlastimil Babka
2016-09-06 13:52   ` Vlastimil Babka
2016-09-15 18:51 ` [PATCH 0/4] reintroduce compaction feedback for OOM decisions Arkadiusz Miskiewicz
2016-09-15 18:51   ` Arkadiusz Miskiewicz
2016-09-21 17:18 ` Michal Hocko
2016-09-21 17:18   ` Michal Hocko
2016-09-22 15:18   ` Vlastimil Babka
2016-09-22 15:18     ` Vlastimil Babka
2016-09-23  8:26     ` Michal Hocko
2016-09-23  8:26       ` Michal Hocko
2016-09-23 10:55       ` Vlastimil Babka
2016-09-23 10:55         ` Vlastimil Babka
2016-09-23 12:09         ` Michal Hocko
2016-09-23 12:09           ` Michal Hocko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.