[PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

Hello,

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

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
--- linux-2.6.20-rc6-/net/sched/sch_prio.c	2007-01-08 20:23:58.000000000 +0100
+++ linux-2.6.20-rc6/net/sched/sch_prio.c	2007-01-30 20:26:31.000000000 +0100
@@ -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 = {

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
> 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
> --- linux-2.6.20-rc6-/net/sched/sch_prio.c	2007-01-08 20:23:58.000000000 +0100
> +++ linux-2.6.20-rc6/net/sched/sch_prio.c	2007-01-30 20:26:31.000000000 +0100
> @@ -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)

Please align with struct Qdisc.

> +{
> +	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.

> +
> +	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.

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

On Wed, Jan 31, 2007 at 02:47:55PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > 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
> > --- linux-2.6.20-rc6-/net/sched/sch_prio.c	2007-01-08 20:23:58.000000000 +0100
> > +++ linux-2.6.20-rc6/net/sched/sch_prio.c	2007-01-30 20:26:31.000000000 +0100
> > @@ -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)
> 
> 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.

> 
> > +{
> > +	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.
 
> > +
> > +	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.

[PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

(Take 2)

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

Signed-off-by: Jarek Poplawski <jarkao2@o2.pl>

---

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
--- linux-2.6.20-rc6-/net/sched/sch_prio.c	2007-01-08 20:23:58.000000000 +0100
+++ linux-2.6.20-rc6/net/sched/sch_prio.c	2007-01-31 15:10:27.000000000 +0100
@@ -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 = {

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Jan 31, 2007 at 02:47:55PM +0100, Patrick McHardy wrote:
> 
>>>+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.

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
> +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).

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

On Wed, Jan 31, 2007 at 03:37:25PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> > +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...

>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.

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
>>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...


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.

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

On Wed, Jan 31, 2007 at 04:06:05PM +0100, Patrick McHardy wrote:
> Jarek Poplawski wrote:
> >>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...
> 
> 
> 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.

[PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Jarek Poplawski]

(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 <jarkao2@o2.pl>

---

diff -Nurp linux-2.6.20-rc6-/net/sched/sch_prio.c linux-2.6.20-rc6/net/sched/sch_prio.c
--- linux-2.6.20-rc6-/net/sched/sch_prio.c	2007-01-08 20:23:58.000000000 +0100
+++ linux-2.6.20-rc6/net/sched/sch_prio.c	2007-01-31 16:08:20.000000000 +0100
@@ -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 = {

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
> On Wed, Jan 31, 2007 at 04:06:05PM +0100, Patrick McHardy wrote:
> 
>>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.

> 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.

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[Patrick McHardy]

Jarek Poplawski wrote:
> 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 <jarkao2@o2.pl>

Acked-by: Patrick McHardy <kaber@trash.net>

Re: [PATCH][NET_SCHED] sch_prio: class statistics printing enabled

[David Miller]

From: Patrick McHardy <kaber@trash.net>
Date: Wed, 31 Jan 2007 16:23:49 +0100

> Jarek Poplawski wrote:
> > 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 <jarkao2@o2.pl>
> 
> Acked-by: Patrick McHardy <kaber@trash.net>

Applied to net-2.6.21, thanks.