Skip to contentSkip to conversation

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

PATCH In review [email protected] 5 participants 19 messages May 17, 2011 – May 26, 2011 1 message not in the archive

Subsystems (5)
Networking Network drivers BPF Generic include/asm header files +1 more subsystems
lore
Share & tools
lore

[email protected]

author
#0 lore raw

PATCH 1/1 sk-filter: Rate-limit WARNing, print dbg info.

From: Ben Greear <[email protected]>

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

---
:100644 100644 afc5837... 8249745... M net/core/filter.c

 net/core/filter.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)
net/core/filter.c +7 -1
index afc5837..8249745 100644
@@ -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;
 		}
 	}
Signature · 1 line
1.7.3.4
#1 lore raw

Review

Quoted 12 lines · On Tue, 2011-05-17 at 11:30 -0700, [email protected] wrote:

From: Ben Greear <[email protected]> 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.

(quoted diff) +7 -1
-			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(-)
net/core/filter.c +4 -1
index 0eb8c44..5b967d0 100644
@@ -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;
 		}
 	}
maintainer
#2 lore raw

From: Joe Perches <[email protected]>
Date: Tue, 17 May 2011 11:52:29 -0700

Quoted 7 lines · On Tue, 2011-05-17 at 11:30 -0700, [email protected] wrote:
Quoted 6 lines

From: Ben Greear <[email protected]> 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.

(quoted diff) +1 -1
-			WARN_ON(1);
+			if (net_ratelimit()) {
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+				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.

Quoted 1 line

I believe the casts aren't necessary.

Also correct.

author
#3 lore raw
Quoted 19 lines · David Miller

From: Joe Perches<[email protected]>
Date: Tue, 17 May 2011 11:52:29 -0700

Quoted 7 lines · On Tue, 2011-05-17 at 11:30 -0700, [email protected] wrote:
Quoted 6 lines

From: Ben Greear<[email protected]>
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.

(quoted diff) +1 -1
-			WARN_ON(1);
+			if (net_ratelimit()) {
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+				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.

Quoted 1 line

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

Quoted 8 lines
Quoted 2 lines

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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Signature · 2 lines
Ben Greear <[email protected]>
Candela Technologies Inc  http://www.candelatech.com
maintainer
#4 lore raw

From: Ben Greear <[email protected]>
Date: Tue, 17 May 2011 12:34:01 -0700

Quoted 20 lines · David Miller
Quoted 19 lines

From: Joe Perches<[email protected]>
Date: Tue, 17 May 2011 11:52:29 -0700

Quoted 15 lines · On Tue, 2011-05-17 at 11:30 -0700, [email protected] wrote:

Review

Quoted 12 lines

From: Ben Greear<[email protected]>
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.

(quoted diff) +7 -1
-			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.

Quoted 3 lines
Quoted 1 line

Yep, it looks better to me.

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

Respin.

#5 lore raw
In reply to Ben Greear
Quoted 16 lines · Ben Greear
Quoted 15 lines · David Miller

From: Joe Perches<[email protected]>

Quoted 14 lines · On Tue, 2011-05-17 at 11:30 -0700, [email protected] wrote:

Review

Quoted 12 lines

From: Ben Greear<[email protected]>
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.

(quoted diff) +7 -1
-			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.

Quoted 3 lines
Quoted 1 line

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(-)
include/asm-generic/bug.h +16 -0
index e5a3f58..12b250c 100644
@@ -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.
net/core/filter.c +3 -1
index 0eb8c44..0e3622f 100644
@@ -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;
 		}
 	}

A message that is not in the archive (sent off-list or not yet received): [email protected]

#7 lore raw

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

Quoted 46 lines · Ben Greear
Quoted 45 lines · Joe Perches

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(-)
include/asm-generic/bug.h +16 -0
index e5a3f58..12b250c 100644
@@ -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.
net/core/filter.c +2 -1
index 0eb8c44..0e3622f 100644
@@ -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,
Quoted 8 lines

net/core/filter.c load_b: · line 358

Quoted 5 lines
net/core/filter.c +1 -0
+				       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

#8 lore raw

