[PATCH] pkt_cls.h incompatiables

[PATCH] pkt_cls.h incompatiables

[Stephen Hemminger]

The recent changes to (6 Jul 04) pkt_cls.h are evil, you can't build a version
of 'tc' to work unless you know the kernel config!

It has several API problems:
  - API data structures change on kernel config options
  - new fields should be added at the end of a structure to allow
    binary compatibility.
 
This patch tries to clean this up.

Signed-off-by: Stephen Hemminger <shemminger@osdl.org>
 
diff -Nru a/include/linux/pkt_cls.h b/include/linux/pkt_cls.h
--- a/include/linux/pkt_cls.h	2004-07-22 13:44:04 -07:00
+++ b/include/linux/pkt_cls.h	2004-07-22 13:44:04 -07:00
@@ -117,17 +117,8 @@
 struct tc_police
 {
 	__u32			index;
-#ifdef CONFIG_NET_CLS_ACT
 	int 			refcnt;
 	int 			bindcnt;
-#endif
-/* Turned off because it requires new tc
- * to work (for now maintain ABI)
- *
-#ifdef CONFIG_NET_CLS_ACT
-	__u32			capab;
-#endif
-*/
 	int			action;
 #define TC_POLICE_UNSPEC	TC_ACT_UNSPEC
 #define TC_POLICE_OK		TC_ACT_OK
@@ -195,12 +186,8 @@
 	TCA_U32_DIVISOR,
 	TCA_U32_SEL,
 	TCA_U32_POLICE,
-#ifdef CONFIG_NET_CLS_ACT
 	TCA_U32_ACT,   
-#endif
-#ifdef CONFIG_NET_CLS_IND
 	TCA_U32_INDEV,
-#endif
 	__TCA_U32_MAX
 };
 
@@ -212,9 +199,7 @@
 	__u32		val;
 	int		off;
 	int		offmask;
-#ifdef CONFIG_CLS_U32_PERF
-	unsigned long	kcnt;
-#endif
+	__u32		kcnt;
 };
 
 struct tc_u32_sel
@@ -229,11 +214,9 @@
 
 	short			hoff;
 	__u32			hmask;
-#ifdef CONFIG_CLS_U32_PERF
+	struct tc_u32_key	keys[0];
 	unsigned long		rcnt;
 	unsigned long		rhit;
-#endif
-	struct tc_u32_key	keys[0];
 };
 
 /* Flags */
@@ -300,12 +283,8 @@
 	TCA_FW_UNSPEC,
 	TCA_FW_CLASSID,
 	TCA_FW_POLICE,
-#ifdef CONFIG_NET_CLS_IND
 	TCA_FW_INDEV,
-#endif
-#ifdef CONFIG_NET_CLS_ACT
 	TCA_FW_ACT,
-#endif
 	__TCA_FW_MAX
 };
 

Re: [PATCH] pkt_cls.h incompatiables

[David S. Miller]

On Thu, 22 Jul 2004 13:45:22 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> The recent changes to (6 Jul 04) pkt_cls.h are evil, you can't build a version
> of 'tc' to work unless you know the kernel config!
> 
> It has several API problems:
>   - API data structures change on kernel config options
>   - new fields should be added at the end of a structure to allow
>     binary compatibility.
>  
> This patch tries to clean this up.

I totally agree and I've discussed this with Jamal already.
We just haven't gotten around to coding up the patch, but
now you have :-)

I'll apply this, thanks Stephen.

Re: [PATCH] pkt_cls.h incompatiables

[YOSHIFUJI Hideaki / 吉藤英明]

In article <20040722134522.4e7e0b07@dell_ss3.pdx.osdl.net> (at Thu, 22 Jul 2004 13:45:22 -0700), Stephen Hemminger <shemminger@osdl.org> says:

> The recent changes to (6 Jul 04) pkt_cls.h are evil, you can't build a version
> of 'tc' to work unless you know the kernel config!
> 
> It has several API problems:
>   - API data structures change on kernel config options
>   - new fields should be added at the end of a structure to allow
>     binary compatibility.
    - We cannot add new variable(s) after the last member which is 
      an variable-length array in effect.

> @@ -229,11 +214,9 @@
>  
>  	short			hoff;
>  	__u32			hmask;
> -#ifdef CONFIG_CLS_U32_PERF
> +	struct tc_u32_key	keys[0];
>  	unsigned long		rcnt;
>  	unsigned long		rhit;
> -#endif
> -	struct tc_u32_key	keys[0];
>  };
>  
>  /* Flags */

