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

Groups > linux.kernel > #1459386 > unrolled thread

[PATCH v6 00/11] make direct compaction more deterministic

Back to article view | Back to linux.kernel

Contents

  [PATCH v6 00/11] make direct compaction more deterministic Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 20:20 +0200
    [PATCH v6 06/11] mm, compaction: more reliably increase direct compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
      Re: [PATCH v6 06/11] mm, compaction: more reliably increase direct  compaction priority Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:10 +0200
        Re: [PATCH v6 06/11] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:40 +0200
      Re: [PATCH v6 06/11] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-08-18 11:20 +0200
        Re: [PATCH v6 06/11] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 11:50 +0200
          Re: [PATCH v6 06/11] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-08-18 12:00 +0200
    [PATCH v6 09/11] mm, compaction: use proper alloc_flags in __compaction_suitable() Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
    [PATCH v6 01/11] mm, compaction: make whole_zone flag ignore cached scanner positions Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
    [PATCH v6 07/11] mm, compaction: use correct watermark when checking compaction success Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
    [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
      Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:20 +0200
        Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:40 +0200
          Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:20 +0200
      Re: [PATCH v6 08/11] mm, compaction: create compact_gap wrapper Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:20 +0200
    [PATCH v6 03/11] mm, compaction: rename COMPACT_PARTIAL to COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 22:50 +0200
      Re: [PATCH v6 03/11] mm, compaction: rename COMPACT_PARTIAL to  COMPACT_SUCCESS Michal Hocko <mhocko@kernel.org> - 2016-08-18 13:10 +0200
    [PATCH v6 02/11] mm, compaction: cleanup unused functions Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 23:30 +0200
    [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 23:30 +0200
      Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:20 +0200
        Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:40 +0200
          Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:50 +0200
            Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for  non-costly orders Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:30 +0200
    [PATCH v6 05/11] mm, compaction: add the ultimate direct compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 23:30 +0200
      Re: [PATCH v6 05/11] mm, compaction: add the ultimate direct  compaction priority Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:00 +0200
        Re: [PATCH v6 05/11] mm, compaction: add the ultimate direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:30 +0200
    [PATCH v6 11/11] mm, vmscan: make compaction_ready() more accurate and readable Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 23:30 +0200
    [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-10 23:30 +0200
      Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:10 +0200
        Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:20 +0200
        Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:00 +0200
      Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Michal Hocko <mhocko@kernel.org> - 2016-08-18 11:10 +0200

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

#1463454 — Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders

On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
> On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>> index 621e4211ce16..a5c0f914ec00 100644
>> --- a/mm/page_alloc.c
>> +++ b/mm/page_alloc.c
>> @@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>>
>>  	if (!is_migrate_isolate(mt)) {
>>  		/* Obey watermarks as if the page was being allocated */
>> -		watermark = low_wmark_pages(zone) + (1 << order);
>> +		watermark = min_wmark_pages(zone) + (1UL << order);
>
> This '1 << order' also needs some comment. Why can't we use
> compact_gap() in this case?

This is just short-cutting the high-order watermark check to check only 
order-0, because we already know the high-order page exists.
We can't use compact_gap() as that's too high to use for a single 
allocation watermark, since we can be already holding some free pages on 
the list. So it would defeat the gap purpose.

> Thanks.
>

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

#1463458 — Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders

On Tue, Aug 16, 2016 at 08:36:12AM +0200, Vlastimil Babka wrote:
> On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
> >On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
> >>diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> >>index 621e4211ce16..a5c0f914ec00 100644
> >>--- a/mm/page_alloc.c
> >>+++ b/mm/page_alloc.c
> >>@@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
> >>
> >> 	if (!is_migrate_isolate(mt)) {
> >> 		/* Obey watermarks as if the page was being allocated */
> >>-		watermark = low_wmark_pages(zone) + (1 << order);
> >>+		watermark = min_wmark_pages(zone) + (1UL << order);
> >
> >This '1 << order' also needs some comment. Why can't we use
> >compact_gap() in this case?
> 
> This is just short-cutting the high-order watermark check to check
> only order-0, because we already know the high-order page exists.
> We can't use compact_gap() as that's too high to use for a single
> allocation watermark, since we can be already holding some free
> pages on the list. So it would defeat the gap purpose.

Oops. I missed that. Thanks for clarifying it.

Thanks.

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

#1465222 — Re: [PATCH v6 10/11] mm, compaction: require only min watermarks for non-costly orders

On 08/16/2016 08:46 AM, Joonsoo Kim wrote:
> On Tue, Aug 16, 2016 at 08:36:12AM +0200, Vlastimil Babka wrote:
>> On 08/16/2016 08:16 AM, Joonsoo Kim wrote:
>>> On Wed, Aug 10, 2016 at 11:12:25AM +0200, Vlastimil Babka wrote:
>>>> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
>>>> index 621e4211ce16..a5c0f914ec00 100644
>>>> --- a/mm/page_alloc.c
>>>> +++ b/mm/page_alloc.c
>>>> @@ -2492,7 +2492,7 @@ int __isolate_free_page(struct page *page, unsigned int order)
>>>>
>>>> 	if (!is_migrate_isolate(mt)) {
>>>> 		/* Obey watermarks as if the page was being allocated */
>>>> -		watermark = low_wmark_pages(zone) + (1 << order);
>>>> +		watermark = min_wmark_pages(zone) + (1UL << order);
>>>
>>> This '1 << order' also needs some comment. Why can't we use
>>> compact_gap() in this case?
>>
>> This is just short-cutting the high-order watermark check to check
>> only order-0, because we already know the high-order page exists.
>> We can't use compact_gap() as that's too high to use for a single
>> allocation watermark, since we can be already holding some free
>> pages on the list. So it would defeat the gap purpose.
> 
> Oops. I missed that. Thanks for clarifying it.

So let's expand the comment?

----8<----
From 5d060f4222a637e1005ff32ae0fd4330625b6675 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka <vbabka@suse.cz>
Date: Thu, 18 Aug 2016 14:18:08 +0200
Subject: [PATCH] mm, compaction: require only min watermarks for non-costly
 orders-fix

Clarify why __isolate_free_page() does a order-0 watermark check with
apparent (1UL << order) gap, per Joonsoo.

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

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a5c0f914ec00..216715504fb4 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2491,7 +2491,12 @@ int __isolate_free_page(struct page *page, unsigned int order)
 	mt = get_pageblock_migratetype(page);
 
 	if (!is_migrate_isolate(mt)) {
-		/* Obey watermarks as if the page was being allocated */
+		/*
+		 * Obey watermarks as if the page was being allocated. We can
+		 * emulate a high-order watermark check with a raised order-0
+		 * watermark, because we already know our high-order page
+		 * exists.
+		 */
 		watermark = min_wmark_pages(zone) + (1UL << order);
 		if (!zone_watermark_ok(zone, 0, watermark, 0, ALLOC_CMA))
 			return 0;
-- 
2.9.2

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

#1459989 — [PATCH v6 05/11] mm, compaction: add the ultimate direct compaction priority

During reclaim/compaction loop, it's desirable to get a final answer from
unsuccessful compaction so we can either fail the allocation or invoke the OOM
killer. However, heuristics such as deferred compaction or pageblock skip bits
can cause compaction to skip parts or whole zones and lead to premature OOM's,
failures or excessive reclaim/compaction retries.

To remedy this, we introduce a new direct compaction priority called
COMPACT_PRIO_SYNC_FULL, which instructs direct compaction to:

- ignore deferred compaction status for a zone
- ignore pageblock skip hints
- ignore cached scanner positions and scan the whole zone

The new priority should get eventually picked up by should_compact_retry() and
this should improve success rates for costly allocations using __GFP_REPEAT,
such as hugetlbfs allocations, and reduce some corner-case OOM's for non-costly
allocations.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 include/linux/compaction.h | 3 ++-
 mm/compaction.c            | 5 ++++-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/include/linux/compaction.h b/include/linux/compaction.h
index e88c037afe47..a1fba9994728 100644
--- a/include/linux/compaction.h
+++ b/include/linux/compaction.h
@@ -6,8 +6,9 @@
  * Lower value means higher priority, analogically to reclaim priority.
  */
 enum compact_priority {
+	COMPACT_PRIO_SYNC_FULL,
+	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
 	COMPACT_PRIO_SYNC_LIGHT,
-	MIN_COMPACT_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/compaction.c b/mm/compaction.c
index a144f58f7193..ae4f40afcca1 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1644,6 +1644,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
 		.alloc_flags = alloc_flags,
 		.classzone_idx = classzone_idx,
 		.direct_compaction = true,
+		.whole_zone = (prio == COMPACT_PRIO_SYNC_FULL),
+		.ignore_skip_hint = (prio == COMPACT_PRIO_SYNC_FULL)
 	};
 	INIT_LIST_HEAD(&cc.freepages);
 	INIT_LIST_HEAD(&cc.migratepages);
@@ -1689,7 +1691,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
 								ac->nodemask) {
 		enum compact_result status;
 
-		if (compaction_deferred(zone, order)) {
+		if (prio > COMPACT_PRIO_SYNC_FULL
+					&& compaction_deferred(zone, order)) {
 			rc = max_t(enum compact_result, COMPACT_DEFERRED, rc);
 			continue;
 		}
-- 
2.9.2

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

#1463417 — Re: [PATCH v6 05/11] mm, compaction: add the ultimate direct compaction priority

On Wed, Aug 10, 2016 at 11:12:20AM +0200, Vlastimil Babka wrote:
> During reclaim/compaction loop, it's desirable to get a final answer from
> unsuccessful compaction so we can either fail the allocation or invoke the OOM
> killer. However, heuristics such as deferred compaction or pageblock skip bits
> can cause compaction to skip parts or whole zones and lead to premature OOM's,
> failures or excessive reclaim/compaction retries.
> 
> To remedy this, we introduce a new direct compaction priority called
> COMPACT_PRIO_SYNC_FULL, which instructs direct compaction to:
> 
> - ignore deferred compaction status for a zone
> - ignore pageblock skip hints
> - ignore cached scanner positions and scan the whole zone
> 
> The new priority should get eventually picked up by should_compact_retry() and
> this should improve success rates for costly allocations using __GFP_REPEAT,
> such as hugetlbfs allocations, and reduce some corner-case OOM's for non-costly
> allocations.
> 
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> Acked-by: Michal Hocko <mhocko@suse.com>
> ---
>  include/linux/compaction.h | 3 ++-
>  mm/compaction.c            | 5 ++++-
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/compaction.h b/include/linux/compaction.h
> index e88c037afe47..a1fba9994728 100644
> --- a/include/linux/compaction.h
> +++ b/include/linux/compaction.h
> @@ -6,8 +6,9 @@
>   * Lower value means higher priority, analogically to reclaim priority.
>   */
>  enum compact_priority {
> +	COMPACT_PRIO_SYNC_FULL,
> +	MIN_COMPACT_PRIORITY = COMPACT_PRIO_SYNC_FULL,
>  	COMPACT_PRIO_SYNC_LIGHT,
> -	MIN_COMPACT_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/compaction.c b/mm/compaction.c
> index a144f58f7193..ae4f40afcca1 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1644,6 +1644,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
>  		.alloc_flags = alloc_flags,
>  		.classzone_idx = classzone_idx,
>  		.direct_compaction = true,
> +		.whole_zone = (prio == COMPACT_PRIO_SYNC_FULL),
> +		.ignore_skip_hint = (prio == COMPACT_PRIO_SYNC_FULL)
>  	};
>  	INIT_LIST_HEAD(&cc.freepages);
>  	INIT_LIST_HEAD(&cc.migratepages);
> @@ -1689,7 +1691,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>  								ac->nodemask) {
>  		enum compact_result status;
>  
> -		if (compaction_deferred(zone, order)) {
> +		if (prio > COMPACT_PRIO_SYNC_FULL
> +					&& compaction_deferred(zone, order)) {
>  			rc = max_t(enum compact_result, COMPACT_DEFERRED, rc);
>  			continue;

Could we provide prio to compaction_deferred() and do the decision in
that that function?

BTW, in kcompactd, compaction_deferred() is checked but
.ignore_skip_hint=true. Is there any reason? If we can remove
compaction_deferred() for kcompactd, we can check .ignore_skip_hint
to determine if defer is needed or not.

Thanks.

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

#1465219 — Re: [PATCH v6 05/11] mm, compaction: add the ultimate direct compaction priority

On 08/16/2016 07:58 AM, Joonsoo Kim wrote:
>> --- a/mm/compaction.c
>> +++ b/mm/compaction.c
>> @@ -1644,6 +1644,8 @@ static enum compact_result compact_zone_order(struct zone *zone, int order,
>>  		.alloc_flags = alloc_flags,
>>  		.classzone_idx = classzone_idx,
>>  		.direct_compaction = true,
>> +		.whole_zone = (prio == COMPACT_PRIO_SYNC_FULL),
>> +		.ignore_skip_hint = (prio == COMPACT_PRIO_SYNC_FULL)
>>  	};
>>  	INIT_LIST_HEAD(&cc.freepages);
>>  	INIT_LIST_HEAD(&cc.migratepages);
>> @@ -1689,7 +1691,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>>  								ac->nodemask) {
>>  		enum compact_result status;
>>
>> -		if (compaction_deferred(zone, order)) {
>> +		if (prio > COMPACT_PRIO_SYNC_FULL
>> +					&& compaction_deferred(zone, order)) {
>>  			rc = max_t(enum compact_result, COMPACT_DEFERRED, rc);
>>  			continue;
>
> Could we provide prio to compaction_deferred() and do the decision in
> that that function?
>
> BTW, in kcompactd, compaction_deferred() is checked but
> .ignore_skip_hint=true. Is there any reason? If we can remove
> compaction_deferred() for kcompactd, we can check .ignore_skip_hint
> to determine if defer is needed or not.

I don't want to change kcompactd right now, as the current series seems 
to help against premature OOMs. But I'll revisit it later.

Thanks.

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

#1459993 — [PATCH v6 11/11] mm, vmscan: make compaction_ready() more accurate and readable

The compaction_ready() is used during direct reclaim for costly order
allocations to skip reclaim for zones where compaction should be attempted
instead. It's combining the standard compaction_suitable() check with its own
watermark check based on high watermark with extra gap, and the result is
confusing at best.

This patch attempts to better structure and document the checks involved.
First, compaction_suitable() can determine that the allocation should either
succeed already, or that compaction doesn't have enough free pages to proceed.
The third possibility is that compaction has enough free pages, but we still
decide to reclaim first - unless we are already above the high watermark with
gap.  This does not mean that the reclaim will actually reach this watermark
during single attempt, this is rather an over-reclaim protection. So document
the code as such. The check for compaction_deferred() is removed completely, as
it in fact had no proper role here.

The result after this patch is mainly a less confusing code. We also skip some
over-reclaim in cases where the allocation should already succed.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Michal Hocko <mhocko@suse.com>
---
 mm/vmscan.c | 43 ++++++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 23 deletions(-)

diff --git a/mm/vmscan.c b/mm/vmscan.c
index b676b4b51db0..f9b3112e963a 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -2617,38 +2617,35 @@ static bool shrink_node(pg_data_t *pgdat, struct scan_control *sc)
 }
 
 /*
- * Returns true if compaction should go ahead for a high-order request, or
- * the high-order allocation would succeed without compaction.
+ * Returns true if compaction should go ahead for a costly-order request, or
+ * the allocation would already succeed without compaction. Return false if we
+ * should reclaim first.
  */
 static inline bool compaction_ready(struct zone *zone, struct scan_control *sc)
 {
 	unsigned long watermark;
-	bool watermark_ok;
+	enum compact_result suitable;
 
-	/*
-	 * Compaction takes time to run and there are potentially other
-	 * callers using the pages just freed. Continue reclaiming until
-	 * there is a buffer of free pages available to give compaction
-	 * a reasonable chance of completing and allocating the page
-	 */
-	watermark = high_wmark_pages(zone) + compact_gap(sc->order);
-	watermark_ok = zone_watermark_ok_safe(zone, 0, watermark, sc->reclaim_idx);
-
-	/*
-	 * If compaction is deferred, reclaim up to a point where
-	 * compaction will have a chance of success when re-enabled
-	 */
-	if (compaction_deferred(zone, sc->order))
-		return watermark_ok;
+	suitable = compaction_suitable(zone, sc->order, 0, sc->reclaim_idx);
+	if (suitable == COMPACT_SUCCESS)
+		/* Allocation should succeed already. Don't reclaim. */
+		return true;
+	if (suitable == COMPACT_SKIPPED)
+		/* Compaction cannot yet proceed. Do reclaim. */
+		return false;
 
 	/*
-	 * If compaction is not ready to start and allocation is not likely
-	 * to succeed without it, then keep reclaiming.
+	 * Compaction is already possible, but it takes time to run and there
+	 * are potentially other callers using the pages just freed. So proceed
+	 * with reclaim to make a buffer of free pages available to give
+	 * compaction a reasonable chance of completing and allocating the page.
+	 * Note that we won't actually reclaim the whole buffer in one attempt
+	 * as the target watermark in should_continue_reclaim() is lower. But if
+	 * we are already above the high+gap watermark, don't reclaim at all.
 	 */
-	if (compaction_suitable(zone, sc->order, 0, sc->reclaim_idx) == COMPACT_SKIPPED)
-		return false;
+	watermark = high_wmark_pages(zone) + compact_gap(sc->order);
 
-	return watermark_ok;
+	return zone_watermark_ok_safe(zone, 0, watermark, sc->reclaim_idx);
 }
 
 /*
-- 
2.9.2

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

#1459998 — [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS

Joonsoo has reminded me that in a later patch changing watermark checks
throughout compaction I forgot to update checks in try_to_compact_pages() and
compactd_do_work(). Closer inspection however shows that they are redundant now
that compact_zone() reliably reports success with COMPACT_SUCCESS, as they just
repeat (a subset) of checks that have just passed. So instead of checking
watermarks again, just test the return value.

Also remove the stray "bool success" variable from kcompactd_do_work().

Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/compaction.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/mm/compaction.c b/mm/compaction.c
index c355bf0d8599..a144f58f7193 100644
--- a/mm/compaction.c
+++ b/mm/compaction.c
@@ -1698,9 +1698,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
 					alloc_flags, ac_classzone_idx(ac));
 		rc = max(status, rc);
 
-		/* If a normal allocation would succeed, stop compacting */
-		if (zone_watermark_ok(zone, order, low_wmark_pages(zone),
-					ac_classzone_idx(ac), alloc_flags)) {
+		/* The allocation should succeed, stop compacting */
+		if (status == COMPACT_SUCCESS) {
 			/*
 			 * We think the allocation will succeed in this zone,
 			 * but it is not certain, hence the false. The caller
@@ -1873,8 +1872,6 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 		.ignore_skip_hint = true,
 
 	};
-	bool success = false;
-
 	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
 							cc.classzone_idx);
 	count_vm_event(KCOMPACTD_WAKE);
@@ -1903,9 +1900,7 @@ static void kcompactd_do_work(pg_data_t *pgdat)
 			return;
 		status = compact_zone(zone, &cc);
 
-		if (zone_watermark_ok(zone, cc.order, low_wmark_pages(zone),
-						cc.classzone_idx, 0)) {
-			success = true;
+		if (status == COMPACT_SUCCESS) {
 			compaction_defer_reset(zone, cc.order, false);
 		} else if (status == COMPACT_PARTIAL_SKIPPED || status == COMPACT_COMPLETE) {
 			/*
-- 
2.9.2

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

#1463431 — Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS

On Wed, Aug 10, 2016 at 11:12:19AM +0200, Vlastimil Babka wrote:
> Joonsoo has reminded me that in a later patch changing watermark checks
> throughout compaction I forgot to update checks in try_to_compact_pages() and
> compactd_do_work(). Closer inspection however shows that they are redundant now
> that compact_zone() reliably reports success with COMPACT_SUCCESS, as they just
> repeat (a subset) of checks that have just passed. So instead of checking
> watermarks again, just test the return value.

In fact, it's not redundant. Even if try_to_compact_pages() returns
!COMPACT_SUCCESS, watermark check could return true.
__compact_finished() calls find_suitable_fallback() and it's slightly
different with watermark check. Anyway, I don't think it is a big
problem.

Thanks.


> 
> Also remove the stray "bool success" variable from kcompactd_do_work().
> 
> Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
> ---
>  mm/compaction.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index c355bf0d8599..a144f58f7193 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1698,9 +1698,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>  					alloc_flags, ac_classzone_idx(ac));
>  		rc = max(status, rc);
>  
> -		/* If a normal allocation would succeed, stop compacting */
> -		if (zone_watermark_ok(zone, order, low_wmark_pages(zone),
> -					ac_classzone_idx(ac), alloc_flags)) {
> +		/* The allocation should succeed, stop compacting */
> +		if (status == COMPACT_SUCCESS) {
>  			/*
>  			 * We think the allocation will succeed in this zone,
>  			 * but it is not certain, hence the false. The caller
> @@ -1873,8 +1872,6 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  		.ignore_skip_hint = true,
>  
>  	};
> -	bool success = false;
> -
>  	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
>  							cc.classzone_idx);
>  	count_vm_event(KCOMPACTD_WAKE);
> @@ -1903,9 +1900,7 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  			return;
>  		status = compact_zone(zone, &cc);
>  
> -		if (zone_watermark_ok(zone, cc.order, low_wmark_pages(zone),
> -						cc.classzone_idx, 0)) {
> -			success = true;
> +		if (status == COMPACT_SUCCESS) {
>  			compaction_defer_reset(zone, cc.order, false);
>  		} else if (status == COMPACT_PARTIAL_SKIPPED || status == COMPACT_COMPLETE) {
>  			/*
> -- 
> 2.9.2
> 
> --
> 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>

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

#1463439 — Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS

On 08/16/2016 08:12 AM, Joonsoo Kim wrote:
> On Wed, Aug 10, 2016 at 11:12:19AM +0200, Vlastimil Babka wrote:
>> Joonsoo has reminded me that in a later patch changing watermark checks
>> throughout compaction I forgot to update checks in try_to_compact_pages() and
>> compactd_do_work(). Closer inspection however shows that they are redundant now
>> that compact_zone() reliably reports success with COMPACT_SUCCESS, as they just
>> repeat (a subset) of checks that have just passed. So instead of checking
>> watermarks again, just test the return value.
>
> In fact, it's not redundant. Even if try_to_compact_pages() returns
> !COMPACT_SUCCESS, watermark check could return true.

Right, I meant they are redundant in the SUCCESS case.

> __compact_finished() calls find_suitable_fallback() and it's slightly
> different with watermark check. Anyway, I don't think it is a big
> problem.

I agree. It might be even better for long-term fragmentation that we 
e.g. try another zone instead of taking page from the "unsuitable 
fallback". If that's not successful, and the allocation is important 
enough there will later eventually be another watermark check permitting 
the unsuitable fallback.

Thanks.

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

#1465202 — Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS

On 08/16/2016 08:12 AM, Joonsoo Kim wrote:
> On Wed, Aug 10, 2016 at 11:12:19AM +0200, Vlastimil Babka wrote:
>> Joonsoo has reminded me that in a later patch changing watermark checks
>> throughout compaction I forgot to update checks in try_to_compact_pages() and
>> compactd_do_work(). Closer inspection however shows that they are redundant now
>> that compact_zone() reliably reports success with COMPACT_SUCCESS, as they just
>> repeat (a subset) of checks that have just passed. So instead of checking
>> watermarks again, just test the return value.
>
> In fact, it's not redundant. Even if try_to_compact_pages() returns
> !COMPACT_SUCCESS, watermark check could return true.
> __compact_finished() calls find_suitable_fallback() and it's slightly
> different with watermark check. Anyway, I don't think it is a big
> problem.

Andrew, can you please replace the changelog to clarify this?

===
Joonsoo has reminded me that in a later patch changing watermark checks
throughout compaction I forgot to update checks in 
try_to_compact_pages() and compactd_do_work(). Closer inspection however 
shows that they are redundant now in the success case, because 
compact_zone() now reliably reports this with COMPACT_SUCCESS. So 
effectively the checks just repeat (a subset) of checks that have just 
passed. So instead of checking watermarks again, just test the return value.

Note it's also possible that compaction would declare failure e.g. 
because its find_suitable_fallback() is more strict than simple 
watermark check, and then the watermark check we are removing would then 
still succeed. After this patch this is not possible and it's arguably 
better, because for long-term fragmentation avoidance we should rather 
try a different zone than allocate with the unsuitable fallback. If 
compaction of all zones fail and the allocation is important enough, it 
will retry and succeed anyway.

Also remove the stray "bool success" variable from kcompactd_do_work().
===

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

#1465021 — Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after COMPACT_SUCCESS

On Wed 10-08-16 11:12:19, Vlastimil Babka wrote:
> Joonsoo has reminded me that in a later patch changing watermark checks
> throughout compaction I forgot to update checks in try_to_compact_pages() and
> compactd_do_work(). Closer inspection however shows that they are redundant now
> that compact_zone() reliably reports success with COMPACT_SUCCESS, as they just
> repeat (a subset) of checks that have just passed. So instead of checking
> watermarks again, just test the return value.

the less watermark checks we do the better because they just increase a
probability of subtle and hard to explain corner cases.

> Also remove the stray "bool success" variable from kcompactd_do_work().
> 
> Reported-by: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

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

> ---
>  mm/compaction.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/compaction.c b/mm/compaction.c
> index c355bf0d8599..a144f58f7193 100644
> --- a/mm/compaction.c
> +++ b/mm/compaction.c
> @@ -1698,9 +1698,8 @@ enum compact_result try_to_compact_pages(gfp_t gfp_mask, unsigned int order,
>  					alloc_flags, ac_classzone_idx(ac));
>  		rc = max(status, rc);
>  
> -		/* If a normal allocation would succeed, stop compacting */
> -		if (zone_watermark_ok(zone, order, low_wmark_pages(zone),
> -					ac_classzone_idx(ac), alloc_flags)) {
> +		/* The allocation should succeed, stop compacting */
> +		if (status == COMPACT_SUCCESS) {
>  			/*
>  			 * We think the allocation will succeed in this zone,
>  			 * but it is not certain, hence the false. The caller
> @@ -1873,8 +1872,6 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  		.ignore_skip_hint = true,
>  
>  	};
> -	bool success = false;
> -
>  	trace_mm_compaction_kcompactd_wake(pgdat->node_id, cc.order,
>  							cc.classzone_idx);
>  	count_vm_event(KCOMPACTD_WAKE);
> @@ -1903,9 +1900,7 @@ static void kcompactd_do_work(pg_data_t *pgdat)
>  			return;
>  		status = compact_zone(zone, &cc);
>  
> -		if (zone_watermark_ok(zone, cc.order, low_wmark_pages(zone),
> -						cc.classzone_idx, 0)) {
> -			success = true;
> +		if (status == COMPACT_SUCCESS) {
>  			compaction_defer_reset(zone, cc.order, false);
>  		} else if (status == COMPACT_PARTIAL_SKIPPED || status == COMPACT_COMPLETE) {
>  			/*
> -- 
> 2.9.2
> 
> --
> 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

[toc] | [prev] | [standalone]

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

Back to top | Article view | linux.kernel

csiph-web