pkt_sched: gen_estimator: Dont report fake rate estimators

RFC In review Eric Dumazet 3 participants 15 messages Oct 1, 2009 – Oct 7, 2009

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Eric Dumazet

author

2009-10-01 19:07 UTC #0 lore raw

RFC 1/1 pkt_sched: gen_estimator: Dont report fake rate estimators

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.

# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

After this patch, tc command output is :

$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

Signed-off-by Eric Dumazet <[email protected]>

---
 include/linux/gen_stats.h |   16 +++++++++++++---
 net/core/gen_estimator.c  |    9 +++++----
 net/core/gen_stats.c      |    9 ++++++---
 net/sched/act_police.c    |    2 +-
 4 files changed, 25 insertions(+), 11 deletions(-)

include/linux/gen_stats.h +13 -3

index 710e901..7678ded 100644
@@ -30,17 +30,27 @@ struct gnet_stats_basic_packed
 } __attribute__ ((packed));
 
 /**
- * struct gnet_stats_rate_est - rate estimator
+ * struct gnet_stats_user_rate_est - rate estimator
  * @bps: current byte rate
  * @pps: current packet rate
  */
-struct gnet_stats_rate_est
-{
+struct gnet_stats_user_rate_est {
 	__u32	bps;
 	__u32	pps;
 };
 
 /**
+ * struct gnet_stats_rate_est - rate estimator with flags
+ * @est: current byte/packet rate
+ * @flags: set to one if estimation is valid
+ */
+struct gnet_stats_rate_est {
+	struct gnet_stats_user_rate_est	est;
+	int				flags;
+};
+#define RATE_EST_VALID 1
+
+/**
  * struct gnet_stats_queue - queuing statistics
  * @qlen: queue length
  * @backlog: backlog size of queue

net/core/gen_estimator.c +5 -4

index 493775f..5ba9d90 100644
@@ -129,12 +129,13 @@ static void est_timer(unsigned long arg)
 		brate = (nbytes - e->last_bytes)<<(7 - idx);
 		e->last_bytes = nbytes;
 		e->avbps += (brate >> e->ewma_log) - (e->avbps >> e->ewma_log);
-		e->rate_est->bps = (e->avbps+0xF)>>5;
+		e->rate_est->est.bps = (e->avbps+0xF)>>5;
 
 		rate = (npackets - e->last_packets)<<(12 - idx);
 		e->last_packets = npackets;
 		e->avpps += (rate >> e->ewma_log) - (e->avpps >> e->ewma_log);
-		e->rate_est->pps = (e->avpps+0x1FF)>>10;
+		e->rate_est->est.pps = (e->avpps+0x1FF)>>10;
+		e->rate_est->flags |= RATE_EST_VALID;
 skip:
 		read_unlock(&est_lock);
 		spin_unlock(e->stats_lock);

@@ -227,9 +228,9 @@ int gen_new_estimator(struct gnet_stats_basic_packed *bstats,
 	est->stats_lock = stats_lock;
 	est->ewma_log = parm->ewma_log;
 	est->last_bytes = bstats->bytes;
-	est->avbps = rate_est->bps<<5;
+	est->avbps = rate_est->est.bps<<5;
 	est->last_packets = bstats->packets;
-	est->avpps = rate_est->pps<<10;
+	est->avpps = rate_est->est.pps<<10;
 
 	if (!elist[idx].timer.function) {
 		INIT_LIST_HEAD(&elist[idx].list);

net/core/gen_stats.c +6 -3

index 8569310..b6f723c 100644
@@ -138,13 +138,16 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
 int
 gnet_stats_copy_rate_est(struct gnet_dump *d, struct gnet_stats_rate_est *r)
 {
+	if (!(r->flags & RATE_EST_VALID))
+		return 0;
+
 	if (d->compat_tc_stats) {
-		d->tc_stats.bps = r->bps;
-		d->tc_stats.pps = r->pps;
+		d->tc_stats.bps = r->est.bps;
+		d->tc_stats.pps = r->est.pps;
 	}
 
 	if (d->tail)
-		return gnet_stats_copy(d, TCA_STATS_RATE_EST, r, sizeof(*r));
+		return gnet_stats_copy(d, TCA_STATS_RATE_EST, &r->est, sizeof(r->est));
 
 	return 0;
 }

net/sched/act_police.c +1 -1

index 723964c..ba01081 100644
@@ -292,7 +292,7 @@ static int tcf_act_police(struct sk_buff *skb, struct tc_action *a,
 	police->tcf_bstats.packets++;
 
 	if (police->tcfp_ewma_rate &&
-	    police->tcf_rate_est.bps >= police->tcfp_ewma_rate) {
+	    police->tcf_rate_est.est.bps >= police->tcfp_ewma_rate) {
 		police->tcf_qstats.overlimits++;
 		if (police->tcf_action == TC_ACT_SHOT)
 			police->tcf_qstats.drops++;

David Miller

maintainer

2009-10-01 19:37 UTC #1 lore raw

From: Eric Dumazet <[email protected]>
Date: Thu, 01 Oct 2009 21:07:51 +0200

Quoted 11 lines

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.
# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

Quoted 7 lines

After this patch, tc command output is :
$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
Signed-off-by Eric Dumazet <[email protected]>

I'm generally fine with this idea.

The new behavior is certainly more intuitive even to me :-)

Unless there are other objections I'm ok with this and I'll apply your final version when I start taking changes for net-next-2.6 (which is probably right after -rc3 is released).

Jarek Poplawski

2009-10-01 21:06 UTC #2 lore raw

David Miller wrote, On 10/01/2009 09:37 PM:

Quoted 20 lines

From: Eric Dumazet <[email protected]>
Date: Thu, 01 Oct 2009 21:07:51 +0200
Quoted 17 lines
We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.
# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)
After this patch, tc command output is :
$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

Quoted 7 lines

Quoted 1 line
Signed-off-by Eric Dumazet <[email protected]>
I'm generally fine with this idea.
The new behavior is certainly more intuitive even to me :-)
Unless there are other objections I'm ok with this and I'll apply

Since you ask... I wonder about this whole int plus quite a bit of struct unreadability for one flag only. Maybe it could be queried on qdisc level (with a flag if necessary), and additional parameter of gnet_stats_copy_rate_est()? (Qdiscs should have no problem with setting this param for their classes too.)

Jarek P.

David Miller

maintainer

2009-10-01 21:13 UTC #3 lore raw

From: Jarek Poplawski <[email protected]>
Date: Thu, 01 Oct 2009 23:05:53 +0200

Quoted 5 lines

Since you ask... I wonder about this whole int plus quite a bit of struct unreadability for one flag only. Maybe it could be queried on qdisc level (with a flag if necessary), and additional parameter of gnet_stats_copy_rate_est()? (Qdiscs should have no problem with setting this param for their classes too.)

Certainly, that's another approach to this problem.

But logically, just like we wouldn't emit a block of RED scheduler data to 'tc' unless RED is actually configured, it seems consistent to not emit estimator data when no estimator is even there.

Jarek Poplawski

2009-10-01 21:22 UTC #4 lore raw

David Miller wrote, On 10/01/2009 11:14 PM:

Quoted 9 lines

From: Jarek Poplawski <[email protected]>
Date: Thu, 01 Oct 2009 23:05:53 +0200
Quoted 5 lines
Since you ask... I wonder about this whole int plus quite a bit of struct unreadability for one flag only. Maybe it could be queried on qdisc level (with a flag if necessary), and additional parameter of gnet_stats_copy_rate_est()? (Qdiscs should have no problem with setting this param for their classes too.)

Quoted 5 lines

Certainly, that's another approach to this problem.
But logically, just like we wouldn't emit a block of RED scheduler data to 'tc' unless RED is actually configured, it seems consistent to not emit estimator data when no estimator is even there.

Sure! I've exaggerated with this additional parameter. ;-)

Jarek P.

Jarek Poplawski

2009-10-02 07:08 UTC #5 lore raw

Quoted 10 lines · Jarek Poplawski

David Miller wrote, On 10/01/2009 11:14 PM:
Quoted 8 lines
From: Jarek Poplawski <[email protected]>
Date: Thu, 01 Oct 2009 23:05:53 +0200
Quoted 5 lines
Since you ask... I wonder about this whole int plus quite a bit of struct unreadability for one flag only. Maybe it could be queried on qdisc level (with a flag if necessary), and additional parameter of gnet_stats_copy_rate_est()? (Qdiscs should have no problem with setting this param for their classes too.)

Quoted 7 lines

Quoted 5 lines
Certainly, that's another approach to this problem.
But logically, just like we wouldn't emit a block of RED scheduler data to 'tc' unless RED is actually configured, it seems consistent to not emit estimator data when no estimator is even there.
Sure! I've exaggerated with this additional parameter. ;-)

To make my point clare: why not something like this?:

static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,

                         u32 pid, u32 seq, u16 flags, int event)
{
	...
	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
	    (gen_estimator_active(&q->bstats, &q->rate_est) &&
             gnet_stats_copy_rate_est(&d, &q->rate_est) < 0) ||
            gnet_stats_copy_queue(&d, &q->qstats) < 0)
                goto nla_put_failure;

BTW, I'm not sure we need to chanage user visible API for this. (Is it really expected to work after updating gen_stats.h only in iproute?)

Jarek P.

Eric Dumazet

author

2009-10-02 07:12 UTC #6 lore raw

Quoted 12 lines · Jarek Poplawski

To make my point clare: why not something like this?:

static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,

                         u32 pid, u32 seq, u16 flags, int event)
{
	...
	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
	    (gen_estimator_active(&q->bstats, &q->rate_est) &&
             gnet_stats_copy_rate_est(&d, &q->rate_est) < 0) ||
            gnet_stats_copy_queue(&d, &q->qstats) < 0)
                goto nla_put_failure;

Quoted 4 lines

BTW, I'm not sure we need to chanage user visible API for this. (Is it really expected to work after updating gen_stats.h only in iproute?)

Thats would be better indeed, do you want to work on it or let me do it ?

Thanks

Jarek Poplawski

2009-10-02 07:17 UTC #7 lore raw

Quoted 14 lines · Eric Dumazet

Quoted 12 lines · Jarek Poplawski

To make my point clare: why not something like this?:

static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,

                         u32 pid, u32 seq, u16 flags, int event)
{
	...
	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
	    (gen_estimator_active(&q->bstats, &q->rate_est) &&
             gnet_stats_copy_rate_est(&d, &q->rate_est) < 0) ||
            gnet_stats_copy_queue(&d, &q->qstats) < 0)
                goto nla_put_failure;

Quoted 6 lines

Quoted 4 lines
BTW, I'm not sure we need to chanage user visible API for this. (Is it really expected to work after updating gen_stats.h only in iproute?)
Thats would be better indeed, do you want to work on it or let me do it ?

I want you work on it.

Thanks,
Jarek P.

Jarek Poplawski

2009-10-02 07:32 UTC #8 lore raw

In reply to Jarek Poplawski

Quoted 1 line · Jarek Poplawski

On 01-10-2009 23:21, Jarek Poplawski wrote:

...

Quoted 1 line

To make my point clare: [...]

Am I clair? ;-)

Jarek P.

Eric Dumazet

author

2009-10-02 10:35 UTC #9 lore raw

RFC 1/1 pkt_sched: gen_estimator: Dont report fake rate estimators

In reply to Jarek Poplawski

Here is second attempt to make this change, thanks Jarek !

This is indeed less intrusive !

[RFC] pkt_sched: gen_estimator: Dont report fake rate estimators

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.

# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

After this patch, tc command output is :

$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

We add a parameter to gnet_stats_copy_rate_est() function so that it can use gen_estimator_active(bstats, r), as suggested by Jarek.

Signed-off-by Eric Dumazet <[email protected]>

---
 include/net/gen_stats.h |    1 +
 net/core/gen_stats.c    |    7 ++++++-
 net/sched/act_api.c     |    2 +-
 net/sched/sch_api.c     |    2 +-
 net/sched/sch_cbq.c     |    2 +-
 net/sched/sch_drr.c     |    2 +-
 net/sched/sch_hfsc.c    |    2 +-
 net/sched/sch_htb.c     |    2 +-
 8 files changed, 13 insertions(+), 7 deletions(-)

include/net/gen_stats.h +1 -0

index c148855..a0800e6 100644
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *bstats,
 				    struct gnet_stats_rate_est *r);
 extern int gnet_stats_copy_queue(struct gnet_dump *d,
 				 struct gnet_stats_queue *q);

net/core/gen_stats.c +6 -1

index 8569310..6f9513e 100644
@@ -136,8 +136,13 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
  * if the room in the socket buffer was not sufficient.
  */
 int
-gnet_stats_copy_rate_est(struct gnet_dump *d, struct gnet_stats_rate_est *r)
+gnet_stats_copy_rate_est(struct gnet_dump *d,
+			 const struct gnet_stats_basic_packed *bstats,
+			 struct gnet_stats_rate_est *r)
 {
+	if (!gen_estimator_active(bstats, r))
+		return 0;
+
 	if (d->compat_tc_stats) {
 		d->tc_stats.bps = r->bps;
 		d->tc_stats.pps = r->pps;

net/sched/act_api.c +1 -1

index 2dfb3e7..2b0d5ee 100644
@@ -618,7 +618,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
 			goto errout;
 
 	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &h->tcf_rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats, &h->tcf_rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &h->tcf_qstats) < 0)
 		goto errout;

net/sched/sch_api.c +1 -1

index 903e418..1acfd29 100644
@@ -1179,7 +1179,7 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 		goto nla_put_failure;
 
 	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &q->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &q->bstats, &q->rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &q->qstats) < 0)
 		goto nla_put_failure;