We cannot do this because keys holds key of variable length.

Solutions are, for example,
 to allocate new TCA_U32_xxx for rcnt and rhit,
or, 
 to rename TCA_U32_SEL to TCA_U32_OLS_SEL and allocate new value for 
 TCA_U32_SEL.

--yoshfuji

Re: [PATCH] pkt_cls.h incompatiables

[Jamal Hadi Salim]

On Thu, 2004-07-22 at 20:04, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <20040722134522.4e7e0b07@dell_ss3.pdx.osdl.net> (at Thu, 22 Jul 2004 13:45:22 -0700), Stephen Hemminger <shemminger@osdl.org> says:
> 
> > The recent changes to (6 Jul 04) pkt_cls.h are evil, you can't build a version
> > of 'tc' to work unless you know the kernel config!
> > 
> > It has several API problems:
> >   - API data structures change on kernel config options
> >   - new fields should be added at the end of a structure to allow
> >     binary compatibility.
>     - We cannot add new variable(s) after the last member which is 
>       an variable-length array in effect.

Agreed. 
The enum types are easy to handle - we just keep adding to the end which
doesnt break any ABI (as long as we keep the enum types in the same spot
even if they are no longer being used). 
For the data structures it does get tricky to augment as proposed above
because it does _break the ABI_.
In particular it gets tricky if someone is to debug why the policy they
are submitting is getting rejected. Maybe a sizeof check followed by a
debug printk would help. 
Clearly even a sizeof is problematic in this case when keys[] is
variable which brings me to a slightly different topic:
One little proposal is to start adding versioning to some of the
important fields. I have done this for the netlink header so i could
figure if tc was broken. Dave is against this idea. His opinion is now
we will get a lot of idjots mod-ing the structures and  then bumping the
version numbers. 
All the actions extensions actually have versioning fields.

Another related topic: tc SHOULD be able to probe configured kernel
options. It is a Good Thing to be able to do. Avoiding it as in this
patch is not the answer. I have some suggestions when i get time i will
speak with code. If someone wants this discussed we can continue on the
list.

> > @@ -229,11 +214,9 @@
> >  
> >  	short			hoff;
> >  	__u32			hmask;
> > -#ifdef CONFIG_CLS_U32_PERF
> > +	struct tc_u32_key	keys[0];
> >  	unsigned long		rcnt;
> >  	unsigned long		rhit;
> > -#endif
> > -	struct tc_u32_key	keys[0];
> >  };
> >  
> >  /* Flags */
> 
> We cannot do this because keys holds key of variable length.

Yes - please change this.

> Solutions are, for example,
>  to allocate new TCA_U32_xxx for rcnt and rhit,
> or, 
>  to rename TCA_U32_SEL to TCA_U32_OLS_SEL and allocate new value for 
>  TCA_U32_SEL.


we could do this, but since we are already fscking the ABI it is not
valuable.

cheeers.
jamal

Re: [PATCH] pkt_cls.h incompatiables

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1090593676.1128.25.camel@jzny.localdomain> (at 23 Jul 2004 10:41:16 -0400), Jamal Hadi Salim <hadi@znyx.com> says:

> > Solutions are, for example,
> >  to allocate new TCA_U32_xxx for rcnt and rhit,
> > or, 
> >  to rename TCA_U32_SEL to TCA_U32_OLS_SEL and allocate new value for 
> >  TCA_U32_SEL.
> 
> 
> we could do this, but since we are already fscking the ABI it is not
> valuable.

BTW, what's for rcnt etc.? I don't see the point. 

They're not (effectively) used in kernel.
I'd suggest to remove these things and to maintain the original ABI.

If rcnt etc. are for other purposes, such as statistics for userspace,
please allocate another structure / interface for it.

(And... cheking size is too strict;
we need to relax it to accept old binaries if we add something
at the tail of structure.)

Thanks.

--yoshfuji

Re: [PATCH] pkt_cls.h incompatiables

[Stephen Hemminger]

On Fri, 23 Jul 2004 11:00:07 -0400 (EDT)
YOSHIFUJI Hideaki / ____________ <yoshfuji@linux-ipv6.org> wrote:

> In article <1090593676.1128.25.camel@jzny.localdomain> (at 23 Jul 2004 10:41:16 -0400), Jamal Hadi Salim <hadi@znyx.com> says:
> 
> > > Solutions are, for example,
> > >  to allocate new TCA_U32_xxx for rcnt and rhit,
> > > or, 
> > >  to rename TCA_U32_SEL to TCA_U32_OLS_SEL and allocate new value for 
> > >  TCA_U32_SEL.
> > 
> > 
> > we could do this, but since we are already fscking the ABI it is not
> > valuable.
> 
> BTW, what's for rcnt etc.? I don't see the point. 
> 
> They're not (effectively) used in kernel.
> I'd suggest to remove these things and to maintain the original ABI.
> 
> If rcnt etc. are for other purposes, such as statistics for userspace,
> please allocate another structure / interface for it.
> 
> (And... cheking size is too strict;
> we need to relax it to accept old binaries if we add something
> at the tail of structure.)

Looking at the netlink style, wouldn't it make sense to add additional
separate payloads for the new features.  This keeps the API the same and
the kernel can easily adapt for new/old values.

Re: [PATCH] pkt_cls.h incompatiables

[Jamal Hadi Salim]

On Fri, 2004-07-23 at 11:00, YOSHIFUJI Hideaki / 吉藤英明 wrote:

> 
> BTW, what's for rcnt etc.? I don't see the point. 
> 
> They're not (effectively) used in kernel.

It is transported to user space for analysis of how to reorganize
hash table optimally.

> I'd suggest to remove these things and to maintain the original ABI.
> 
> If rcnt etc. are for other purposes, such as statistics for userspace,
> please allocate another structure / interface for it.

I could send a separate TLV, i dont think this will resolve the problem
of ABI breakage but it will be more of a cleans. Part of the counters i need for analysis is in the
key structure.

> (And... cheking size is too strict;
> we need to relax it to accept old binaries if we add something
> at the tail of structure.)

A lot of size checks for every structure at both kernel/userspace is
needed. Not much is done today as is - which means old binaries may
break when modified structures are transported to user space.
I think one of the main challenges with the u32 structures is the keys[]
table. In the future if we could make that a TLV it would help. 
We need not just be backward compatible but also forward compatible.

I will do some testing when i get back next week to see if current patch
breaks anything.

cheers,
jamal

Re: [PATCH] pkt_cls.h incompatiables

[Jamal Hadi Salim]

On Fri, 2004-07-23 at 12:21, Stephen Hemminger wrote:

> Looking at the netlink style, wouldn't it make sense to add additional
> separate payloads for the new features.  This keeps the API the same and
> the kernel can easily adapt for new/old values.

If things were perfect this is the way to go. Maybe in 2.7 we can start
scrubbing some of this issues.

cheers,
jamal

Re: [PATCH] pkt_cls.h incompatiables

[David S. Miller]

The current 2.6.x tree is obviously busted.

Can someone send me a patch soon'ish that does something
about it, so that 2.6.8 doesn't go out with things
like this?

Thanks.

Re: [PATCH] pkt_cls.h incompatiables

[Jamal Hadi Salim]

I will take care of it next week when i get back. 
If 2.6.8 is coming out before end of next week, i would suggest undoing
Stephens patch.
The way it was before was backward compatible as long as you dont
turn on those features; and those features all have a strong warning not
to turn them on unless you a new iproute2;-> And if you turn them on and
used my patches to tc all will work just fine ...

cheers,
jamal

On Fri, 2004-07-23 at 16:59, David S. Miller wrote:
> The current 2.6.x tree is obviously busted.
> 
> Can someone send me a patch soon'ish that does something
> about it, so that 2.6.8 doesn't go et with things
> like this?
> 
> Thanks.

Re: [PATCH] pkt_cls.h incompatiables

[YOSHIFUJI Hideaki / 吉藤英明]

In article <1090630154.1134.6.camel@jzny.localdomain> (at 23 Jul 2004 20:49:15 -0400), Jamal Hadi Salim <hadi@znyx.com> says:

> I will take care of it next week when i get back. 
> If 2.6.8 is coming out before end of next week, i would suggest undoing
> Stephens patch.

I've talked about this with Jamal.

I believe it is very important to maintain th original (or 
traditional) ABI, which has been used in -2.6.7(?).

So, I'd rather say, let's remove (or disable) whole thing 
until we find the way to avoid breaking ABIs.

--yoshfuji

Re: [PATCH] pkt_cls.h incompatiables

[Jamal Hadi Salim]

On Fri, 2004-07-23 at 21:42, YOSHIFUJI Hideaki / 吉藤英明 wrote:
> In article <1090630154.1134.6.camel@jzny.localdomain> (at 23 Jul 2004 20:49:15 -0400), Jamal Hadi Salim <hadi@znyx.com> says:
> 
> > I will take care of it next week when i get back. 
> > If 2.6.8 is coming out before end of next week, i would suggest undoing
> > Stephens patch.
> 
> I've talked about this with Jamal.
> 
> I believe it is very important to maintain th original (or 
> traditional) ABI, which has been used in -2.6.7(?).
> 
> So, I'd rather say, let's remove (or disable) whole thing 
> until we find the way to avoid breaking ABIs.

Just back out Stephens patch for now until i get back.

cheers,
jamal

Re: [PATCH] pkt_cls.h incompatiables

[jamal]

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


alright heres a patch against the old code(before Stevens patch).
Makes the performance counters a separate TLV -  a little bit of a pain,
but acceptable given its optional.
Compiles but hasnt been tested; user space code will have to change.
I dont want to touch the policer for now.
Dont apply yet, i need to run my regression tests when i get back.
In the minimal back out Stephens change.

cheers,
jamal


[-- Attachment #2: ols1 --]
[-- Type: text/plain, Size: 3513 bytes --]

--- a/include/linux/pkt_cls.h	2004/07/24 12:35:40	1.1
+++ b/include/linux/pkt_cls.h	2004/07/24 12:57:20
@@ -117,17 +117,6 @@
 struct tc_police
 {
 	__u32			index;
-#ifdef CONFIG_NET_CLS_ACT
-	int 			refcnt;
-	int 			bindcnt;
-#endif
-/* Turned off because it requires new tc
- * to work (for now maintain ABI)
- *
-#ifdef CONFIG_NET_CLS_ACT
-	__u32			capab;
-#endif
-*/
 	int			action;
 #define TC_POLICE_UNSPEC	TC_ACT_UNSPEC
 #define TC_POLICE_OK		TC_ACT_OK
@@ -140,6 +129,17 @@
 	__u32			mtu;
 	struct tc_ratespec	rate;
 	struct tc_ratespec	peakrate;
+#ifdef CONFIG_NET_CLS_ACT
+	int 			refcnt;
+	int 			bindcnt;
+#endif
+/* Turned off because it requires new tc
+ * to work (for now maintain ABI)
+ *
+#ifdef CONFIG_NET_CLS_ACT
+	__u32			capab;
+#endif
+*/
 };
 
 struct tcf_t
@@ -195,12 +195,9 @@
 	TCA_U32_DIVISOR,
 	TCA_U32_SEL,
 	TCA_U32_POLICE,
-#ifdef CONFIG_NET_CLS_ACT
 	TCA_U32_ACT,   
-#endif
-#ifdef CONFIG_NET_CLS_IND
 	TCA_U32_INDEV,
-#endif
+	TCA_U32_PCNT,
 	__TCA_U32_MAX
 };
 
@@ -212,9 +209,6 @@
 	__u32		val;
 	int		off;
 	int		offmask;
-#ifdef CONFIG_CLS_U32_PERF
-	unsigned long	kcnt;
-#endif
 };
 
 struct tc_u32_sel
@@ -229,13 +223,17 @@
 
 	short			hoff;
 	__u32			hmask;
+	struct tc_u32_key	keys[0];
+};
+
 #ifdef CONFIG_CLS_U32_PERF
+struct tc_u32_pcnt
+{
 	unsigned long		rcnt;
 	unsigned long		rhit;
-#endif
-	struct tc_u32_key	keys[0];
+	unsigned long		kcnts[];
 };
-
+#endif
 /* Flags */
 
 #define TC_U32_TERMINAL		1
@@ -300,12 +298,8 @@
 	TCA_FW_UNSPEC,
 	TCA_FW_CLASSID,
 	TCA_FW_POLICE,
-#ifdef CONFIG_NET_CLS_IND
-	TCA_FW_INDEV,
-#endif
-#ifdef CONFIG_NET_CLS_ACT
-	TCA_FW_ACT,
-#endif
+	TCA_FW_INDEV, /*  used by CONFIG_NET_CLS_IND */
+	TCA_FW_ACT, /* used by CONFIG_NET_CLS_ACT */
 	__TCA_FW_MAX
 };
 
