* [PATCH] TBF: stop qdisc infanticide
@ 2010-05-13 16:17 Stephen Hemminger
2010-05-13 16:22 ` Patrick McHardy
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2010-05-13 16:17 UTC (permalink / raw)
To: Patrick McHardy, David Miller; +Cc: netdev
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;
+ } else if (qopt->limit > 0) {
child = fifo_create_dflt(sch, &bfifo_qdisc_ops, qopt->limit);
if (IS_ERR(child)) {
err = PTR_ERR(child);
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TBF: stop qdisc infanticide
2010-05-13 16:17 [PATCH] TBF: stop qdisc infanticide Stephen Hemminger
@ 2010-05-13 16:22 ` Patrick McHardy
2010-05-13 16:27 ` Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Patrick McHardy @ 2010-05-13 16:22 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
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);
>
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TBF: stop qdisc infanticide
2010-05-13 16:22 ` Patrick McHardy
@ 2010-05-13 16:27 ` Stephen Hemminger
2010-05-13 16:30 ` Patrick McHardy
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2010-05-13 16:27 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] TBF: stop qdisc infanticide
2010-05-13 16:27 ` Stephen Hemminger
@ 2010-05-13 16:30 ` Patrick McHardy
2010-05-15 0:38 ` [PATCH] tbf: stop wanton destruction of children (v2) Stephen Hemminger
0 siblings, 1 reply; 5+ messages in thread
From: Patrick McHardy @ 2010-05-13 16:30 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: David Miller, netdev
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.
^ permalink raw reply [flat|nested] 5+ messages in thread* [PATCH] tbf: stop wanton destruction of children (v2)
2010-05-13 16:30 ` Patrick McHardy
@ 2010-05-15 0:38 ` Stephen Hemminger
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Hemminger @ 2010-05-15 0:38 UTC (permalink / raw)
To: Patrick McHardy; +Cc: David Miller, netdev
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@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
@@ -273,7 +273,11 @@ 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);
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2010-05-15 0:39 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-05-13 16:17 [PATCH] TBF: stop qdisc infanticide Stephen Hemminger
2010-05-13 16:22 ` Patrick McHardy
2010-05-13 16:27 ` Stephen Hemminger
2010-05-13 16:30 ` Patrick McHardy
2010-05-15 0:38 ` [PATCH] tbf: stop wanton destruction of children (v2) Stephen Hemminger
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).