Re: [PATCH 2/4] mm, compaction: more reliably increase direct compaction priority
From: Vlastimil Babka
Date: Fri Sep 23 2016 - 06:48:14 EST
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@xxxxxxx>
>> 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@xxxxxxxxxxxxxxx: fix missing pointer dereferences]
>> Signed-off-by: Vlastimil Babka <vbabka@xxxxxxx>
>> Acked-by: Hillf Danton <hillf.zj@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxx>
> 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@xxxxxxxx>
>
> 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@xxxxxxxxxx For more info on Linux MM,
>> see: http://www.linux-mm.org/ .
>> Don't email: <a href=mailto:"dont@xxxxxxxxx"> email@xxxxxxxxx </a>
>