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

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

[Charan Teja Reddy]

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@codeaurora.org>
---

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

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@codeaurora.org>
> ---
> 
> 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@redhat.com>



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