[PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[greearb]

From: Ben Greear <greearb@candelatech.com>

A mis-configured filter can spam the logs with
lots of stack traces.  Rate-limit the warnings
and add printout of the bogus filter information.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
:100644 100644 afc5837... 8249745... M	net/core/filter.c
 net/core/filter.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index afc5837..8249745 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -409,7 +409,13 @@ load_b:
 			continue;
 		}
 		default:
-			WARN_ON(1);
+			if (net_ratelimit()) {
+				pr_err("filter: Unknown code: %hu jt: %u tf: %u"
+				       " k: %u\n",
+				       fentry->code, (unsigned int)(fentry->jt),
+				       (unsigned int)(fentry->jf), fentry->k);
+				WARN_ON(1);
+			}
 			return 0;
 		}
 	}
-- 
1.7.3.4

Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[Joe Perches]

On Tue, 2011-05-17 at 11:30 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> A mis-configured filter can spam the logs with
> lots of stack traces.  Rate-limit the warnings
> and add printout of the bogus filter information.
> -			WARN_ON(1);
> +			if (net_ratelimit()) {
> +				pr_err("filter: Unknown code: %hu jt: %u tf: %u"
> +				       " k: %u\n",
> +				       fentry->code, (unsigned int)(fentry->jt),
> +				       (unsigned int)(fentry->jf), fentry->k);
> +				WARN_ON(1);
> +			}

Maybe just using WARN is better.
I believe the casts aren't necessary.

 net/core/filter.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 0eb8c44..5b967d0 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -350,7 +350,10 @@ load_b:
 			continue;
 		}
 		default:
-			WARN_ON(1);
+			if (net_ratelimit())
+				WARN(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
+				     fentry->code, fentry->jt,
+				     fentry->jf, fentry->k);
 			return 0;
 		}
 	}

Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 17 May 2011 11:52:29 -0700

> On Tue, 2011-05-17 at 11:30 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>> A mis-configured filter can spam the logs with
>> lots of stack traces.  Rate-limit the warnings
>> and add printout of the bogus filter information.
>> -			WARN_ON(1);
>> +			if (net_ratelimit()) {
>> +				pr_err("filter: Unknown code: %hu jt: %u tf: %u"
>> +				       " k: %u\n",
>> +				       fentry->code, (unsigned int)(fentry->jt),
>> +				       (unsigned int)(fentry->jf), fentry->k);
>> +				WARN_ON(1);
>> +			}
> 
> Maybe just using WARN is better.

Yep, it looks better to me.

> I believe the casts aren't necessary.

Also correct.

Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[Ben Greear]

On 05/17/2011 12:23 PM, David Miller wrote:
> From: Joe Perches<joe@perches.com>
> Date: Tue, 17 May 2011 11:52:29 -0700
>
>> On Tue, 2011-05-17 at 11:30 -0700, greearb@candelatech.com wrote:
>>> From: Ben Greear<greearb@candelatech.com>
>>> A mis-configured filter can spam the logs with
>>> lots of stack traces.  Rate-limit the warnings
>>> and add printout of the bogus filter information.
>>> -			WARN_ON(1);
>>> +			if (net_ratelimit()) {
>>> +				pr_err("filter: Unknown code: %hu jt: %u tf: %u"
>>> +				       " k: %u\n",
>>> +				       fentry->code, (unsigned int)(fentry->jt),
>>> +				       (unsigned int)(fentry->jf), fentry->k);
>>> +				WARN_ON(1);
>>> +			}
>>
>> Maybe just using WARN is better.
>
> Yep, it looks better to me.

You want to just take his, or shall I re-spin it?

Either is fine with me.

Thanks,
Ben

>
>> I believe the casts aren't necessary.
>
> Also correct.
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[David Miller]

From: Ben Greear <greearb@candelatech.com>
Date: Tue, 17 May 2011 12:34:01 -0700

