| Message ID | 20100513091717.78bd7f1f@nehalam |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
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@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 > @@ -273,7 +273,11 @@ 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); > -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 13 May 2010 18:22:56 +0200 Patrick McHardy <kaber@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@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 > > @@ -273,7 +273,11 @@ 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Stephen Hemminger wrote: > On Thu, 13 May 2010 18:22:56 +0200 > Patrick McHardy <kaber@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@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 >>> @@ -273,7 +273,11 @@ 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. OK, but it does need to make sure the child is not a noop_qdisc, otherwise it won't create the default bfifo. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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 @@ -273,7 +273,11 @@ 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);
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@vyatta.com> -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html