[PATCH] net sched: cleanup and rate limit warning

[PATCH] net sched: cleanup and rate limit warning

[Stephen Hemminger]

If the user has a bad classification configuration, and gets a packet
that goes through too many steps. Chances are more packets will arrive,
and the message spew will overrun syslog because it is not rate limited.
And because it is not tagged with appropriate priority it can't not be screened.

Added the qdisc to the message to try and give some more context when
the message does arrive.

Signed-off-by: Stephen Hemminger <shemminger@vyatta.com>

---
Please think about this for 2.6.34 and could even be -stable material.

--- a/net/sched/sch_api.c	2010-05-11 17:08:42.177374275 -0700
+++ b/net/sched/sch_api.c	2010-05-11 17:16:59.560612078 -0700
@@ -1637,9 +1638,12 @@ reclassify:
 		tp = otp;
 
 		if (verd++ >= MAX_REC_LOOP) {
-			printk("rule prio %u protocol %02x reclassify loop, "
-			       "packet dropped\n",
-			       tp->prio&0xffff, ntohs(tp->protocol));
+			if (net_ratelimit())
+				printk(KERN_NOTICE
+				       "%s: packet reclassify loop"
+					  " rule prio %u protocol %02x\n",
+				       tp->q->ops->id,
+				       tp->prio & 0xffff, ntohs(tp->protocol));
 			return TC_ACT_SHOT;
 		}
 		skb->tc_verd = SET_TC_VERD(skb->tc_verd, verd);

Re: [PATCH] net sched: cleanup and rate limit warning

[jamal]

On Tue, 2010-05-11 at 17:24 -0700, Stephen Hemminger wrote:
> If the user has a bad classification configuration, and gets a packet
> that goes through too many steps.

Can you pass me the setup which caused this to be hit?

cheers,
jamal

Re: [PATCH] net sched: cleanup and rate limit warning

[Stephen Hemminger]

On Wed, 12 May 2010 13:17:54 -0400
jamal <hadi@cyberus.ca> wrote:

> On Tue, 2010-05-11 at 17:24 -0700, Stephen Hemminger wrote:
> > If the user has a bad classification configuration, and gets a packet
> > that goes through too many steps.
> 
> Can you pass me the setup which caused this to be hit?
> 
> cheers,
> jamal


The Vyatta syntax is:

 traffic-limiter test-traffic-limit {
     class 2048 {
         bandwidth 1mbit
         burst 500kbit
         match onebox {
             ip {
                 destination {
                     address 192.168.100.99/32
                 }
             }
         }
     }
 }

Which generates these TC commands.

root@VC6:~# tc qdisc show dev eth0
qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
qdisc ingress ffff: parent ffff:fff1 ---------------- 

root@VC6:~# tc filter show dev eth0 parent ffff:
filter protocol all pref 20 u32 
filter protocol all pref 20 u32 fh 800: ht divisor 1 
filter protocol all pref 20 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid ffff:800 
  match c0a86463/ffffffff at 16
 police 0x3 rate 1000Kbit burst 63999b mtu 2Kb action reclassify overhead 0b 
ref 1 bind 1


I think the bad part is the huge burst size.

Re: [PATCH] net sched: cleanup and rate limit warning

[jamal]

Thanks for the info Stephen.

On Wed, 2010-05-12 at 11:17 -0700, Stephen Hemminger wrote:

> The Vyatta syntax is:
> 
>  traffic-limiter test-traffic-limit {
>      class 2048 {
>          bandwidth 1mbit
>          burst 500kbit
>          match onebox {
>              ip {
>                  destination {
>                      address 192.168.100.99/32
>                  }
>              }
>          }
>      }
>  }
> 

;-> I guess kids these days prefer juniperism over ciscoism?
Why dont they just learn linuxism?;->

> Which generates these TC commands.
> 
> root@VC6:~# tc qdisc show dev eth0
> qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
> qdisc ingress ffff: parent ffff:fff1 ---------------- 
> 
> root@VC6:~# tc filter show dev eth0 parent ffff:
> filter protocol all pref 20 u32 
> filter protocol all pref 20 u32 fh 800: ht divisor 1 
> filter protocol all pref 20 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid ffff:800 
>   match c0a86463/ffffffff at 16
>  police 0x3 rate 1000Kbit burst 63999b mtu 2Kb action reclassify overhead 0b 
> ref 1 bind 1
> 
> 
> I think the bad part is the huge burst size.

That may be - but it seems your tool is the culprit.
It is generating wrong tc commands if i read the intent correctly.
Basically what the tc command is saying is "if you exceed the 1Mbit upto
a burst of 500kbit then reclassify".
"Reclassify" means literally that: to reuse the same classification rule
again, which will find that  we have exceeded 1M which will ask
reclassify .... loop.... I am glad that code is there ;->

cheers,
jamal

Re: [PATCH] net sched: cleanup and rate limit warning

[Stephen Hemminger]

On Wed, 12 May 2010 15:13:48 -0400
jamal <hadi@cyberus.ca> wrote:

