cls_u32 compile failure in current 2.6.12-rc1+BK tree

cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Meelis Roos]

   CC [M]  net/sched/cls_u32.o
net/sched/cls_u32.c: In function `u32_dump':
net/sched/cls_u32.c:778: error: structure has no member named `action'
net/sched/cls_u32.c:778: error: structure has no member named `action'

-- 
Meelis Roos (mroos@linux.ee)

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

Hi there,
For now just turn on actions under qos menu.
The solution needs some thought or major surgery.

cheers,
jamal

On Sun, 2005-03-27 at 14:49, Meelis Roos wrote:
>    CC [M]  net/sched/cls_u32.o
> net/sched/cls_u32.c: In function `u32_dump':
> net/sched/cls_u32.c:778: error: structure has no member named `action'
> net/sched/cls_u32.c:778: error: structure has no member named `action'

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Meelis Roos]

> For now just turn on actions under qos menu.

The help tells
You MUST NOT turn this on if you dont have an update iproute2.
So I did not turn it on. I have iproute 20041019-3 in Debian unstable. 
Will something break? I'm not using QoS actively, sonly try some setups 
sometimes.

-- 
Meelis Roos (mroos@linux.ee)

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

On Mon, 2005-03-28 at 02:18, Meelis Roos wrote:
> > For now just turn on actions under qos menu.
> 
> The help tells
> You MUST NOT turn this on if you dont have an update iproute2.
> So I did not turn it on. I have iproute 20041019-3 in Debian unstable. 
> Will something break? I'm not using QoS actively, sonly try some setups 
> sometimes.

Well, it is NOT supposed to break - but lets find out shall we? ;->

Thomas: I have been doing some thinking.
If we are going to make u32 an exception then this code should be
surrounded by #ifdef ..ACTION..
The rest of the classifiers can also follow the same scheme.
Or the rest of the classifiers can have their code in the generic stats
(and removed from within the classifiers).
Which means that the other classifiers may have to use a slightly
different function name unless you want to pass a variable to
distinguish the two cases. My time is about to become scarce, so if you
have time please create the patch.

cheers,
jamal

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* jamal <1112012657.1089.1013.camel@jzny.localdomain> 2005-03-28 07:24
> If we are going to make u32 an exception then this code should be
> surrounded by #ifdef ..ACTION..

The same condition is needed for tcf_exts_dump and tcf_exts_dump_stats
in u32, they're only apart to cut the TCA_OPTIONS nested TLV so I'm
trying to find out is whether we can get rid of dump_stats alltogether
and hide it. I'm working on a patch to do this, I hope to have enough time
to finish it today.

> My time is about to become scarce, so if you
> have time please create the patch.

Again, don't waste cycles on this. It was my fault and I will fix it. ;->

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

On Mon, 2005-03-28 at 07:38, Thomas Graf wrote:
> * jamal <1112012657.1089.1013.camel@jzny.localdomain> 2005-03-28 07:24
> > If we are going to make u32 an exception then this code should be
> > surrounded by #ifdef ..ACTION..
> 
> The same condition is needed for tcf_exts_dump 

This one seems fine even for u32, no?

cheers,
jamal

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* jamal <1112014261.1089.1018.camel@jzny.localdomain> 2005-03-28 07:51
> On Mon, 2005-03-28 at 07:38, Thomas Graf wrote:
> > * jamal <1112012657.1089.1013.camel@jzny.localdomain> 2005-03-28 07:24
> > > If we are going to make u32 an exception then this code should be
> > > surrounded by #ifdef ..ACTION..
> > 
> > The same condition is needed for tcf_exts_dump 
> 
> This one seems fine even for u32, no?

Not sure what you mean, I don't want any ifdefs or actions bits
in the classifiers itself anymore. Everytime we dump the actions
we must be able to also dump the statistics so I think we can
safely move tcf_exts_dump_stats into tcf_exts_dump and hide all
dumping in that single API.

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

On Mon, 2005-03-28 at 07:58, Thomas Graf wrote:

> Not sure what you mean, I don't want any ifdefs or actions bits
> in the classifiers itself anymore. Everytime we dump the actions
> we must be able to also dump the statistics so I think we can
> safely move tcf_exts_dump_stats into tcf_exts_dump and hide all
> dumping in that single API.
> 