--- a/net/sched/cls_u32.c	2004/07/24 12:38:52	1.1
+++ b/net/sched/cls_u32.c	2004/07/24 13:22:29
@@ -76,6 +76,9 @@
 	struct tcf_result	res;
 	struct tc_u_hnode	*ht_down;
 	struct tc_u32_sel	sel;
+#ifdef CONFIG_CLS_U32_PERF
+	struct tc_u32_pcnt	pf;
+#endif
 };
 
 struct tc_u_hnode
@@ -120,6 +123,9 @@
 	int sdepth = 0;
 	int off2 = 0;
 	int sel = 0;
+#ifdef CONFIG_CLS_U32_PERF
+	int j;
+#endif
 	int i;
 
 next_ht:
@@ -130,7 +136,8 @@
 		struct tc_u32_key *key = n->sel.keys;
 
 #ifdef CONFIG_CLS_U32_PERF
-		n->sel.rcnt +=1;
+		n->pf.rcnt +=1;
+		j = 0;
 #endif
 		for (i = n->sel.nkeys; i>0; i--, key++) {
 
@@ -139,7 +146,8 @@
 				goto next_knode;
 			}
 #ifdef CONFIG_CLS_U32_PERF
-			key->kcnt +=1;
+			n->pf.kcnts[j] +=1;
+			j++;
 #endif
 		}
 		if (n->ht_down == NULL) {
@@ -164,7 +172,7 @@
 				}
 #endif
 #ifdef CONFIG_CLS_U32_PERF
-		n->sel.rhit +=1;
+		n->pf.rhit +=1;
 #endif
 				if (n->action) {
 					int pol_res = tcf_action_exec(skb, n->action);
@@ -682,16 +690,10 @@
 
 	s = RTA_DATA(tb[TCA_U32_SEL-1]);
 
-#ifdef CONFIG_CLS_U32_PERF
-	if (RTA_PAYLOAD(tb[TCA_U32_SEL-1]) < 
-		(s->nkeys*sizeof(struct tc_u32_key)) + sizeof(struct tc_u32_sel)) {
-			printk("Please upgrade your iproute2 tools or compile proper options in!\n");
-			return -EINVAL;
-}
-#endif
 	n = kmalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
 	if (n == NULL)
 		return -ENOBUFS;
+
 	memset(n, 0, sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key));
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
 	n->ht_up = ht;
@@ -851,6 +853,11 @@
 	}
 #endif
 #endif
+#ifdef CONFIG_CLS_U32_PERF
+	RTA_PUT(skb, TCA_U32_PCNT, 
+		sizeof(struct tc_u32_pcnt) + n->sel.nkeys*sizeof(unsigned long),
+			&n->pf);
+#endif
 	return skb->len;
 
 rtattr_failure:

Re: [PATCH] pkt_cls.h incompatiables

[YOSHIFUJI Hideaki / 吉藤英明]

Hello.

In article <1090675993.1161.11.camel@jzny.localdomain> (at 24 Jul 2004 09:33:52 -0400), jamal <hadi@cyberus.ca> says:

> Makes the performance counters a separate TLV -  a little bit of a pain,
> but acceptable given its optional.
> Compiles but hasnt been tested; user space code will have to change.

basically, looks fine.

1. is it okay to use unsigned long?
   I'd rather prefer __u32 or __u64.

2. please use kcnts[0] instead of kcnts[] because
   user probably want to do sizeof(struct tc_u32_pcnt)

> I dont want to touch the policer for now.

hmm. :-P

Thanks.

-- 
Hideaki YOSHIFUJI @ USAGI Project <yoshfuji@linux-ipv6.org>
GPG FP: 9022 65EB 1ECF 3AD1 0BDF  80D8 4807 F894 E062 0EEA

Re: [PATCH] pkt_cls.h incompatiables

[David S. Miller]

On 24 Jul 2004 09:33:52 -0400
jamal <hadi@cyberus.ca> wrote:

> Dont apply yet, i need to run my regression tests when i get back.
> In the minimal back out Stephens change.

Ok, I'll keep this in my inbox until you give the word.
Thanks.

Re: [PATCH] pkt_cls.h incompatiables

[David S. Miller]

