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

[Vlastimil Babka (SUSE)]

Hm I also noticed you didn't cc everyone from PAGE ALLOCATOR section in
MAINTAINERS. Please use scripts/get_maintainers.pl next time.

On 7/31/26 05:42, 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.
> 
> Reading /proc/sys/vm/lowmem_reserve_ratio should not recompute per-zone
> lowmem_reserve[] and totalreserve_pages.  Only do so when the sysctl is
> written, matching min_free_kbytes and watermark_scale_factor handlers.
> 
> Also propagate errors from proc_dointvec_minmax() instead of ignoring
> them.
> 
> Drop the manual "< 1 -> 0" sanitization loop in the handler and set
> .extra1 = SYSCTL_ZERO on the ctl_table entry so proc_dointvec_minmax()
> enforces the minimum on write (suggested by Vlastimil Babka).

It would be fair to explain the -EINVAL change in the compatibility note
too. FWIW I consider it low-risk.

> Compatibility note:
> Previously a read also sanitized sysctl_lowmem_reserve_ratio[] and
> called setup_per_zone_lowmem_reserve(), which rewrites each zone's
> lowmem_reserve[] and recalculates pgdat->totalreserve_pages /
> totalreserve_pages (visible via /proc/zoneinfo "protection" and used
> by page allocation fallback and dirty-limit accounting).  After this
> change only a write does that.  Documentation describes the meaning of
> the ratio and the derived protection pages, but does not document any
> read side-effect.
> 
> Worst case for odd userspace that treated a read as a refresh of those
> derived values: lowmem_reserve[] and totalreserve_pages remain at their
> last written/setup values until the next write of this sysctl, or until
> another existing updater runs (e.g. adjust_managed_page_count() on
> managed-page changes, or init/watermark setup paths).  Until then,
> allocation fallback into lower zones and per-node dirtyable memory
> (node_dirtyable_memory() subtracts pgdat->totalreserve_pages) may not
> reflect a refresh that such userspace expected from the read alone.
> Normal readers that only consume the ratio array are unaffected.

Well there should be no change to the totalreserve if nobody wrote new
ratios. Other paths that may change the result do call the update as you
describe. So this should have no observable effects anyway.

However, except watermark boosting hidden here in
calculate_totalreserve_pages():

    /* we treat the high watermark as reserved pages. */
    max += high_wmark_pages(zone);

The effect of boosting on this calculation was probably overlooked. I wonder
if we should also (as a separate patch) update it to use a value that
doesn't include boosting to make it stable and deterministic.

> Changes in v2:
> - Add .extra1 = SYSCTL_ZERO to the ctl_table entry
> - Remove the manual sanitization loop (negative writes now return
>   -EINVAL instead of being silently coerced to 0)

Changes normally go under to the diffstat area* and are not part of commit
log. Hence the suggestion for the note for -EINVAL to be elsewhere.

> 
> Link: https://lore.kernel.org/linux-mm/tencent_FFD4F4D728AAE8A8AE0AF277A59854A29A06@xxxxxx/
> 
> Signed-off-by: Jianlin Shi <shijianlin11@xxxxxxxxxxx>
> ---

*here

Otherwise LGTM.
Reviewed-by: Vlastimil Babka (SUSE) <vbabka@xxxxxxxxxx>


>  mm/page_alloc.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 0387d2afd..a7381327d 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -6683,16 +6683,15 @@ 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;
> +	int rc;
>  
> -	proc_dointvec_minmax(table, write, buffer, length, ppos);
> +	rc = proc_dointvec_minmax(table, write, buffer, length, ppos);
> +	if (rc)
> +		return rc;
>  
> -	for (i = 0; i < MAX_NR_ZONES; i++) {
> -		if (sysctl_lowmem_reserve_ratio[i] < 1)
> -			sysctl_lowmem_reserve_ratio[i] = 0;
> -	}
> +	if (write)
> +		setup_per_zone_lowmem_reserve();
>  
> -	setup_per_zone_lowmem_reserve();
>  	return 0;
>  }
>  
> @@ -6791,6 +6790,7 @@ static const struct ctl_table page_alloc_sysctl_table[] = {
>  		.maxlen		= sizeof(sysctl_lowmem_reserve_ratio),
>  		.mode		= 0644,
>  		.proc_handler	= lowmem_reserve_ratio_sysctl_handler,
> +		.extra1		= SYSCTL_ZERO,
>  	},
>  #ifdef CONFIG_NUMA
>  	{