RFC/PATCH capture qdisc requeue event in stats

RFC/PATCH capture qdisc requeue event in stats

[jamal]

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



The requeue event is useful in finding out when a device is overloaded
on the egress (bus or bandwidth).
Atached patch introduces this. I would have used the overlimit bits
but at the moment thats being used for different semantical reasons.
I have not done extensive testing on it. 

Opinions welcome - If all is good, Dave please apply.

cheers,
jamal

[-- Attachment #2: reqs-patch --]
[-- Type: text/plain, Size: 4106 bytes --]

--- a/net/sched/sch_atm.c	2004/08/29 16:43:53	1.1
+++ b/net/sched/sch_atm.c	2004/08/29 16:58:26
@@ -545,8 +545,10 @@
 
 	D2PRINTK("atm_tc_requeue(skb %p,sch %p,[qdisc %p])\n",skb,sch,p);
 	ret = p->link.q->ops->requeue(skb,p->link.q);
-	if (!ret) sch->q.qlen++;
-	else {
+	if (!ret) {
+	       	sch->q.qlen++;
+	       	sch->stats.reqs++;
+	} else {
 		sch->stats.drops++;
 		p->link.stats.drops++;
 	}
--- a/net/sched/sch_cbq.c	2004/08/29 16:40:52	1.1
+++ b/net/sched/sch_cbq.c	2004/08/29 16:56:58
@@ -485,6 +485,7 @@
 #endif
 	if ((ret = cl->q->ops->requeue(skb, cl->q)) == 0) {
 		sch->q.qlen++;
+		sch->stats.reqs++;
 		if (!cl->next_alive)
 			cbq_activate_class(cl);
 		return 0;
--- a/net/sched/sch_dsmark.c	2004/08/29 16:41:51	1.1
+++ b/net/sched/sch_dsmark.c	2004/08/29 17:01:27
@@ -297,6 +297,7 @@
 	D2PRINTK("dsmark_requeue(skb %p,sch %p,[qdisc %p])\n",skb,sch,p);
         if ((ret = p->q->ops->requeue(skb, p->q)) == 0) {
 		sch->q.qlen++;
+		sch->stats.reqs++;
 		return 0;
 	}
 	sch->stats.drops++;
--- a/net/sched/sch_fifo.c	2004/08/29 16:42:52	1.1
+++ b/net/sched/sch_fifo.c	2004/08/29 16:52:47
@@ -67,6 +67,7 @@
 {
 	__skb_queue_head(&sch->q, skb);
 	sch->stats.backlog += skb->len;
+	sch->stats.reqs++;
 	return 0;
 }
 
@@ -126,6 +127,7 @@
 pfifo_requeue(struct sk_buff *skb, struct Qdisc* sch)
 {
 	__skb_queue_head(&sch->q, skb);
+	sch->stats.reqs++;
 	return 0;
 }
 
--- a/net/sched/sch_generic.c	2004/08/29 16:39:47	1.1
+++ b/net/sched/sch_generic.c	2004/08/29 16:56:44
@@ -327,6 +327,7 @@
 
 	__skb_queue_head(list, skb);
 	qdisc->q.qlen++;
+	qdisc->stats.reqs++;
 	return 0;
 }
 
--- a/net/sched/sch_gred.c	2004/08/29 16:44:44	1.1
+++ b/net/sched/sch_gred.c	2004/08/29 16:50:07
@@ -222,6 +222,7 @@
 
 	__skb_queue_head(&sch->q, skb);
 	sch->stats.backlog += skb->len;
+	sch->stats.reqs++;
 	q->backlog += skb->len;
 	return 0;
 }
--- a/net/sched/sch_hfsc.c	2004/08/29 16:41:19	1.1
+++ b/net/sched/sch_hfsc.c	2004/08/29 16:57:11
@@ -1803,6 +1803,7 @@
 
 	__skb_queue_head(&q->requeue, skb);
 	sch->q.qlen++;
+	sch->stats.reqs++;
 	return NET_XMIT_SUCCESS;
 }
 
--- a/net/sched/sch_htb.c	2004/08/29 16:44:17	1.1
+++ b/net/sched/sch_htb.c	2004/08/29 17:00:24
@@ -794,6 +794,7 @@
 	    htb_activate (q,cl);
 
     sch->q.qlen++;
