Re: [RFC][PATCH] QoS TBF and latency configuration misbehavior

[Dan Kruchinin <dkruchinin@acm.org>]

Hello, Alexey.

On Wed, 1 Sep 2010 02:34:02 +0400
Alexey Kuznetsov <kuznet@ms2.inr.ac.ru> wrote:

> Hello!
> 
> Hmm. Seems, you are right and I was wrong all these years.
> Somehow, I did wrong calculation once and this rustied to the brains.
> After some thinking the calulation is obviously wrong: no matter what,
> in steady state tbf queue is processed with rate R. What a stupid mistake... :-)
> 
> Please, also think how to fix the second part of calculation which deals
> with peak rate. IMHO (for now :-)) it does not even contribute to latency
> and should be deleted.

I absolutely agree about latency option. As I understood from http://www.docum.org/docum.org/docs/other/tbf02_kw.ps
latency should affect only limit. It has not any sense in context of peakrate. So I think that limit calculation
by latency _and_ peakrate should be removed from tc code because limit is clearly determined by latency and rate.
So here is a patch that(I hope) fixes it(please correct me if I'm wrong):

diff --git a/tc/q_tbf.c b/tc/q_tbf.c
index dc556fe..643c1e0 100644
--- a/tc/q_tbf.c
+++ b/tc/q_tbf.c
@@ -178,12 +178,10 @@ static int tbf_parse_opt(struct qdisc_util *qu, int argc, char **argv, struct nl
        }
 
        if (opt.limit == 0) {
-               double lim = opt.rate.rate*(double)latency/TIME_UNITS_PER_SEC + buffer;
-               if (opt.peakrate.rate) {
-                       double lim2 = opt.peakrate.rate*(double)latency/TIME_UNITS_PER_SEC + mtu;
-                       if (lim2 < lim)
-                               lim = lim2;
-               }
+               double lim = opt.rate.rate*(double)latency/TIME_UNITS_PER_SEC;
+               if (opt.peakrate.rate && (lim < mtu))
+                       lim = mtu;
+
                opt.limit = lim;
        }
 
@@ -263,12 +261,7 @@ static int tbf_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
        if (show_raw)
                fprintf(f, "limit %s ", sprint_size(qopt->limit, b1));
 
-       latency = TIME_UNITS_PER_SEC*(qopt->limit/(double)qopt->rate.rate) - tc_core_tick2time(qopt->buffer);
-       if (qopt->peakrate.rate) {
-               double lat2 = TIME_UNITS_PER_SEC*(qopt->limit/(double)qopt->peakrate.rate) - tc_core_tick2time(qopt->mtu);
-               if (lat2 > latency)
-                       latency = lat2;
-       }
+       latency = TIME_UNITS_PER_SEC*(qopt->limit/(double)qopt->rate.rate);
        fprintf(f, "lat %s ", sprint_time(latency, b1));
 
        if (qopt->rate.overhead) {

> 
> To Jarek: about the scripts. I do not think something will be broken
> by fixing this error. Eventually, if someone used "latency", he meant
> something about real latency. And even if the value was generated
> using the same wrong logic as tc did, using correct formula would just
> increase limit setting.
> 
> Alexey



-- 
W.B.R.
Dan Kruchinin