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

[Michal Hocko]

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 
> >> 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...@alibaba-inc.com: fix missing pointer dereferences]
> >> Signed-off-by: Vlastimil Babka 
> >> Acked-by: Hillf Danton 
> > 
> > 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

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

[Michal Hocko]

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 
> >> 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...@alibaba-inc.com: fix missing pointer dereferences]
> >> Signed-off-by: Vlastimil Babka 
> >> Acked-by: Hillf Danton 
> > 
> > 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

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

[Vlastimil Babka]

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 
>> 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...@alibaba-inc.com: fix missing pointer dereferences]
>> Signed-off-by: Vlastimil Babka 
>> Acked-by: Hillf Danton 
> 
> 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/

> 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 
> 
> 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, _progress_loops))
>>  goto retry;
>>  
>>  /*
>> -- 
>> 2.10.0
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majord...@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 
> 

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

[Vlastimil Babka]

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 
>> 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...@alibaba-inc.com: fix missing pointer dereferences]
>> Signed-off-by: Vlastimil Babka 
>> Acked-by: Hillf Danton 
> 
> 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/

> 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 
> 
> 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, _progress_loops))
>>  goto retry;
>>  
>>  /*
>> -- 
>> 2.10.0
>>
>>
>> --
>> To unsubscribe, send a message with 'unsubscribe linux-mm' in
>> the body to majord...@kvack.org.  For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 
> 

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

[Michal Hocko]

On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
[...]
> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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...@alibaba-inc.com: fix missing pointer dereferences]
> Signed-off-by: Vlastimil Babka 
> Acked-by: Hillf Danton 

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 

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, _progress_loops))
>   goto retry;
>  
>   /*
> -- 
> 2.10.0
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Fri 23-09-16 08:55:33, Vlastimil Babka wrote:
[...]
> >From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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...@alibaba-inc.com: fix missing pointer dereferences]
> Signed-off-by: Vlastimil Babka 
> Acked-by: Hillf Danton 

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 

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, _progress_loops))
>   goto retry;
>  
>   /*
> -- 
> 2.10.0
> 
> 
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majord...@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: mailto:"d...@kvack.org;> em...@kvack.org 

-- 
Michal Hocko
SUSE Labs

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

[Vlastimil Babka]

On 09/23/2016 06:04 AM, Hillf Danton wrote:
>>
>> 8<
>> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka 
>> 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 
>> ---
>>  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 

Thanks!

8<
>From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka 
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...@alibaba-inc.com: fix missing pointer dereferences]
Signed-off-by: Vlastimil Babka 
Acked-by: Hillf Danton 
---
 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++;
-

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

[Vlastimil Babka]

On 09/23/2016 06:04 AM, Hillf Danton wrote:
>>
>> 8<
>> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
>> From: Vlastimil Babka 
>> 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 
>> ---
>>  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 

Thanks!

8<
>From 1623d5bd441160569ffad3808aeeec852048e558 Mon Sep 17 00:00:00 2001
From: Vlastimil Babka 
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...@alibaba-inc.com: fix missing pointer dereferences]
Signed-off-by: Vlastimil Babka 
Acked-by: Hillf Danton 
---
 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, 

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

[Hillf Danton]

> 
> 8<
> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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 
> ---
>  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 

> +
> + /*
>* 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, _progress_loops))
>   goto retry;
> 
>   /*
> --
> 2.10.0

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

[Hillf Danton]

> 
> 8<
> From a7921e57ba1189b9c08fc4879358a908c390e47c Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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 
> ---
>  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 

> +
> + /*
>* 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, _progress_loops))
>   goto retry;
> 
>   /*
> --
> 2.10.0

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

[Vlastimil Babka]

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 
>> > 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 
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 
---
 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, _progress_loops))
goto retry;
 
/*
-- 
2.10.0

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

[Vlastimil Babka]

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 
>> > 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 
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 
---
 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, _progress_loops))
goto retry;
 
/*
-- 
2.10.0

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

[Vlastimil Babka]

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 
>> > 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 
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 
Signed-off-by: Vlastimil Babka 
Acked-by: Michal Hocko 
---
 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, _priority,
-   compaction_retries))
+   _retries))
goto retry;
 
/* Reclaim has failed us, start killing things */
-- 
2.10.0

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

[Vlastimil Babka]

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 
>> > 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 
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 
Signed-off-by: Vlastimil Babka 
Acked-by: Michal Hocko 
---
 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, _priority,
-   compaction_retries))
+   _retries))
goto retry;
 
/* Reclaim has failed us, start killing things */
-- 
2.10.0

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

[Michal Hocko]

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 
> > 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 
> > Signed-off-by: Vlastimil Babka 
> 
> Anyway
> Acked-by: Michal Hocko 
> 
> > ---
> >  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, _priority,
> > -   compaction_retries))
> > +   _retries))
> > goto retry;
> >  
> > /* Reclaim has failed us, start killing things */
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

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 
> > 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 
> > Signed-off-by: Vlastimil Babka 
> 
> Anyway
> Acked-by: Michal Hocko 
> 
> > ---
> >  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, _priority,
> > -   compaction_retries))
> > +   _retries))
> > goto retry;
> >  
> > /* Reclaim has failed us, start killing things */
> > -- 
> > 2.10.0
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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 
> Signed-off-by: Vlastimil Babka 

Anyway
Acked-by: Michal Hocko 

> ---
>  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, _priority,
> - compaction_retries))
> + _retries))
>   goto retry;
>  
>   /* Reclaim has failed us, start killing things */
> -- 
> 2.10.0
> 

-- 
Michal Hocko
SUSE Labs

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

[Michal Hocko]

On Thu 22-09-16 14:51:48, Vlastimil Babka wrote:
> >From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
> From: Vlastimil Babka 
> 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 
> Signed-off-by: Vlastimil Babka 

Anyway
Acked-by: Michal Hocko 

> ---
>  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, _priority,
> - compaction_retries))
> + _retries))
>   goto retry;
>  
>   /* Reclaim has failed us, start killing things */
> -- 
> 2.10.0
> 

-- 
Michal Hocko
SUSE Labs

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

[Vlastimil Babka]

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 

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

8<
>From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
From: Vlastimil Babka 
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 
Signed-off-by: Vlastimil Babka 
---
 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, _priority,
-   compaction_retries))
+   _retries))
goto retry;
 
/* Reclaim has failed us, start killing things */
-- 
2.10.0

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

[Vlastimil Babka]

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 

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

8<
>From 465e1bd61b7a6d6901a44f09b1a76514dbc220fa Mon Sep 17 00:00:00 2001
From: Vlastimil Babka 
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 
Signed-off-by: Vlastimil Babka 
---
 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, _priority,
-   compaction_retries))
+   _retries))
goto retry;
 
/* Reclaim has failed us, start killing things */
-- 
2.10.0

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

[Michal Hocko]

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 

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

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

[Michal Hocko]

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 

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

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

[Vlastimil Babka]

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 
Cc: Michal Hocko 
Cc: Mel Gorman 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Rik van Riel 
---
 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

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

[Vlastimil Babka]

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 
Cc: Michal Hocko 
Cc: Mel Gorman 
Cc: Joonsoo Kim 
Cc: David Rientjes 
Cc: Rik van Riel 
---
 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