+    sch->stats.reqs++;
     HTB_DBG(1,1,"htb_req_ok cl=%X skb=%p\n",(cl && cl != HTB_DIRECT)?cl->classid:0,skb);
     return NET_XMIT_SUCCESS;
 }
--- a/net/sched/sch_netem.c	2004/08/29 16:46:10	1.1
+++ b/net/sched/sch_netem.c	2004/08/29 16:48:44
@@ -662,8 +662,10 @@
 	struct netem_sched_data *q = qdisc_priv(sch);
 	int ret;
 
-	if ((ret = q->qdisc->ops->requeue(skb, q->qdisc)) == 0)
+	if ((ret = q->qdisc->ops->requeue(skb, q->qdisc)) == 0) {
 		sch->q.qlen++;
+		sch->q.reqs++;
+	}
 
 	return ret;
 }
--- a/net/sched/sch_prio.c	2004/08/29 16:43:21	1.1
+++ b/net/sched/sch_prio.c	2004/08/29 16:57:52
@@ -139,6 +139,7 @@
 
 	if ((ret = qdisc->ops->requeue(skb, qdisc)) == 0) {
 		sch->q.qlen++;
+		sch->stats.reqs++;
 		return 0;
 	}
 dropped:
--- a/net/sched/sch_red.c	2004/08/29 16:46:46	1.1
+++ b/net/sched/sch_red.c	2004/08/29 16:49:34
@@ -309,6 +309,8 @@
 
 	__skb_queue_head(&sch->q, skb);
 	sch->stats.backlog += skb->len;
+	sch->stats.reqs++;
+
 	return 0;
 }
 
--- a/net/sched/sch_tbf.c	2004/08/29 16:40:15	1.1
+++ b/net/sched/sch_tbf.c	2004/08/29 16:56:24
@@ -166,8 +166,10 @@
 	struct tbf_sched_data *q = qdisc_priv(sch);
 	int ret;
 
-	if ((ret = q->qdisc->ops->requeue(skb, q->qdisc)) == 0)
+	if ((ret = q->qdisc->ops->requeue(skb, q->qdisc)) == 0) {
 		sch->q.qlen++;
+		sch->stats.reqs++;
+	}
 
 	return ret;
 }
--- a/net/sched/sch_teql.c	2004/08/29 16:42:23	1.1
+++ b/net/sched/sch_teql.c	2004/08/29 16:53:13
@@ -112,6 +112,7 @@
 	struct teql_sched_data *q = qdisc_priv(sch);
 
 	__skb_queue_head(&q->q, skb);
+	sch->stats.reqs++;
 	return 0;
 }
 
--- a/include/linux/pkt_sched.h	2004-08-29 13:07:26.000000000 -0400
+++ b/include/linux/pkt_sched.h	2004-08-29 12:36:29.000000000 -0400
@@ -38,6 +38,7 @@
 	__u32	pps;			/* Current flow packet rate */
 	__u32	qlen;
 	__u32	backlog;
+	__u32	reqs;			/* requeues */
 };
 
 struct tc_estimator

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On 29 Aug 2004 13:13:52 -0400
jamal <hadi@cyberus.ca> wrote:

> Opinions welcome - If all is good, Dave please apply.

tc_stats changes size, how will existing applications cope?
Since it is just RTA_PUT() into userspace, will that just work
or will existing apps bomb because they'll do a check of the
size field of the rtnetlink attribute?

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

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

On Mon, 2004-08-30 at 17:40, David S. Miller wrote:
> On 29 Aug 2004 13:13:52 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > Opinions welcome - If all is good, Dave please apply.
> 
> tc_stats changes size, how will existing applications cope?
> Since it is just RTA_PUT() into userspace, will that just work
> or will existing apps bomb because they'll do a check of the
> size field of the rtnetlink attribute?

To look at an existing app such as tc; attached is a patch.
In the tc case. The possibilities are:

1) old kernels + old tc --> should work 
2) new kernels + old tc --> should work
3) new kernels + new tc --> should work
4) old kernel +  new tc --> will bomb (sizeof check will fail)