On Sat, 24 Jul 2004 21:33:23 -0400 (EDT)
YOSHIFUJI Hideaki / ^[$B5HF#1QL@^[(B <yoshfuji@linux-ipv6.org> wrote:

> 1. is it okay to use unsigned long?
>    I'd rather prefer __u32 or __u64.

Me too, for public APIs use strictly sized types
please.

Re: [PATCH] pkt_cls.h incompatiables

[jamal]

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

On Sun, 2004-07-25 at 01:28, David S. Miller wrote:

> Ok, I'll keep this in my inbox until you give the word.

[Still trying to recover after all those conferences].

Heres a patch that passes all my regression tests. I use a 
__u64 instead of u64 for perf counters since user space understands it.

cheers,
jamal

PS:- at some point we need to scrub the general backward compatibility
issues going forward. 

[-- Attachment #2: p2 --]
[-- Type: text/plain, Size: 7045 bytes --]

--- a/267-bk20-act+dummy/net/sched/cls_u32.c	2004/07/24 12:38:52	1.1
+++ b/267-bk20-act+dummy/net/sched/cls_u32.c	2004/07/28 18:37:06
@@ -64,17 +64,20 @@
 	struct tc_u_hnode	*ht_up;
 #ifdef CONFIG_NET_CLS_ACT
 	struct tc_action	*action;
-#ifdef CONFIG_NET_CLS_IND
-	char                     indev[IFNAMSIZ];
-#endif
 #else
 #ifdef CONFIG_NET_CLS_POLICE
 	struct tcf_police	*police;
 #endif
 #endif
+#ifdef CONFIG_NET_CLS_IND
+	char                     indev[IFNAMSIZ];
+#endif
 	u8			fshift;
 	struct tcf_result	res;
 	struct tc_u_hnode	*ht_down;
+#ifdef CONFIG_CLS_U32_PERF
+	struct tc_u32_pcnt	*pf;
+#endif
 	struct tc_u32_sel	sel;
 };
 
@@ -120,6 +123,9 @@
 	int sdepth = 0;
 	int off2 = 0;
 	int sel = 0;
+#ifdef CONFIG_CLS_U32_PERF
+	int j;
+#endif
 	int i;
 
 next_ht:
@@ -130,7 +136,8 @@
 		struct tc_u32_key *key = n->sel.keys;
 
 #ifdef CONFIG_CLS_U32_PERF
-		n->sel.rcnt +=1;
+		n->pf->rcnt +=1;
+		j = 0;
 #endif
 		for (i = n->sel.nkeys; i>0; i--, key++) {
 
@@ -139,7 +146,8 @@
 				goto next_knode;
 			}
 #ifdef CONFIG_CLS_U32_PERF
-			key->kcnt +=1;
+			n->pf->kcnts[j] +=1;
+			j++;
 #endif
 		}
 		if (n->ht_down == NULL) {
@@ -147,7 +155,6 @@
 			if (n->sel.flags&TC_U32_TERMINAL) {
 
 				*res = n->res;
-#ifdef CONFIG_NET_CLS_ACT
 #ifdef CONFIG_NET_CLS_IND
 				/* yes, i know it sucks but the feature is 
 				** optional dammit! - JHS */
@@ -164,8 +171,9 @@
 				}
 #endif
 #ifdef CONFIG_CLS_U32_PERF
-		n->sel.rhit +=1;
+				n->pf->rhit +=1;
 #endif
+#ifdef CONFIG_NET_CLS_ACT
 				if (n->action) {
 					int pol_res = tcf_action_exec(skb, n->action);
 					if (skb->tc_classid > 0) {
@@ -358,6 +366,10 @@
 #endif
 	if (n->ht_down)
 		n->ht_down->refcnt--;
+#ifdef CONFIG_CLS_U32_PERF
+	if (n && (NULL != n->pf))
+		kfree(n->pf);
+#endif
 	kfree(n);
 	return 0;
 }
@@ -571,18 +583,6 @@
 		tcf_action_destroy(act, TCA_ACT_UNBIND);
 	}
 
-#ifdef CONFIG_NET_CLS_IND
-	n->indev[0] = 0;
-	if(tb[TCA_U32_INDEV-1]) {
-		struct rtattr *input_dev = tb[TCA_U32_INDEV-1];
-		if (RTA_PAYLOAD(input_dev) >= IFNAMSIZ) {
-			printk("cls_u32: bad indev name %s\n",(char*)RTA_DATA(input_dev));
-			/* should we clear state first? */
-			return  -EINVAL;
-		}
-		sprintf(n->indev, "%s", (char*)RTA_DATA(input_dev));
-	}
-#endif
 
 #else
 #ifdef CONFIG_NET_CLS_POLICE
@@ -595,6 +595,19 @@
 	}
 #endif
 #endif
