Re: [PATCH v2] mm, page_alloc: reset the zone->watermark_boost early

[Baoquan He]

On 05/14/20 at 05:28pm, Charan Teja Reddy wrote:
> Updating the zone watermarks by any means, like min_free_kbytes,
> water_mark_scale_factor e.t.c, when ->watermark_boost is set will result
> into the higher low and high watermarks than the user asked.
> 
> Below are the steps pursued to reproduce the problem on system setup
> of Android kernel running on Snapdragon hardware.
> 1) Default settings of the system are as below:
>    #cat /proc/sys/vm/min_free_kbytes = 5162
>    #cat /proc/zoneinfo | grep -e boost -e low -e "high " -e min -e Node
> 	Node 0, zone   Normal
> 		min      797
> 		low      8340
> 		high     8539
> 
> 2) Monitor the zone->watermark_boost(by adding a debug print in
> the kernel) and whenever it is greater than zero value, write the
> same value of min_free_kbytes obtained from step 1.
>    #echo 5162 > /proc/sys/vm/min_free_kbytes
> 
> 3) Then read the zone watermarks in the system while the
> ->watermark_boost is zero. This should show the same values of
> watermarks as step 1 but shown a higher values than asked.
>    #cat /proc/zoneinfo | grep -e boost -e low -e "high " -e min -e Node
> 	Node 0, zone   Normal
> 		min      797
> 		low      21148
> 		high     21347
> 
> These higher values are because of updating the zone watermarks using
> the macro min_wmark_pages(zone) which also adds the
> zone->watermark_boost.
> 	#define min_wmark_pages(z) (z->_watermark[WMARK_MIN] +
> 					z->watermark_boost)
> 
> So the steps that lead to the issue is like below:
> 1) On the extfrag event, watermarks are boosted by storing the required
> value in ->watermark_boost.
> 
> 2) User tries to update the zone watermarks level in the system through
> min_free_kbytes or watermark_scale_factor.

> 
> 3) Later, when kswapd woke up, it resets the zone->watermark_boost to
> zero.
> 
> In step 2), we use the min_wmark_pages() macro to store the watermarks
> in the zone structure thus the values are always offsetted by
> ->watermark_boost value. This can be avoided by resetting the
> ->watermark_boost to zero before it is used.
> 
> Signed-off-by: Charan Teja Reddy <charante@xxxxxxxxxxxxxx>
> ---
> 
> v2: Improve the commit message
> 
> v1: (https://patchwork.kernel.org/patch/11540751/)
> 
>  mm/page_alloc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index cef05d3..d001d61 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7761,9 +7761,9 @@ static void __setup_per_zone_wmarks(void)
>  			    mult_frac(zone_managed_pages(zone),
>  				      watermark_scale_factor, 10000));
>  
> +		zone->watermark_boost = 0;
>  		zone->_watermark[WMARK_LOW]  = min_wmark_pages(zone) + tmp;
>  		zone->_watermark[WMARK_HIGH] = min_wmark_pages(zone) + tmp * 2;
> -		zone->watermark_boost = 0;

Yeah, watermark_boost is a temporary value which is used for reclaim,
and will be reset after reclaim finished. Here we should respect the
watermark setting from user.

This fix looks good to me.

Reviewed-by: Baoquan He <bhe@xxxxxxxxxx>



>  
>  		spin_unlock_irqrestore(&zone->lock, flags);
>  	}
> -- 
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by The Linux Foundation
>