Cant think of something smart to do with option #4. I have contemplated
playing with offsetoff() etc. The best thing i can think of right
now is to complain about it as in that fprintf()

Thoughts?

cheers,
jamal


[-- Attachment #2: reqs-patch-iproute2-2.6.8 --]
[-- Type: text/plain, Size: 1466 bytes --]

--- iproute2-2.6.8/include/linux/pkt_sched.h	2004-08-23 16:22:16.000000000 -0400
+++ iproute2-2.6.8-mod/include/linux/pkt_sched.h	2004-08-27 10:18:32.000000000 -0400
@@ -38,6 +38,7 @@
 	__u32	pps;			/* Current flow packet rate */
 	__u32	qlen;
 	__u32	backlog;
+	__u32	reqs;
 };
 
 struct tc_estimator
--- iproute2-2.6.8/tc/tc_qdisc.c	2004-08-23 16:22:16.000000000 -0400
+++ iproute2-2.6.8-mod/tc/tc_qdisc.c	2004-08-29 13:20:25.000000000 -0400
@@ -171,8 +171,8 @@
 {
 	SPRINT_BUF(b1);
 
-	fprintf(fp, " Sent %llu bytes %u pkts (dropped %u, overlimits %u ) ",
-		(unsigned long long)st->bytes, st->packets, st->drops, st->overlimits);
+	fprintf(fp, " Sent %llu bytes %u pkts (dropped %u, overlimits %u requeus%u ) ",
+		(unsigned long long)st->bytes, st->packets, st->drops, st->overlimits, st->reqs);
 	if (st->bps || st->pps || st->qlen || st->backlog) {
 		fprintf(fp, "\n ");
 		if (st->bps || st->pps) {
@@ -255,8 +255,13 @@
 	fprintf(fp, "\n");
 	if (show_stats) {
 		if (tb[TCA_STATS]) {
+			/* FIXME: need to work with old kernels.
+			 * At the moment this just catches them
+			*/
 			if (RTA_PAYLOAD(tb[TCA_STATS]) < sizeof(struct tc_stats))
-				fprintf(fp, "statistics truncated");
+				fprintf(fp, "statistics truncated ");
+				fprintf (fp, "expected %d bytes got %d\n",sizeof(struct tc_stats),RTA_PAYLOAD(tb[TCA_STATS]));
+				fprintf("Maybe an old kernel?");
 			else {
 				struct tc_stats st;
 				memcpy(&st, RTA_DATA(tb[TCA_STATS]), sizeof(st));

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On 30 Aug 2004 18:14:48 -0400
jamal <hadi@cyberus.ca> wrote:

> 4) old kernel +  new tc --> will bomb (sizeof check will fail)
> 
> Cant think of something smart to do with option #4. I have contemplated
> playing with offsetoff() etc. The best thing i can think of right
> now is to complain about it as in that fprintf()
> 
> Thoughts?

Check the size, if it matches the older tc_stats size sans
requeue stat addition, then don't try to reference that struct member.

	if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats_old)) {
		/* report everything sans requeue */
	} else if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats)) {
		/* report all stats, including requeue */
	} else {
		/* report error, etc. */
	}

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Mon, 2004-08-30 at 18:44, David S. Miller wrote:
> On 30 Aug 2004 18:14:48 -0400
> jamal <hadi@cyberus.ca> wrote:

> Check the size, if it matches the older tc_stats size sans
> requeue stat addition, then don't try to reference that struct member.
> 
> 	if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats_old)) {
> 		/* report everything sans requeue */
> 	} else if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats)) {
> 		/* report all stats, including requeue */
> 	} else {
> 		/* report error, etc. */
> 	}

Sounds reasonable to me. Some of the BSDs do this to maintain old compat
- just means keeping old struct around. Steve, agreeable to you? 

Change to both kernel and user space or just user space?

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On 30 Aug 2004 18:56:32 -0400
jamal <hadi@cyberus.ca> wrote:

> Sounds reasonable to me. Some of the BSDs do this to maintain old compat
> - just means keeping old struct around. Steve, agreeable to you? 
> 
> Change to both kernel and user space or just user space?

But this takes care of 'tc' only.  What about other programs
grabbing TC_STATS?  The whole world of netlink is not the
iproute2 tree.