net/sched/sch_cbq.c +1 -1

index 5b132c4..3846d65 100644
@@ -1609,7 +1609,7 @@ cbq_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 		cl->xstats.undertime = cl->undertime - q->now;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_drr.c +1 -1

index 5a888af..a65604f 100644
@@ -280,7 +280,7 @@ static int drr_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	}
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qdisc->qstats) < 0)
 		return -1;

net/sched/sch_hfsc.c +1 -1

index 2c5c76b..b38b39c 100644
@@ -1375,7 +1375,7 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	xstats.rtwork  = cl->cl_cumul;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_htb.c +1 -1

index 85acab9..8352fa3 100644
@@ -1105,7 +1105,7 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	cl->xstats.ctokens = cl->ctokens;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

Jarek Poplawski

2009-10-02 11:25 UTC #10 lore raw

Quoted 17 lines · Eric Dumazet

Here is second attempt to make this change, thanks Jarek !
This is indeed less intrusive !
[RFC] pkt_sched: gen_estimator: Dont report fake rate estimators
We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.
# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0
User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

Quoted 8 lines

After this patch, tc command output is :
$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0
We add a parameter to gnet_stats_copy_rate_est() function so that it can use gen_estimator_active(bstats, r), as suggested by Jarek.

So you prefer the additional parameter version, but since these _active tests are not needed e.g. for HTB classes, which got it active by default, so maybe bstats == NULL would let skip such a test?

