sch_prio: class statistics printing enabled

PATCH In review Jarek Poplawski 3 participants 13 messages Jan 31, 2007

Subsystems (3)

Networking Network drivers Traffic control (tc)

lore

Share & tools

lore

Jarek Poplawski

author

2007-01-31 07:51 UTC #0 lore raw

PATCH 1/1 sch_prio: class statistics printing enabled

Hello,

This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.

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

---

net/sched/sch_prio.c +18 -0

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
@@ -372,6 +372,23 @@ static int prio_dump_class(struct Qdisc 
 	return 0;
 }
 
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+							struct gnet_dump *d)
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	if (cl - 1 > q->bands)
+		return -ENOENT;
+
+	cl_q = q->queues[cl - 1];
+	if (gnet_stats_copy_basic(d, &cl_q->bstats) < 0 ||
+	    gnet_stats_copy_queue(d, &cl_q->qstats) < 0)
+		return -1;
+
+	return 0;
+}
+
 static void prio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
 {
 	struct prio_sched_data *q = qdisc_priv(sch);

@@ -414,6 +431,7 @@ static struct Qdisc_class_ops prio_class
 	.bind_tcf	=	prio_bind,
 	.unbind_tcf	=	prio_put,
 	.dump		=	prio_dump_class,
+	.dump_stats	=	prio_dump_class_stats,
 };
 
 static struct Qdisc_ops prio_qdisc_ops = {

Jarek Poplawski

author

2007-01-31 14:28 UTC #2 lore raw

Quoted 11 lines · Patrick McHardy

Quoted 10 lines · Jarek Poplawski
This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.
diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
net/sched/sch_prio.c +0 -0
@@ -372,6 +372,23 @@ static int prio_dump_class(struct Qdisc 
 	return 0;
 }
 

Quoted 4 lines

net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 376
Quoted 2 lines
net/sched/sch_prio.c +2 -0
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+							struct gnet_dump *d)
Please align with struct Qdisc.

I'm not sure it's fully compliant to the CodingStyle text (e.g. spaces as indentation) but I'm a polite guy.

Quoted 11 lines

net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 382
Quoted 7 lines
net/sched/sch_prio.c +6 -0
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	if (cl - 1 > q->bands)
+		return -ENOENT;
This would indicate a bug in prio_walk or prio_get. errno-codes have no meaning for dump_stats callbacks, either rely on the correctness of walk/get (as other qdiscs do) or call BUG.

Sure! To much cut & paste.

Quoted 10 lines

net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 390
Quoted 8 lines
net/sched/sch_prio.c +8 -0
+
+	cl_q = q->queues[cl - 1];
+	if (gnet_stats_copy_basic(d, &cl_q->bstats) < 0 ||
+	    gnet_stats_copy_queue(d, &cl_q->qstats) < 0)
+		return -1;
+
+	return 0;
+}
ACK for the rest.

Thanks very much - I'll resend this patch in a minute.

Regards,
Jarek P.

Patrick McHardy

2007-01-31 14:35 UTC #3 lore raw

Quoted 10 lines · Jarek Poplawski

Quoted 4 lines · Patrick McHardy
net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 376
Quoted 2 lines
net/sched/sch_prio.c +2 -0
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+							struct gnet_dump *d)
Please align with struct Qdisc.
I'm not sure it's fully compliant to the CodingStyle text (e.g. spaces as indentation) but I'm a polite guy.

Probably not, but it stays in line with the remaining file.

Jarek Poplawski

author

2007-01-31 14:32 UTC #4 lore raw

PATCH 1/1 sch_prio: class statistics printing enabled

In reply to Patrick McHardy

(Take 2)

This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.

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

---