Maybe instead we should create TC_STATS2?  That's a lot of
work just to add this one new statistic, I must say.

Re: RFC/PATCH capture qdisc requeue event in stats

[Stephen Hemminger]

On 30 Aug 2004 18:56:32 -0400
jamal <hadi@cyberus.ca> wrote:

> On Mon, 2004-08-30 at 18:44, David S. Miller wrote:
> > On 30 Aug 2004 18:14:48 -0400
> > jamal <hadi@cyberus.ca> wrote:
> 
> > Check the size, if it matches the older tc_stats size sans
> > requeue stat addition, then don't try to reference that struct member.
> > 
> > 	if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats_old)) {
> > 		/* report everything sans requeue */
> > 	} else if (RTA_PAYLOAD(tb[TCA_STATS]) == sizeof(struct tc_stats)) {
> > 		/* report all stats, including requeue */
> > 	} else {
> > 		/* report error, etc. */
> > 	}
> 
> Sounds reasonable to me. Some of the BSDs do this to maintain old compat
> - just means keeping old struct around. Steve, agreeable to you? 
> 
> Change to both kernel and user space or just user space?

I have no problem but easier to just do something:
	struct tc_stats mystats;

	memset(&mystats, 0, sizeof(mystats));
	memcpy(&mystats, RTA_DATA(tb[TCA_STATS]), RTA_PAYLOAD(tb[TCA_STATS]));

that way it can grow as much as we want and don't have v1, v2, v3, ... 
size structures.

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Mon, 2004-08-30 at 19:00, David S. Miller wrote:
> On 30 Aug 2004 18:56:32 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > Sounds reasonable to me. Some of the BSDs do this to maintain old compat
> > - just means keeping old struct around. Steve, agreeable to you? 
> > 
> > Change to both kernel and user space or just user space?
> 
> But this takes care of 'tc' only.  What about other programs
> grabbing TC_STATS?  The whole world of netlink is not the
> iproute2 tree.

Agreed. Note, however the only time it wont work was case for

4) old kernel +  new tc --> will bomb (sizeof check will fail)

s/tc/randomnetlinkapp

i.e old apps will continue to work.
We could probably impose some rules like the code you posted.
I actually only see reason to keep old_tc_stats in user space only. New
apps wanting to support old structures must copy them.

> Maybe instead we should create TC_STATS2?  That's a lot of
> work just to add this one new statistic, I must say.

That too - it will mean keeping both old and new in user space and
kernel. It is from an ABI perspective cleaner than the first one.
Not sure if it is worth for this one element in a structure.
Theres also already the *_XSTAS which is supposed to be a speacial non
generic stat that could be used to transport the requeue it just seems
to be such a waste to use it for transporting a sinle 32 bit value
(which just happens to be generic for qdiscs). It would also involve
more code overall.

One of these days i will code the discovery thoughts i have been having.
Just ask the kernel what it has and we are set.

On Mon, 2004-08-30 at 19:05, Stephen Hemminger wrote: 

> I have no problem but easier to just do something:
> 	struct tc_stats mystats;
> 
> 	memset(&mystats, 0, sizeof(mystats));
> 	memcpy(&mystats, RTA_DATA(tb[TCA_STATS]), RTA_PAYLOAD(tb[TCA_STATS]));
> 
> that way it can grow as much as we want and don't have v1, v2, v3, ... 
> size structures.

Not sure i parsed that. You mean the tc_stats in user space will be the new 
one.

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On 30 Aug 2004 21:43:13 -0400
jamal <hadi@cyberus.ca> wrote:

> On Mon, 2004-08-30 at 19:05, Stephen Hemminger wrote: 
> 
> > I have no problem but easier to just do something:
> > 	struct tc_stats mystats;
> > 
> > 	memset(&mystats, 0, sizeof(mystats));
> > 	memcpy(&mystats, RTA_DATA(tb[TCA_STATS]), RTA_PAYLOAD(tb[TCA_STATS]));
> > 
> > that way it can grow as much as we want and don't have v1, v2, v3, ... 
> > size structures.
> 
> Not sure i parsed that. You mean the tc_stats in user space will be the new 
> one.

