Re: [PATCH net-next V2] htb: improved accuracy at high rates

[Eric Dumazet <eric.dumazet@gmail.com>]

On Sat, 2012-10-20 at 09:38 -0700, Vimalkumar wrote:
> Current HTB (and TBF) uses rate table computed by the "tc"
> userspace program, which has the following issue:
> 
> The rate table has 256 entries to map packet lengths
> to token (time units).  With TSO sized packets, the
> 256 entry granularity leads to loss/gain of rate,
> making the token bucket inaccurate.
> 
> Thus, instead of relying on rate table, this patch
> explicitly computes the time and accounts for packet
> transmission times with nanosecond granularity.
> 
> This greatly improves accuracy of HTB with a wide
> range of packet sizes.
> 
> Example:
> 
> tc qdisc add dev $dev root handle 1: \
>         htb default 1
> 
> tc class add dev $dev classid 1:1 parent 1: \
>         rate 5Gbit mtu 64k
> 
> Here is an example of inaccuracy:
> 
> $ iperf -c host -t 10 -i 1
> 
> With old htb:
> eth4:   34.76 Mb/s In  5827.98 Mb/s Out -  65836.0 p/s In  481273.0 p/s Out
> [SUM]  9.0-10.0 sec   669 MBytes  5.61 Gbits/sec
> [SUM]  0.0-10.0 sec  6.50 GBytes  5.58 Gbits/sec
> 
> With new htb:
> eth4:   28.36 Mb/s In  5208.06 Mb/s Out -  53704.0 p/s In  430076.0 p/s Out
> [SUM]  9.0-10.0 sec   594 MBytes  4.98 Gbits/sec
> [SUM]  0.0-10.0 sec  5.80 GBytes  4.98 Gbits/sec
> 
> The bits per second on the wire is still 5200Mb/s with new HTB
> because qdisc accounts for packet length using skb->len, which
> is smaller than total bytes on the wire if GSO is used.  But
> that is for another patch regardless of how time is accounted.
> 
> Many thanks to Eric Dumazet for review and feedback.
> 
> Signed-off-by: Vimalkumar <j.vimal@gmail.com>
> ---
>  net/sched/sch_htb.c |  123 +++++++++++++++++++++++++++++++++------------------
>  1 files changed, 80 insertions(+), 43 deletions(-)
> 
> diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
> index 9d75b77..1f26d6b 100644
> --- a/net/sched/sch_htb.c
> +++ b/net/sched/sch_htb.c
> @@ -55,6 +55,7 @@
>  
>  static int htb_hysteresis __read_mostly = 0; /* whether to use mode hysteresis for speedup */
>  #define HTB_VER 0x30011		/* major must be matched with number suplied by TC as version */
> +#define HTB_MIN_PKT_BYTES (64)

This is not used in your patch.


is htb_precompute_ratedata() safe/correct for very low speeds, as 8000
bits/sec ?

If my maths are correct, there is an overflow.

factor = 8LLU * NSEC_PER_SEC * (1 << r->shift);

so factor = 262144000000000

r->mult = div64_u64(factor, r->rate_bps);

the result of the divide is 32768000000, and its bigger than an u32

So you should have a loop to reduce r->shift to make sure you dont have
an overflow for r->mult

> -	if (nla_put(skb, TCA_HTB_INIT, sizeof(gopt), &gopt))
> -		goto nla_put_failure;
> +	NLA_PUT(skb, TCA_HTB_INIT, sizeof(gopt), &gopt);
>  	nla_nest_end(skb, nest);
...

> -	if (nla_put(skb, TCA_HTB_PARMS, sizeof(opt), &opt))
> -		goto nla_put_failure;
> +	NLA_PUT(skb, TCA_HTB_PARMS, sizeof(opt), &opt);


Thats unfortunate, you apparently didnt compile your module and tested
it.

# make net/sched/sch_htb.ko
make[1]: Nothing to be done for `all'.
make[1]: Nothing to be done for `relocs'.
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CALL    scripts/checksyscalls.sh
  CC [M]  net/sched/sch_htb.o
net/sched/sch_htb.c: In function ‘htb_dump’:
net/sched/sch_htb.c:1092: error: implicit declaration of function
‘NLA_PUT’
make[1]: *** [net/sched/sch_htb.o] Error 1
make: *** [net/sched/sch_htb.ko] Error 2


You should understand this is an absolute requirement, or else people
are less likely to take a serious look at your work.

So triple check next time you submit.

Read again Documentation/SubmitChecklist

Take your time, so that other people dont loose their time.

Thanks

PS : Documentation/SubmittingPatches  
   10) Don't get discouraged.  Re-submit.