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 <[email protected]>
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 <[email protected]>
cheers, Joe
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
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 <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Joe Perches <[email protected]>
---
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
From: Joe Perches <[email protected]>
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!
* Joe Perches <[email protected]> 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 <[email protected]>
> Signed-off-by: Joe Perches <[email protected]>
> ---
> 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 <[email protected]>
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 <[email protected]>
---
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)
On Thu, 2011-05-26 at 14:31 +0200, Ingo Molnar wrote:
> * Joe Perches <[email protected]> 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 <[email protected]>
> > Signed-off-by: Joe Perches <[email protected]>
> > ---
> > 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?
From: Joe Perches <[email protected]>
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)
On Thu, 26 May 2011 14:38:43 -0400 (EDT) David Miller wrote:
> From: Joe Perches <[email protected]>
> 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 <[email protected]>
> 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 ***
From: Randy Dunlap <[email protected]>
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 <[email protected]>
>> 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 <[email protected]>
Thanks for reviewing Randy, I've put this into net-2.6 and will
push it out to Linus soon.
On Thu, 2011-05-26 at 14:38 -0400, David Miller wrote:
> From: Joe Perches <[email protected]>
> 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?
* David Miller <[email protected]> wrote:
> From: Joe Perches <[email protected]>
> 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
* Ingo Molnar <[email protected]> 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