Yes, that's his idea.  This way on older kernel the "new" statistics
just show up as zero.

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Mon, 2004-08-30 at 22:17, David S. Miller wrote:
> On 30 Aug 2004 21:43:13 -0400
> jamal <hadi@cyberus.ca> wrote:

> > Not sure i parsed that. You mean the tc_stats in user space will be the new 
> > one.
> 
> Yes, that's his idea.  This way on older kernel the "new" statistics
> just show up as zero.

thmart. Lets go with this then. Steve, do you wanna take it from here
with the iproute2 patch i sent or you want me to generate a new one? Now
how do we tell all other people who have written netlink apps
to beware?

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On 30 Aug 2004 22:37:03 -0400
jamal <hadi@cyberus.ca> wrote:

> thmart. Lets go with this then. Steve, do you wanna take it from here
> with the iproute2 patch i sent or you want me to generate a new one? Now
> how do we tell all other people who have written netlink apps
> to beware?

Yeah, how are you going to "tell those other people"?  And how
are you going to get every user to know to upgrade all of those
apps when they try to use a kernel with your patch applied?

Look, let's get real about this topic.  We can't be breaking shit
like this all the time.  We're nearly letting it happen a lot
lately.

These data structures are user visible APIs, they are just like
system call data structures, and if we cannot modify
them without potentially breaking some existing application we
cannot make that change.

So please find another way to add make this statistic visible to
userspace Jamal.  Thanks a lot.

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Tue, 2004-08-31 at 00:29, David S. Miller wrote:
> On 30 Aug 2004 22:37:03 -0400
> jamal <hadi@cyberus.ca> wrote:
> 
> > thmart. Lets go with this then. Steve, do you wanna take it from here
> > with the iproute2 patch i sent or you want me to generate a new one? Now
> > how do we tell all other people who have written netlink apps
> > to beware?
> 
> Yeah, how are you going to "tell those other people"?  And how
> are you going to get every user to know to upgrade all of those
> apps when they try to use a kernel with your patch applied?


Again, thats not an issue. new kernel. old app should work
Challenge is old kernel, new app.

> Look, let's get real about this topic.  We can't be breaking shit
> like this all the time.  We're nearly letting it happen a lot
> lately.

Agreed.

> These data structures are user visible APIs, they are just like
> system call data structures, and if we cannot modify
> them without potentially breaking some existing application we
> cannot make that change.
> 
> So please find another way to add make this statistic visible to
> userspace Jamal.  Thanks a lot.

Well, Franks A Lot(TM) Dave ;->
Let me rethink - I will be back.

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[Stephen Hemminger]

On Sun, 2004-08-29 at 13:13 -0400, jamal wrote:
> 
> The requeue event is useful in finding out when a device is overloaded
> on the egress (bus or bandwidth).
> Atached patch introduces this. I would have used the overlimit bits
> but at the moment thats being used for different semantical reasons.
> I have not done extensive testing on it. 
> 
> Opinions welcome - If all is good, Dave please apply.

Dave, what happened to this?  I put the stuff into iproute2 but the
requeue stat never made it into 2.6.  Is it a bad idea?

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On Tue, 28 Sep 2004 16:10:09 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> On Sun, 2004-08-29 at 13:13 -0400, jamal wrote:
> > 
> > The requeue event is useful in finding out when a device is overloaded
> > on the egress (bus or bandwidth).
> > Atached patch introduces this. I would have used the overlimit bits
> > but at the moment thats being used for different semantical reasons.
> > I have not done extensive testing on it. 
> > 
> > Opinions welcome - If all is good, Dave please apply.
> 
> Dave, what happened to this?  I put the stuff into iproute2 but the
> requeue stat never made it into 2.6.  Is it a bad idea?

Yes, API breaker.

Re: RFC/PATCH capture qdisc requeue event in stats

[Stephen Hemminger]

