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

On 8/4/26 14:20, Joel Granados wrote:
> On Mon, Aug 03, 2026 at 10:27:46AM +0200, Vlastimil Babka (SUSE) wrote:
>> +Cc: sysctl maintainers
>> 
>> On 8/1/26 20:44, Andrew Morton wrote:
>> > On Sat,  1 Aug 2026 23:11:25 +0800 Jianlin Shi <[email protected]> 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 two 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.
>> > 
>> > AI review suggest that this caused a new problem:
>> > 
>> > 	https://sashiko.dev/#/patchset/[email protected]
>> > 
>> > Not sure what to do here.  Perhaps pass proc_dointvec_minmax() a
>> > temporary then copy that into sysctl_lowmem_reserve_ratio if all
>> > proc_dointvec_minmax() returns "OK".
>> 
>> Seeing the v4 [1] it seems easier to keep the current fixup code until
>> proc_dointvec_minmax() is fixed.
> 
> 1. V3 -vs- V4:
>   I would prefer V4 as it actually prevents the partial write of the
>   vector in case of an error & returns that error back to user space.
>   Whereas V3 returns the error back to user space but keeps the partial
>   write.
> 
> That patterns of using a temp ctltable entry is seldom used but not
> unheard of.
> 
>> 
>> > But really this is a flaw in proc_dointvec_minmax() isn't it?  It
>> > shouldn't update the table data until all the data has been validated.
>> 
>> I agree. What do the maintainers think?
> 
> I agree. The arrays being changed are not too big so we can easily have
> a staging variable that that gets written when all validations are done.
> The only "con" that I see for this solution is for when these
> proc_handlers get used with temp variables; in these cases we will be
> staging an already staging variable.

Maybe have a variant of the function e.g. proc_dointvec_minmax() that takes
an extra parameter pointing to the staging array? That way the caller can
allocate it on stack according to its needs (like v4 does) but there's no
fiddling with a temp ctltable entry. That way the core code doesn't need a
staging variable big enough for everyone, and users can be converted to the
new variant, and there's not double staging at any point.

> I have added this to my Todos
> 
> Best
>