...

include/net/gen_stats.h extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type, · line 33

Quoted 7 lines

include/net/gen_stats.h +1 -0

@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *bstats,

It seems these *b/*bstats defs could look more consistent. Otherwise it looks OK to me.

Thanks,
Jarek P.

Eric Dumazet

author

2009-10-02 12:39 UTC #11 lore raw

Quoted 5 lines · Jarek Poplawski

So you prefer the additional parameter version, but since these _active tests are not needed e.g. for HTB classes, which got it active by default, so maybe bstats == NULL would let skip such a test?
...

Quoted 10 lines

include/net/gen_stats.h extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type, · line 33
Quoted 7 lines
include/net/gen_stats.h +1 -0
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *bstats,
It seems these *b/*bstats defs could look more consistent. Otherwise it looks OK to me.

Agreed, here is the updated version, I added your Signoff if you dont mind :)

[RFC] pkt_sched: gen_estimator: Dont report fake rate estimators

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.

# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

After this patch, tc command output is :

$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

We add a parameter to gnet_stats_copy_rate_est() function so that it can use gen_estimator_active(bstats, r), as suggested by Jarek.

This parameter can be NULL if check is not necessary, (htb for example has a mandatory rate estimator)

Signed-off-by Eric Dumazet <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---
 include/net/gen_stats.h |    1 +
 net/core/gen_stats.c    |    7 ++++++-
 net/sched/act_api.c     |    2 +-
 net/sched/sch_api.c     |    2 +-
 net/sched/sch_cbq.c     |    2 +-
 net/sched/sch_drr.c     |    2 +-
 net/sched/sch_hfsc.c    |    2 +-
 net/sched/sch_htb.c     |    2 +-
 8 files changed, 13 insertions(+), 7 deletions(-)

include/net/gen_stats.h +1 -0

index c148855..eb87a14 100644
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *b,
 				    struct gnet_stats_rate_est *r);
 extern int gnet_stats_copy_queue(struct gnet_dump *d,
 				 struct gnet_stats_queue *q);

net/core/gen_stats.c +6 -1

index 8569310..054a49c 100644
@@ -136,8 +136,13 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
  * if the room in the socket buffer was not sufficient.
  */
 int