On Tue, 2004-09-28 at 16:18 -0700, David S. Miller wrote:
> On Tue, 28 Sep 2004 16:10:09 -0700
> Stephen Hemminger <shemminger@osdl.org> wrote:
> 
> > On Sun, 2004-08-29 at 13:13 -0400, jamal wrote:
> > > 
> > > The requeue event is useful in finding out when a device is overloaded
> > > on the egress (bus or bandwidth).
> > > Atached patch introduces this. I would have used the overlimit bits
> > > but at the moment thats being used for different semantical reasons.
> > > I have not done extensive testing on it. 
> > > 
> > > Opinions welcome - If all is good, Dave please apply.
> > 
> > Dave, what happened to this?  I put the stuff into iproute2 but the
> > requeue stat never made it into 2.6.  Is it a bad idea?
> 
> Yes, API breaker.

Well it seems to work for me:
	  kernel
        New	Old
tc New  Ok      Ok(0)
   Old  Ok(0)   Ok

Because tc correctly handles the returned TLV size.

Re: RFC/PATCH capture qdisc requeue event in stats

[David S. Miller]

On Tue, 28 Sep 2004 17:01:41 -0700
Stephen Hemminger <shemminger@osdl.org> wrote:

> On Tue, 2004-09-28 at 16:18 -0700, David S. Miller wrote:
> > On Tue, 28 Sep 2004 16:10:09 -0700
> > Stephen Hemminger <shemminger@osdl.org> wrote:
> > 
> > > On Sun, 2004-08-29 at 13:13 -0400, jamal wrote:
> > > > 
> > > > The requeue event is useful in finding out when a device is overloaded
> > > > on the egress (bus or bandwidth).
> > > > Atached patch introduces this. I would have used the overlimit bits
> > > > but at the moment thats being used for different semantical reasons.
> > > > I have not done extensive testing on it. 
> > > > 
> > > > Opinions welcome - If all is good, Dave please apply.
> > > 
> > > Dave, what happened to this?  I put the stuff into iproute2 but the
> > > requeue stat never made it into 2.6.  Is it a bad idea?
> > 
> > Yes, API breaker.
> 
> Well it seems to work for me:
> 	  kernel
>         New	Old
> tc New  Ok      Ok(0)
>    Old  Ok(0)   Ok
> 
> Because tc correctly handles the returned TLV size.

Because tc has all the length checking stuff added to
it, other applications using netlink might not.  The
whole world is not 'tc'.

I'm just rehashing how the most recent thread between
Jamal and myself ended about this topic.

Re: RFC/PATCH capture qdisc requeue event in stats

[Thomas Graf]

* David S. Miller <20040830212910.78047bcd.davem@davemloft.net> 2004-08-30 21:29
> Look, let's get real about this topic.  We can't be breaking shit
> like this all the time.  We're nearly letting it happen a lot
> lately.
> 
> These data structures are user visible APIs, they are just like
> system call data structures, and if we cannot modify
> them without potentially breaking some existing application we
> cannot make that change.

Why not do it by using nested TLVs?:

  TCA_STATS2 [
      TCA_STAT_BYTES
      TCA_STAT_PACKETS
      TCA_STAT_DROPS
      ...
  ]

This way we can add as many new stats as we want without
even thinking about backward compatibility in the future.
This would also allow to implement dynamic size statistics
or introduce TCA_STAT_*_64 if ever needed.

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Tue, 2004-09-28 at 20:03, David S. Miller wrote:

> > 
> > Well it seems to work for me:
> > 	  kernel
> >         New	Old
> > tc New  Ok      Ok(0)
> >    Old  Ok(0)   Ok
> > 
> > Because tc correctly handles the returned TLV size.
> 
> Because tc has all the length checking stuff added to
> it, other applications using netlink might not.  The
> whole world is not 'tc'.
> 
> I'm just rehashing how the most recent thread between
> Jamal and myself ended about this topic.

Steve just kill it for now - Dave is right. I think maybe something
based out of a cleaned gnet_stats patch i posted earlier would be the
best path forward. I will try to clean it up.

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Tue, 2004-09-28 at 20:36, Thomas Graf wrote:
> * David S. Miller <20040830212910.78047bcd.davem@davemloft.net> 2004-08-30 21:29
> > Look, let's get real about this topic.  We can't be breaking shit
> > like this all the time.  We're nearly letting it happen a lot
> > lately.
> > 
> > These data structures are user visible APIs, they are just like
> > system call data structures, and if we cannot modify
> > them without potentially breaking some existing application we
> > cannot make that change.
> 
> Why not do it by using nested TLVs?:
> 
>   TCA_STATS2 [
>       TCA_STAT_BYTES
>       TCA_STAT_PACKETS
>       TCA_STAT_DROPS
>       ...
>   ]

