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

Baoquan He
2020-05-14T15:10:20Z

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
>