-gnet_stats_copy_rate_est(struct gnet_dump *d, struct gnet_stats_rate_est *r)
+gnet_stats_copy_rate_est(struct gnet_dump *d,
+			 const struct gnet_stats_basic_packed *b,
+			 struct gnet_stats_rate_est *r)
 {
+	if (b && !gen_estimator_active(b, r))
+		return 0;
+
 	if (d->compat_tc_stats) {
 		d->tc_stats.bps = r->bps;
 		d->tc_stats.pps = r->pps;

net/sched/act_api.c +1 -1

index 2dfb3e7..2b0d5ee 100644
@@ -618,7 +618,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
 			goto errout;
 
 	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &h->tcf_rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats, &h->tcf_rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &h->tcf_qstats) < 0)
 		goto errout;

net/sched/sch_api.c +1 -1

index 903e418..1acfd29 100644
@@ -1179,7 +1179,7 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 		goto nla_put_failure;
 
 	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &q->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &q->bstats, &q->rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &q->qstats) < 0)
 		goto nla_put_failure;

net/sched/sch_cbq.c +1 -1

index 5b132c4..3846d65 100644
@@ -1609,7 +1609,7 @@ cbq_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 		cl->xstats.undertime = cl->undertime - q->now;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_drr.c +1 -1

index 5a888af..a65604f 100644
@@ -280,7 +280,7 @@ static int drr_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	}
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qdisc->qstats) < 0)
 		return -1;