Refer to my earlier email; i think this is a noble approach.
Lets do it on gnet stats though so we can make it more accessible.
I think your granularity maybe too thin: bytes,packets, drops
may need to be in the same TLV.

> This way we can add as many new stats as we want without
> even thinking about backward compatibility in the future.
> This would also allow to implement dynamic size statistics
> or introduce TCA_STAT_*_64 if ever needed.

Yep.
do you have cycles to run with that patch?

cheers,
jamal

Re: RFC/PATCH capture qdisc requeue event in stats

[Thomas Graf]

* jamal <1096426464.1045.133.camel@jzny.localdomain> 2004-09-28 22:54
> Lets do it on gnet stats though so we can make it more accessible.
> I think your granularity maybe too thin: bytes,packets, drops
> may need to be in the same TLV.

I partially agree as long as no structs visible to userspace will
ever change after introduction.

I'd really like to avoid maintaining different structs for collection
and transfer which must be kept in sync.

Having the application do its own struct and fill it based on
if (tb[TCA_STATS_*]) conditions avoids all possible compatibility 
issues. It would be possible for the application to choose between
the original and a _64 variant if we ever introduce them.

So this would be my approach:

Leave gnet_stats as-is and extend it as needed but hide it from
userspace.

Extend gen_copy_stats to add a nested TLV for every stat in
the structure. We could even introduce a bitmap to allow the
struct gnet_stats user to define which statistics are defined
and which aren't. This way we could avoid the mess with pfifo
and bfifo both using backlog but with a different unit in mind
(packets/bytes).

> do you have cycles to run with that patch?

Yes, it is related to my work so it's not a problem.

Re: RFC/PATCH capture qdisc requeue event in stats

[jamal]

On Wed, 2004-09-29 at 08:48, Thomas Graf wrote:
> * jamal <1096426464.1045.133.camel@jzny.localdomain> 2004-09-28 22:54
> > Lets do it on gnet stats though so we can make it more accessible.
> > I think your granularity maybe too thin: bytes,packets, drops
> > may need to be in the same TLV.
> 
> I partially agree as long as no structs visible to userspace will
> ever change after introduction.
> 

Note, this comes as a lesson from the current TC_STAT maintainance
headache. We messed up there but didnt have the hindsight we do now.
Adding a new user space visible entry becomes a matter of intrioducing 
a new TLV.

> I'd really like to avoid maintaining different structs for collection
> and transfer which must be kept in sync.

As a general principle i agree with you - this is why tc_stat got us in
trouble to begin with. The side effect is the cost of TLV 32 bit header
to transport a 32 bit or worse 8 bit. 
Putting entries into semantical groups will help reduce the overhead but
should only be done when makes sense to do so.
ex i see someone counting packets will also count bytes and maybe count
drops. Essentially if things dont make sense grouping, then dont group.
Later we figure it was a mistake, sure add a new TLV.
The problem with tc_stat is it was too fat.

> Having the application do its own struct and fill it based on
> if (tb[TCA_STATS_*]) conditions avoids all possible compatibility 
> issues. It would be possible for the application to choose between
> the original and a _64 variant if we ever introduce them.

Well, same principle applies to XSTAT; but thats app specific.

> So this would be my approach:
> 
> Leave gnet_stats as-is and extend it as needed but hide it from
> userspace.

Its too fat ;-> I dont see why an action which doesnt queue need to
have access to queue stats.

> Extend gen_copy_stats to add a nested TLV for every stat in
> the structure. We could even introduce a bitmap to allow the
> struct gnet_stats user to define which statistics are defined
> and which aren't. This way we could avoid the mess with pfifo
> and bfifo both using backlog but with a different unit in mind
> (packets/bytes).
> 

I think we should kill gnet_stat in its current form. The lessons of
tc_stat still haunt me.

> > do you have cycles to run with that patch?
> 
> Yes, it is related to my work so it's not a problem.

Thanks. We can talk offline then.

cheers,
jamal