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

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 <[email protected]>.

Signed-off-by: Naohiro Ooiwa <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
Documentation/kernel-parameters.txt | 11 +++++++++--
kernel/signal.c | 20 ++++++++++++++++----
2 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/Documentation/kernel-parameters.txt b/Documentation/kernel-parameters.txt
index 9107b38..3bbd92f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2032,8 +2032,15 @@ and is between 256 and 4096 characters. It is defined in the file

print-fatal-signals=
[KNL] debug: print fatal signals
- print-fatal-signals=1: print segfault info to
- the kernel console.
+
+ If enabled, warn about various signal handling
+ related application anomalies: too many signals,
+ too many POSIX.1 timers, fatal signals causing a
+ coredump - etc.
+
+ If you hit the warning due to signal overflow,
+ you might want to try "ulimit -i unlimited".
+
default: off.

printk.time= Show timing data prefixed to each printk message line
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(KERN_INFO "%s/%d: reached the limit of pending signals.\n",
+ current->comm, current->pid);
+}
+
/*
* allocate a new signal queue record
* - this may be called without locks if and only if t == current, otherwise an
@@ -209,8 +219,12 @@ static struct sigqueue *__sigqueue_alloc(struct task_struct *t, gfp_t flags,
atomic_inc(&user->sigpending);
if (override_rlimit ||
atomic_read(&user->sigpending) <=
- t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur)
+ t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur) {
q = kmem_cache_alloc(sigqueue_cachep, flags);
+ } else {
+ if (print_fatal_signals)
+ show_reach_rlimit_sigpending();
+ }
if (unlikely(q == NULL)) {
atomic_dec(&user->sigpending);
free_uid(user);
@@ -925,11 +939,9 @@ static int send_signal(int sig, struct siginfo *info, struct task_struct *t,
return __send_signal(sig, info, t, group, from_ancestor_ns);
}

-int print_fatal_signals;
-
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);

#if defined(__i386__) && !defined(__arch_um__)


2009-10-30 21:35:10

by Andrew Morton

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

On Fri, 30 Oct 2009 20:36:31 +0900
Naohiro Ooiwa <[email protected]> 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 <[email protected]>.
>
>
> ...
>
> 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.


> + 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.

> 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.

2009-10-30 21:45:58

by Joe Perches

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

On Fri, 2009-10-30 at 14:33 -0700, Andrew Morton wrote:
> On Fri, 30 Oct 2009 20:36:31 +0900
> Naohiro Ooiwa <[email protected]> wrote:
> > +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.

http://lkml.org/lkml/2009/9/21/323

I think there should be a generic kernel.h macro for this.
Something like:

#define printk_ratelimited(fmt, arg...) \
({ static struct ratelimit_state _rs = { \
.interval = DEFAULT_RATELIMIT_INTERVAL, \
.burst = DEFAULT_RATELIMIT_BURST, \
}; \
int rtn; \
\
if (!__ratelimit(&_rs)) \
rtn = printk(fmt, ##arg); \
else \
rtn = 0; \
rtn; \
})
#define pr_info_rl(fmt, arg) \
printk_ratelimited(KERN_INFO pr_fmt(fmt), ##arg)
etc...


2009-10-30 23:21:48

by Joe Perches

[permalink] [raw]
Subject: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

Add a printk_ratelimited statement expression macro
that uses a per-call ratelimit_state so that multiple
subsystems output messages are not suppressed by a global
__ratelimit state.

Signed-off-by: Joe Perches <[email protected]>

diff --git a/include/linux/kernel.h b/include/linux/kernel.h
index f4e3184..555560c 100644
--- a/include/linux/kernel.h
+++ b/include/linux/kernel.h
@@ -407,6 +407,50 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
#endif

/*
+ * ratelimited messages with local ratelimit_state,
+ * no local ratelimit_state used in the !PRINTK case
+ */
+#ifdef CONFIG_PRINTK
+#define printk_ratelimited(fmt, ...) ({ \
+ static struct ratelimit_state _rs = { \
+ .interval = DEFAULT_RATELIMIT_INTERVAL, \
+ .burst = DEFAULT_RATELIMIT_BURST, \
+ }; \
+ \
+ if (!__ratelimit(&_rs)) \
+ printk(fmt, ##__VA_ARGS__); \
+})
+#else
+/* No effect, but we still get type checking even in the !PRINTK case: */
+#define printk_ratelimited printk
+#endif
+
+#define pr_emerg_rl(fmt, ...) \
+ printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_alert_rl(fmt, ...) \
+ printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_crit_rl(fmt, ...) \
+ printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_err_rl(fmt, ...) \
+ printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_warning_rl(fmt, ...) \
+ printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_notice_rl(fmt, ...) \
+ printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
+#define pr_info_rl(fmt, ...) \
+ printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
+/* no pr_cont_rl, don't do that... */
+/* If you are writing a driver, please use dev_dbg instead */
+#if defined(DEBUG)
+#define pr_debug_rl(fmt, ...) \
+ printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
+#else
+#define pr_debug_rl(fmt, ...) \
+ ({ if (0) printk_ratelimited(KERN_DEBUG pr_fmt(fmt), \
+ ##__VA_ARGS__); 0; })
+#endif
+
+/*
* General tracing related utility functions - trace_printk(),
* tracing_on/tracing_off and tracing_start()/tracing_stop
*

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

Andrew Morton wrote:
> On Fri, 30 Oct 2009 20:36:31 +0900
> Naohiro Ooiwa <[email protected]> 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 <[email protected]>.
>>
>>
>> ...
>>
>> 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

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

Naohiro Ooiwa wrote:
> Andrew Morton wrote:
>> On Fri, 30 Oct 2009 20:36:31 +0900
>> Naohiro Ooiwa <[email protected]> wrote:
>>>
>>> +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.

I fixed my patch.
Could you please check it.

Thanks you.
Naohiro Ooiwa


Signed-off-by: Naohiro Ooiwa <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
Documentation/kernel-parameters.txt | 11 +++++++++--
kernel/signal.c | 21 ++++++++++++++++++---
2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 9107b38..3bbd92f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2032,8 +2032,15 @@ and is between 256 and 4096 characters. It is defined in
the file

print-fatal-signals=
[KNL] debug: print fatal signals
- print-fatal-signals=1: print segfault info to
- the kernel console.
+
+ If enabled, warn about various signal handling
+ related application anomalies: too many signals,
+ too many POSIX.1 timers, fatal signals causing a
+ coredump - etc.
+
+ If you hit the warning due to signal overflow,
+ you might want to try "ulimit -i unlimited".
+
default: off.

printk.time= Show timing data prefixed to each printk message line
diff --git a/kernel/signal.c b/kernel/signal.c
index 6705320..624a626 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,17 @@ int next_signal(struct sigpending *pending, sigset_t *mask)
return sig;
}

+static void show_reach_rlimit_sigpending(void)
+{
+ DEFINE_RATELIMIT_STATE(printk_rl_state, 5 * HZ, 10);
+
+ if (!__ratelimit(&printk_rl_state))
+ return;
+
+ printk(KERN_INFO "%s/%d: reached RLIMIT_SIGPENDING.\n",
+ current->comm, current->pid);
+}
+
/*
* allocate a new signal queue record
* - this may be called without locks if and only if t == current, otherwise an
@@ -209,8 +222,12 @@ static struct sigqueue *__sigqueue_alloc(struct task_struct
*t, gfp_t flags,
atomic_inc(&user->sigpending);
if (override_rlimit ||
atomic_read(&user->sigpending) <=
- t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur)
+ t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur) {
q = kmem_cache_alloc(sigqueue_cachep, flags);
+ } else {
+ if (print_fatal_signals)
+ show_reach_rlimit_sigpending();
+ }
if (unlikely(q == NULL)) {
atomic_dec(&user->sigpending);
free_uid(user);
@@ -925,8 +942,6 @@ static int send_signal(int sig, struct siginfo *info, struct
task_struct *t,
return __send_signal(sig, info, t, group, from_ancestor_ns);
}

-int print_fatal_signals;
-
static void print_fatal_signal(struct pt_regs *regs, int signr)
{
printk("%s/%d: potentially unexpected fatal signal %d.\n",

2009-10-31 08:58:20

by Andrew Morton

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

On Sat, 31 Oct 2009 17:50:14 +0900 Naohiro Ooiwa <[email protected]> wrote:

> Naohiro Ooiwa wrote:
> > Andrew Morton wrote:
> >> On Fri, 30 Oct 2009 20:36:31 +0900
> >> Naohiro Ooiwa <[email protected]> wrote:
> >>>
> >>> +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.
>
> I fixed my patch.
> Could you please check it.
>

Please always include the full changelog and signoff with each
iteration of a patch. That changelog might of course need updating as
the patch changes.


> ---
> Documentation/kernel-parameters.txt | 11 +++++++++--
> kernel/signal.c | 21 ++++++++++++++++++---
> 2 files changed, 27 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index 9107b38..3bbd92f 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -2032,8 +2032,15 @@ and is between 256 and 4096 characters. It is defined in
> the file
>
> print-fatal-signals=
> [KNL] debug: print fatal signals
> - print-fatal-signals=1: print segfault info to
> - the kernel console.
> +
> + If enabled, warn about various signal handling
> + related application anomalies: too many signals,
> + too many POSIX.1 timers, fatal signals causing a
> + coredump - etc.
> +
> + If you hit the warning due to signal overflow,
> + you might want to try "ulimit -i unlimited".
> +
> default: off.
>
> printk.time= Show timing data prefixed to each printk message line
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 6705320..624a626 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,17 @@ int next_signal(struct sigpending *pending, sigset_t *mask)
> return sig;
> }
>
> +static void show_reach_rlimit_sigpending(void)
> +{
> + DEFINE_RATELIMIT_STATE(printk_rl_state, 5 * HZ, 10);

This needs to have `static' storage. This bug should have been
apparent in your testing?

> + if (!__ratelimit(&printk_rl_state))
> + return;
> +
> + printk(KERN_INFO "%s/%d: reached RLIMIT_SIGPENDING.\n",
> + current->comm, current->pid);
> +}
> ...
>

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

Andrew Morton wrote:
> On Sat, 31 Oct 2009 17:50:14 +0900 Naohiro Ooiwa <[email protected]> wrote:
>
>> Naohiro Ooiwa wrote:
>>> Andrew Morton wrote:
>>>> On Fri, 30 Oct 2009 20:36:31 +0900
>>>> Naohiro Ooiwa <[email protected]> wrote:
>
> Please always include the full changelog and signoff with each
> iteration of a patch. That changelog might of course need updating as
> the patch changes.
>

I'm sorry...
I will be very careful from the next time.

>>
>> +static void show_reach_rlimit_sigpending(void)
>> +{
>> + DEFINE_RATELIMIT_STATE(printk_rl_state, 5 * HZ, 10);
>
> This needs to have `static' storage. This bug should have been
> apparent in your testing?
>

Again, I'm sorry, I failed to make sure.
But right now, I have test environment.
I tested my patch, result is good.

Thanks you.
Naohiro Ooiwa


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 <[email protected]>.


Signed-off-by: Naohiro Ooiwa <[email protected]>
Acked-by: Ingo Molnar <[email protected]>
---
Documentation/kernel-parameters.txt | 11 +++++++++--
kernel/signal.c | 21 ++++++++++++++++++---
2 files changed, 27 insertions(+), 5 deletions(-)

diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 9107b38..3bbd92f 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -2032,8 +2032,15 @@ and is between 256 and 4096 characters. It is defined in
the file

print-fatal-signals=
[KNL] debug: print fatal signals
- print-fatal-signals=1: print segfault info to
- the kernel console.
+
+ If enabled, warn about various signal handling
+ related application anomalies: too many signals,
+ too many POSIX.1 timers, fatal signals causing a
+ coredump - etc.
+
+ If you hit the warning due to signal overflow,
+ you might want to try "ulimit -i unlimited".
+
default: off.

printk.time= Show timing data prefixed to each printk message line
diff --git a/kernel/signal.c b/kernel/signal.c
index 6705320..56e9c00 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,17 @@ int next_signal(struct sigpending *pending, sigset_t *mask)
return sig;
}

+static void show_reach_rlimit_sigpending(void)
+{
+ static DEFINE_RATELIMIT_STATE(printk_rl_state, 5 * HZ, 10);
+
+ if (!__ratelimit(&printk_rl_state))
+ return;
+
+ printk(KERN_INFO "%s/%d: reached RLIMIT_SIGPENDING.\n",
+ current->comm, current->pid);
+}
+
/*
* allocate a new signal queue record
* - this may be called without locks if and only if t == current, otherwise an
@@ -209,8 +222,12 @@ static struct sigqueue *__sigqueue_alloc(struct task_struct
*t, gfp_t flags,
atomic_inc(&user->sigpending);
if (override_rlimit ||
atomic_read(&user->sigpending) <=
- t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur)
+ t->signal->rlim[RLIMIT_SIGPENDING].rlim_cur) {
q = kmem_cache_alloc(sigqueue_cachep, flags);
+ } else {
+ if (print_fatal_signals)
+ show_reach_rlimit_sigpending();
+ }
if (unlikely(q == NULL)) {
atomic_dec(&user->sigpending);
free_uid(user);
@@ -925,8 +942,6 @@ static int send_signal(int sig, struct siginfo *info, struct
task_struct *t,
return __send_signal(sig, info, t, group, from_ancestor_ns);
}

-int print_fatal_signals;
-
static void print_fatal_signal(struct pt_regs *regs, int signr)
{
printk("%s/%d: potentially unexpected fatal signal %d.\n",
-- 1.5.4.1

2009-11-02 15:59:27

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl


* Joe Perches <[email protected]> wrote:

> Add a printk_ratelimited statement expression macro that uses a
> per-call ratelimit_state so that multiple subsystems output messages
> are not suppressed by a global __ratelimit state.
>
> Signed-off-by: Joe Perches <[email protected]>
>
> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index f4e3184..555560c 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -407,6 +407,50 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
> #endif
>
> /*
> + * ratelimited messages with local ratelimit_state,
> + * no local ratelimit_state used in the !PRINTK case
> + */
> +#ifdef CONFIG_PRINTK
> +#define printk_ratelimited(fmt, ...) ({ \
> + static struct ratelimit_state _rs = { \
> + .interval = DEFAULT_RATELIMIT_INTERVAL, \
> + .burst = DEFAULT_RATELIMIT_BURST, \
> + }; \
> + \
> + if (!__ratelimit(&_rs)) \
> + printk(fmt, ##__VA_ARGS__); \
> +})
> +#else
> +/* No effect, but we still get type checking even in the !PRINTK case: */
> +#define printk_ratelimited printk
> +#endif
> +
> +#define pr_emerg_rl(fmt, ...) \
> + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_alert_rl(fmt, ...) \
> + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_crit_rl(fmt, ...) \
> + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_err_rl(fmt, ...) \
> + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_warning_rl(fmt, ...) \
> + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_notice_rl(fmt, ...) \
> + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info_rl(fmt, ...) \
> + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> +/* no pr_cont_rl, don't do that... */
> +/* If you are writing a driver, please use dev_dbg instead */
> +#if defined(DEBUG)
> +#define pr_debug_rl(fmt, ...) \
> + printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
> +#else
> +#define pr_debug_rl(fmt, ...) \
> + ({ if (0) printk_ratelimited(KERN_DEBUG pr_fmt(fmt), \
> + ##__VA_ARGS__); 0; })
> +#endif
> +

Looks like a useful addition. Somewhat bloatier, but then again, more
correct and the bloat problem can be solved by explicit state
definitions.

Ingo

Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

Ingo Molnar wrote:
> * Joe Perches <[email protected]> wrote:
>
>> Add a printk_ratelimited statement expression macro that uses a
>> per-call ratelimit_state so that multiple subsystems output messages
>> are not suppressed by a global __ratelimit state.
>>
>> Signed-off-by: Joe Perches <[email protected]>
>>
>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>> index f4e3184..555560c 100644
>> --- a/include/linux/kernel.h
>> +++ b/include/linux/kernel.h
>> @@ -407,6 +407,50 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
>> #endif
>>
>> /*
>> + * ratelimited messages with local ratelimit_state,
>> + * no local ratelimit_state used in the !PRINTK case
>> + */
>> +#ifdef CONFIG_PRINTK
>> +#define printk_ratelimited(fmt, ...) ({ \
>> + static struct ratelimit_state _rs = { \
>> + .interval = DEFAULT_RATELIMIT_INTERVAL, \
>> + .burst = DEFAULT_RATELIMIT_BURST, \
>> + }; \
>> + \
>> + if (!__ratelimit(&_rs)) \
>> + printk(fmt, ##__VA_ARGS__); \
>> +})
>> +#else
>> +/* No effect, but we still get type checking even in the !PRINTK case: */
>> +#define printk_ratelimited printk
>> +#endif
>> +
>> +#define pr_emerg_rl(fmt, ...) \
>> + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_alert_rl(fmt, ...) \
>> + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_crit_rl(fmt, ...) \
>> + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_err_rl(fmt, ...) \
>> + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_warning_rl(fmt, ...) \
>> + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_notice_rl(fmt, ...) \
>> + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
>> +#define pr_info_rl(fmt, ...) \
>> + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>> +/* no pr_cont_rl, don't do that... */
>> +/* If you are writing a driver, please use dev_dbg instead */
>> +#if defined(DEBUG)
>> +#define pr_debug_rl(fmt, ...) \
>> + printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>> +#else
>> +#define pr_debug_rl(fmt, ...) \
>> + ({ if (0) printk_ratelimited(KERN_DEBUG pr_fmt(fmt), \
>> + ##__VA_ARGS__); 0; })
>> +#endif
>> +
>
> Looks like a useful addition. Somewhat bloatier, but then again, more
> correct and the bloat problem can be solved by explicit state
> definitions.
>
> Ingo

I waiting for this patch to merge.
And then, I think I will remake my patch.

How do you delete printk_ratelimit() in this patch at a same time ?

I have a personal question.
Why aren't they codes in the include/linux/ratelimit.h ?


Thanks.
Naohiro Ooiwa

Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

Naohiro Ooiwa wrote:
> Ingo Molnar wrote:
>> * Joe Perches <[email protected]> wrote:
>>
>>> Add a printk_ratelimited statement expression macro that uses a
>>> per-call ratelimit_state so that multiple subsystems output messages
>>> are not suppressed by a global __ratelimit state.
>>>
>>> Signed-off-by: Joe Perches <[email protected]>
>>>
>>> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
>>> index f4e3184..555560c 100644
>>> --- a/include/linux/kernel.h
>>> +++ b/include/linux/kernel.h
>>> @@ -407,6 +407,50 @@ static inline char *pack_hex_byte(char *buf, u8 byte)
>>> #endif
>>>
>>> /*
>>> + * ratelimited messages with local ratelimit_state,
>>> + * no local ratelimit_state used in the !PRINTK case
>>> + */
>>> +#ifdef CONFIG_PRINTK
>>> +#define printk_ratelimited(fmt, ...) ({ \
>>> + static struct ratelimit_state _rs = { \
>>> + .interval = DEFAULT_RATELIMIT_INTERVAL, \
>>> + .burst = DEFAULT_RATELIMIT_BURST, \
>>> + }; \
>>> + \
>>> + if (!__ratelimit(&_rs)) \
>>> + printk(fmt, ##__VA_ARGS__); \
>>> +})
>>> +#else
>>> +/* No effect, but we still get type checking even in the !PRINTK case: */
>>> +#define printk_ratelimited printk
>>> +#endif
>>> +
>>> +#define pr_emerg_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_alert_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_crit_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_err_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_warning_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_notice_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_info_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>>> +/* no pr_cont_rl, don't do that... */
>>> +/* If you are writing a driver, please use dev_dbg instead */
>>> +#if defined(DEBUG)
>>> +#define pr_debug_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_DEBUG pr_fmt(fmt), ##__VA_ARGS__)
>>> +#else
>>> +#define pr_debug_rl(fmt, ...) \
>>> + ({ if (0) printk_ratelimited(KERN_DEBUG pr_fmt(fmt), \
>>> + ##__VA_ARGS__); 0; })
>>> +#endif
>>> +
>> Looks like a useful addition. Somewhat bloatier, but then again, more
>> correct and the bloat problem can be solved by explicit state
>> definitions.
>>
>> Ingo
>
> I waiting for this patch to merge.
> And then, I think I will remake my patch.
>
> How do you delete printk_ratelimit() in this patch at a same time ?
>
> I have a personal question.
> Why aren't they codes in the include/linux/ratelimit.h ?
>

Ah, They are originally in kernel.h . Sorry.

>
> Thanks.
> Naohiro Ooiwa
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

2009-11-09 21:49:57

by Andrew Morton

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

On Fri, 30 Oct 2009 16:21:47 -0700
Joe Perches <[email protected]> wrote:

> +#define pr_emerg_rl(fmt, ...) \
> + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_alert_rl(fmt, ...) \
> + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_crit_rl(fmt, ...) \
> + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_err_rl(fmt, ...) \
> + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_warning_rl(fmt, ...) \
> + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_notice_rl(fmt, ...) \
> + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> +#define pr_info_rl(fmt, ...) \
> + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)

Would prefer pr_emerg_ratelimited personally. It's longer, but one
doesn't ask "wtf does _rl" mean and it avoids having two identifiers
which refer to the same thing.

2009-11-09 22:05:18

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

On Mon, 2009-11-09 at 13:49 -0800, Andrew Morton wrote:
> On Fri, 30 Oct 2009 16:21:47 -0700
> Joe Perches <[email protected]> wrote:
> > +#define pr_emerg_rl(fmt, ...) \
> > + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_alert_rl(fmt, ...) \
> > + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_crit_rl(fmt, ...) \
> > + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_err_rl(fmt, ...) \
> > + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_warning_rl(fmt, ...) \
> > + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_notice_rl(fmt, ...) \
> > + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_info_rl(fmt, ...) \
> > + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>
> Would prefer pr_emerg_ratelimited personally. It's longer, but one
> doesn't ask "wtf does _rl" mean and it avoids having two identifiers
> which refer to the same thing.

I don't have a strong opinion either way.
_rl is shorter and that has some value.

I think pr_crit_rl, pr_emerg_rl and pr_alert_rl likely
aren't useful. Is there a sensible use case for those?

I added them for completeness, but...

2009-11-09 22:31:03

by Randy Dunlap

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

Joe Perches wrote:
> On Mon, 2009-11-09 at 13:49 -0800, Andrew Morton wrote:
>> On Fri, 30 Oct 2009 16:21:47 -0700
>> Joe Perches <[email protected]> wrote:
>>> +#define pr_emerg_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_alert_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_crit_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_err_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_warning_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_notice_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
>>> +#define pr_info_rl(fmt, ...) \
>>> + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>> Would prefer pr_emerg_ratelimited personally. It's longer, but one
>> doesn't ask "wtf does _rl" mean and it avoids having two identifiers
>> which refer to the same thing.
>
> I don't have a strong opinion either way.
> _rl is shorter and that has some value.

but we have a long history of not using cryptic abbreviations,
so I agree with Andrew.

> I think pr_crit_rl, pr_emerg_rl and pr_alert_rl likely
> aren't useful. Is there a sensible use case for those?

Not likely.

> I added them for completeness, but...

2009-11-10 05:18:01

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl


* Andrew Morton <[email protected]> wrote:

> On Fri, 30 Oct 2009 16:21:47 -0700
> Joe Perches <[email protected]> wrote:
>
> > +#define pr_emerg_rl(fmt, ...) \
> > + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_alert_rl(fmt, ...) \
> > + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_crit_rl(fmt, ...) \
> > + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_err_rl(fmt, ...) \
> > + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_warning_rl(fmt, ...) \
> > + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_notice_rl(fmt, ...) \
> > + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> > +#define pr_info_rl(fmt, ...) \
> > + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
>
> Would prefer pr_emerg_ratelimited personally. It's longer, but one
> doesn't ask "wtf does _rl" mean and it avoids having two identifiers
> which refer to the same thing.

Yeah. It will be rarely used so that it wont ever really be 'obvious at
a glance', even to folks well versed in kernel source code details.

Ingo

2009-11-10 05:18:26

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl


* Joe Perches <[email protected]> wrote:

> On Mon, 2009-11-09 at 13:49 -0800, Andrew Morton wrote:
> > On Fri, 30 Oct 2009 16:21:47 -0700
> > Joe Perches <[email protected]> wrote:
> > > +#define pr_emerg_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_alert_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_crit_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_err_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_warning_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_notice_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_info_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> >
> > Would prefer pr_emerg_ratelimited personally. It's longer, but one
> > doesn't ask "wtf does _rl" mean and it avoids having two identifiers
> > which refer to the same thing.
>
> I don't have a strong opinion either way.
> _rl is shorter and that has some value.
>
> I think pr_crit_rl, pr_emerg_rl and pr_alert_rl likely
> aren't useful. Is there a sensible use case for those?
>
> I added them for completeness, but...

Yes, we want it for API completeness mostly.

Ingo

2009-11-10 07:33:56

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

On Tue, 2009-11-10 at 06:17 +0100, Ingo Molnar wrote:
> * Andrew Morton <[email protected]> wrote:
>
> > On Fri, 30 Oct 2009 16:21:47 -0700
> > Joe Perches <[email protected]> wrote:
> >
> > > +#define pr_emerg_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_alert_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_crit_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_err_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_warning_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_notice_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> > > +#define pr_info_rl(fmt, ...) \
> > > + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> >
> > Would prefer pr_emerg_ratelimited personally. It's longer, but one
> > doesn't ask "wtf does _rl" mean and it avoids having two identifiers
> > which refer to the same thing.
>
> Yeah. It will be rarely used so that it wont ever really be 'obvious at
> a glance', even to folks well versed in kernel source code details.

Is there a reason for all this pr_ nonsense? will we depricate printk()?

2009-11-10 07:39:34

by Ingo Molnar

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl


* Peter Zijlstra <[email protected]> wrote:

> On Tue, 2009-11-10 at 06:17 +0100, Ingo Molnar wrote:
> > * Andrew Morton <[email protected]> wrote:
> >
> > > On Fri, 30 Oct 2009 16:21:47 -0700
> > > Joe Perches <[email protected]> wrote:
> > >
> > > > +#define pr_emerg_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_EMERG pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_alert_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_ALERT pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_crit_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_CRIT pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_err_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_warning_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_WARNING pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_notice_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_NOTICE pr_fmt(fmt), ##__VA_ARGS__)
> > > > +#define pr_info_rl(fmt, ...) \
> > > > + printk_ratelimited(KERN_INFO pr_fmt(fmt), ##__VA_ARGS__)
> > >
> > > Would prefer pr_emerg_ratelimited personally. It's longer, but one
> > > doesn't ask "wtf does _rl" mean and it avoids having two identifiers
> > > which refer to the same thing.
> >
> > Yeah. It will be rarely used so that it wont ever really be 'obvious at
> > a glance', even to folks well versed in kernel source code details.
>
> Is there a reason for all this pr_ nonsense? will we depricate printk()?

Yes, pr_*() has established itself as a printk shortcut. The benefits
of:

pr_info("stuff\n");

versus:

printk(KERN_INFO "stuff\n");

are sufficiently large:

- it's shorter by 9 characters (more than a level of indentation)

- you cannot forget to add a KERN_ prefix - which is required for 98%
of all printks but which is forgotten from 50% of the submitted
patches.

so pr_*(), while named in a sucky way (all 2 letter abbrevs are sucky),
has advantages, makes stuff more readable and reduces churn.

printk wont go away as an ad-hoc print-this tool. (Nor will we convert
most of the remaining 18,000+ uses of printk() in the kernel, so
printk() will be with us forever i guess.)

Ingo

2009-11-10 07:54:18

by Joe Perches

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

On Tue, 2009-11-10 at 08:39 +0100, Ingo Molnar wrote:
> * Peter Zijlstra <[email protected]> wrote:
> > Is there a reason for all this pr_ nonsense? will we depricate printk()?
> Yes, pr_*() has established itself as a printk shortcut. The benefits
> of: pr_info("stuff\n"); versus: printk(KERN_INFO "stuff\n");
> are sufficiently large:
> - it's shorter by 9 characters (more than a level of indentation)
> - you cannot forget to add a KERN_ prefix - which is required for 98%
> of all printks but which is forgotten from 50% of the submitted
> patches.
> so pr_*(), while named in a sucky way (all 2 letter abbrevs are sucky),
> has advantages, makes stuff more readable and reduces churn.

pr_*()s can be prefixed by pr_fmt
pr_*()s could save text space by storing pr_fmt once

2009-11-10 08:22:08

by Peter Zijlstra

[permalink] [raw]
Subject: Re: [PATCH] kernel.h: Add printk_ratelimited and pr_<level>_rl

On Tue, 2009-11-10 at 08:39 +0100, Ingo Molnar wrote:
>
> printk wont go away as an ad-hoc print-this tool. (Nor will we convert
> most of the remaining 18,000+ uses of printk() in the kernel, so
> printk() will be with us forever i guess.)

Ok good, /me purges all knowledge of pr_* and will henceforth ignore
it ;-)