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

Groups > linux.kernel > #1488288 > unrolled thread

Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions

Back to article view | Back to linux.kernel

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

Contents

  Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions Michal Hocko <mhocko@kernel.org> - 2016-09-21 19:20 +0200
    Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions Vlastimil Babka <vbabka@suse.cz> - 2016-09-22 17:20 +0200
      Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions Michal Hocko <mhocko@kernel.org> - 2016-09-23 10:30 +0200
        Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions Vlastimil Babka <vbabka@suse.cz> - 2016-09-23 13:00 +0200
          Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions Michal Hocko <mhocko@kernel.org> - 2016-09-23 14:20 +0200

#1488288 — Re: [PATCH 0/4] reintroduce compaction feedback for OOM decisions

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

[toc] | [next] | [standalone]

#1488974

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

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

#1489840

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

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

#1489940

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?

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

#1489990

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web