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

Groups > linux.kernel > #1463431 > unrolled thread

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

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 v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Joonsoo Kim <iamjoonsoo.kim@lge.com> - 2016-08-16 08:10 +0200
    Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-16 08:20 +0200
    Re: [PATCH v6 04/11] mm, compaction: don't recheck watermarks after  COMPACT_SUCCESS Vlastimil Babka <vbabka@suse.cz> - 2016-08-18 14:00 +0200

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

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

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

Thanks.


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

[toc] | [next] | [standalone]

#1463439

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

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

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

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

Thanks.

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

#1465202

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

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

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

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

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web