net/sched/sch_hfsc.c +1 -1

index 2c5c76b..b38b39c 100644
@@ -1375,7 +1375,7 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	xstats.rtwork  = cl->cl_cumul;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_htb.c +1 -1

index 85acab9..2e38d1a 100644
@@ -1105,7 +1105,7 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	cl->xstats.ctokens = cl->ctokens;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, NULL, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

Jarek Poplawski

2009-10-02 20:12 UTC #12 lore raw

Eric Dumazet wrote, On 10/02/2009 02:39 PM:

Quoted 17 lines

Quoted 5 lines · Jarek Poplawski
So you prefer the additional parameter version, but since these _active tests are not needed e.g. for HTB classes, which got it active by default, so maybe bstats == NULL would let skip such a test?
...
Quoted 9 lines
include/net/gen_stats.h extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type, · line 33
Quoted 7 lines
include/net/gen_stats.h +1 -0
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *bstats,
It seems these *b/*bstats defs could look more consistent. Otherwise it looks OK to me.

Quoted 1 line

Agreed, here is the updated version, I added your Signoff if you dont mind :)

Hmm... So you made me to do some "real" work here, and guess what?:
there is one serious checkpatch warning! ;-) Plus, this new parameter should be added to the function description. Otherwise:

Signed-off-by Jarek Poplawski <[email protected]>

Thanks,
Jarek P.

PS: I guess full "Don't" would show we really mean it...

Quoted 153 lines

[RFC] pkt_sched: gen_estimator: Dont report fake rate estimators

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.

# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

After this patch, tc command output is :

$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

We add a parameter to gnet_stats_copy_rate_est() function so that it can use gen_estimator_active(bstats, r), as suggested by Jarek.

This parameter can be NULL if check is not necessary, (htb for example has a mandatory rate estimator)

Signed-off-by Eric Dumazet <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---
 include/net/gen_stats.h |    1 +
 net/core/gen_stats.c    |    7 ++++++-
 net/sched/act_api.c     |    2 +-
 net/sched/sch_api.c     |    2 +-
 net/sched/sch_cbq.c     |    2 +-
 net/sched/sch_drr.c     |    2 +-
 net/sched/sch_hfsc.c    |    2 +-
 net/sched/sch_htb.c     |    2 +-
 8 files changed, 13 insertions(+), 7 deletions(-)

include/net/gen_stats.h +1 -0

index c148855..eb87a14 100644
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *b,
 				    struct gnet_stats_rate_est *r);
 extern int gnet_stats_copy_queue(struct gnet_dump *d,
 				 struct gnet_stats_queue *q);

net/core/gen_stats.c +6 -1

index 8569310..054a49c 100644
@@ -136,8 +136,13 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
  * if the room in the socket buffer was not sufficient.
  */
 int
-gnet_stats_copy_rate_est(struct gnet_dump *d, struct gnet_stats_rate_est *r)
+gnet_stats_copy_rate_est(struct gnet_dump *d,
+			 const struct gnet_stats_basic_packed *b,
+			 struct gnet_stats_rate_est *r)
 {
+	if (b && !gen_estimator_active(b, r))
+		return 0;
+
 	if (d->compat_tc_stats) {
 		d->tc_stats.bps = r->bps;
 		d->tc_stats.pps = r->pps;

net/sched/act_api.c +1 -1

index 2dfb3e7..2b0d5ee 100644
@@ -618,7 +618,7 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
 			goto errout;
 
 	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &h->tcf_rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats, &h->tcf_rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &h->tcf_qstats) < 0)
 		goto errout;

net/sched/sch_api.c +1 -1

index 903e418..1acfd29 100644
@@ -1179,7 +1179,7 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 		goto nla_put_failure;
 
 	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &q->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &q->bstats, &q->rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &q->qstats) < 0)
 		goto nla_put_failure;

net/sched/sch_cbq.c +1 -1

index 5b132c4..3846d65 100644
@@ -1609,7 +1609,7 @@ cbq_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 		cl->xstats.undertime = cl->undertime - q->now;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_drr.c +1 -1

index 5a888af..a65604f 100644
@@ -280,7 +280,7 @@ static int drr_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	}
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qdisc->qstats) < 0)
 		return -1;

net/sched/sch_hfsc.c +1 -1

index 2c5c76b..b38b39c 100644
@@ -1375,7 +1375,7 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	xstats.rtwork  = cl->cl_cumul;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_htb.c +1 -2

index 85acab9..2e38d1a 100644
@@ -1105,7 +1105,7 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	cl->xstats.ctokens = cl->ctokens;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, NULL, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;
 
--

To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Eric Dumazet

author

2009-10-02 20:32 UTC #13 lore raw

Quoted 11 lines · Jarek Poplawski

Hmm... So you made me to do some "real" work here, and guess what?:
there is one serious checkpatch warning! ;-) Plus, this new parameter should be added to the function description. Otherwise:
Signed-off-by Jarek Poplawski <[email protected]>
Thanks,
Jarek P.
PS: I guess full "Don't" would show we really mean it...

Okay :) Here is the last round, before the night !

Thanks again

[RFC] pkt_sched: gen_estimator: Don't report fake rate estimators

We currently send TCA_STATS_RATE_EST elements to netlink users, even if no estimator is running.

# tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 112833764978 bytes 1495081739 pkt (dropped 0, overlimits 0 requeues 0)
 rate 0bit 0pps backlog 0b 0p requeues 0

User has no way to tell if the "rate 0bit 0pps" is a real estimation, or a fake one (because no estimator is active)

After this patch, tc command output is :

$ tc -s -d qdisc
qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
 Sent 561075 bytes 1196 pkt (dropped 0, overlimits 0 requeues 0)
 backlog 0b 0p requeues 0

We add a parameter to gnet_stats_copy_rate_est() function so that it can use gen_estimator_active(bstats, r), as suggested by Jarek.

This parameter can be NULL if check is not necessary, (htb for example has a mandatory rate estimator)

Signed-off-by Eric Dumazet <[email protected]>

Signed-off-by Jarek Poplawski <[email protected]>

---
 include/net/gen_stats.h |    1 +
 net/core/gen_stats.c    |    8 +++++++-
 net/sched/act_api.c     |    3 ++-
 net/sched/sch_api.c     |    2 +-
 net/sched/sch_cbq.c     |    2 +-
 net/sched/sch_drr.c     |    2 +-
 net/sched/sch_hfsc.c    |    2 +-
 net/sched/sch_htb.c     |    2 +-
 8 files changed, 15 insertions(+), 7 deletions(-)

include/net/gen_stats.h +1 -0

