PATCH: action stats double dip

PATCH: action stats double dip

[jamal]

[-- Attachment #1: Type: text/plain, Size: 132 bytes --]


Dave,

Following patch fixes a scenario where we have forward compatibility
but action stats being double dipped. 

cheers,
jamal


[-- Attachment #2: p_st2 --]
[-- Type: text/plain, Size: 1654 bytes --]

--- a/net/sched/cls_u32.c	2005/03/25 15:11:01	1.1
+++ b/net/sched/cls_u32.c	2005/03/25 15:44:01
@@ -783,7 +783,7 @@
 	}
 
 	rta->rta_len = skb->tail - b;
-	if (TC_U32_KEY(n->handle))
+	if (TC_U32_KEY(n->handle) && n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
 		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
 	return skb->len;
--- a/net/sched/cls_tcindex.c	2005/03/25 16:04:46	1.1
+++ b/net/sched/cls_tcindex.c	2005/03/25 16:05:20
@@ -496,8 +496,9 @@
 			goto rtattr_failure;
 		rta->rta_len = skb->tail-b;
 
-		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
-			goto rtattr_failure;
+		if (n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
+			if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
+				goto rtattr_failure;
 	}
 	
 	return skb->len;
--- a/net/sched/cls_fw.c	2005/03/25 15:48:27	1.1
+++ b/net/sched/cls_fw.c	2005/03/25 16:02:22
@@ -338,8 +338,9 @@
 
 	rta->rta_len = skb->tail - b;
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
-		goto rtattr_failure;
+	if (n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
+		if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+			goto rtattr_failure;
 
 	return skb->len;
 
--- a/net/sched/cls_route.c	2005/03/25 16:03:11	1.1
+++ b/net/sched/cls_route.c	2005/03/25 16:04:34
@@ -599,8 +599,9 @@
 
 	rta->rta_len = skb->tail - b;
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
-		goto rtattr_failure;
+	if (n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
+		if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+			goto rtattr_failure;
 
 	return skb->len;
 

Re: PATCH: action stats double dip

[jamal]

[-- Attachment #1: Type: text/plain, Size: 264 bytes --]

Dave, 
Ignore previous one - this one compiles better ;->


cheers,
jamal

On Fri, 2005-03-25 at 11:25, jamal wrote:
> Dave,
> 
> Following patch fixes a scenario where we have forward compatibility
> but action stats being double dipped. 
> 
> cheers,
> jamal
> 

[-- Attachment #2: p_st3 --]
[-- Type: text/plain, Size: 1718 bytes --]

--- 26115-mod/net/sched/cls_route.c	2005/03/25 16:03:11	1.1
+++ 26115-mod/net/sched/cls_route.c	2005/03/25 16:34:02
@@ -599,8 +599,9 @@
 
 	rta->rta_len = skb->tail - b;
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
-		goto rtattr_failure;
+	if (f->exts.action && f->exts.action->type == TCA_OLD_COMPAT)
+		if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+			goto rtattr_failure;
 
 	return skb->len;
 
--- 26115-mod/net/sched/cls_u32.c	2005/03/25 15:11:01	1.1
+++ 26115-mod/net/sched/cls_u32.c	2005/03/25 15:44:01
@@ -783,7 +783,7 @@
 	}
 
 	rta->rta_len = skb->tail - b;
-	if (TC_U32_KEY(n->handle))
+	if (TC_U32_KEY(n->handle) && n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
 		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
 	return skb->len;
--- 26115-mod/net/sched/cls_tcindex.c	2005/03/25 16:04:46	1.1
+++ 26115-mod/net/sched/cls_tcindex.c	2005/03/25 16:36:47
@@ -496,8 +496,9 @@
 			goto rtattr_failure;
 		rta->rta_len = skb->tail-b;
 
-		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
-			goto rtattr_failure;
+		if (r->exts.action && r->exts.action->type == TCA_OLD_COMPAT)
+			if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
+				goto rtattr_failure;
 	}
 	
 	return skb->len;
--- 26115-mod/net/sched/cls_fw.c	2005/03/25 15:48:27	1.1
+++ 26115-mod/net/sched/cls_fw.c	2005/03/25 16:36:20
@@ -338,8 +338,9 @@
 
 	rta->rta_len = skb->tail - b;
 
-	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
-		goto rtattr_failure;
+	if (f->exts.action && f->exts.action->type == TCA_OLD_COMPAT)
+		if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+			goto rtattr_failure;
 
 	return skb->len;
 

Re: PATCH: action stats double dip

[David S. Miller]

On 25 Mar 2005 11:41:24 -0500
jamal <hadi@cyberus.ca> wrote:

> Ignore previous one - this one compiles better ;->

Applied, thanks a lot Jamal.

Re: PATCH: action stats double dip

[Thomas Graf]

* jamal <1111768884.1092.533.camel@jzny.localdomain> 2005-03-25 11:41
>  
> -	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> -		goto rtattr_failure;
> +	if (f->exts.action && f->exts.action->type == TCA_OLD_COMPAT)
> +		if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> +			goto rtattr_failure;

Why is this needed? Maybe I'm missing something in the logic but
tcf_exts_dump_stats checks for exts->action and if in compat mode
provides the old stats TLVs. I'm not claiming that the current code
is correct but the fix should go into tcf_exts_dump_stats rather
than into every classifier.

Re: PATCH: action stats double dip

[jamal]

On Fri, 2005-03-25 at 15:06, Thomas Graf wrote:
> * jamal <1111768884.1092.533.camel@jzny.localdomain> 2005-03-25 11:41
> >  
> > -	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> > -		goto rtattr_failure;
> > +	if (f->exts.action && f->exts.action->type == TCA_OLD_COMPAT)
> > +		if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> > +			goto rtattr_failure;
> 
> Why is this needed? Maybe I'm missing something in the logic but
> tcf_exts_dump_stats checks for exts->action and if in compat mode
> provides the old stats TLVs. I'm not claiming that the current code
> is correct but the fix should go into tcf_exts_dump_stats rather
> than into every classifier.

tcf_exts_dump (called above the those calls) already dumps stats if you
are not in old compatibility mode. It doesnt when you are in old compat
mode. 
You could probably play tricks to move things into it; 
The main exception would be cls_u32 where you need to check
TC_U32_KEY(n->handle) and old compatibility;
I take it when you were creating the patches you left things
in the classifiers for consistency? I just followed the way you already
had it (which is fine in my opinion).
Otherwise all classifiers but u32 could have their code moved.

cheers,
jamal

Re: PATCH: action stats double dip

[Thomas Graf]

* jamal <1111782325.1089.641.camel@jzny.localdomain> 2005-03-25 15:25
> On Fri, 2005-03-25 at 15:06, Thomas Graf wrote:
> > * jamal <1111768884.1092.533.camel@jzny.localdomain> 2005-03-25 11:41
> > >  
> > > -	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> > > -		goto rtattr_failure;
> > > +	if (f->exts.action && f->exts.action->type == TCA_OLD_COMPAT)
> > > +		if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
> > > +			goto rtattr_failure;
> > 
> > Why is this needed? Maybe I'm missing something in the logic but
> > tcf_exts_dump_stats checks for exts->action and if in compat mode
> > provides the old stats TLVs. I'm not claiming that the current code
> > is correct but the fix should go into tcf_exts_dump_stats rather
> > than into every classifier.
> 
> tcf_exts_dump (called above the those calls) already dumps stats if you
> are not in old compatibility mode. It doesnt when you are in old compat
> mode. 
> You could probably play tricks to move things into it; 
> The main exception would be cls_u32 where you need to check
> TC_U32_KEY(n->handle) and old compatibility;

Right, I had to leave it splitted for u32 but there are still
issues. With your changes, the old policer will not get its stats
dumped anymore. Any reason for keeping the old policer alive? or
can we purge it and make everyone use the compatibility police
action?  Don't waste any time on this, it's my fault so let me
resolve this.

Re: PATCH: action stats double dip

[jamal]

On Fri, 2005-03-25 at 15:41, Thomas Graf wrote:
> * jamal <1111782325.1089.641.camel@jzny.localdomain> 2005-03-25 15:25

> > tcf_exts_dump (called above the those calls) already dumps stats if you
> > are not in old compatibility mode. It doesnt when you are in old compat
> > mode. 
> > You could probably play tricks to move things into it; 
> > The main exception would be cls_u32 where you need to check
> > TC_U32_KEY(n->handle) and old compatibility;
> 
> Right, I had to leave it splitted for u32 but there are still
> issues. With your changes, the old policer will not get its stats
> dumped anymore.

My worry is this was always the case. 

>  Any reason for keeping the old policer alive? or
> can we purge it and make everyone use the compatibility police
> action? 

Well, old tc binaries exist and should continue to work. I suppose
we can put a time limit on how long such things can be supported.
It could get killed if it becomes a nuisance but it will have to be
advertised in some kernel changelogs 

>  Don't waste any time on this, it's my fault so let me
> resolve this.

I dont think that its devasting actually after i moved around 
the TCA_ACT_STAT attribute in previous patch (so even without that
fix all that would happen is tb[TCA_ACT_STATS] will be written twice.
And i dont think its your fault - bugs are expected(note my wisecrack
comments to Andy Furniss;->) and the patch was widely discussed none of
us saw this. 
Dave has already taken in the patch - if you want to amend things
i would suggest you do it against the patch i posted.

cheers,
jamal

Re: PATCH: action stats double dip

[Thomas Graf]

* jamal <1111786752.1090.678.camel@jzny.localdomain> 2005-03-25 16:39
> Well, old tc binaries exist and should continue to work. I suppose
> we can put a time limit on how long such things can be supported.
> It could get killed if it becomes a nuisance but it will have to be
> advertised in some kernel changelogs 

Yes but they should continue to work via the compatiblity code
in the action, right? It's just that we could get rid of all
the ifdef mess. These bugs don't come out of nowhere, the main
cause is exactly this mess I think ;->

> >  Don't waste any time on this, it's my fault so let me
> > resolve this.
> 
> I dont think that its devasting actually after i moved around 
> the TCA_ACT_STAT attribute in previous patch (so even without that
> fix all that would happen is tb[TCA_ACT_STATS] will be written twice.
> And i dont think its your fault - bugs are expected(note my wisecrack
> comments to Andy Furniss;->) and the patch was widely discussed none of
> us saw this. 
> Dave has already taken in the patch - if you want to amend things
> i would suggest you do it against the patch i posted.

Agreed, I have to look into this more deeply. As it seems things
are really messed up anyways. We have the same stats TLV being
put into TCA_OPTIONS but also into the root array which I think
lead you to move that TCA_ACT_STATS, both is wrong ;->

Re: PATCH: action stats double dip

[jamal]

On Fri, 2005-03-25 at 16:58, Thomas Graf wrote:
> * jamal <1111786752.1090.678.camel@jzny.localdomain> 2005-03-25 16:39

> 
> Agreed, I have to look into this more deeply. As it seems things
> are really messed up anyways. We have the same stats TLV being
> put into TCA_OPTIONS but also into the root array which I think
> lead you to move that TCA_ACT_STATS, both is wrong ;->
> 

The stats for actions seem fine - Andy and to a small extent myself have
verified this. 
What i moved was TCA_ACT_STATS which really belongs to the action
hierachy. The other level still has STATS like before used by qdiscs
etc. I quickly checked those and they looked sane as well. Trust me i
panicked after looking at the code the first time i found the stats were
messed when Andy reported them. Then i realized it was a simple issue of
moving around TCA_ACT_STATS.

cheers,
jamal

Re: PATCH: action stats double dip

[Thomas Graf]

* jamal <1111791991.1091.732.camel@jzny.localdomain> 2005-03-25 18:06
> On Fri, 2005-03-25 at 16:58, Thomas Graf wrote:
> > * jamal <1111786752.1090.678.camel@jzny.localdomain> 2005-03-25 16:39
> 
> > 
> > Agreed, I have to look into this more deeply. As it seems things
> > are really messed up anyways. We have the same stats TLV being
> > put into TCA_OPTIONS but also into the root array which I think
> > lead you to move that TCA_ACT_STATS, both is wrong ;->
> > 
> 
> The stats for actions seem fine - Andy and to a small extent myself have
> verified this. 
> What i moved was TCA_ACT_STATS which really belongs to the action
> hierachy. The other level still has STATS like before used by qdiscs
> etc. I quickly checked those and they looked sane as well. Trust me i
> panicked after looking at the code the first time i found the stats were
> messed when Andy reported them. Then i realized it was a simple issue of
> moving around TCA_ACT_STATS.

I'm quite sure that you see them perfectly fine but the same stats
are put once into TCA_OPTIONS (tcf_exts_dump) and again outside of
it (tcf_exts_dump_stats) which either is simply a duplication or may
lead to overwriting of a TLV in userspace. But let me check this
tomorrow, my tree doesn't include your patches yet etc.

Re: PATCH: action stats double dip

[Thomas Graf]

* Thomas Graf <20050325232431.GJ3086@postel.suug.ch> 2005-03-26 00:24
> I'm quite sure that you see them perfectly fine but the same stats
> are put once into TCA_OPTIONS (tcf_exts_dump) and again outside of
> it (tcf_exts_dump_stats) which either is simply a duplication or may
> lead to overwriting of a TLV in userspace. But let me check this
> tomorrow, my tree doesn't include your patches yet etc.

Heh, your patch obviously fixes this. Never mind, patch is fine. ;->