But you cant completely do this for u32 tcf_exts_dump_stats.
in the case of ACTION compiled in but someone using old API.
The code that Meelis just mention that compiler complained about. 
There was no problem with u32 and tcf_exts_dump - with that you can hide
all #ifedfs. 

cheers,
jamal

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* jamal <1112015725.1089.1045.camel@jzny.localdomain> 2005-03-28 08:15
> On Mon, 2005-03-28 at 07:58, Thomas Graf wrote:
> 
> > Not sure what you mean, I don't want any ifdefs or actions bits
> > in the classifiers itself anymore. Everytime we dump the actions
> > we must be able to also dump the statistics so I think we can
> > safely move tcf_exts_dump_stats into tcf_exts_dump and hide all
> > dumping in that single API.
> > 
> 
> But you cant completely do this for u32 tcf_exts_dump_stats.
> in the case of ACTION compiled in but someone using old API.
> The code that Meelis just mention that compiler complained about. 
> There was no problem with u32 and tcf_exts_dump - with that you can hide
> all #ifedfs. 

I think we can avoid your fixes. The initial reason for having
dump_stats was that the statistics were put into a TLV outside
of TCA_OPTIONS (which was wrong). Now that both, the new and old
compatibility stats are inside of TCA_OPTIONS it is no problem
do dump everything in tcf_exts_dump and do the ifdef thing there
instead of putting it into every classifier.

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

On Mon, 2005-03-28 at 10:55, Thomas Graf wrote:
> * jamal <1112015725.1089.1045.camel@jzny.localdomain> 2005-03-28 08:15
> > On Mon, 2005-03-28 at 07:58, Thomas Graf wrote:

> > But you cant completely do this for u32 tcf_exts_dump_stats.
> > in the case of ACTION compiled in but someone using old API.
> > The code that Meelis just mention that compiler complained about. 
> > There was no problem with u32 and tcf_exts_dump - with that you can hide
> > all #ifedfs. 
> 
> I think we can avoid your fixes. 

You are still missing the question;->

You can do this for every classifier but u32. Or is it me who is missing
something?

cheers,
jamal

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* jamal <1112038128.1119.5.camel@jzny.localdomain> 2005-03-28 14:28
> On Mon, 2005-03-28 at 10:55, Thomas Graf wrote:
> > * jamal <1112015725.1089.1045.camel@jzny.localdomain> 2005-03-28 08:15
> > > On Mon, 2005-03-28 at 07:58, Thomas Graf wrote:
> 
> > > But you cant completely do this for u32 tcf_exts_dump_stats.
> > > in the case of ACTION compiled in but someone using old API.
> > > The code that Meelis just mention that compiler complained about. 
> > > There was no problem with u32 and tcf_exts_dump - with that you can hide
> > > all #ifedfs. 
> > 
> > I think we can avoid your fixes. 
> 
> You are still missing the question;->
> 
> You can do this for every classifier but u32. Or is it me who is missing
> something?

I think what you are refering to is the TC_U32_KEY() check, right?
If you look at the place where tcf_exts_dump is called you can
see that the same pre condition is given since the call is in a
else branch of a TC_U32_KEY() == 0 condition.

I haven't had the time to come up with a patch, too many other
things to take care of but I think it should work out ok.

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* Thomas Graf <20050328195320.GE3086@postel.suug.ch> 2005-03-28 21:53
> * jamal <1112038128.1119.5.camel@jzny.localdomain> 2005-03-28 14:28
> > On Mon, 2005-03-28 at 10:55, Thomas Graf wrote:
> > > * jamal <1112015725.1089.1045.camel@jzny.localdomain> 2005-03-28 08:15
> > > > On Mon, 2005-03-28 at 07:58, Thomas Graf wrote:
> > 
> > > > But you cant completely do this for u32 tcf_exts_dump_stats.
> > > > in the case of ACTION compiled in but someone using old API.
> > > > The code that Meelis just mention that compiler complained about. 
> > > > There was no problem with u32 and tcf_exts_dump - with that you can hide
> > > > all #ifedfs. 
> > > 
> > > I think we can avoid your fixes. 
> > 
> > You are still missing the question;->
> > 
> > You can do this for every classifier but u32. Or is it me who is missing
> > something?
> 
> I think what you are refering to is the TC_U32_KEY() check, right?
> If you look at the place where tcf_exts_dump is called you can
> see that the same pre condition is given since the call is in a
> else branch of a TC_U32_KEY() == 0 condition.
> 
> I haven't had the time to come up with a patch, too many other
> things to take care of but I think it should work out ok.

