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

Groups > linux.kernel > #1488291 > unrolled thread

Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority

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 2/4] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-09-21 19:20 +0200
    Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-09-22 15:00 +0200
      Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-09-22 16:10 +0200
        Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-09-22 17:00 +0200
          Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-09-22 17:10 +0200
            Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority "Hillf Danton" <hillf.zj@alibaba-inc.com> - 2016-09-23 06:20 +0200
              Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-09-23 09:00 +0200
                Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-09-23 10:30 +0200
                  Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-09-23 12:50 +0200
                    Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Michal Hocko <mhocko@kernel.org> - 2016-09-23 14:10 +0200
          Re: [PATCH 2/4] mm, compaction: more reliably increase direct  compaction priority Vlastimil Babka <vbabka@suse.cz> - 2016-09-22 17:10 +0200

#1488291 — Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority

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

[toc] | [next] | [standalone]

#1488862

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

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

#1488921

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

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

#1488962

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

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

#1488965

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

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

#1489742 — Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority

> 
> ----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

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

#1489794

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

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

#1489841

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

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

#1489938

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>
> 

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

#1489982

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

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

#1488966

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web