* [PATCH net v5 0/2] net: sched: fix some issues
@ 2013-12-05 7:10 Yang Yingliang
2013-12-05 7:10 ` [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size Yang Yingliang
2013-12-05 7:10 ` [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum Yang Yingliang
0 siblings, 2 replies; 9+ messages in thread
From: Yang Yingliang @ 2013-12-05 7:10 UTC (permalink / raw)
To: davem, netdev; +Cc: eric.dumazet, brouer, jpirko, jbrouer
fix calculation of max_size in tbf
fix quantum calculation introduced by 64bit rates.
v2
patch 1/2: redescribe the regression.
patch 2/2: add Eric's ack.
v3
patch 1/2: use psched_l2t_ns to calculate max_size
and cleanup exit/done section suggested by Jesper.
v4
patch 1/2:
1.Update commit message suggested by Jesper.
2.Use a macro to replace 65535 constant.
3.Add condition that when peakrate is lower than rate, return -EINVAL.
4.Don't use cell_log anymore.
v5
patch 1/2:
1.Remove rtab and ptab use suggested by Eric.
2.Don't reduce max_size to 65536, as Eric suggested that
if a burst set to 200KB, we do not want tbf use 64KB
or even less. So add a helper psched_ns_t2l to calculate
max_size directly.
Yang Yingliang (2):
net: sched: tbf: fix calculation of max_size
net: sched: htb: fix calculation of quantum
include/net/sch_generic.h | 46 ++++++++++++++++++++++++++++
net/sched/sch_htb.c | 18 ++++++-----
net/sched/sch_tbf.c | 76 ++++++++++++++++++++---------------------------
3 files changed, 89 insertions(+), 51 deletions(-)
--
1.8.0
^ permalink raw reply [flat|nested] 9+ messages in thread* [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size 2013-12-05 7:10 [PATCH net v5 0/2] net: sched: fix some issues Yang Yingliang @ 2013-12-05 7:10 ` Yang Yingliang 2013-12-05 12:15 ` Eric Dumazet 2013-12-05 7:10 ` [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum Yang Yingliang 1 sibling, 1 reply; 9+ messages in thread From: Yang Yingliang @ 2013-12-05 7:10 UTC (permalink / raw) To: davem, netdev; +Cc: eric.dumazet, brouer, jpirko, jbrouer Current max_size is caluated from rate table. Now, the rate table has been replaced and it's wrong to caculate max_size based on this rate table. It can lead wrong calculation of max_size. The burst in kernel may be lower than user asked, because burst may gets some loss when transform it to buffer(E.g. "burst 40kb rate 30mbit/s") and it seems we cannot avoid this loss. Burst's value(max_size) based on rate table may be equal user asked. If a packet's length is max_size, this packet will be stalled in tbf_dequeue() because its length is above the burst in kernel so that it cannot get enough tokens. The max_size guards against enqueuing packet sizes above q->buffer "time" in tbf_enqueue(). To make consistent with the calculation of tokens, this patch add a helper psched_ns_t2l() to calculate burst(max_size) directly to fix this problem. After this fix, we can support to using 64bit rates to calculate burst as well. Suggested-by: Jesper Dangaard Brouer <jbrouer@redhat•com> Suggested-by: Eric Dumazet <edumazet@google•com> Signed-off-by: Yang Yingliang <yangyingliang@huawei•com> --- include/net/sch_generic.h | 46 ++++++++++++++++++++++++++++ net/sched/sch_tbf.c | 76 ++++++++++++++++++++--------------------------- 2 files changed, 79 insertions(+), 43 deletions(-) diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h index d0a6321..8da64f3 100644 --- a/include/net/sch_generic.h +++ b/include/net/sch_generic.h @@ -701,6 +701,52 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r, return ((u64)len * r->mult) >> r->shift; } +/* Time to Length, convert time in ns to length in bytes + * to determinate how many bytes can be sent in given time. + */ +static inline u64 psched_ns_t2l(const struct psched_ratecfg *r, + u64 time_in_ns) +{ + u64 len = time_in_ns; + u8 shift = r->shift; + bool is_div = false; + + /* The formula is : + * len = (time_in_ns << shift) / mult + * when time_in_ns does shift, it would overflow. + * If overflow happens first time, do division. + * Then do shift. If it happens again, + * set lenth to ~0ULL. + */ + while (shift) { + if (len & (1ULL << 63)) { + if (!is_div) { + len = div64_u64(len, r->mult); + is_div = true; + } else { + /* overflow happens */ + len = ~0ULL; + is_div = true; + break; + } + } + len <<= 1; + shift--; + } + if (!is_div) + len = div64_u64(len, r->mult); + + if (unlikely(r->linklayer == TC_LINKLAYER_ATM)) + len = (len / 53) * 48; + + if (len > r->overhead) + len -= r->overhead; + else + len = 0; + + return len; +} + void psched_ratecfg_precompute(struct psched_ratecfg *r, const struct tc_ratespec *conf, u64 rate64); diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c index a609005..2dd7cea 100644 --- a/net/sched/sch_tbf.c +++ b/net/sched/sch_tbf.c @@ -289,10 +289,8 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) struct tbf_sched_data *q = qdisc_priv(sch); struct nlattr *tb[TCA_TBF_MAX + 1]; struct tc_tbf_qopt *qopt; - struct qdisc_rate_table *rtab = NULL; - struct qdisc_rate_table *ptab = NULL; struct Qdisc *child = NULL; - int max_size, n; + u64 max_size; u64 rate64 = 0, prate64 = 0; err = nla_parse_nested(tb, TCA_TBF_MAX, opt, tbf_policy); @@ -304,38 +302,13 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) goto done; qopt = nla_data(tb[TCA_TBF_PARMS]); - rtab = qdisc_get_rtab(&qopt->rate, tb[TCA_TBF_RTAB]); - if (rtab == NULL) - goto done; - - if (qopt->peakrate.rate) { - if (qopt->peakrate.rate > qopt->rate.rate) - ptab = qdisc_get_rtab(&qopt->peakrate, tb[TCA_TBF_PTAB]); - if (ptab == NULL) - goto done; - } - - for (n = 0; n < 256; n++) - if (rtab->data[n] > qopt->buffer) - break; - max_size = (n << qopt->rate.cell_log) - 1; - if (ptab) { - int size; - - for (n = 0; n < 256; n++) - if (ptab->data[n] > qopt->mtu) - break; - size = (n << qopt->peakrate.cell_log) - 1; - if (size < max_size) - max_size = size; - } - if (max_size < 0) - goto done; + if (qopt->rate.linklayer == TC_LINKLAYER_UNAWARE) + qdisc_put_rtab(qdisc_get_rtab(&qopt->rate, + tb[TCA_TBF_RTAB])); - if (max_size < psched_mtu(qdisc_dev(sch))) - pr_warn_ratelimited("sch_tbf: burst %u is lower than device %s mtu (%u) !\n", - max_size, qdisc_dev(sch)->name, - psched_mtu(qdisc_dev(sch))); + if (qopt->peakrate.linklayer == TC_LINKLAYER_UNAWARE) + qdisc_put_rtab(qdisc_get_rtab(&qopt->peakrate, + tb[TCA_TBF_PTAB])); if (q->qdisc != &noop_qdisc) { err = fifo_set_limit(q->qdisc, qopt->limit); @@ -357,30 +330,47 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt) } q->limit = qopt->limit; q->mtu = PSCHED_TICKS2NS(qopt->mtu); - q->max_size = max_size; q->buffer = PSCHED_TICKS2NS(qopt->buffer); q->tokens = q->buffer; q->ptokens = q->mtu; if (tb[TCA_TBF_RATE64]) rate64 = nla_get_u64(tb[TCA_TBF_RATE64]); - psched_ratecfg_precompute(&q->rate, &rtab->rate, rate64); - if (ptab) { + psched_ratecfg_precompute(&q->rate, &qopt->rate, rate64); + if (!q->rate.rate_bytes_ps) + goto unlock_done; + + max_size = min_t(u64, psched_ns_t2l(&q->rate, q->buffer), ~0U); + + if (qopt->peakrate.rate) { if (tb[TCA_TBF_PRATE64]) prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]); - psched_ratecfg_precompute(&q->peak, &ptab->rate, prate64); + psched_ratecfg_precompute(&q->peak, &qopt->peakrate, prate64); + if (q->peak.rate_bytes_ps <= q->rate.rate_bytes_ps) { + pr_err("Peakrate must be higher than rate!\n"); + goto unlock_done; + } + + max_size = min_t(u64, max_size, psched_ns_t2l(&q->peak, q->mtu)); q->peak_present = true; } else { q->peak_present = false; } + if (max_size < psched_mtu(qdisc_dev(sch))) + pr_warn_ratelimited("sch_tbf: burst %llu is lower than device %s mtu (%u) !\n", + max_size, qdisc_dev(sch)->name, + psched_mtu(qdisc_dev(sch))); + + q->max_size = max_size; + + sch_tree_unlock(sch); + return 0; + +unlock_done: sch_tree_unlock(sch); - err = 0; + err = -EINVAL; done: - if (rtab) - qdisc_put_rtab(rtab); - if (ptab) - qdisc_put_rtab(ptab); return err; } -- 1.8.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size 2013-12-05 7:10 ` [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size Yang Yingliang @ 2013-12-05 12:15 ` Eric Dumazet 2013-12-06 2:24 ` Yang Yingliang 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2013-12-05 12:15 UTC (permalink / raw) To: Yang Yingliang; +Cc: davem, netdev, brouer, jpirko, jbrouer On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: > Current max_size is caluated from rate table. Now, the rate table > has been replaced and it's wrong to caculate max_size based on this > rate table. It can lead wrong calculation of max_size. > > The burst in kernel may be lower than user asked, because burst may gets > some loss when transform it to buffer(E.g. "burst 40kb rate 30mbit/s") > and it seems we cannot avoid this loss. Burst's value(max_size) based on > rate table may be equal user asked. If a packet's length is max_size, this > packet will be stalled in tbf_dequeue() because its length is above the > burst in kernel so that it cannot get enough tokens. The max_size guards > against enqueuing packet sizes above q->buffer "time" in tbf_enqueue(). > > To make consistent with the calculation of tokens, this patch add a helper > psched_ns_t2l() to calculate burst(max_size) directly to fix this problem. > > After this fix, we can support to using 64bit rates to calculate burst as well. > > Suggested-by: Jesper Dangaard Brouer <jbrouer@redhat•com> > Suggested-by: Eric Dumazet <edumazet@google•com> No, I did not suggest this patch. > Signed-off-by: Yang Yingliang <yangyingliang@huawei•com> > --- > include/net/sch_generic.h | 46 ++++++++++++++++++++++++++++ > net/sched/sch_tbf.c | 76 ++++++++++++++++++++--------------------------- > 2 files changed, 79 insertions(+), 43 deletions(-) > > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h > index d0a6321..8da64f3 100644 > --- a/include/net/sch_generic.h > +++ b/include/net/sch_generic.h > @@ -701,6 +701,52 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r, > return ((u64)len * r->mult) >> r->shift; > } > > +/* Time to Length, convert time in ns to length in bytes > + * to determinate how many bytes can be sent in given time. > + */ > +static inline u64 psched_ns_t2l(const struct psched_ratecfg *r, > + u64 time_in_ns) inline ? Really ? This is management path, there is no point inlining this. > +{ > + u64 len = time_in_ns; > + u8 shift = r->shift; > + bool is_div = false; > + > + /* The formula is : > + * len = (time_in_ns << shift) / mult > + * when time_in_ns does shift, it would overflow. > + * If overflow happens first time, do division. > + * Then do shift. If it happens again, > + * set lenth to ~0ULL. > + */ > + while (shift) { > + if (len & (1ULL << 63)) { > + if (!is_div) { > + len = div64_u64(len, r->mult); > + is_div = true; > + } else { > + /* overflow happens */ > + len = ~0ULL; > + is_div = true; > + break; > + } > + } > + len <<= 1; > + shift--; > + } > + if (!is_div) > + len = div64_u64(len, r->mult); Thats terrible. Given that the intended formula was : time_in_ns = (NSEC_PER_SEC * len) / rate_bps; This translates to following optimal C code u64 len = time_in_ns * r->rate_bytes_ps; do_div(len, NSEC_PER_SEC); Why do you use r->shift and r->mult which are optimized for the reverse operation in fast path (no divide), I do not know. > + max_size = min_t(u64, psched_ns_t2l(&q->rate, q->buffer), ~0U); > + > + if (qopt->peakrate.rate) { > if (tb[TCA_TBF_PRATE64]) > prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]); > - psched_ratecfg_precompute(&q->peak, &ptab->rate, prate64); > + psched_ratecfg_precompute(&q->peak, &qopt->peakrate, prate64); > + if (q->peak.rate_bytes_ps <= q->rate.rate_bytes_ps) { > + pr_err("Peakrate must be higher than rate!\n"); Do not add messages like that without rate limiting them. Plus there is no context, we know nothing. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size 2013-12-05 12:15 ` Eric Dumazet @ 2013-12-06 2:24 ` Yang Yingliang 2013-12-06 3:46 ` Eric Dumazet 0 siblings, 1 reply; 9+ messages in thread From: Yang Yingliang @ 2013-12-06 2:24 UTC (permalink / raw) To: Eric Dumazet; +Cc: davem, netdev, brouer, jpirko, jbrouer Hi, Eric Thanks for your reviewing and advice! On 2013/12/5 20:15, Eric Dumazet wrote: > On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: >> >> Suggested-by: Jesper Dangaard Brouer <jbrouer@redhat•com> >> Suggested-by: Eric Dumazet <edumazet@google•com> > > No, I did not suggest this patch. You suggested that if (qopt->rate.linklayer == TC_LINKLAYER_UNAWARE) qdisc_put_rtab(qdisc_get_rtab(&qopt->rate, tb[TCA_TBF_RTAB])); so I added your suggestion. > >> Signed-off-by: Yang Yingliang <yangyingliang@huawei•com> >> --- >> include/net/sch_generic.h | 46 ++++++++++++++++++++++++++++ >> net/sched/sch_tbf.c | 76 ++++++++++++++++++++--------------------------- >> 2 files changed, 79 insertions(+), 43 deletions(-) >> >> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h >> index d0a6321..8da64f3 100644 >> --- a/include/net/sch_generic.h >> +++ b/include/net/sch_generic.h >> @@ -701,6 +701,52 @@ static inline u64 psched_l2t_ns(const struct psched_ratecfg *r, >> return ((u64)len * r->mult) >> r->shift; >> } >> >> +/* Time to Length, convert time in ns to length in bytes >> + * to determinate how many bytes can be sent in given time. >> + */ >> +static inline u64 psched_ns_t2l(const struct psched_ratecfg *r, >> + u64 time_in_ns) > > inline ? > > Really ? > > This is management path, there is no point inlining this. > >> +{ >> + u64 len = time_in_ns; >> + u8 shift = r->shift; >> + bool is_div = false; >> + >> + /* The formula is : >> + * len = (time_in_ns << shift) / mult >> + * when time_in_ns does shift, it would overflow. >> + * If overflow happens first time, do division. >> + * Then do shift. If it happens again, >> + * set lenth to ~0ULL. >> + */ >> + while (shift) { >> + if (len & (1ULL << 63)) { >> + if (!is_div) { >> + len = div64_u64(len, r->mult); >> + is_div = true; >> + } else { >> + /* overflow happens */ >> + len = ~0ULL; >> + is_div = true; >> + break; >> + } >> + } >> + len <<= 1; >> + shift--; >> + } >> + if (!is_div) >> + len = div64_u64(len, r->mult); > > Thats terrible. > > Given that the intended formula was : > > time_in_ns = (NSEC_PER_SEC * len) / rate_bps; > > This translates to following optimal C code > > u64 len = time_in_ns * r->rate_bytes_ps; > do_div(len, NSEC_PER_SEC); > > Why do you use r->shift and r->mult which are optimized for the reverse > operation in fast path (no divide), I do not know. The formula to calculate tokens is : (len * r->mult) >> r->shift so I tried to use r->shift and r->mult to calculate the len. Your way looks better, I'll change it in v6. > >> + max_size = min_t(u64, psched_ns_t2l(&q->rate, q->buffer), ~0U); >> + >> + if (qopt->peakrate.rate) { >> if (tb[TCA_TBF_PRATE64]) >> prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]); >> - psched_ratecfg_precompute(&q->peak, &ptab->rate, prate64); >> + psched_ratecfg_precompute(&q->peak, &qopt->peakrate, prate64); >> + if (q->peak.rate_bytes_ps <= q->rate.rate_bytes_ps) { >> + pr_err("Peakrate must be higher than rate!\n"); > > Do not add messages like that without rate limiting them. > > Plus there is no context, we know nothing. OK, I'll use pr_warn_ratelimited instead. Regards, Yang ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size 2013-12-06 2:24 ` Yang Yingliang @ 2013-12-06 3:46 ` Eric Dumazet 2013-12-06 3:57 ` Yang Yingliang 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2013-12-06 3:46 UTC (permalink / raw) To: Yang Yingliang; +Cc: davem, netdev, brouer, jpirko, jbrouer On Fri, 2013-12-06 at 10:24 +0800, Yang Yingliang wrote: > Hi, Eric > Thanks for your reviewing and advice! > > On 2013/12/5 20:15, Eric Dumazet wrote: > > On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: > >> > >> Suggested-by: Jesper Dangaard Brouer <jbrouer@redhat•com> > >> Suggested-by: Eric Dumazet <edumazet@google•com> > > > > No, I did not suggest this patch. > > You suggested that > > if (qopt->rate.linklayer == TC_LINKLAYER_UNAWARE) > qdisc_put_rtab(qdisc_get_rtab(&qopt->rate, > tb[TCA_TBF_RTAB])); > > so I added your suggestion. OK : The exact meaning of 'Suggested-by: XX' is : 1) the patch idea was given by XX. 2) Then you implemented it. This is not really the case here : You had the idea of this patch. I made some feedbacks, you have no special tag to add for that. Later, if I agree with your next patch set I'll add my own "Acked-by: ..." If not (for example I am slow to respond and it got merged), its not a big deal. Thanks ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size 2013-12-06 3:46 ` Eric Dumazet @ 2013-12-06 3:57 ` Yang Yingliang 0 siblings, 0 replies; 9+ messages in thread From: Yang Yingliang @ 2013-12-06 3:57 UTC (permalink / raw) To: Eric Dumazet; +Cc: davem, netdev, brouer, jpirko, jbrouer On 2013/12/6 11:46, Eric Dumazet wrote: > On Fri, 2013-12-06 at 10:24 +0800, Yang Yingliang wrote: >> Hi, Eric >> Thanks for your reviewing and advice! >> >> On 2013/12/5 20:15, Eric Dumazet wrote: >>> On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: >>>> >>>> Suggested-by: Jesper Dangaard Brouer <jbrouer@redhat•com> >>>> Suggested-by: Eric Dumazet <edumazet@google•com> >>> >>> No, I did not suggest this patch. >> >> You suggested that >> >> if (qopt->rate.linklayer == TC_LINKLAYER_UNAWARE) >> qdisc_put_rtab(qdisc_get_rtab(&qopt->rate, >> tb[TCA_TBF_RTAB])); >> >> so I added your suggestion. > > OK : The exact meaning of 'Suggested-by: XX' is : > > 1) the patch idea was given by XX. > 2) Then you implemented it. > > This is not really the case here : You had the idea of this patch. > > I made some feedbacks, you have no special tag to add for that. > > Later, if I agree with your next patch set I'll add my own > "Acked-by: ..." > > If not (for example I am slow to respond and it got merged), its not a > big deal. > > Thanks > Got it. Thanks for your explanation! > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum 2013-12-05 7:10 [PATCH net v5 0/2] net: sched: fix some issues Yang Yingliang 2013-12-05 7:10 ` [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size Yang Yingliang @ 2013-12-05 7:10 ` Yang Yingliang 2013-12-05 12:18 ` Eric Dumazet 1 sibling, 1 reply; 9+ messages in thread From: Yang Yingliang @ 2013-12-05 7:10 UTC (permalink / raw) To: davem, netdev; +Cc: eric.dumazet, brouer, jpirko, jbrouer Now, 32bit rates may be not the true rate. So use rate_bytes_ps which is from max(rate32, rate64) to calcualte quantum. Signed-off-by: Yang Yingliang <yangyingliang@huawei•com> Acked-by: Eric Dumazet <edumazet@google•com> --- net/sched/sch_htb.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c index 0e1e38b..57c6678 100644 --- a/net/sched/sch_htb.c +++ b/net/sched/sch_htb.c @@ -1477,11 +1477,20 @@ static int htb_change_class(struct Qdisc *sch, u32 classid, sch_tree_lock(sch); } + rate64 = tb[TCA_HTB_RATE64] ? nla_get_u64(tb[TCA_HTB_RATE64]) : 0; + + ceil64 = tb[TCA_HTB_CEIL64] ? nla_get_u64(tb[TCA_HTB_CEIL64]) : 0; + + psched_ratecfg_precompute(&cl->rate, &hopt->rate, rate64); + psched_ratecfg_precompute(&cl->ceil, &hopt->ceil, ceil64); + /* it used to be a nasty bug here, we have to check that node * is really leaf before changing cl->un.leaf ! */ if (!cl->level) { - cl->quantum = hopt->rate.rate / q->rate2quantum; + u64 quantum = div64_u64(cl->rate.rate_bytes_ps, + q->rate2quantum); + cl->quantum = min_t(u64, quantum, INT_MAX); if (!hopt->quantum && cl->quantum < 1000) { pr_warning( "HTB: quantum of class %X is small. Consider r2q change.\n", @@ -1500,13 +1509,6 @@ static int htb_change_class(struct Qdisc *sch, u32 classid, cl->prio = TC_HTB_NUMPRIO - 1; } - rate64 = tb[TCA_HTB_RATE64] ? nla_get_u64(tb[TCA_HTB_RATE64]) : 0; - - ceil64 = tb[TCA_HTB_CEIL64] ? nla_get_u64(tb[TCA_HTB_CEIL64]) : 0; - - psched_ratecfg_precompute(&cl->rate, &hopt->rate, rate64); - psched_ratecfg_precompute(&cl->ceil, &hopt->ceil, ceil64); - cl->buffer = PSCHED_TICKS2NS(hopt->buffer); cl->cbuffer = PSCHED_TICKS2NS(hopt->cbuffer); -- 1.8.0 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum 2013-12-05 7:10 ` [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum Yang Yingliang @ 2013-12-05 12:18 ` Eric Dumazet 2013-12-06 2:36 ` Yang Yingliang 0 siblings, 1 reply; 9+ messages in thread From: Eric Dumazet @ 2013-12-05 12:18 UTC (permalink / raw) To: Yang Yingliang; +Cc: davem, netdev, brouer, jpirko, jbrouer On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: > Now, 32bit rates may be not the true rate. > So use rate_bytes_ps which is from > max(rate32, rate64) to calcualte quantum. > > Signed-off-by: Yang Yingliang <yangyingliang@huawei•com> > Acked-by: Eric Dumazet <edumazet@google•com> > --- > net/sched/sch_htb.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c > index 0e1e38b..57c6678 100644 > --- a/net/sched/sch_htb.c > +++ b/net/sched/sch_htb.c > @@ -1477,11 +1477,20 @@ static int htb_change_class(struct Qdisc *sch, u32 classid, > sch_tree_lock(sch); > } > > + rate64 = tb[TCA_HTB_RATE64] ? nla_get_u64(tb[TCA_HTB_RATE64]) : 0; > + > + ceil64 = tb[TCA_HTB_CEIL64] ? nla_get_u64(tb[TCA_HTB_CEIL64]) : 0; > + > + psched_ratecfg_precompute(&cl->rate, &hopt->rate, rate64); > + psched_ratecfg_precompute(&cl->ceil, &hopt->ceil, ceil64); > + > /* it used to be a nasty bug here, we have to check that node > * is really leaf before changing cl->un.leaf ! > */ > if (!cl->level) { > - cl->quantum = hopt->rate.rate / q->rate2quantum; > + u64 quantum = div64_u64(cl->rate.rate_bytes_ps, > + q->rate2quantum); q->rate2quantum being 32bit, prefer the following : u64 quantum = cl->rate.rate_bytes_ps; do_div(quantum, q->rate2quantum); Since it maps to better assembly code on 32bit arches. > + cl->quantum = min_t(u64, quantum, INT_MAX); > if (!hopt->quantum && cl->quantum < 1000) { > pr_warning( > "HTB: quantum of class %X is small. Consider r2q change.\n", > @@ -1500,13 +1509,6 @@ static int htb_change_class(struct Qdisc *sch, u32 classid, > cl->prio = TC_HTB_NUMPRIO - 1; > } > > - rate64 = tb[TCA_HTB_RATE64] ? nla_get_u64(tb[TCA_HTB_RATE64]) : 0; > - > - ceil64 = tb[TCA_HTB_CEIL64] ? nla_get_u64(tb[TCA_HTB_CEIL64]) : 0; > - > - psched_ratecfg_precompute(&cl->rate, &hopt->rate, rate64); > - psched_ratecfg_precompute(&cl->ceil, &hopt->ceil, ceil64); > - > cl->buffer = PSCHED_TICKS2NS(hopt->buffer); > cl->cbuffer = PSCHED_TICKS2NS(hopt->cbuffer); > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum 2013-12-05 12:18 ` Eric Dumazet @ 2013-12-06 2:36 ` Yang Yingliang 0 siblings, 0 replies; 9+ messages in thread From: Yang Yingliang @ 2013-12-06 2:36 UTC (permalink / raw) To: Eric Dumazet; +Cc: davem, netdev, brouer, jpirko, jbrouer On 2013/12/5 20:18, Eric Dumazet wrote: > On Thu, 2013-12-05 at 15:10 +0800, Yang Yingliang wrote: >> if (!cl->level) { >> - cl->quantum = hopt->rate.rate / q->rate2quantum; >> + u64 quantum = div64_u64(cl->rate.rate_bytes_ps, >> + q->rate2quantum); > > q->rate2quantum being 32bit, prefer the following : > > u64 quantum = cl->rate.rate_bytes_ps; > > do_div(quantum, q->rate2quantum); > > Since it maps to better assembly code on 32bit arches. Change it in v6, thanks! Regards, Yang ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-06 3:59 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-12-05 7:10 [PATCH net v5 0/2] net: sched: fix some issues Yang Yingliang 2013-12-05 7:10 ` [PATCH net v5 1/2] net: sched: tbf: fix calculation of max_size Yang Yingliang 2013-12-05 12:15 ` Eric Dumazet 2013-12-06 2:24 ` Yang Yingliang 2013-12-06 3:46 ` Eric Dumazet 2013-12-06 3:57 ` Yang Yingliang 2013-12-05 7:10 ` [PATCH net v5 2/2] net: sched: htb: fix calculation of quantum Yang Yingliang 2013-12-05 12:18 ` Eric Dumazet 2013-12-06 2:36 ` Yang Yingliang
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox