| Message ID | 1360428312-1277-9-git-send-email-jiri@resnulli.us |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: > q->ptokens is in ns and we are assigning q->mtu directly to it. That is > wrong. psched_l2t_ns() should be used to compute correct value. > Not clear why its a separate patch, and not folded in the 6th > Signed-off-by: Jiri Pirko <jiri@resnulli.us> > --- > net/sched/sch_tbf.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c > index dc562a8..6e8b670 100644 > --- a/net/sched/sch_tbf.c > +++ b/net/sched/sch_tbf.c > @@ -165,8 +165,8 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch) > > if (q->peak_present) { > ptoks = toks + q->ptokens; > - if (ptoks > (long)q->mtu) > - ptoks = q->mtu; > + if (ptoks > (s64) psched_l2t_ns(&q->peak, q->mtu)) > + ptoks = (s64) psched_l2t_ns(&q->peak, q->mtu); It seems a bit expensive to perform this in fast path. Please add a variable to cache psched_l2t_ns(&q->peak, q->mtu) > ptoks -= (s64) psched_l2t_ns(&q->peak, len); > } > toks += q->tokens; > @@ -215,7 +215,8 @@ static void tbf_reset(struct Qdisc *sch) > sch->q.qlen = 0; > q->t_c = ktime_to_ns(ktime_get()); > q->tokens = q->buffer; > - q->ptokens = q->mtu; > + if (q->peak_present) > + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); > qdisc_watchdog_cancel(&q->watchdog); > } > > @@ -296,11 +297,11 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) > q->max_size = max_size; > q->buffer = PSCHED_TICKS2NS(qopt->buffer); > q->tokens = q->buffer; > - q->ptokens = q->mtu; > > psched_ratecfg_precompute(&q->rate, rtab->rate.rate); > if (ptab) { > psched_ratecfg_precompute(&q->peak, ptab->rate.rate); > + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); Here probably. -- 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
Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote: >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is >> wrong. psched_l2t_ns() should be used to compute correct value. >> > > >Not clear why its a separate patch, and not folded in the 6th This is independent on 6th. Would be needed even if 6th wouldn't be there. > > >> Signed-off-by: Jiri Pirko <jiri@resnulli.us> >> --- >> net/sched/sch_tbf.c | 9 +++++---- >> 1 file changed, 5 insertions(+), 4 deletions(-) >> >> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c >> index dc562a8..6e8b670 100644 >> --- a/net/sched/sch_tbf.c >> +++ b/net/sched/sch_tbf.c >> @@ -165,8 +165,8 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch) >> >> if (q->peak_present) { >> ptoks = toks + q->ptokens; >> - if (ptoks > (long)q->mtu) >> - ptoks = q->mtu; >> + if (ptoks > (s64) psched_l2t_ns(&q->peak, q->mtu)) >> + ptoks = (s64) psched_l2t_ns(&q->peak, q->mtu); > >It seems a bit expensive to perform this in fast path. > >Please add a variable to cache psched_l2t_ns(&q->peak, q->mtu) Okay, I did not think that this is necessary, but sure, I will do that. Thanks! > > >> ptoks -= (s64) psched_l2t_ns(&q->peak, len); >> } >> toks += q->tokens; >> @@ -215,7 +215,8 @@ static void tbf_reset(struct Qdisc *sch) >> sch->q.qlen = 0; >> q->t_c = ktime_to_ns(ktime_get()); >> q->tokens = q->buffer; >> - q->ptokens = q->mtu; >> + if (q->peak_present) >> + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); >> qdisc_watchdog_cancel(&q->watchdog); >> } >> >> @@ -296,11 +297,11 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) >> q->max_size = max_size; >> q->buffer = PSCHED_TICKS2NS(qopt->buffer); >> q->tokens = q->buffer; >> - q->ptokens = q->mtu; >> >> psched_ratecfg_precompute(&q->rate, rtab->rate.rate); >> if (ptab) { >> psched_ratecfg_precompute(&q->peak, ptab->rate.rate); >> + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); > >Here probably. > > > -- 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 Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote: > Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote: > >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: > >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is > >> wrong. psched_l2t_ns() should be used to compute correct value. > >> > > > > > >Not clear why its a separate patch, and not folded in the 6th > > This is independent on 6th. Would be needed even if 6th wouldn't be > there. When was this bug introduced then ? -- 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
Sun, Feb 10, 2013 at 09:21:38AM CET, eric.dumazet@gmail.com wrote: >On Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote: >> Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote: >> >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: >> >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is >> >> wrong. psched_l2t_ns() should be used to compute correct value. >> >> >> > >> > >> >Not clear why its a separate patch, and not folded in the 6th >> >> This is independent on 6th. Would be needed even if 6th wouldn't be >> there. > >When was this bug introduced then ? This has been present since the beginning of git. I'm unable to find bk repo right now to tell you bk commit :/ > > -- 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 Sun, 2013-02-10 at 09:51 +0100, Jiri Pirko wrote: > Sun, Feb 10, 2013 at 09:21:38AM CET, eric.dumazet@gmail.com wrote: > >On Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote: > >> Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote: > >> >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: > >> >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is > >> >> wrong. psched_l2t_ns() should be used to compute correct value. > >> >> > >> > > >> > > >> >Not clear why its a separate patch, and not folded in the 6th > >> > >> This is independent on 6th. Would be needed even if 6th wouldn't be > >> there. > > > >When was this bug introduced then ? > > This has been present since the beginning of git. I'm unable to find bk > repo right now to tell you bk commit :/ Current code doesnt store ptokens in ns, your patch is very confusing. q->mtu is properly setup by tc command. Its not dev->mtu at all. I claim you fix a bug added in 6th patch. It makes no sense to me to introduce a bug in 6th and fix it in 8th. -- 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
Sun, Feb 10, 2013 at 07:00:08PM CET, eric.dumazet@gmail.com wrote: >On Sun, 2013-02-10 at 09:51 +0100, Jiri Pirko wrote: >> Sun, Feb 10, 2013 at 09:21:38AM CET, eric.dumazet@gmail.com wrote: >> >On Sun, 2013-02-10 at 09:18 +0100, Jiri Pirko wrote: >> >> Sun, Feb 10, 2013 at 02:30:46AM CET, eric.dumazet@gmail.com wrote: >> >> >On Sat, 2013-02-09 at 17:45 +0100, Jiri Pirko wrote: >> >> >> q->ptokens is in ns and we are assigning q->mtu directly to it. That is >> >> >> wrong. psched_l2t_ns() should be used to compute correct value. >> >> >> >> >> > >> >> > >> >> >Not clear why its a separate patch, and not folded in the 6th >> >> >> >> This is independent on 6th. Would be needed even if 6th wouldn't be >> >> there. >> > >> >When was this bug introduced then ? >> >> This has been present since the beginning of git. I'm unable to find bk >> repo right now to tell you bk commit :/ > >Current code doesnt store ptokens in ns, your patch is very confusing. > >q->mtu is properly setup by tc command. Now I get it. q->mtu is not bytes but time instead. I will fix my patches. > >Its not dev->mtu at all. > >I claim you fix a bug added in 6th patch. > >It makes no sense to me to introduce a bug in 6th and fix it in 8th. > > > -- 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
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c index dc562a8..6e8b670 100644 --- a/net/sched/sch_tbf.c +++ b/net/sched/sch_tbf.c @@ -165,8 +165,8 @@ static struct sk_buff *tbf_dequeue(struct Qdisc *sch) if (q->peak_present) { ptoks = toks + q->ptokens; - if (ptoks > (long)q->mtu) - ptoks = q->mtu; + if (ptoks > (s64) psched_l2t_ns(&q->peak, q->mtu)) + ptoks = (s64) psched_l2t_ns(&q->peak, q->mtu); ptoks -= (s64) psched_l2t_ns(&q->peak, len); } toks += q->tokens; @@ -215,7 +215,8 @@ static void tbf_reset(struct Qdisc *sch) sch->q.qlen = 0; q->t_c = ktime_to_ns(ktime_get()); q->tokens = q->buffer; - q->ptokens = q->mtu; + if (q->peak_present) + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); qdisc_watchdog_cancel(&q->watchdog); } @@ -296,11 +297,11 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) q->max_size = max_size; q->buffer = PSCHED_TICKS2NS(qopt->buffer); q->tokens = q->buffer; - q->ptokens = q->mtu; psched_ratecfg_precompute(&q->rate, rtab->rate.rate); if (ptab) { psched_ratecfg_precompute(&q->peak, ptab->rate.rate); + q->ptokens = (s64) psched_l2t_ns(&q->peak, q->mtu); q->peak_present = true; } else { q->peak_present = false;
q->ptokens is in ns and we are assigning q->mtu directly to it. That is wrong. psched_l2t_ns() should be used to compute correct value. Signed-off-by: Jiri Pirko <jiri@resnulli.us> --- net/sched/sch_tbf.c | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-)