+#ifdef CONFIG_NET_CLS_IND
+	n->indev[0] = 0;
+	if(tb[TCA_U32_INDEV-1]) {
+		struct rtattr *input_dev = tb[TCA_U32_INDEV-1];
+		if (RTA_PAYLOAD(input_dev) >= IFNAMSIZ) {
+			printk("cls_u32: bad indev name %s\n",(char*)RTA_DATA(input_dev));
+			/* should we clear state first? */
+			return  -EINVAL;
+		}
+		sprintf(n->indev, "%s", (char*)RTA_DATA(input_dev));
+		printk("got IND %s\n",n->indev);
+	}
+#endif
 
 	return 0;
 }
@@ -682,17 +695,20 @@
 
 	s = RTA_DATA(tb[TCA_U32_SEL-1]);
 
-#ifdef CONFIG_CLS_U32_PERF
-	if (RTA_PAYLOAD(tb[TCA_U32_SEL-1]) < 
-		(s->nkeys*sizeof(struct tc_u32_key)) + sizeof(struct tc_u32_sel)) {
-			printk("Please upgrade your iproute2 tools or compile proper options in!\n");
-			return -EINVAL;
-}
-#endif
 	n = kmalloc(sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key), GFP_KERNEL);
 	if (n == NULL)
 		return -ENOBUFS;
+
 	memset(n, 0, sizeof(*n) + s->nkeys*sizeof(struct tc_u32_key));
+#ifdef CONFIG_CLS_U32_PERF
+	n->pf = kmalloc(sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(__u64), GFP_KERNEL);
+	if (n->pf == NULL) {
+		kfree(n);
+		return -ENOBUFS;
+	}
+	memset(n->pf, 0, sizeof(struct tc_u32_pcnt) + s->nkeys*sizeof(__u64));
+#endif
+
 	memcpy(&n->sel, s, sizeof(*s) + s->nkeys*sizeof(struct tc_u32_key));
 	n->ht_up = ht;
 	n->handle = handle;
@@ -721,6 +737,10 @@
 		*arg = (unsigned long)n;
 		return 0;
 	}
+#ifdef CONFIG_CLS_U32_PERF
+	if (n && (NULL != n->pf))
+		kfree(n->pf);
+#endif
 	kfree(n);
 	return err;
 }
@@ -812,13 +832,6 @@
 
 			p_rta->rta_len = skb->tail - (u8*)p_rta;
 		}
-#ifdef CONFIG_NET_CLS_IND
-		if(strlen(n->indev)) {
-			struct rtattr * p_rta = (struct rtattr*)skb->tail;
-			RTA_PUT(skb, TCA_U32_INDEV, IFNAMSIZ, n->indev);
-			p_rta->rta_len = skb->tail - (u8*)p_rta;
-		}
-#endif
 
 #else
 #ifdef CONFIG_NET_CLS_POLICE
@@ -834,13 +847,28 @@
 		}
 #endif
 #endif
+
+#ifdef CONFIG_NET_CLS_IND
+		if(strlen(n->indev)) {
+			struct rtattr * p_rta = (struct rtattr*)skb->tail;
+			RTA_PUT(skb, TCA_U32_INDEV, IFNAMSIZ, n->indev);
+			p_rta->rta_len = skb->tail - (u8*)p_rta;
+		}
+#endif
+#ifdef CONFIG_CLS_U32_PERF
+		RTA_PUT(skb, TCA_U32_PCNT, 
+		sizeof(struct tc_u32_pcnt) + n->sel.nkeys*sizeof(__u64),
+			n->pf);
+#endif
 	}
 
 	rta->rta_len = skb->tail - b;
 #ifdef CONFIG_NET_CLS_ACT
