Re: [PATCH] mm, oom: enable rate-limiting controls for oom dumps

[Petr Mladek <pmladek@suse.com>]

> Add two sysctl entries (vm.oom_dump_ratelimit and
> vm.oom_dump_ratelimit_burst) to control the rate limiting interval and
> burst, respectively, of the OOM killer output (oom_kill_process()).
> 
> These entries are disabled by default and can be enabled during kernel
> configuration with CONFIG_DUMP_RATELIMIT. They take
> DEFAULT_RATELIMIT_INTERVAL and DEFAULT_RATELIMIT_BURST as their default
> values.
> 
> Signed-off-by: Ricardo Cañuelo <ricardo.canuelo@collabora.com>
> ---
> 
> In some setups, the amount of output that the OOM killer generates when
> it kills a process and dumps the list of tasks can be too
> large. Unfortunately, the rate-limiting used for it uses the default
> values for the rate limit interval and burst. This patch allows the user
> to configure these values.

It might be pretty hard to set any reasonable values. It depends on
the console speed and the amount of processes on the system. I wonder
who many people would be able to use it in reality.

What about introducing some feedback from the printk code?

     static u64 printk_last_report_seq;

     if (consoles_seen(printk_last_report_seq)) {
	dump_header();
	printk_last_report_seq = printk_get_last_seq();
     }

By other words. It would skip the massive report when the consoles
were not able to see the previous one.


> I created a new menu selection inside "printk and dmesg options" for
> this option. Many other parts of the kernel use either hardcoded or
> default values for the rate limiting parameters. If, in the future, more
> of these cases need to be parameterized, this new submenu can be used to
> hold any new config option related to rate limiting.
> 
>  include/linux/oom.h |  2 ++
>  kernel/sysctl.c     | 16 ++++++++++++++++
>  lib/Kconfig.debug   | 13 +++++++++++++
>  mm/oom_kill.c       |  5 +++++
>  4 files changed, 36 insertions(+)
> 
> diff --git a/include/linux/oom.h b/include/linux/oom.h
> index 2db9a1432511..76c560a1a4c7 100644
> --- a/include/linux/oom.h
> +++ b/include/linux/oom.h
> @@ -127,4 +127,6 @@ extern struct task_struct *find_lock_task_mm(struct task_struct *p);
>  extern int sysctl_oom_dump_tasks;
>  extern int sysctl_oom_kill_allocating_task;
>  extern int sysctl_panic_on_oom;
> +extern int sysctl_oom_dump_ratelimit;
> +extern int sysctl_oom_dump_ratelimit_burst;
>  #endif /* _INCLUDE_LINUX_OOM_H */
> diff --git a/kernel/sysctl.c b/kernel/sysctl.c
> index ce75c67572b9..d348eac7e561 100644
> --- a/kernel/sysctl.c
> +++ b/kernel/sysctl.c
> @@ -2708,6 +2708,22 @@ static struct ctl_table vm_table[] = {
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec,
>  	},
> +#ifdef CONFIG_OOM_DUMP_RATELIMIT

I do not see a reason to have this build configurable. The options are
either useful or not.

> +	{
> +		.procname	= "oom_dump_ratelimit",
> +		.data		= &sysctl_oom_dump_ratelimit,
> +		.maxlen		= sizeof(sysctl_oom_dump_ratelimit),
> +		.mode		= 0644,
> +		.proc_handler	= proc_dointvec,
> +	},

> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -54,6 +54,8 @@
>  int sysctl_panic_on_oom;
>  int sysctl_oom_kill_allocating_task;
>  int sysctl_oom_dump_tasks = 1;
> +int sysctl_oom_dump_ratelimit = DEFAULT_RATELIMIT_INTERVAL;
> +int sysctl_oom_dump_ratelimit_burst = DEFAULT_RATELIMIT_BURST;
>  
>  /*
>   * Serializes oom killer invocations (out_of_memory()) from all contexts to
> @@ -959,6 +961,9 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>  	static DEFINE_RATELIMIT_STATE(oom_rs, DEFAULT_RATELIMIT_INTERVAL,
>  					      DEFAULT_RATELIMIT_BURST);
>  
> +	oom_rs.interval = sysctl_oom_dump_ratelimit;
> +	oom_rs.burst = sysctl_oom_dump_ratelimit_burst;

Why is _interval suffix omitted in the first variable? I find this
pretty confusing.

Best Regards,
Petr