Before I go ahead, let's make sure I do it right this time. We do
have the following TLVs that need to filled out:

old policer:
  TCA_STATS2 (gnet_stats_*)
  TCA_STATS (tc_stats)

action:
  TCA_ACT_STATS (gnet_stats_*)

action in compatiblity mode:
  TCA_ACT_STATS (gnet_stats_*)
  TCA_STATS (tc_stats)
  TCA_XSTATS

I see that you moved TCA_ACT_STATS into the action TLV array which
doesn't work with the gnet_stats architecture anymore since the
backward compatiblity TCA_STATS and TCA_XSTATS must be outside of
the action TLV. So we need to split the statistics dumping if we don't
want to change iproute2 and have TCA_ACT_STATS being on the same level
as TCA_OPTIONS of the classifier. I didn't notice this breakage when
you posted the patch, sorry.

Idea:
We dump TCA_ACT_STATS in tcf_exts_dump and handle all the backward
compibility including dumping policer stats in tcf_exts_dump_stats.
This means we have to adapt gnet_stats a bit to allow for this.

Thoughts?

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

* Thomas Graf <20050328223310.GH3086@postel.suug.ch> 2005-03-29 00:33
> We dump TCA_ACT_STATS in tcf_exts_dump and handle all the backward
> compibility including dumping policer stats in tcf_exts_dump_stats.
> This means we have to adapt gnet_stats a bit to allow for this.

Attached patches is what I meant. Untested but you should get
the idea.

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/29 02:45:52+02:00 tgraf@suug.ch 
#   Cset exclude: hadi@cyberus.ca|ChangeSet|20050325173452|50562
# 
# net/sched/cls_u32.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_tcindex.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_route.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_fw.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
diff -Nru a/net/sched/cls_fw.c b/net/sched/cls_fw.c
--- a/net/sched/cls_fw.c	2005-03-29 03:07:44 +02:00
+++ b/net/sched/cls_fw.c	2005-03-29 03:07:44 +02:00
@@ -338,9 +338,8 @@
 
 	rta->rta_len = skb->tail - b;
 
-	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;
+	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+		goto rtattr_failure;
 
 	return skb->len;
 
diff -Nru a/net/sched/cls_route.c b/net/sched/cls_route.c
--- a/net/sched/cls_route.c	2005-03-29 03:07:44 +02:00
+++ b/net/sched/cls_route.c	2005-03-29 03:07:44 +02:00
@@ -599,9 +599,8 @@
 
 	rta->rta_len = skb->tail - b;
 
-	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;
+	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+		goto rtattr_failure;
 
 	return skb->len;
 
diff -Nru a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
--- a/net/sched/cls_tcindex.c	2005-03-29 03:07:44 +02:00
+++ b/net/sched/cls_tcindex.c	2005-03-29 03:07:44 +02:00
@@ -496,9 +496,8 @@
 			goto rtattr_failure;
 		rta->rta_len = skb->tail-b;
 
-		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;
+		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
+			goto rtattr_failure;
 	}
 	
 	return skb->len;
diff -Nru a/net/sched/cls_u32.c b/net/sched/cls_u32.c
--- a/net/sched/cls_u32.c	2005-03-29 03:07:44 +02:00
+++ b/net/sched/cls_u32.c	2005-03-29 03:07:44 +02:00
@@ -775,7 +775,7 @@
 	}
 
 	rta->rta_len = skb->tail - b;