-	if (TC_U32_KEY(n->handle) && n->action && n->action->type == TCA_OLD_COMPAT) {
-		if (tcf_action_copy_stats(skb,n->action))
-			goto rtattr_failure;
+	if (TC_U32_KEY(n->handle) != 0) {
+		if (TC_U32_KEY(n->handle) && n->action && n->action->type == TCA_OLD_COMPAT) {
+			if (tcf_action_copy_stats(skb,n->action))
+				goto rtattr_failure;
+		}
 	}
 #else
 #ifdef CONFIG_NET_CLS_POLICE
@@ -875,6 +903,19 @@
 
 static int __init init_u32(void)
 {
+	printk("u32 classifier\n");
+#ifdef CONFIG_CLS_U32_PERF
+	printk("    Perfomance counters on\n");
+#endif
+#ifdef CONFIG_NET_CLS_POLICE
+	printk("    OLD policer on \n");
+#endif
+#ifdef CONFIG_NET_CLS_IND
+	printk("    input device check on \n");
+#endif
+#ifdef CONFIG_NET_CLS_ACT
+	printk("    Actions configured \n");
+#endif
 	return register_tcf_proto_ops(&cls_u32_ops);
 }
 
--- a/267-bk20-act+dummy/include/linux/pkt_cls.h	2004/07/24 12:35:40	1.1
+++ b/267-bk20-act+dummy/include/linux/pkt_cls.h	2004/07/28 18:38:01
@@ -117,17 +117,6 @@
 struct tc_police
 {
 	__u32			index;
-#ifdef CONFIG_NET_CLS_ACT
-	int 			refcnt;
-	int 			bindcnt;
-#endif
-/* Turned off because it requires new tc
- * to work (for now maintain ABI)
- *
-#ifdef CONFIG_NET_CLS_ACT
-	__u32			capab;
-#endif
-*/
 	int			action;
 #define TC_POLICE_UNSPEC	TC_ACT_UNSPEC
 #define TC_POLICE_OK		TC_ACT_OK
@@ -140,6 +129,9 @@
 	__u32			mtu;
 	struct tc_ratespec	rate;
 	struct tc_ratespec	peakrate;
+	int 			refcnt;
+	int 			bindcnt;
+	__u32			capab;
 };
 
 struct tcf_t
@@ -195,12 +187,9 @@
 	TCA_U32_DIVISOR,
 	TCA_U32_SEL,
 	TCA_U32_POLICE,
-#ifdef CONFIG_NET_CLS_ACT
 	TCA_U32_ACT,   
-#endif
-#ifdef CONFIG_NET_CLS_IND
 	TCA_U32_INDEV,
-#endif
+	TCA_U32_PCNT,
 	__TCA_U32_MAX
 };
 
@@ -212,9 +201,6 @@
 	__u32		val;
 	int		off;
 	int		offmask;
-#ifdef CONFIG_CLS_U32_PERF
-	unsigned long	kcnt;
-#endif
 };
 
 struct tc_u32_sel
@@ -229,13 +215,17 @@
 
 	short			hoff;
 	__u32			hmask;
-#ifdef CONFIG_CLS_U32_PERF
-	unsigned long		rcnt;
-	unsigned long		rhit;
-#endif
 	struct tc_u32_key	keys[0];
 };
 
+#ifdef CONFIG_CLS_U32_PERF
+struct tc_u32_pcnt
+{
+	__u64 rcnt;
+	__u64 rhit;
+	__u64 kcnts[0];
+};
+#endif
 /* Flags */
 
 #define TC_U32_TERMINAL		1
@@ -300,12 +290,8 @@
 	TCA_FW_UNSPEC,
 	TCA_FW_CLASSID,
 	TCA_FW_POLICE,
-#ifdef CONFIG_NET_CLS_IND
-	TCA_FW_INDEV,
-#endif
-#ifdef CONFIG_NET_CLS_ACT
-	TCA_FW_ACT,
-#endif
+	TCA_FW_INDEV, /*  used by CONFIG_NET_CLS_IND */
+	TCA_FW_ACT, /* used by CONFIG_NET_CLS_ACT */
 	__TCA_FW_MAX
 };
 

Re: [PATCH] pkt_cls.h incompatiables

[jamal]

Patch was against 2.6.8-rc2 not bk20.

cheers,
jamal

On Thu, 2004-07-29 at 07:22, jamal wrote:
> On Sun, 2004-07-25 at 01:28, David S. Miller wrote:
> 
> > Ok, I'll keep this in my inbox until you give the word.
> 
> [Still trying to recover after all those conferences].
> 
> Heres a patch that passes all my regression tests. I use a 
> __u64 instead of u64 for perf counters since user space understands it.
> 
> cheers,
> jamal
> 
> PS:- at some point we need to scrub the general backward compatibility
> issues going forward. 

Re: [PATCH] pkt_cls.h incompatiables

[David S. Miller]

On 29 Jul 2004 07:30:02 -0400
jamal <hadi@cyberus.ca> wrote:

> Patch was against 2.6.8-rc2 not bk20.

Applied, thanks Jamal.