> On 05/17/2011 12:23 PM, David Miller wrote:
>> From: Joe Perches<joe@perches.com>
>> Date: Tue, 17 May 2011 11:52:29 -0700
>>
>>> On Tue, 2011-05-17 at 11:30 -0700, greearb@candelatech.com wrote:
>>>> From: Ben Greear<greearb@candelatech.com>
>>>> A mis-configured filter can spam the logs with
>>>> lots of stack traces.  Rate-limit the warnings
>>>> and add printout of the bogus filter information.
>>>> -			WARN_ON(1);
>>>> +			if (net_ratelimit()) {
>>>> + pr_err("filter: Unknown code: %hu jt: %u tf: %u"
>>>> +				       " k: %u\n",
>>>> + fentry->code, (unsigned int)(fentry->jt),
>>>> + (unsigned int)(fentry->jf), fentry->k);
>>>> +				WARN_ON(1);
>>>> +			}
>>>
>>> Maybe just using WARN is better.
>>
>> Yep, it looks better to me.
> 
> You want to just take his, or shall I re-spin it?

Respin.

Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.

[Joe Perches]

On Tue, 2011-05-17 at 12:34 -0700, Ben Greear wrote:
> On 05/17/2011 12:23 PM, David Miller wrote:
> > From: Joe Perches<joe@perches.com>
> >> On Tue, 2011-05-17 at 11:30 -0700, greearb@candelatech.com wrote:
> >>> From: Ben Greear<greearb@candelatech.com>
> >>> A mis-configured filter can spam the logs with
> >>> lots of stack traces.  Rate-limit the warnings
> >>> and add printout of the bogus filter information.
> >>> -			WARN_ON(1);
> >>> +			if (net_ratelimit()) {
> >>> +				pr_err("filter: Unknown code: %hu jt: %u tf: %u"
> >>> +				       " k: %u\n",
> >>> +				       fentry->code, (unsigned int)(fentry->jt),
> >>> +				       (unsigned int)(fentry->jf), fentry->k);
> >>> +				WARN_ON(1);
> >>> +			}
> >> Maybe just using WARN is better.
> > Yep, it looks better to me.
> You want to just take his, or shall I re-spin it?
> Either is fine with me.

Another option is to add a WARN_ON_RATELIMIT
(there is already a WARN_RATELIMIT) to avoid
missing other messages.

Something like:

 include/asm-generic/bug.h |   16 ++++++++++++++++
 net/core/filter.c         |    4 +++-
 2 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index e5a3f58..12b250c 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -165,6 +165,22 @@ extern void warn_slowpath_null(const char *file, const int line);
 #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...)			\
+({								\
+	static DEFINE_RATELIMIT_STATE(_rs,			\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);	\
+	__WARN_RATELIMIT(condition, &_rs, format);		\
+})
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
diff --git a/net/core/filter.c b/net/core/filter.c
index 0eb8c44..0e3622f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -350,7 +350,9 @@ load_b:
 			continue;
 		}
 		default:
-			WARN_ON(1);
+			WARN_RATELIMIT(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
+				       fentry->code, fentry->jt,
+				       fentry->jf, fentry->k);
 			return 0;
 		}
 	}

RFC: Add WARN_RATELIMIT to bug.h (was: Re: [PATCH] sk-filter: Rate-limit WARNing, print dbg info.)

[Joe Perches]

On Tue, 2011-05-17 at 13:33 -0700, Ben Greear wrote:
> On 05/17/2011 12:53 PM, Joe Perches wrote:
> > Another option is to add a WARN_RATELIMIT
> > (there is already a WARN_ON_RATELIMIT) to avoid
> > missing other messages.
> > Something like:
> >   include/asm-generic/bug.h |   16 ++++++++++++++++
> >   net/core/filter.c         |    4 +++-
> >   2 files changed, 19 insertions(+), 1 deletions(-)
> > diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> > index e5a3f58..12b250c 100644
> > --- a/include/asm-generic/bug.h
> > +++ b/include/asm-generic/bug.h
> > @@ -165,6 +165,22 @@ extern void warn_slowpath_null(const char *file, const int line);
> >   #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...)			\
> > +({								\
> > +	static DEFINE_RATELIMIT_STATE(_rs,			\
> > +				      DEFAULT_RATELIMIT_INTERVAL,	\
> > +				      DEFAULT_RATELIMIT_BURST);	\
> > +	__WARN_RATELIMIT(condition,&_rs, format);		\
> > +})
> > +
> >   /*
> >    * WARN_ON_SMP() is for cases that the warning is either
> >    * meaningless for !SMP or may even cause failures.
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 0eb8c44..0e3622f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -350,7 +350,9 @@ load_b:
> >   			continue;
> >   		}
> >   		default:
> > -			WARN_ON(1);
> > +			WARN_RATELIMIT(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
> > +				       fentry->code, fentry->jt,
> > +				       fentry->jf, fentry->k);
> >   			return 0;
> >   		}
> >   	}
> >

> Sounds fine to me.  You want to just send an official
> patch with all this?

I added a some cc's for wider exposure.
original post: http://www.spinics.net/lists/netdev/msg164521.html

Let's wait to see if David has anything to say.

The biggest negative I see is adding RATELIMIT_STATE
to asm-generic/bug.h, though it already has a use of
__ratelimit in WARN_ON_RATELIMIT there.

WARN_ON_RATELIMIT is unused today.

The only user seems to have been RCU_PREEMPT
which was deleted in:

commit 6b3ef48adf847f7adf11c870e3ffacac150f1564
Author: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
Date:   Sat Aug 22 13:56:53 2009 -0700

    rcu: Remove CONFIG_PREEMPT_RCU

Maybe the old definition should be removed instead.

If there are no comments after a day or two, I'll
sign and send this as 2 patches with the filter
one marked as:

Original-patch-by: Ben Greear <greearb@candelatech.com>

cheers, Joe

[PATCH 0/2] Add and use WARN_RATELIMIT

[Joe Perches]

Generic mechanism to ratelimit WARN uses.

Joe Perches (2):
  bug.h: Add WARN_RATELIMIT
  net: filter: Use WARN_RATELIMIT

 include/asm-generic/bug.h |   16 ++++++++++++++++
 net/core/filter.c         |    4 +++-
 2 files changed, 19 insertions(+), 1 deletions(-)

-- 
1.7.5.rc3.dirty

[PATCH 1/2] bug.h: Add WARN_RATELIMIT

[Joe Perches]

Add a generic mechanism to ratelimit WARN(foo, fmt, ...) messages
using a hidden per call site static struct ratelimit_state.

Also add an __WARN_RATELIMIT variant to be able to use a specific
struct ratelimit_state.

Signed-off-by: Joe Perches <joe@perches.com>
---
 include/asm-generic/bug.h |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index e5a3f58..12b250c 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -165,6 +165,22 @@ extern void warn_slowpath_null(const char *file, const int line);
 #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...)			\
+({								\
+	static DEFINE_RATELIMIT_STATE(_rs,			\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);	\
+	__WARN_RATELIMIT(condition, &_rs, format);		\
+})
+
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
-- 
1.7.5.rc3.dirty

[PATCH 2/2] net: filter: Use WARN_RATELIMIT

[Joe Perches]

A mis-configured filter can spam the logs with lots of stack traces.

Rate-limit the warnings and add printout of the bogus filter information.

Original-patch-by: Ben Greear <greearb@candelatech.com>
Signed-off-by: Joe Perches <joe@perches.com>
---
 net/core/filter.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 0eb8c44..0e3622f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -350,7 +350,9 @@ load_b:
 			continue;
 		}
 		default:
-			WARN_ON(1);
+			WARN_RATELIMIT(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
+				       fentry->code, fentry->jt,
+				       fentry->jf, fentry->k);
 			return 0;
 		}
 	}
-- 
1.7.5.rc3.dirty

[patch] net/core/filter.c: Fix build error

[Ingo Molnar]

* Joe Perches <joe@perches.com> wrote:

> A mis-configured filter can spam the logs with lots of stack traces.
> 
> Rate-limit the warnings and add printout of the bogus filter information.
> 
> Original-patch-by: Ben Greear <greearb@candelatech.com>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
>  net/core/filter.c |    4 +++-
>  1 files changed, 3 insertions(+), 1 deletions(-)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 0eb8c44..0e3622f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -350,7 +350,9 @@ load_b:
>  			continue;
>  		}
>  		default:
> -			WARN_ON(1);
> +			WARN_RATELIMIT(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
> +				       fentry->code, fentry->jt,
> +				       fentry->jf, fentry->k);
>  			return 0;
>  		}

This change (now upstream) fails to build in about 20% of all 
randconfigs. Fix is below.

Thanks,

	Ingo

--------------------->
From b658026bc4915d16ff3e0f59b0edda11dbd6b991 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Thu, 26 May 2011 14:11:14 +0200
Subject: [PATCH] net/core/filter.c: Fix build error
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fix:

  net/core/filter.c:353:4: error: invalid storage class for function ‘DEFINE_RATELIMIT_STATE’

Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 net/core/filter.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index 0e3622f..36f975f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -38,6 +38,7 @@
 #include <asm/unaligned.h>
 #include <linux/filter.h>
 #include <linux/reciprocal_div.h>
+#include <linux/ratelimit.h>
 
 /* No hurry in this branch */
 static void *__load_pointer(const struct sk_buff *skb, int k, unsigned int size)

Re: [patch] net/core/filter.c: Fix build error

[Joe Perches]

On Thu, 2011-05-26 at 14:31 +0200, Ingo Molnar wrote:
> * Joe Perches <joe@perches.com> wrote:
> > A mis-configured filter can spam the logs with lots of stack traces.
> > Rate-limit the warnings and add printout of the bogus filter information.
> > Original-patch-by: Ben Greear <greearb@candelatech.com>
> > Signed-off-by: Joe Perches <joe@perches.com>
> > ---
> >  net/core/filter.c |    4 +++-
> >  1 files changed, 3 insertions(+), 1 deletions(-)
> > 
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 0eb8c44..0e3622f 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -350,7 +350,9 @@ load_b:
> >  			continue;
> >  		}
> >  		default:
> > -			WARN_ON(1);
> > +			WARN_RATELIMIT(1, "Unknown code:%u jt:%u tf:%u k:%u\n",
> > +				       fentry->code, fentry->jt,
> > +				       fentry->jf, fentry->k);
> >  			return 0;
> >  		}
> This change (now upstream) fails to build in about 20% of all 
> randconfigs. Fix is below.

We've had problems with ratelimit uses in the past
as well.

There's a logical separation issue with the #includes
of bug.h, printk.h, ratelimit.h kernel.h and the
spinlock includes.

My suggestion would be to see about again adding
#include <linux/ratelimit.h> somehow
back to kernel.h which commit 3fff4c42bd0a removed
in 2009 because of the spinlock issues.

Any suggestion on how best to fix it generically?

Re: [patch] net/core/filter.c: Fix build error

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Thu, 26 May 2011 08:31:06 -0700

> My suggestion would be to see about again adding
> #include <linux/ratelimit.h> somehow
> back to kernel.h which commit 3fff4c42bd0a removed
> in 2009 because of the spinlock issues.
> 
> Any suggestion on how best to fix it generically?

I don't think we want spinlock_t's definition being sucked
into kernel.h's dependency food chain.

Even if desirable, I think it'd be quite a bit of surgery,
too much to do at this stage.

So for now how about we make the ratelimit warn interfaces be a true,
instead of a pseudo, dependency on ratelimit.h by moving those
definitions into ratelimit.h?

diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
index 9178484..dfb0ec6 100644
--- a/include/asm-generic/bug.h
+++ b/include/asm-generic/bug.h
@@ -162,46 +162,6 @@ extern void warn_slowpath_null(const char *file, const int line);
 	unlikely(__ret_warn_once);				\
 })
 
-#ifdef CONFIG_PRINTK
-
-#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...)			\
-({								\
-	static DEFINE_RATELIMIT_STATE(_rs,			\
-				      DEFAULT_RATELIMIT_INTERVAL,	\
-				      DEFAULT_RATELIMIT_BURST);	\
-	__WARN_RATELIMIT(condition, &_rs, format);		\
-})
-
-#else
-
-#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...)			\
-({								\
-	int rtn = WARN(condition, format);			\
-	rtn;							\
-})
-
-#endif
-
 /*
  * WARN_ON_SMP() is for cases that the warning is either
  * meaningless for !SMP or may even cause failures.
diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
index 03ff67b..2f00715 100644
--- a/include/linux/ratelimit.h
+++ b/include/linux/ratelimit.h
@@ -41,4 +41,44 @@ extern struct ratelimit_state printk_ratelimit_state;
 extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
 #define __ratelimit(state) ___ratelimit(state, __func__)
 
+#ifdef CONFIG_PRINTK
+
+#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...)			\
+({								\
+	static DEFINE_RATELIMIT_STATE(_rs,			\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);	\
+	__WARN_RATELIMIT(condition, &_rs, format);		\
+})
+
+#else
+
+#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...)			\
+({								\
+	int rtn = WARN(condition, format);			\
+	rtn;							\
+})
+
+#endif
+
 #endif /* _LINUX_RATELIMIT_H */
diff --git a/net/core/filter.c b/net/core/filter.c
index 0e3622f..36f975f 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -38,6 +38,7 @@
 #include <asm/unaligned.h>
 #include <linux/filter.h>
 #include <linux/reciprocal_div.h>
+#include <linux/ratelimit.h>
 
 /* No hurry in this branch */
 static void *__load_pointer(const struct sk_buff *skb, int k, unsigned int size)

Re: [patch] net/core/filter.c: Fix build error

[Randy Dunlap]

On Thu, 26 May 2011 14:38:43 -0400 (EDT) David Miller wrote:

> From: Joe Perches <joe@perches.com>
> Date: Thu, 26 May 2011 08:31:06 -0700
> 
> > My suggestion would be to see about again adding
> > #include <linux/ratelimit.h> somehow
> > back to kernel.h which commit 3fff4c42bd0a removed
> > in 2009 because of the spinlock issues.
> > 
> > Any suggestion on how best to fix it generically?
> 
> I don't think we want spinlock_t's definition being sucked
> into kernel.h's dependency food chain.
> 
> Even if desirable, I think it'd be quite a bit of surgery,
> too much to do at this stage.
> 
> So for now how about we make the ratelimit warn interfaces be a true,
> instead of a pseudo, dependency on ratelimit.h by moving those
> definitions into ratelimit.h?

Works for me.  Thanks.

Acked-by: Randy Dunlap <randy.dunlap@oracle.com>


> diff --git a/include/asm-generic/bug.h b/include/asm-generic/bug.h
> index 9178484..dfb0ec6 100644
> --- a/include/asm-generic/bug.h
> +++ b/include/asm-generic/bug.h
> @@ -162,46 +162,6 @@ extern void warn_slowpath_null(const char *file, const int line);
>  	unlikely(__ret_warn_once);				\
>  })
>  
> -#ifdef CONFIG_PRINTK
> -
> -#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...)			\
> -({								\
> -	static DEFINE_RATELIMIT_STATE(_rs,			\
> -				      DEFAULT_RATELIMIT_INTERVAL,	\
> -				      DEFAULT_RATELIMIT_BURST);	\
> -	__WARN_RATELIMIT(condition, &_rs, format);		\
> -})
> -
> -#else
> -
> -#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...)			\
> -({								\
> -	int rtn = WARN(condition, format);			\
> -	rtn;							\
> -})
> -
> -#endif
> -
>  /*
>   * WARN_ON_SMP() is for cases that the warning is either
>   * meaningless for !SMP or may even cause failures.
> diff --git a/include/linux/ratelimit.h b/include/linux/ratelimit.h
> index 03ff67b..2f00715 100644
> --- a/include/linux/ratelimit.h
> +++ b/include/linux/ratelimit.h
> @@ -41,4 +41,44 @@ extern struct ratelimit_state printk_ratelimit_state;
>  extern int ___ratelimit(struct ratelimit_state *rs, const char *func);
>  #define __ratelimit(state) ___ratelimit(state, __func__)
>  
> +#ifdef CONFIG_PRINTK
> +
> +#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...)			\
> +({								\
> +	static DEFINE_RATELIMIT_STATE(_rs,			\
> +				      DEFAULT_RATELIMIT_INTERVAL,	\
> +				      DEFAULT_RATELIMIT_BURST);	\
> +	__WARN_RATELIMIT(condition, &_rs, format);		\
> +})
> +
> +#else
> +
> +#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...)			\
> +({								\
> +	int rtn = WARN(condition, format);			\
> +	rtn;							\
> +})
> +
> +#endif
> +
>  #endif /* _LINUX_RATELIMIT_H */
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 0e3622f..36f975f 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -38,6 +38,7 @@
>  #include <asm/unaligned.h>
>  #include <linux/filter.h>
>  #include <linux/reciprocal_div.h>
> +#include <linux/ratelimit.h>
>  
>  /* No hurry in this branch */
>  static void *__load_pointer(const struct sk_buff *skb, int k, unsigned int size)
> --


---
~Randy
*** Remember to use Documentation/SubmitChecklist when testing your code ***

Re: [patch] net/core/filter.c: Fix build error

[David Miller]

From: Randy Dunlap <randy.dunlap@oracle.com>
Date: Thu, 26 May 2011 11:57:21 -0700

> On Thu, 26 May 2011 14:38:43 -0400 (EDT) David Miller wrote:
> 
>> From: Joe Perches <joe@perches.com>
>> Date: Thu, 26 May 2011 08:31:06 -0700
>> 
>> > My suggestion would be to see about again adding
>> > #include <linux/ratelimit.h> somehow
>> > back to kernel.h which commit 3fff4c42bd0a removed
>> > in 2009 because of the spinlock issues.
>> > 
>> > Any suggestion on how best to fix it generically?
>> 
>> I don't think we want spinlock_t's definition being sucked
>> into kernel.h's dependency food chain.
>> 
>> Even if desirable, I think it'd be quite a bit of surgery,
>> too much to do at this stage.
>> 
>> So for now how about we make the ratelimit warn interfaces be a true,
>> instead of a pseudo, dependency on ratelimit.h by moving those
>> definitions into ratelimit.h?
> 
> Works for me.  Thanks.
> 
> Acked-by: Randy Dunlap <randy.dunlap@oracle.com>

Thanks for reviewing Randy, I've put this into net-2.6 and will
push it out to Linus soon.

Re: [patch] net/core/filter.c: Fix build error

[Joe Perches]

On Thu, 2011-05-26 at 14:38 -0400, David Miller wrote:
> From: Joe Perches <joe@perches.com>
> Date: Thu, 26 May 2011 08:31:06 -0700
> > My suggestion would be to see about again adding
> > #include <linux/ratelimit.h> somehow
> > back to kernel.h which commit 3fff4c42bd0a removed
> > in 2009 because of the spinlock issues.
> > Any suggestion on how best to fix it generically?
> I don't think we want spinlock_t's definition being sucked
> into kernel.h's dependency food chain.
> Even if desirable, I think it'd be quite a bit of surgery,
> too much to do at this stage.
> So for now how about we make the ratelimit warn interfaces be a true,
> instead of a pseudo, dependency on ratelimit.h by moving those
> definitions into ratelimit.h?