cover Add and use WARN_RATELIMIT

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(-)
Signature · 1 line
1.7.5.rc3.dirty
maintainer
#9 lore raw

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

From: Joe Perches <[email protected]>
Date: Sat, 21 May 2011 10:48:38 -0700

Quoted 9 lines

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!

#10 lore raw

PATCH 1/2 bug.h: Add WARN_RATELIMIT

In reply to 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 <[email protected]>
---
 include/asm-generic/bug.h |   16 ++++++++++++++++
 1 files changed, 16 insertions(+), 0 deletions(-)
include/asm-generic/bug.h +16 -0
index e5a3f58..12b250c 100644
@@ -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.
Signature · 1 line
1.7.5.rc3.dirty
#11 lore raw

PATCH 2/2 net: filter: Use WARN_RATELIMIT

In reply to 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 <[email protected]>
Signed-off-by Joe Perches <[email protected]>
---
 net/core/filter.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
net/core/filter.c +3 -1
index 0eb8c44..0e3622f 100644
@@ -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;
 		}
 	}
Signature · 1 line
1.7.5.rc3.dirty
#12 lore raw

PATCH 1/1 net/core/filter.c: Fix build error

Quoted 15 lines · 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 <[email protected]>
Signed-off-by Joe Perches <[email protected]>
---
 net/core/filter.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
net/core/filter.c +0 -0
index 0eb8c44..0e3622f 100644

net/core/filter.c load_b: · line 357

Quoted 9 lines
net/core/filter.c +3 -1
@@ -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(-)
net/core/filter.c +1 -0
index 0e3622f..36f975f 100644
@@ -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)
#13 lore raw

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

Quoted 9 lines · Ingo Molnar
Quoted 8 lines · 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 <[email protected]>
Signed-off-by Joe Perches <[email protected]>
---
 net/core/filter.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
Quoted 16 lines
Quoted 5 lines
net/core/filter.c +0 -0
index 0eb8c44..0e3622f 100644

net/core/filter.c load_b: · line 357

Quoted 9 lines
net/core/filter.c +3 -1
@@ -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?

maintainer
#14 lore raw

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

From: Joe Perches <[email protected]>
Date: Thu, 26 May 2011 08:31:06 -0700

Quoted 6 lines

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?

include/asm-generic/bug.h +0 -40
index 9178484..dfb0ec6 100644
@@ -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.
include/linux/ratelimit.h +40 -0
index 03ff67b..2f00715 100644
@@ -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 */
net/core/filter.c +1 -0
index 0e3622f..36f975f 100644
@@ -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)
Acked-by
#15 lore raw

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

Quoted 13 lines · 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

Quoted 6 lines

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.

Quoted 6 lines

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]>
Quoted 105 lines
include/asm-generic/bug.h +0 -40
index 9178484..dfb0ec6 100644
@@ -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.
include/linux/ratelimit.h +40 -0
index 03ff67b..2f00715 100644
@@ -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 */
net/core/filter.c +0 -0
index 0e3622f..36f975f 100644

net/core/filter.c

Quoted 8 lines
net/core/filter.c +1 -1
@@ -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 ***

maintainer
#16 lore raw

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

From: Randy Dunlap <[email protected]>
Date: Thu, 26 May 2011 11:57:21 -0700

Quoted 18 lines · On Thu, 26 May 2011 14:38:43 -0400 (EDT) David Miller wrote:
Quoted 17 lines

From: Joe Perches <[email protected]>
Date: Thu, 26 May 2011 08:31:06 -0700

Quoted 6 lines

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.

Quoted 7 lines
Quoted 3 lines

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.

#17 lore raw

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

In reply to David Miller
Quoted 7 lines · David Miller

From: Joe Perches <[email protected]>
Date: Thu, 26 May 2011 08:31:06 -0700

Quoted 5 lines

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?

Quoted 7 lines

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?

#18 lore raw

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

In reply to David Miller
Quoted 8 lines · David Miller

From: Joe Perches <[email protected]>
Date: Thu, 26 May 2011 08:31:06 -0700

Quoted 5 lines

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.

Quoted 4 lines
Quoted 1 line

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

#19 lore raw

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

Quoted 1 line · Ingo Molnar

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