Re: [PATCH] mm/vmstat: reject zero vm.stat_interval to prevent busy-loop

[Michal Hocko <mhocko@suse.com>]

On Wed 04-03-26 08:27:38, Maximilian Pezzullo via B4 Relay wrote:
> From: Maximilian Pezzullo <maximilianpezzullo@gmail.com>
> 
> Setting vm.stat_interval to 0 causes excessive kworker CPU usage
> because vmstat_shepherd() and vmstat_update() reschedule themselves
> with round_jiffies_relative(0), which resolves to an immediate
> reschedule and creates a busy-loop.
> 
> Add a custom sysctl handler that rejects 0 and restores the previous
> value, similar to how dirtytime_interval_handler() handles
> vm.dirtytime_expire_seconds.
> 
> Reported-by: Terry M <terrym3201@bugzilla.kernel.org>
> Closes: https://bugzilla.kernel.org/show_bug.cgi?id=220226
> Signed-off-by: Maximilian Pezzullo <maximilianpezzullo@gmail.com>
> ---
>  mm/vmstat.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/mm/vmstat.c b/mm/vmstat.c
> index 86b14b0f77b5..6eeb4341b215 100644
> --- a/mm/vmstat.c
> +++ b/mm/vmstat.c
> @@ -2114,6 +2114,18 @@ void vmstat_flush_workqueue(void)
>  	flush_workqueue(mm_percpu_wq);
>  }
>  
> +static int vmstat_interval_handler(const struct ctl_table *table, int write,
> +				    void *buffer, size_t *lenp, loff_t *ppos)
> +{
> +	int ret = proc_dointvec_jiffies(table, write, buffer, lenp, ppos);
> +
> +	if (ret == 0 && write && sysctl_stat_interval == 0) {
> +		sysctl_stat_interval = HZ;
> +		return -EINVAL;

So you update the value and report the failure. Nope, this is not
correct way to handle that. Either tou check the value and fail before
any side effects or you correct the value, report that to the log and
return success.

I would preffer to not do that at all. Setting any arbitrary small value
will have some side effects. This is admin only interface and we expect
those do know what they are doing.

NAK to the patch
-- 
Michal Hocko
SUSE Labs