Thanks, I suppose that's good enough for now.

Perhaps it'd also be good to move the pr_<level>_ratelimited
declarations from printk.h.

It seems that would not cause new compilation problems.

$ grep -rP --include=*.[ch] -wl "pr_[a-z]+_ratelimited" * | \
  xargs grep -L "include.*ratelimit\.h"
include/linux/printk.h

And, though it's sure to cause some compilation problems:

$ grep -rP --include=*.[ch] -wl "printk_ratelimit" * | \
  xargs grep -L "include.*ratelimit\.h" | wc -l
127

Perhaps it'd also be good to move the printk_ratelimit
block from printk.h into ratelimited.h and add
#include <linux/ratelimited.h> to the current source
files that use it in a later patchset.

Maybe Jiri could pick it up through trivial.  Jiri?

Re: [patch] net/core/filter.c: Fix build error

[Ingo Molnar]

* David Miller <davem@davemloft.net> wrote:

> From: Joe Perches <joe@perches.com>
> Date: Thu, 26 May 2011 08:31:06 -0700
> 
> > My suggestion would be to see about again adding
> > #include <linux/ratelimit.h> somehow
> > back to kernel.h which commit 3fff4c42bd0a removed
> > in 2009 because of the spinlock issues.
> > 
> > Any suggestion on how best to fix it generically?
> 
> I don't think we want spinlock_t's definition being sucked
> into kernel.h's dependency food chain.

Agreed.

Also, i don't think it's unreasonable to require code that uses 
DEFINE_RATELIMIT_STATE() to #include ratelimit.h. That's what we 
require from DEFINE_MUTEX() and DEFINE_SPINLOCK() users and many 
other users as well.

The only exception is printk_ratelimit() itself - that should not - 
and currently does not - require the inclusion of ratelimit.h.

The *real* solution here would be to remove the spurious and .config 
dependent inclusion of ratelimit.h in include/linux/net.h.  It's fine 
to provide the net_ratelimit() interface (it'sanalogous to 
printk_ratelimit()), but it's not fine to declare net_ratelimit_state 
in that header and include that header in half of all networking 
code, bringing in ratelimit.h implicitly.

So no, i don't think your patch is the real solution. The problem was 
that the .config's you tested had CONFIG_SYSCTL=y, which brought in 
ratelimit.h into half of the networking code. That's unnecessary and 
problematic - the interface between net/core/sysctl_net_core.c and 
net/core/utils.c should be done via a dedicated header (not included 
by other code), or via explicit extern declared in the .c file (more 
dangerous, but used by pretty much all sysctl code in the kernel).

Thanks,

	Ingo

Re: [patch] net/core/filter.c: Fix build error

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> So no, i don't think your patch is the real solution. [...]

s/real/complete

Note, your patch solves a real problem: the ratelimited WARN_ON()s 
should not be in the generic bug.h header, that's just broken.

I just don't think this is the only problem: the other problem was 
what hid the bug in the first place, the spurious ratelimit.h 
inclusion via net.h.

Thanks,

	Ingo

Re: [PATCH 0/2] Add and use WARN_RATELIMIT

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Sat, 21 May 2011 10:48:38 -0700

> Generic mechanism to ratelimit WARN uses.
> 
> Joe Perches (2):
>   bug.h: Add WARN_RATELIMIT
>   net: filter: Use WARN_RATELIMIT
> 
>  include/asm-generic/bug.h |   16 ++++++++++++++++
>  net/core/filter.c         |    4 +++-
>  2 files changed, 19 insertions(+), 1 deletions(-)

This looks fine to me, and no objections have been voiced otherwise,
so applied to net-2.6, thanks!