> Thanks for the info Stephen.
> 
> On Wed, 2010-05-12 at 11:17 -0700, Stephen Hemminger wrote:
> 
> > The Vyatta syntax is:
> > 
> >  traffic-limiter test-traffic-limit {
> >      class 2048 {
> >          bandwidth 1mbit
> >          burst 500kbit
> >          match onebox {
> >              ip {
> >                  destination {
> >                      address 192.168.100.99/32
> >                  }
> >              }
> >          }
> >      }
> >  }
> > 
> 
> ;-> I guess kids these days prefer juniperism over ciscoism?
> Why dont they just learn linuxism?;->
> 
> > Which generates these TC commands.
> > 
> > root@VC6:~# tc qdisc show dev eth0
> > qdisc pfifo_fast 0: root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
> > qdisc ingress ffff: parent ffff:fff1 ---------------- 
> > 
> > root@VC6:~# tc filter show dev eth0 parent ffff:
> > filter protocol all pref 20 u32 
> > filter protocol all pref 20 u32 fh 800: ht divisor 1 
> > filter protocol all pref 20 u32 fh 800::800 order 2048 key ht 800 bkt 0 flowid ffff:800 
> >   match c0a86463/ffffffff at 16
> >  police 0x3 rate 1000Kbit burst 63999b mtu 2Kb action reclassify overhead 0b 
> > ref 1 bind 1
> > 
> > 
> > I think the bad part is the huge burst size.
> 
> That may be - but it seems your tool is the culprit.
> It is generating wrong tc commands if i read the intent correctly.
> Basically what the tc command is saying is "if you exceed the 1Mbit upto
> a burst of 500kbit then reclassify".
> "Reclassify" means literally that: to reuse the same classification rule
> again, which will find that  we have exceeded 1M which will ask
> reclassify .... loop.... I am glad that code is there ;->

The tool isn't generating an action (just tc filter ... police ..)
so it is getting the unfortunate default of reclassify.


-- 

Re: [PATCH] net sched: cleanup and rate limit warning

[jamal]

On Wed, 2010-05-12 at 13:20 -0700, Stephen Hemminger wrote:

> The tool isn't generating an action (just tc filter ... police ..)
> so it is getting the unfortunate default of reclassify.

Ah ok. 
My advice: you should never ever depend on defaults when you
can be explicit and say "drop". Or have the users in your tool be able
to specify what action to take if rate is exceeded etc (actually i think
juniper does that) 
I think "drop" would be the sane default for over-limit - my memory is
hazy because i assumed that was the default but there may have been some
reservations on that default. Patrick?

cheers,
jamal

Re: [PATCH] net sched: cleanup and rate limit warning

[Patrick McHardy]

jamal wrote:
> On Wed, 2010-05-12 at 13:20 -0700, Stephen Hemminger wrote:
> 
>> The tool isn't generating an action (just tc filter ... police ..)
>> so it is getting the unfortunate default of reclassify.
> 
> Ah ok. 
> My advice: you should never ever depend on defaults when you
> can be explicit and say "drop". Or have the users in your tool be able
> to specify what action to take if rate is exceeded etc (actually i think
> juniper does that) 
> I think "drop" would be the sane default for over-limit - my memory is
> hazy because i assumed that was the default but there may have been some
> reservations on that default. Patrick?

I don't remeber ever discussing that, the choice of "reclassify" as
default precedes TC actions and is already present in the oldest
iproute2 version I could find (2.2.4-ss000225).

Re: [PATCH] net sched: cleanup and rate limit warning

[jamal]

On Thu, 2010-05-13 at 15:12 +0200, Patrick McHardy wrote:

> I don't remeber ever discussing that, the choice of "reclassify" as
> default precedes TC actions and is already present in the oldest
> iproute2 version I could find (2.2.4-ss000225).

I think i confused it with unrelated discussion.

If its been there from day 0 then we should leave it as is - rationale:
it will help to catch config bugs.
Stephen needs to fix his tool regardless...

cheers,
jamal

Re: [PATCH] net sched: cleanup and rate limit warning

[Stephen Hemminger]

On Thu, 13 May 2010 12:20:26 -0400
jamal <hadi@cyberus.ca> wrote:

> On Thu, 2010-05-13 at 15:12 +0200, Patrick McHardy wrote:
> 
> > I don't remeber ever discussing that, the choice of "reclassify" as
> > default precedes TC actions and is already present in the oldest
> > iproute2 version I could find (2.2.4-ss000225).
> 
> I think i confused it with unrelated discussion.
> 
> If its been there from day 0 then we should leave it as is - rationale:
> it will help to catch config bugs.
> Stephen needs to fix his tool regardless...
> 
> cheers,
> jamal
> 

And the kernel message needs to be fixed to prevent total
log overload for the next poor sop who makes the same mistake.

Please accept the patch.

-- 

Re: [PATCH] net sched: cleanup and rate limit warning

[jamal]

On Thu, 2010-05-13 at 09:26 -0700, Stephen Hemminger wrote:

> > 
> 
> And the kernel message needs to be fixed to prevent total
> log overload for the next poor sop who makes the same mistake.
> 
> Please accept the patch.

I have no problem with the patch. Dont think it needs to go to stable
but i'll let Dave call it.

Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>


cheers,
jamal

Re: [PATCH] net sched: cleanup and rate limit warning

[David Miller]

From: jamal <hadi@cyberus.ca>
Date: Thu, 13 May 2010 12:40:08 -0400

> On Thu, 2010-05-13 at 09:26 -0700, Stephen Hemminger wrote:
> 
>> > 
>> 
>> And the kernel message needs to be fixed to prevent total
>> log overload for the next poor sop who makes the same mistake.
>> 
>> Please accept the patch.
> 
> I have no problem with the patch. Dont think it needs to go to stable
> but i'll let Dave call it.
> 
> Acked-by: Jamal Hadi Salim <hadi@cyberus.ca>

Applied, I don't think it's -stable material personally.  If we start
queueing stuff like this, it will be 200 instead of 100 patches in
each -stable release :-)