Stephen Hemminger | 13 May 18:17 2010

[PATCH] TBF: stop qdisc infanticide

Several netem users have complained that when using TBF for rate control
that any change to TBF parameters destroys the child qdisc. A typical
use is to have a test that sets up netem + TBF then changes bandwidth
setting.  But every time the parameters of TBF are changed it destroys
the child qdisc, requiring reconfiguration. Other qdisc's like HTB
don't do this.

Signed-off-by: Stephen Hemminger <shemminger <at> vyatta.com>

--- a/net/sched/sch_tbf.c	2010-05-12 20:41:06.257006386 -0700
+++ b/net/sched/sch_tbf.c	2010-05-12 20:52:35.671216316 -0700
 <at>  <at>  -273,7 +273,11  <at>  <at>  static int tbf_change(struct Qdisc* sch,
 	if (max_size < 0)
 		goto done;

-	if (qopt->limit > 0) {
+	if (q->qdisc) {
+		err = fifo_set_limit(q->qdisc, qopt->limit);
+		if (err)
+			goto done;
+	} else if (qopt->limit > 0) {
 		child = fifo_create_dflt(sch, &bfifo_qdisc_ops, qopt->limit);
 		if (IS_ERR(child)) {
 			err = PTR_ERR(child);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

(Continue reading)

Patrick McHardy | 13 May 18:22 2010
Picon

Re: [PATCH] TBF: stop qdisc infanticide

Stephen Hemminger wrote:
> Several netem users have complained that when using TBF for rate control
> that any change to TBF parameters destroys the child qdisc. A typical
> use is to have a test that sets up netem + TBF then changes bandwidth
> setting.  But every time the parameters of TBF are changed it destroys
> the child qdisc, requiring reconfiguration. Other qdisc's like HTB
> don't do this.
> 
> Signed-off-by: Stephen Hemminger <shemminger <at> vyatta.com>
> 
> 
> --- a/net/sched/sch_tbf.c	2010-05-12 20:41:06.257006386 -0700
> +++ b/net/sched/sch_tbf.c	2010-05-12 20:52:35.671216316 -0700
>  <at>  <at>  -273,7 +273,11  <at>  <at>  static int tbf_change(struct Qdisc* sch,
>  	if (max_size < 0)
>  		goto done;
>  
> -	if (qopt->limit > 0) {
> +	if (q->qdisc) {
> +		err = fifo_set_limit(q->qdisc, qopt->limit);
> +		if (err)
> +			goto done;

q->qdisc is never NULL since a noop_qdisc is assigned by default. Also
this should check that the child is in fact one of the *fifos.

> +	} else if (qopt->limit > 0) {
>  		child = fifo_create_dflt(sch, &bfifo_qdisc_ops, qopt->limit);
>  		if (IS_ERR(child)) {
>  			err = PTR_ERR(child);
(Continue reading)

Stephen Hemminger | 13 May 18:27 2010

Re: [PATCH] TBF: stop qdisc infanticide

On Thu, 13 May 2010 18:22:56 +0200
Patrick McHardy <kaber <at> trash.net> wrote:

> Stephen Hemminger wrote:
> > Several netem users have complained that when using TBF for rate control
> > that any change to TBF parameters destroys the child qdisc. A typical
> > use is to have a test that sets up netem + TBF then changes bandwidth
> > setting.  But every time the parameters of TBF are changed it destroys
> > the child qdisc, requiring reconfiguration. Other qdisc's like HTB
> > don't do this.
> > 
> > Signed-off-by: Stephen Hemminger <shemminger <at> vyatta.com>
> > 
> > 
> > --- a/net/sched/sch_tbf.c	2010-05-12 20:41:06.257006386 -0700
> > +++ b/net/sched/sch_tbf.c	2010-05-12 20:52:35.671216316 -0700
> >  <at>  <at>  -273,7 +273,11  <at>  <at>  static int tbf_change(struct Qdisc* sch,
> >  	if (max_size < 0)
> >  		goto done;
> >  
> > -	if (qopt->limit > 0) {
> > +	if (q->qdisc) {
> > +		err = fifo_set_limit(q->qdisc, qopt->limit);
> > +		if (err)
> > +			goto done;
> 
> q->qdisc is never NULL since a noop_qdisc is assigned by default. Also
> this should check that the child is in fact one of the *fifos.

But the child will be netem and fifo_set_limit ignores non-fifo.
(Continue reading)

Patrick McHardy | 13 May 18:30 2010
Picon

Re: [PATCH] TBF: stop qdisc infanticide

Stephen Hemminger wrote:
> On Thu, 13 May 2010 18:22:56 +0200
> Patrick McHardy <kaber <at> trash.net> wrote:
> 
>> Stephen Hemminger wrote:
>>> Several netem users have complained that when using TBF for rate control
>>> that any change to TBF parameters destroys the child qdisc. A typical
>>> use is to have a test that sets up netem + TBF then changes bandwidth
>>> setting.  But every time the parameters of TBF are changed it destroys
>>> the child qdisc, requiring reconfiguration. Other qdisc's like HTB
>>> don't do this.
>>>
>>> Signed-off-by: Stephen Hemminger <shemminger <at> vyatta.com>
>>>
>>>
>>> --- a/net/sched/sch_tbf.c	2010-05-12 20:41:06.257006386 -0700
>>> +++ b/net/sched/sch_tbf.c	2010-05-12 20:52:35.671216316 -0700
>>>  <at>  <at>  -273,7 +273,11  <at>  <at>  static int tbf_change(struct Qdisc* sch,
>>>  	if (max_size < 0)
>>>  		goto done;
>>>  
>>> -	if (qopt->limit > 0) {
>>> +	if (q->qdisc) {
>>> +		err = fifo_set_limit(q->qdisc, qopt->limit);
>>> +		if (err)
>>> +			goto done;
>> q->qdisc is never NULL since a noop_qdisc is assigned by default. Also
>> this should check that the child is in fact one of the *fifos.
> 
> But the child will be netem and fifo_set_limit ignores non-fifo.
(Continue reading)

Stephen Hemminger | 15 May 02:38 2010

[PATCH] tbf: stop wanton destruction of children (v2)

Several netem users use TBF for rate control. But every time the parameters
of TBF are changed it destroys the child qdisc, requiring reconfigation.
Better to just keep child qdisc and just notify it of changed limit.

Signed-off-by: Stephen Hemminger <shemminger <at> vyatta.com>

--- a/net/sched/sch_tbf.c	2010-05-14 15:04:56.297095729 -0700
+++ b/net/sched/sch_tbf.c	2010-05-14 15:09:57.296733332 -0700
 <at>  <at>  -273,7 +273,11  <at>  <at>  static int tbf_change(struct Qdisc* sch,
 	if (max_size < 0)
 		goto done;

-	if (qopt->limit > 0) {
+	if (q->qdisc != &noop_qdisc) {
+		err = fifo_set_limit(q->qdisc, qopt->limit);
+		if (err)
+			goto done;
+	} else if (qopt->limit > 0) {
 		child = fifo_create_dflt(sch, &bfifo_qdisc_ops, qopt->limit);
 		if (IS_ERR(child)) {
 			err = PTR_ERR(child);
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo <at> vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Gmane