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

Naohiro Ooiwa
2009-10-31T07:58:48Z

Andrew Morton wrote:
> On Fri, 30 Oct 2009 20:36:31 +0900
> Naohiro Ooiwa <nooiwa@miraclelinux.com> wrote:
>
>> Hi Ingo,
>>
>> I wrote proper changelog entry.
>> And I resent the patch. I added KERN_INFO to printk.
>>
>>
>>
>> When the system has too many timers or too many aggregate
>> queued signals, the EAGAIN error is returned to application
>> from kernel, including timer_create().
>> It means that exceeded limit of pending signals at all.
>> But we can't imagine it.
>>
>> This patch show the message when reached limit of pending signals.
>> If you see this message and your system behaved unexpectedly,
>> you can run following command.
>> # ulimit -i unlimited
>>
>> With help from Hiroshi Shimamoto <h-shimamoto@ct.jp.nec.com>.
>>
>>
>> ...
>>
>> diff --git a/kernel/signal.c b/kernel/signal.c
>> index 6705320..50e10dc 100644
>> --- a/kernel/signal.c
>> +++ b/kernel/signal.c
>> @@ -41,6 +41,8 @@
>>
>> static struct kmem_cache *sigqueue_cachep;
>>
>> +int print_fatal_signals __read_mostly;
>> +
>> static void __user *sig_handler(struct task_struct *t, int sig)
>> {
>> return t->sighand->action[sig - 1].sa.sa_handler;
>> @@ -188,6 +190,14 @@ int next_signal(struct sigpending *pending, sigset_t *mask)
>> return sig;
>> }
>>
>> +static void show_reach_rlimit_sigpending(void)
>> +{
>> + if (!printk_ratelimit())
>> + return;
>
> printk_ratelimit() is a bad thing and we should be working toward
> removing it altogether, not adding new callers.
>
> Because it uses global state. So if subsystem A is trying to generate
> lots of printk's, subsystem B's important message might get
> accidentally suppressed.
>
> It's better to use DEFINE_RATELIMIT_STATE() and __ratelimit() directly.

Thank you for your advices.
And I was glad to talk to you in Japan Linux Symposium.

I got it, now that you mention it.
I will fix my patch.

>
>> + printk(KERN_INFO "%s/%d: reached the limit of pending signals.\n",
>> + current->comm, current->pid);
>
> I suggest that this be
>
> "reached RLIMIT_SIGPENDING"
>
> because RLIMIT_SIGPENDING is a well-understood term and concept.
>

OK, I see.

>> static void print_fatal_signal(struct pt_regs *regs, int signr)
>> {
>> - printk("%s/%d: potentially unexpected fatal signal %d.\n",
>> + printk(KERN_INFO "%s/%d: potentially unexpected fatal signal %d.\n",
>> current->comm, task_pid_nr(current), signr);
>>
>
> This is an unchangelogged, unrelated, non-backward-compatible
> user-visible change. For some people, their machine which used to
> print this warning will mysteriously stop doing so when they upgrade
> their kernels.
>
> That doesn't mean that we shouldn't make the change. But we should
> have a think about it and we shouldn't hide changes of this nature
> inside some other patch like this.
>

You are right.
I'm sorry, I shouldn't habe done it.

Thanks you.
Naohiro Ooiwa