Re: [PATCH] show message when exceeded rlimit of pending signals

[Ingo Molnar <mingo@elte.hu>]

* Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote:

> Hi Andrew,
> 
> I was glad to talk to you in Japan Linux Symposium.
> I'm writing about it.
> 
> 
> I'm working to support kernel.
> Recently, I got a inquiry about unexpected system behavior.
> I analyzed application of our customer includeing kernel.
> 
> Eventually, there was no bug in application or kernel.
> I found the cause was the limit of pending signals.
> I ran following command. and system behaved expectedly.
>    # ulimit -i unlimited
> 
> When system behaved unexpectedly, the timer_create() in application
> had returned -EAGAIN value.
> But we can't imagine the -EAGAIN means that it exceeded limit of
> pending signals at all.
> 
> Then I thought kernel should at least show some message about it.
> And I tried to create a patch.
> 
> I'm sure that system engineeres will not have to have the same experience as I did.
> How do you think about this idea ?
> 
> Thank you
> Naohiro Ooiwa.
> 
> Signed-off-by: Naohiro Ooiwa <nooiwa@miraclelinux.com>
> ---
>  kernel/signal.c |   13 +++++++++++++
>  1 files changed, 13 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 6705320..0bc4934 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -188,6 +188,9 @@ int next_signal(struct sigpending *pending, sigset_t *mask)
>  	return sig;
>  }
> 
> +#define MAX_RLIMIT_CAUTION 5
> +static int rlimit_caution_count = 0;
> +
>  /*
>   * allocate a new signal queue record
>   * - this may be called without locks if and only if t == current, otherwise an
> @@ -211,6 +214,16 @@ static struct sigqueue *__sigqueue_alloc(struct task_struct *t, gfp_t flags,
>  	    atomic_read(&user->sigpending) <=
>  			t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur)
>  		q = kmem_cache_alloc(sigqueue_cachep, flags);
> +	else {
> +		if (rlimit_caution_count <= MAX_RLIMIT_CAUTION ){
> +			printk(KERN_WARNING "reached the limit of pending signalis on pid %d\n", current->pid);
> +			/* Last time, show the advice */
> +			if (rlimit_caution_count == MAX_RLIMIT_CAUTION)
> +				printk(KERN_WARNING "If unexpected your system behavior, you can try ulimit -i unlimited\n");
> +			rlimit_caution_count++;
> +		}
> +	}
> +
>  	if (unlikely(q == NULL)) {
>  		atomic_dec(&user->sigpending);
>  		free_uid(user);

This new warning looks quite useful, i've seen several apps get into 
trouble silently due to that, again and again.

The memory overhead of the signal queue was a problem 15 years ago ... 
not so much today and people (and apps) dont expect to get in trouble 
here. So the limit and its defaults are somewhat arcane, and the 
behavior is catastrophic and hard to debug (because it's a dynamic 
failure).

Regarding the patch, i've got a few (very) small suggestions.

Firstly, please update the if / else sequence from:

	if (...)
		...
	else {
		...
	}

to:

	if (...) {
		...
	} else {
		...
	}

as we strive for curly brace symmetries.

also, a small typo: s/signalis/signals

Plus, instead of using a pre-cooked global limit print_ratelimit() could 
be used as well. That makes it useful for long-lived systems that run 
into this limit occasionally. We wont spam the log - nor will we lose 
(potentially essential) messages in the process.

Thanks,

	Ingo