[PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[Jiri Slaby]

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

Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[Joe Perches]

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;							\
 })
 

Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[Jiri Slaby]

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

Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[Joe Perches]

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?

Re: [PATCH] ratelimit: check the condition in WARN_RATELIMIT first

[Jiri Slaby]

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