diff mbox

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

Message ID 1305662020.1722.42.camel@Joe-Laptop
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Joe Perches May 17, 2011, 7:53 p.m. UTC 
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(-)



--
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

Comments

Joe Perches May 17, 2011, 9:13 p.m. UTC | #1 
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

--
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
Joe Perches May 21, 2011, 5:48 p.m. UTC | #2 
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(-)
David Miller May 23, 2011, 9:38 p.m. UTC | #3 
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!
--
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
diff mbox

Patch

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