* [PATCH] ratelimit: check the condition in WARN_RATELIMIT first @ 2012-08-17 13:42 Jiri Slaby 2012-08-17 17:39 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2012-08-17 13:42 UTC (permalink / raw) To: davem; +Cc: jirislaby, linux-kernel, Jiri Slaby, Joe Perches Before calling __ratelimit in __WARN_RATELIMIT, check the condition first. When this check was not there, we got constant income of: tty_init_dev: 60 callbacks suppressed tty_init_dev: 59 callbacks suppressed Signed-off-by: Jiri Slaby <jslaby@suse.cz> Cc: Joe Perches <joe@perches.com> --- include/linux/ratelimit.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index e11ccb4..966d35c 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); #define __WARN_RATELIMIT(condition, state, format...) \ ({ \ int rtn = 0; \ - if (unlikely(__ratelimit(state))) \ - rtn = WARN(condition, format); \ + int __rtcond = !!condition; \ + if (unlikely(__rtcond && __ratelimit(state))) \ + rtn = WARN(__rtcond, format); \ rtn; \ }) -- 1.7.11.5 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first 2012-08-17 13:42 [PATCH] ratelimit: check the condition in WARN_RATELIMIT first Jiri Slaby @ 2012-08-17 17:39 ` Joe Perches 2012-08-17 18:15 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Joe Perches @ 2012-08-17 17:39 UTC (permalink / raw) To: Jiri Slaby; +Cc: davem, jirislaby, linux-kernel On Fri, 2012-08-17 at 15:42 +0200, Jiri Slaby wrote: > Before calling __ratelimit in __WARN_RATELIMIT, check the condition > first. When this check was not there, we got constant income of: > tty_init_dev: 60 callbacks suppressed > tty_init_dev: 59 callbacks suppressed [] > diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h [] > @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); > #define __WARN_RATELIMIT(condition, state, format...) \ > ({ \ > int rtn = 0; \ > - if (unlikely(__ratelimit(state))) \ > - rtn = WARN(condition, format); \ > + int __rtcond = !!condition; \ > + if (unlikely(__rtcond && __ratelimit(state))) \ > + rtn = WARN(__rtcond, format); \ > rtn; \ > }) > Hi Jiri. This seems fine to me but are there any conditions that are computationally expensive? ratelimit(state) isn't and this will now always do condition. (looks instead of speculates) There's 1 current use of WARN_RATELIMIT and there's a condition of 1 so there's no problem here. __WARN_RATELIMIT is pretty stupid. It's only called from WARN_RATELIMIT. I think it shouldn't exist at all. Maybe something like this? (has some neatening as well) include/linux/ratelimit.h | 27 +++++++++------------------ 1 files changed, 9 insertions(+), 18 deletions(-) diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h index e11ccb4..f4acd61 100644 --- a/include/linux/ratelimit.h +++ b/include/linux/ratelimit.h @@ -46,20 +46,17 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); #define WARN_ON_RATELIMIT(condition, state) \ WARN_ON((condition) && __ratelimit(state)) -#define __WARN_RATELIMIT(condition, state, format...) \ -({ \ - int rtn = 0; \ - if (unlikely(__ratelimit(state))) \ - rtn = WARN(condition, format); \ - rtn; \ -}) - -#define WARN_RATELIMIT(condition, format...) \ +#define WARN_RATELIMIT(condition, fmt, ...) \ ({ \ static DEFINE_RATELIMIT_STATE(_rs, \ DEFAULT_RATELIMIT_INTERVAL, \ DEFAULT_RATELIMIT_BURST); \ - __WARN_RATELIMIT(condition, &_rs, format); \ + int rtn = !!(condition); \ + \ + if (unlikely(rtn && __ratelimit(state))) \ + WARN(rtn, fmt, ##__VA_ARGS__); \ + \ + rtn; \ }) #else @@ -67,15 +64,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); #define WARN_ON_RATELIMIT(condition, state) \ WARN_ON(condition) -#define __WARN_RATELIMIT(condition, state, format...) \ -({ \ - int rtn = WARN(condition, format); \ - rtn; \ -}) - -#define WARN_RATELIMIT(condition, format...) \ +#define WARN_RATELIMIT(condition, fmt, ...) \ ({ \ - int rtn = WARN(condition, format); \ + int rtn = WARN(condition, fmt, ##__VA_ARGS__); \ rtn; \ }) ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first 2012-08-17 17:39 ` Joe Perches @ 2012-08-17 18:15 ` Jiri Slaby 2012-08-17 18:45 ` Joe Perches 0 siblings, 1 reply; 5+ messages in thread From: Jiri Slaby @ 2012-08-17 18:15 UTC (permalink / raw) To: Joe Perches; +Cc: davem, jirislaby, linux-kernel On 08/17/2012 07:39 PM, Joe Perches wrote: > On Fri, 2012-08-17 at 15:42 +0200, Jiri Slaby wrote: >> Before calling __ratelimit in __WARN_RATELIMIT, check the condition >> first. When this check was not there, we got constant income of: >> tty_init_dev: 60 callbacks suppressed >> tty_init_dev: 59 callbacks suppressed > [] >> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h > [] >> @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); >> #define __WARN_RATELIMIT(condition, state, format...) \ >> ({ \ >> int rtn = 0; \ >> - if (unlikely(__ratelimit(state))) \ >> - rtn = WARN(condition, format); \ >> + int __rtcond = !!condition; \ >> + if (unlikely(__rtcond && __ratelimit(state))) \ >> + rtn = WARN(__rtcond, format); \ >> rtn; \ >> }) >> > > Hi Jiri. > > This seems fine to me but are there any conditions that > are computationally expensive? It's not about expensiveness of the computation. The complexity remained the same except I moved the computation one layer up. > ratelimit(state) isn't > and this will now always do condition. > > (looks instead of speculates) > > There's 1 current use of WARN_RATELIMIT and there's > a condition of 1 so there's no problem here. There is going to be one more in monday's -next. I've just added one to the TTY code. The thing is that when you call ratelimit(state) it will emit how many times you have called that function like I described in the changelog: tty_init_dev: 60 callbacks suppressed Even when the condition is always false. Hence I added the condition to the if and lazy evaluation will take care and ratelimit() won't be called at all... > __WARN_RATELIMIT is pretty stupid. > It's only called from WARN_RATELIMIT. > I think it shouldn't exist at all. > > Maybe something like this? Yup, something like that looks OK to me. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first 2012-08-17 18:15 ` Jiri Slaby @ 2012-08-17 18:45 ` Joe Perches 2012-08-17 20:54 ` Jiri Slaby 0 siblings, 1 reply; 5+ messages in thread From: Joe Perches @ 2012-08-17 18:45 UTC (permalink / raw) To: Jiri Slaby; +Cc: davem, jirislaby, linux-kernel On Fri, 2012-08-17 at 20:15 +0200, Jiri Slaby wrote: > On 08/17/2012 07:39 PM, Joe Perches wrote: > > On Fri, 2012-08-17 at 15:42 +0200, Jiri Slaby wrote: > >> Before calling __ratelimit in __WARN_RATELIMIT, check the condition > >> first. When this check was not there, we got constant income of: > >> tty_init_dev: 60 callbacks suppressed > >> tty_init_dev: 59 callbacks suppressed > > [] > >> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h > > [] > >> @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); > >> #define __WARN_RATELIMIT(condition, state, format...) \ > >> ({ \ > >> int rtn = 0; \ > >> - if (unlikely(__ratelimit(state))) \ > >> - rtn = WARN(condition, format); \ > >> + int __rtcond = !!condition; \ > >> + if (unlikely(__rtcond && __ratelimit(state))) \ > >> + rtn = WARN(__rtcond, format); \ > >> rtn; \ > >> }) > >> > > > > Hi Jiri. > > > > This seems fine to me but are there any conditions that > > are computationally expensive? > > It's not about expensiveness of the computation. The complexity remained > the same except I moved the computation one layer up. If ratelimit(state) is not true, condition wasn't tested or performed at all. With this change, it's always done. > > Maybe something like this? [] > Yup, something like that looks OK to me. OK, David, do you want an official patch? ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first 2012-08-17 18:45 ` Joe Perches @ 2012-08-17 20:54 ` Jiri Slaby 0 siblings, 0 replies; 5+ messages in thread From: Jiri Slaby @ 2012-08-17 20:54 UTC (permalink / raw) To: Joe Perches; +Cc: davem, jirislaby, linux-kernel On 08/17/2012 08:45 PM, Joe Perches wrote: > On Fri, 2012-08-17 at 20:15 +0200, Jiri Slaby wrote: >> On 08/17/2012 07:39 PM, Joe Perches wrote: >>> On Fri, 2012-08-17 at 15:42 +0200, Jiri Slaby wrote: >>>> Before calling __ratelimit in __WARN_RATELIMIT, check the condition >>>> first. When this check was not there, we got constant income of: >>>> tty_init_dev: 60 callbacks suppressed >>>> tty_init_dev: 59 callbacks suppressed >>> [] >>>> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h >>> [] >>>> @@ -49,8 +49,9 @@ extern int ___ratelimit(struct ratelimit_state *rs, const char *func); >>>> #define __WARN_RATELIMIT(condition, state, format...) \ >>>> ({ \ >>>> int rtn = 0; \ >>>> - if (unlikely(__ratelimit(state))) \ >>>> - rtn = WARN(condition, format); \ >>>> + int __rtcond = !!condition; \ >>>> + if (unlikely(__rtcond && __ratelimit(state))) \ >>>> + rtn = WARN(__rtcond, format); \ >>>> rtn; \ >>>> }) >>>> >>> >>> Hi Jiri. >>> >>> This seems fine to me but are there any conditions that >>> are computationally expensive? >> >> It's not about expensiveness of the computation. The complexity remained >> the same except I moved the computation one layer up. > > If ratelimit(state) is not true, condition wasn't tested > or performed at all. With this change, it's always done. Ah, you meant this. Actually this was wrong/unexpected. When devs pass something to a function/macro they expect it to be evaluated. Exactly once. Like in this (maybe not so good) code: void put_ref(int refcnt) { WARN_RATELIMIT(!--refcnt, "refcnt reached 0 unexpectedly"); } You want the refcnt to be decremented no matter what ratelimit() returns. thanks, -- js suse labs ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-08-17 20:54 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-08-17 13:42 [PATCH] ratelimit: check the condition in WARN_RATELIMIT first Jiri Slaby 2012-08-17 17:39 ` Joe Perches 2012-08-17 18:15 ` Jiri Slaby 2012-08-17 18:45 ` Joe Perches 2012-08-17 20:54 ` Jiri Slaby
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).