index c148855..eb87a14 100644
@@ -30,6 +30,7 @@ extern int gnet_stats_start_copy_compat(struct sk_buff *skb, int type,
 extern int gnet_stats_copy_basic(struct gnet_dump *d,
 				 struct gnet_stats_basic_packed *b);
 extern int gnet_stats_copy_rate_est(struct gnet_dump *d,
+				    const struct gnet_stats_basic_packed *b,
 				    struct gnet_stats_rate_est *r);
 extern int gnet_stats_copy_queue(struct gnet_dump *d,
 				 struct gnet_stats_queue *q);

net/core/gen_stats.c +7 -1

index 8569310..393b1d8 100644
@@ -127,6 +127,7 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
 /**
  * gnet_stats_copy_rate_est - copy rate estimator statistics into statistics TLV
  * @d: dumping handle
+ * @b: basic statistics
  * @r: rate estimator statistics
  *
  * Appends the rate estimator statistics to the top level TLV created by

@@ -136,8 +137,13 @@ gnet_stats_copy_basic(struct gnet_dump *d, struct gnet_stats_basic_packed *b)
  * if the room in the socket buffer was not sufficient.
  */
 int
-gnet_stats_copy_rate_est(struct gnet_dump *d, struct gnet_stats_rate_est *r)
+gnet_stats_copy_rate_est(struct gnet_dump *d,
+			 const struct gnet_stats_basic_packed *b,
+			 struct gnet_stats_rate_est *r)
 {
+	if (b && !gen_estimator_active(b, r))
+		return 0;
+
 	if (d->compat_tc_stats) {
 		d->tc_stats.bps = r->bps;
 		d->tc_stats.pps = r->pps;

net/sched/act_api.c +2 -1

index 2dfb3e7..ca2e1fd 100644
@@ -618,7 +618,8 @@ int tcf_action_copy_stats(struct sk_buff *skb, struct tc_action *a,
 			goto errout;
 
 	if (gnet_stats_copy_basic(&d, &h->tcf_bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &h->tcf_rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &h->tcf_bstats,
+				     &h->tcf_rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &h->tcf_qstats) < 0)
 		goto errout;

net/sched/sch_api.c +1 -1

index 903e418..1acfd29 100644
@@ -1179,7 +1179,7 @@ static int tc_fill_qdisc(struct sk_buff *skb, struct Qdisc *q, u32 clid,
 		goto nla_put_failure;
 
 	if (gnet_stats_copy_basic(&d, &q->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(&d, &q->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(&d, &q->bstats, &q->rate_est) < 0 ||
 	    gnet_stats_copy_queue(&d, &q->qstats) < 0)
 		goto nla_put_failure;

net/sched/sch_cbq.c +1 -1

index 5b132c4..3846d65 100644
@@ -1609,7 +1609,7 @@ cbq_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 		cl->xstats.undertime = cl->undertime - q->now;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_drr.c +1 -1

index 5a888af..a65604f 100644
@@ -280,7 +280,7 @@ static int drr_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	}
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qdisc->qstats) < 0)
 		return -1;

net/sched/sch_hfsc.c +1 -1

index 2c5c76b..b38b39c 100644
@@ -1375,7 +1375,7 @@ hfsc_dump_class_stats(struct Qdisc *sch, unsigned long arg,
 	xstats.rtwork  = cl->cl_cumul;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, &cl->bstats, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

net/sched/sch_htb.c +1 -1

index 85acab9..2e38d1a 100644
@@ -1105,7 +1105,7 @@ htb_dump_class_stats(struct Qdisc *sch, unsigned long arg, struct gnet_dump *d)
 	cl->xstats.ctokens = cl->ctokens;
 
 	if (gnet_stats_copy_basic(d, &cl->bstats) < 0 ||
-	    gnet_stats_copy_rate_est(d, &cl->rate_est) < 0 ||
+	    gnet_stats_copy_rate_est(d, NULL, &cl->rate_est) < 0 ||
 	    gnet_stats_copy_queue(d, &cl->qstats) < 0)
 		return -1;

David Miller

maintainer Applied

2009-10-07 08:27 UTC #14 lore raw

From: Eric Dumazet <[email protected]>
Date: Fri, 02 Oct 2009 22:32:18 +0200

Quoted 11 lines

Quoted 10 lines · Jarek Poplawski
Hmm... So you made me to do some "real" work here, and guess what?:
there is one serious checkpatch warning! ;-) Plus, this new parameter should be added to the function description. Otherwise:
Signed-off-by Jarek Poplawski <[email protected]>
Thanks,
Jarek P.

Quoted 8 lines

Quoted 1 line
PS: I guess full "Don't" would show we really mean it...
Okay :) Here is the last round, before the night !
Thanks again
[RFC] pkt_sched: gen_estimator: Don't report fake rate estimators

Applied, thanks!