Re: [PATCH v5] mm/page_alloc: only update lowmem_reserve_ratio on sysctl write

[Johannes Weiner]

On Thu, Aug 06, 2026 at 04:27:34PM +0800, Jianlin Shi wrote:
> lowmem_reserve_ratio_sysctl_handler() ignores the return value of
> proc_dointvec_minmax() and always calls setup_per_zone_lowmem_reserve(),
> even for read operations.
> 
> Fix three issues:
> 
> 1. Propagate errors from proc_dointvec_minmax() instead of always
>    returning success.  For example, writing non-integer garbage to the
>    sysctl now returns an error instead of silently succeeding with
>    unchanged values.
> 
> 2. Only call setup_per_zone_lowmem_reserve() when the sysctl is
>    actually written, matching the write-only refresh pattern of
>    min_free_kbytes and watermark_scale_factor handlers.
> 
> 3. On write, parse into a temporary ratio[] array and only copy into
>    sysctl_lowmem_reserve_ratio[] and refresh derived state after the
>    full vector is validated.  This avoids leaving the ratio array
>    partially updated while skipping setup when proc_dointvec_minmax()
>    returns an error on a later element (suggested by Andrew Morton).
> 
> Drop the manual "< 1 -> 0" sanitization loop and set .extra1 =
> SYSCTL_ZERO on the ctl_table entry so proc_dointvec_minmax() enforces
> the minimum on write; negative values now return -EINVAL instead of
> being silently coerced to 0 (suggested by Vlastimil Babka).
> 
> Link: https://lore.kernel.org/linux-mm/tencent_FFD4F4D728AAE8A8AE0AF277A59854A29A06@xxxxxx/
> 
> Signed-off-by: Jianlin Shi <shijianlin11@xxxxxxxxxxx>
> ---
> Changes in v5:
> - Size temporary ratio[] with ARRAY_SIZE(sysctl_lowmem_reserve_ratio)
>   (Andrew Morton)
> 
> Changes in v4:
> - Parse writes into a temporary ratio[] array; commit and setup only on
>   full success (Andrew Morton)
> - Update handler comment: proc_dointvec_minmax() and write-only setup
> 
> Changes in v3:
> - Rewrite commit log to focus on the two tangible fixes as suggested by
>   Johannes Weiner.
> 
> Changes in v2:
> - Add .extra1 = SYSCTL_ZERO to ctl_table entry
> - Remove manual sanitization loop; negative writes now return -EINVAL
> 
> v1: https://lore.kernel.org/linux-mm/tencent_FFD4F4D728AAE8A8AE0AF277A59854A29A06@xxxxxx/
> 
> mm/page_alloc.c | 24 ++++++++++++++++--------
>  1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0387d2afd535..51f35e44132a 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6673,8 +6673,8 @@ static int sysctl_min_slab_ratio_sysctl_handler(const struct ctl_table *table, i
>  
>  /*
>   * lowmem_reserve_ratio_sysctl_handler - just a wrapper around
> - *	proc_dointvec() so that we can call setup_per_zone_lowmem_reserve()
> - *	whenever sysctl_lowmem_reserve_ratio changes.
> + *	proc_dointvec_minmax() so that we can call
> + *	setup_per_zone_lowmem_reserve() when the sysctl is written.
>   *
>   * The reserve ratio obviously has absolutely no relation with the
>   * minimum watermarks. The lowmem reserve ratio can only make sense
> @@ -6683,16 +6683,23 @@ static int sysctl_min_slab_ratio_sysctl_handler(const struct ctl_table *table, i
>  static int lowmem_reserve_ratio_sysctl_handler(const struct ctl_table *table,
>  		int write, void *buffer, size_t *length, loff_t *ppos)
>  {
> -	int i;
> +	struct ctl_table tmp = *table;
> +	int ratio[ARRAY_SIZE(sysctl_lowmem_reserve_ratio)];
> +	int rc;
>  
> -	proc_dointvec_minmax(table, write, buffer, length, ppos);
> +	if (!write)
> +		return proc_dointvec_minmax(table, write, buffer, length, ppos);
>  
> -	for (i = 0; i < MAX_NR_ZONES; i++) {
> -		if (sysctl_lowmem_reserve_ratio[i] < 1)
> -			sysctl_lowmem_reserve_ratio[i] = 0;
> -	}

This could use a comment. How about:

	/*
	 * proc_dointvec_max() works incrementally. Use a buffer
	 * and only set the values if all of them parse cleanly.
	 */

> +	memcpy(ratio, sysctl_lowmem_reserve_ratio, sizeof(ratio));
> +	tmp.data = ratio;
>  
> +	rc = proc_dointvec_minmax(&tmp, write, buffer, length, ppos);
> +	if (rc)
> +		return rc;
> +
> +	memcpy(sysctl_lowmem_reserve_ratio, ratio, sizeof(ratio));
>  	setup_per_zone_lowmem_reserve();

But otherwise, looks good to me now.

Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>