-	if (TC_U32_KEY(n->handle) && n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
+	if (TC_U32_KEY(n->handle))
 		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
 	return skb->len;

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/29 02:55:11+02:00 tgraf@suug.ch 
#   [NET]: Make primary TLV type optional
#   
#   Allows the use of the gnet_stats API for backward compatiblity
#   cases where no "modern" TLV structure is needed.
#   
#   Signed-off-by: Thomas Graf <tgraf@suug.ch>
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/core/gen_stats.c
#   2005/03/29 02:54:56+02:00 tgraf@suug.ch +7 -3
#   [NET]: Make primary TLV type optional
# 
diff -Nru a/net/core/gen_stats.c b/net/core/gen_stats.c
--- a/net/core/gen_stats.c	2005-03-29 03:07:59 +02:00
+++ b/net/core/gen_stats.c	2005-03-29 03:07:59 +02:00
@@ -26,7 +26,9 @@
 static inline int
 gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
 {
-	RTA_PUT(d->skb, type, size, buf);
+	if (type)
+		RTA_PUT(d->skb, type, size, buf);
+
 	return 0;
 
 rtattr_failure:
@@ -58,7 +60,8 @@
 {
 	spin_lock_bh(lock);
 	d->lock = lock;
-	d->tail = (struct rtattr *) skb->tail;
+	if (type)
+		d->tail = (struct rtattr *) skb->tail;
 	d->skb = skb;
 	d->compat_tc_stats = tc_stats_type;
 	d->compat_xstats = xstats_type;
@@ -194,7 +197,8 @@
 int
 gnet_stats_finish_copy(struct gnet_dump *d)
 {
-	d->tail->rta_len = d->skb->tail - (u8 *) d->tail;
+	if (d->tail)
+		d->tail->rta_len = d->skb->tail - (u8 *) d->tail;
 
 	if (d->compat_tc_stats)
 		if (gnet_stats_copy(d, d->compat_tc_stats, &d->tc_stats,

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/29 02:59:25+02:00 tgraf@suug.ch 
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
#   
#   Extends the action dumping function by a parameter to differ between
#   regular calls and the one supposed to add the backward compatiblity
#   bits for old userspace applications.
#   
#   Signed-off-by: Thomas Graf <tgraf@suug.ch>
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/sched/cls_api.c
#   2005/03/29 02:59:11+02:00 tgraf@suug.ch +1 -1
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
# net/sched/act_api.c
#   2005/03/29 02:59:11+02:00 tgraf@suug.ch +8 -4
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
# include/net/act_api.h
#   2005/03/29 02:59:11+02:00 tgraf@suug.ch +1 -1
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
diff -Nru a/include/net/act_api.h b/include/net/act_api.h
--- a/include/net/act_api.h	2005-03-29 03:08:12 +02:00
+++ b/include/net/act_api.h	2005-03-29 03:08:12 +02:00
@@ -81,7 +81,7 @@
 extern int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
-extern int tcf_action_copy_stats (struct sk_buff *,struct tc_action *);
+extern int tcf_action_copy_stats (struct sk_buff *,struct tc_action *, int);
 #endif /* CONFIG_NET_CLS_ACT */
 
 extern int tcf_police(struct sk_buff *skb, struct tcf_police *p);
diff -Nru a/net/sched/act_api.c b/net/sched/act_api.c
--- a/net/sched/act_api.c	2005-03-29 03:08:12 +02:00
+++ b/net/sched/act_api.c	2005-03-29 03:08:12 +02:00
@@ -228,7 +228,7 @@
 		return err;
 
 	RTA_PUT(skb, TCA_KIND, IFNAMSIZ, a->ops->kind);
-	if (tcf_action_copy_stats(skb, a))
+	if (tcf_action_copy_stats(skb, a, 0))
 		goto rtattr_failure;
 	r = (struct rtattr*) skb->tail;
 	RTA_PUT(skb, TCA_OPTIONS, 0, NULL);
@@ -380,7 +380,8 @@
 	return NULL;
 }
 
-int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a)
+int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
+			  int compat_mode)
 {
 	int err;
 	struct gnet_dump d;
@@ -389,8 +390,11 @@
 	if (h == NULL)
 		goto errout;
 
-	if (a->type == TCA_OLD_COMPAT)
-		err = gnet_stats_start_copy_compat(skb, TCA_ACT_STATS,
+	/* compat_mode being true specifies a call that is supposed
+	 * to add additional backward compatiblity statistic TLVs.
+	 */
+	if (compat_mode && a->type == TCA_OLD_COMPAT)
+		err = gnet_stats_start_copy_compat(skb, 0,
 			TCA_STATS, TCA_XSTATS, h->stats_lock, &d);
 	else
 		err = gnet_stats_start_copy(skb, TCA_ACT_STATS,
diff -Nru a/net/sched/cls_api.c b/net/sched/cls_api.c
--- a/net/sched/cls_api.c	2005-03-29 03:08:12 +02:00
+++ b/net/sched/cls_api.c	2005-03-29 03:08:12 +02:00
@@ -602,7 +602,7 @@
 {
 #ifdef CONFIG_NET_CLS_ACT
 	if (exts->action)
-		if (tcf_action_copy_stats(skb, exts->action) < 0)
+		if (tcf_action_copy_stats(skb, exts->action, 1) < 0)
 			goto rtattr_failure;
 #elif defined CONFIG_NET_CLS_POLICE
 	if (exts->police)

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

In general (from 1 mile away looks fine).
One issue: a->type could be used for in the future to carry
other things in addition to backward compatibility. So you cant assume
it being set means only backward compat is needed (and nothing else).

cheers,
jamal

On Mon, 2005-03-28 at 20:10, Thomas Graf wrote:
> * Thomas Graf <20050328223310.GH3086@postel.suug.ch> 2005-03-29 00:33
> > We dump TCA_ACT_STATS in tcf_exts_dump and handle all the backward
> > compibility including dumping policer stats in tcf_exts_dump_stats.
> > This means we have to adapt gnet_stats a bit to allow for this.
> 
> Attached patches is what I meant. Untested but you should get
> the idea.

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[Thomas Graf]

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

* jamal <1112129516.1076.90.camel@jzny.localdomain> 2005-03-29 15:51
> 
> In general (from 1 mile away looks fine).
> One issue: a->type could be used for in the future to carry
> other things in addition to backward compatibility. So you cant assume
> it being set means only backward compat is needed (and nothing else).

Not sure if I get you but maybe a mistake of mine while mapping
the logic in mind into code has confused you. The code really
should have been:

	/* compat_mode being true specifies a call that is supposed
	 * to add additional backward compatiblity statistic TLVs.
	 */
	if (compat_mode) {
		if (a->type == TCA_OLD_COMPAT)
			err = gnet_stats_start_copy_compat(skb, 0,
				TCA_STATS, TCA_XSTATS, h->stats_lock, &d);
	} else
		err = gnet_stats_start_copy(skb, TCA_ACT_STATS,
			h->stats_lock, &d);

So we always dump TCA_ACT_STATS when calling from tcf_exts_dump
and add the backward compatibility TLVs in the second call
comming from tcf_exts_dump_stats for actions that need it.

Patches tested, works for me but might be worth for someone
else to give it a try as well just to make sure. Meelis, could
you give the 3 attched patches a try?

Dave, you can pull from bk://kernel.bkbits.net/tgraf/net-2.6-tcf_exts
if everyone can agree on this.

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

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/29 02:45:52+02:00 tgraf@suug.ch 
#   Cset exclude: hadi@cyberus.ca|ChangeSet|20050325173452|50562
# 
# net/sched/cls_u32.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_tcindex.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_route.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
# net/sched/cls_fw.c
#   2005/03/29 02:45:46+02:00 tgraf@suug.ch +0 -0
#   Exclude
# 
diff -Nru a/net/sched/cls_fw.c b/net/sched/cls_fw.c
--- a/net/sched/cls_fw.c	2005-03-30 02:59:05 +02:00
+++ b/net/sched/cls_fw.c	2005-03-30 02:59:05 +02:00
@@ -338,9 +338,8 @@
 
 	rta->rta_len = skb->tail - b;
 
-	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;
+	if (tcf_exts_dump_stats(skb, &f->exts, &fw_ext_map) < 0)
+		goto rtattr_failure;
 
 	return skb->len;
 
diff -Nru a/net/sched/cls_route.c b/net/sched/cls_route.c
--- a/net/sched/cls_route.c	2005-03-30 02:59:05 +02:00
+++ b/net/sched/cls_route.c	2005-03-30 02:59:05 +02:00
@@ -599,9 +599,8 @@
 
 	rta->rta_len = skb->tail - b;
 
-	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;
+	if (tcf_exts_dump_stats(skb, &f->exts, &route_ext_map) < 0)
+		goto rtattr_failure;
 
 	return skb->len;
 
diff -Nru a/net/sched/cls_tcindex.c b/net/sched/cls_tcindex.c
--- a/net/sched/cls_tcindex.c	2005-03-30 02:59:05 +02:00
+++ b/net/sched/cls_tcindex.c	2005-03-30 02:59:05 +02:00
@@ -496,9 +496,8 @@
 			goto rtattr_failure;
 		rta->rta_len = skb->tail-b;
 
-		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;
+		if (tcf_exts_dump_stats(skb, &r->exts, &tcindex_ext_map) < 0)
+			goto rtattr_failure;
 	}
 	
 	return skb->len;
diff -Nru a/net/sched/cls_u32.c b/net/sched/cls_u32.c
--- a/net/sched/cls_u32.c	2005-03-30 02:59:05 +02:00
+++ b/net/sched/cls_u32.c	2005-03-30 02:59:05 +02:00
@@ -775,7 +775,7 @@
 	}
 
 	rta->rta_len = skb->tail - b;
-	if (TC_U32_KEY(n->handle) && n->exts.action && n->exts.action->type == TCA_OLD_COMPAT)
+	if (TC_U32_KEY(n->handle))
 		if (tcf_exts_dump_stats(skb, &n->exts, &u32_ext_map) < 0)
 			goto rtattr_failure;
 	return skb->len;

[-- Attachment #3: 2 --]
[-- Type: text/plain, Size: 1427 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/29 02:55:11+02:00 tgraf@suug.ch 
#   [NET]: Make primary TLV type optional
#   
#   Allows the use of the gnet_stats API for backward compatiblity
#   cases where no "modern" TLV structure is needed.
#   
#   Signed-off-by: Thomas Graf <tgraf@suug.ch>
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/core/gen_stats.c
#   2005/03/29 02:54:56+02:00 tgraf@suug.ch +7 -3
#   [NET]: Make primary TLV type optional
# 
diff -Nru a/net/core/gen_stats.c b/net/core/gen_stats.c
--- a/net/core/gen_stats.c	2005-03-30 02:59:19 +02:00
+++ b/net/core/gen_stats.c	2005-03-30 02:59:19 +02:00
@@ -26,7 +26,9 @@
 static inline int
 gnet_stats_copy(struct gnet_dump *d, int type, void *buf, int size)
 {
-	RTA_PUT(d->skb, type, size, buf);
+	if (type)
+		RTA_PUT(d->skb, type, size, buf);
+
 	return 0;
 
 rtattr_failure:
@@ -58,7 +60,8 @@
 {
 	spin_lock_bh(lock);
 	d->lock = lock;
-	d->tail = (struct rtattr *) skb->tail;
+	if (type)
+		d->tail = (struct rtattr *) skb->tail;
 	d->skb = skb;
 	d->compat_tc_stats = tc_stats_type;
 	d->compat_xstats = xstats_type;
@@ -194,7 +197,8 @@
 int
 gnet_stats_finish_copy(struct gnet_dump *d)
 {
-	d->tail->rta_len = d->skb->tail - (u8 *) d->tail;
+	if (d->tail)
+		d->tail->rta_len = d->skb->tail - (u8 *) d->tail;
 
 	if (d->compat_tc_stats)
 		if (gnet_stats_copy(d, d->compat_tc_stats, &d->tc_stats,

[-- Attachment #4: 3 --]
[-- Type: text/plain, Size: 3287 bytes --]

# This is a BitKeeper generated diff -Nru style patch.
#
# ChangeSet
#   2005/03/30 01:34:30+02:00 tgraf@suug.ch 
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
#   
#   Extends the action dumping function by a parameter to differ between
#   regular calls and the one supposed to add the backward compatiblity
#   bits for old userspace applications.
#   
#   Signed-off-by: Thomas Graf <tgraf@suug.ch>
#   Signed-off-by: David S. Miller <davem@davemloft.net>
# 
# net/sched/cls_api.c
#   2005/03/30 01:34:14+02:00 tgraf@suug.ch +1 -1
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
# net/sched/act_api.c
#   2005/03/30 01:34:14+02:00 tgraf@suug.ch +12 -7
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
# include/net/act_api.h
#   2005/03/30 01:34:14+02:00 tgraf@suug.ch +1 -1
#   [PKT_SCHED]: Fix action statistics dumping in compatibility mode
# 
diff -Nru a/include/net/act_api.h b/include/net/act_api.h
--- a/include/net/act_api.h	2005-03-30 03:00:38 +02:00
+++ b/include/net/act_api.h	2005-03-30 03:00:38 +02:00
@@ -81,7 +81,7 @@
 extern int tcf_action_dump(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_old(struct sk_buff *skb, struct tc_action *a, int, int);
 extern int tcf_action_dump_1(struct sk_buff *skb, struct tc_action *a, int, int);
-extern int tcf_action_copy_stats (struct sk_buff *,struct tc_action *);
+extern int tcf_action_copy_stats (struct sk_buff *,struct tc_action *, int);
 #endif /* CONFIG_NET_CLS_ACT */
 
 extern int tcf_police(struct sk_buff *skb, struct tcf_police *p);
diff -Nru a/net/sched/act_api.c b/net/sched/act_api.c
--- a/net/sched/act_api.c	2005-03-30 03:00:38 +02:00
+++ b/net/sched/act_api.c	2005-03-30 03:00:38 +02:00
@@ -228,7 +228,7 @@
 		return err;
 
 	RTA_PUT(skb, TCA_KIND, IFNAMSIZ, a->ops->kind);
-	if (tcf_action_copy_stats(skb, a))
+	if (tcf_action_copy_stats(skb, a, 0))
 		goto rtattr_failure;
 	r = (struct rtattr*) skb->tail;
 	RTA_PUT(skb, TCA_OPTIONS, 0, NULL);
@@ -380,19 +380,24 @@
 	return NULL;
 }
 
-int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a)
+int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
+			  int compat_mode)
 {
-	int err;
+	int err = 0;
 	struct gnet_dump d;
 	struct tcf_act_hdr *h = a->priv;
 	
 	if (h == NULL)
 		goto errout;
 
-	if (a->type == TCA_OLD_COMPAT)
-		err = gnet_stats_start_copy_compat(skb, TCA_ACT_STATS,
-			TCA_STATS, TCA_XSTATS, h->stats_lock, &d);
-	else
+	/* compat_mode being true specifies a call that is supposed
+	 * to add additional backward compatiblity statistic TLVs.
+	 */
+	if (compat_mode) {
+		if (a->type == TCA_OLD_COMPAT)
+			err = gnet_stats_start_copy_compat(skb, 0,
+				TCA_STATS, TCA_XSTATS, h->stats_lock, &d);
+	} else
 		err = gnet_stats_start_copy(skb, TCA_ACT_STATS,
 			h->stats_lock, &d);
 
diff -Nru a/net/sched/cls_api.c b/net/sched/cls_api.c
--- a/net/sched/cls_api.c	2005-03-30 03:00:38 +02:00
+++ b/net/sched/cls_api.c	2005-03-30 03:00:38 +02:00
@@ -602,7 +602,7 @@
 {
 #ifdef CONFIG_NET_CLS_ACT
 	if (exts->action)
-		if (tcf_action_copy_stats(skb, exts->action) < 0)
+		if (tcf_action_copy_stats(skb, exts->action, 1) < 0)
 			goto rtattr_failure;
 #elif defined CONFIG_NET_CLS_POLICE
 	if (exts->police)

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[jamal]

On Tue, 2005-03-29 at 20:02, Thomas Graf wrote:

> So we always dump TCA_ACT_STATS when calling from tcf_exts_dump
> and add the backward compatibility TLVs in the second call
> comming from tcf_exts_dump_stats for actions that need it.
> 

Looks good from this side.

> Patches tested, works for me but might be worth for someone
> else to give it a try as well just to make sure. Meelis, could
> you give the 3 attched patches a try?
> 

Andy, can you just double check we dont have the problem you found
with stats?

cheers,
jamal

Re: cls_u32 compile failure in current 2.6.12-rc1+BK tree

[David S. Miller]

On Wed, 30 Mar 2005 03:02:09 +0200
Thomas Graf <tgraf@suug.ch> wrote:

> Dave, you can pull from bk://kernel.bkbits.net/tgraf/net-2.6-tcf_exts
> if everyone can agree on this.

Done, thanks Thomas.