net/sched/sch_prio.c +18 -0

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
@@ -372,6 +372,23 @@ static int prio_dump_class(struct Qdisc 
 	return 0;
 }
 
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+				 struct gnet_dump *d)
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	if (cl - 1 > q->bands)
+		return -1;
+
+	cl_q = q->queues[cl - 1];
+	if (gnet_stats_copy_basic(d, &cl_q->bstats) < 0 ||
+	    gnet_stats_copy_queue(d, &cl_q->qstats) < 0)
+		return -1;
+
+	return 0;
+}
+
 static void prio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
 {
 	struct prio_sched_data *q = qdisc_priv(sch);

@@ -414,6 +431,7 @@ static struct Qdisc_class_ops prio_class
 	.bind_tcf	=	prio_bind,
 	.unbind_tcf	=	prio_put,
 	.dump		=	prio_dump_class,
+	.dump_stats	=	prio_dump_class_stats,
 };
 
 static struct Qdisc_ops prio_qdisc_ops = {

Patrick McHardy

2007-01-31 14:37 UTC #5 lore raw

net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 382

Quoted 8 lines · Jarek Poplawski

net/sched/sch_prio.c +8 -0

+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+				 struct gnet_dump *d)
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	if (cl - 1 > q->bands)
+		return -1;

Thats not what I meant, it still hides the bug. Either do nothing (don't check) or do BUG_ON(cl - 1 > q->bands).

Jarek Poplawski

author

2007-01-31 14:58 UTC #6 lore raw

Quoted 12 lines · Patrick McHardy

net/sched/sch_prio.c static int prio_dump_class(struct Qdisc · line 382
Quoted 8 lines · Jarek Poplawski
net/sched/sch_prio.c +8 -0
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+				 struct gnet_dump *d)
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	if (cl - 1 > q->bands)
+		return -1;
Thats not what I meant, it still hides the bug. Either do nothing (don't check) or do BUG_ON(cl - 1 > q->bands).

Sorry - problems with reading!

And with understanding...

Quoted 11 lines

From sch_api.c:

       if (cl_ops->dump && cl_ops->dump(q, cl, skb, tcm) < 0)
               goto rtattr_failure;

       if (gnet_stats_start_copy_compat(skb, TCA_STATS2, TCA_STATS,
                       TCA_XSTATS, q->stats_lock, &d) < 0)
               goto rtattr_failure;

       if (cl_ops->dump_stats && cl_ops->dump_stats(q, cl, &d) < 0)
               goto rtattr_failure;

I can't see any difference between calling ->dump and ->dump_stats? Of course we may forsee this error should jump over cl_ops...

But I'm polite still, so in a minute...

Jarek P.

Patrick McHardy

2007-01-31 15:06 UTC #7 lore raw

Quoted 10 lines · Jarek Poplawski

Quoted 10 lines

From sch_api.c:

      if (cl_ops->dump && cl_ops->dump(q, cl, skb, tcm) < 0)
              goto rtattr_failure;

      if (gnet_stats_start_copy_compat(skb, TCA_STATS2, TCA_STATS,
                      TCA_XSTATS, q->stats_lock, &d) < 0)
              goto rtattr_failure;

Quoted 7 lines

Quoted 2 lines
      if (cl_ops->dump_stats && cl_ops->dump_stats(q, cl, &d) < 0)
              goto rtattr_failure;
I can't see any difference between calling ->dump and ->dump_stats? Of course we may forsee this error should jump over cl_ops...

Why should there be a difference?

The class passed to both ->dump and ->dump_stats is not a classid but a qdisc-internal identifier (pointer, integer, whatever) which comes from either ->get or ->walk, and thus is valid unless these functions have bugs. Your check would cover the bug up and has no other purpose.

Jarek Poplawski

author

2007-01-31 15:14 UTC #8 lore raw

Quoted 20 lines · Patrick McHardy

Quoted 10 lines · Jarek Poplawski
Quoted 10 lines
From sch_api.c:
      if (cl_ops->dump && cl_ops->dump(q, cl, skb, tcm) < 0)
              goto rtattr_failure;

      if (gnet_stats_start_copy_compat(skb, TCA_STATS2, TCA_STATS,
                      TCA_XSTATS, q->stats_lock, &d) < 0)
              goto rtattr_failure;
Quoted 7 lines
Quoted 2 lines
      if (cl_ops->dump_stats && cl_ops->dump_stats(q, cl, &d) < 0)
              goto rtattr_failure;
I can't see any difference between calling ->dump and ->dump_stats? Of course we may forsee this error should jump over cl_ops...

Quoted 6 lines

Why should there be a difference?
The class passed to both ->dump and ->dump_stats is not a classid but a qdisc-internal identifier (pointer, integer, whatever) which comes from either ->get or ->walk, and thus is valid unless these functions have bugs. Your check would cover the bug up and has no other purpose.

Maybe I miss something, but it's not "my check". I tried only to "stay in line with the remaining file"...

Jarek P.

Patrick McHardy

2007-01-31 15:23 UTC #9 lore raw

Quoted 9 lines · Jarek Poplawski

Quoted 4 lines · Patrick McHardy
The class passed to both ->dump and ->dump_stats is not a classid but a qdisc-internal identifier (pointer, integer, whatever) which comes from either ->get or ->walk, and thus is valid unless these functions have bugs. Your check would cover the bug up and has no other purpose.
Maybe I miss something, but it's not "my check".

I don't care.

Quoted 1 line

I tried only to "stay in line with the remaining file"...

Keeping in line may make sense in case of coding style, but its no replacement for understanding the code you're copying around. The same check in prio_get is correct and necessary, while in the function you add it is incorrect and covers up potential bugs.

So please remove it so we can stop this pointless discussion.

Jarek Poplawski

author

2007-01-31 15:20 UTC #10 lore raw

PATCH 1/1 sch_prio: class statistics printing enabled

In reply to Patrick McHardy

(Take 3)

This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.
(With help of Patrick McHardy).

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

---

net/sched/sch_prio.c +15 -0

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
@@ -372,6 +372,20 @@ static int prio_dump_class(struct Qdisc 
 	return 0;
 }
 
+static int prio_dump_class_stats(struct Qdisc *sch, unsigned long cl,
+				 struct gnet_dump *d)
+{
+	struct prio_sched_data *q = qdisc_priv(sch);
+	struct Qdisc *cl_q;
+
+	cl_q = q->queues[cl - 1];
+	if (gnet_stats_copy_basic(d, &cl_q->bstats) < 0 ||
+	    gnet_stats_copy_queue(d, &cl_q->qstats) < 0)
+		return -1;
+
+	return 0;
+}
+
 static void prio_walk(struct Qdisc *sch, struct qdisc_walker *arg)
 {
 	struct prio_sched_data *q = qdisc_priv(sch);

@@ -414,6 +428,7 @@ static struct Qdisc_class_ops prio_class
 	.bind_tcf	=	prio_bind,
 	.unbind_tcf	=	prio_put,
 	.dump		=	prio_dump_class,
+	.dump_stats	=	prio_dump_class_stats,
 };
 
 static struct Qdisc_ops prio_qdisc_ops = {

Patrick McHardy

Acked-by on v1 1/1

2007-01-31 15:23 UTC #11 lore raw

Quoted 5 lines · Jarek Poplawski

This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.
(With help of Patrick McHardy).
Signed-off-by Jarek Poplawski <[email protected]>

Acked-by Patrick McHardy <[email protected]>

David Miller

maintainer Applied

2007-01-31 20:21 UTC #12 lore raw

From: Patrick McHardy <[email protected]>
Date: Wed, 31 Jan 2007 16:23:49 +0100

Quoted 8 lines

Quoted 5 lines · Jarek Poplawski
This patch adds a dump_stats callback to enable
printing of basic statistics of prio classes.
(With help of Patrick McHardy).
Signed-off-by Jarek Poplawski <[email protected]>
Acked-by Patrick McHardy <[email protected]>

Applied to net-2.6.21, thanks.