[PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Jesper Dangaard Brouer]

commit 6fdc0f061be94f5e297650961360fb7a9d1cc85d
Author: Jesper Dangaard Brouer <hawk@comx.dk>
Date:   Thu Aug 30 17:53:42 2007 +0200

    [NET_SCHED]: Make all rate based scheduler work with TSO.
    
     Change L2T (length to time) macros, in all rate based schedulers, to
     call a common function qdisc_l2t() that does the rate table lookup.
     This function handles if the packet size lookup is larger than the
     rate table, which often occurs with TSO enabled.
    
     Note: bstats.packets is only incremented correctly for HTB.
     See: commit c9726d6890f7f3a892c879e067c3ed839f61e745.
    
    Signed-off-by: Jesper Dangaard Brouer <hawk@comx.dk>

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 8a67f24..4ebd615 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -302,4 +302,16 @@ drop:
 	return NET_XMIT_DROP;
 }
 
+/* Length to Time (L2T) lookup in a qdisc_rate_table, to determine how
+   long it will take to send a packet given its size.
+ */
+static inline u32 qdisc_l2t(struct qdisc_rate_table* rtab, unsigned int pktlen)
+{
+	int slot = pktlen;
+	slot >>= rtab->rate.cell_log;
+	if (slot > 255)
+		return (rtab->data[255]*(slot >> 8) + rtab->data[slot & 0xFF]);
+	return rtab->data[slot];
+}
+
 #endif
diff --git a/net/sched/act_police.c b/net/sched/act_police.c
index 6085be5..8599e47 100644
--- a/net/sched/act_police.c
+++ b/net/sched/act_police.c
@@ -21,8 +21,8 @@
 #include <net/act_api.h>
 #include <net/netlink.h>
 
-#define L2T(p,L)   ((p)->tcfp_R_tab->data[(L)>>(p)->tcfp_R_tab->rate.cell_log])
-#define L2T_P(p,L) ((p)->tcfp_P_tab->data[(L)>>(p)->tcfp_P_tab->rate.cell_log])
+#define L2T(p,L)   ((p)->tcfp_R_tab, L)
+#define L2T_P(p,L) ((p)->tcfp_P_tab, L)
 
 #define POL_TAB_MASK     15
 static struct tcf_common *tcf_police_ht[POL_TAB_MASK + 1];
diff --git a/net/sched/sch_cbq.c b/net/sched/sch_cbq.c
index e38c283..aed2af2 100644
--- a/net/sched/sch_cbq.c
+++ b/net/sched/sch_cbq.c
@@ -175,7 +175,7 @@ struct cbq_sched_data
 };
 
 
-#define L2T(cl,len)	((cl)->R_tab->data[(len)>>(cl)->R_tab->rate.cell_log])
+#define L2T(cl,len)	qdisc_l2t((cl)->R_tab,len)
 
 
 static __inline__ unsigned cbq_hash(u32 h)
diff --git a/net/sched/sch_htb.c b/net/sched/sch_htb.c
index 246a2f9..5e608a6 100644
--- a/net/sched/sch_htb.c
+++ b/net/sched/sch_htb.c
@@ -132,10 +132,8 @@ struct htb_class {
 static inline long L2T(struct htb_class *cl, struct qdisc_rate_table *rate,
 			   int size)
 {
-	int slot = size >> rate->rate.cell_log;
-	if (slot > 255)
-		return (rate->data[255]*(slot >> 8) + rate->data[slot & 0xFF]);
-	return rate->data[slot];
+	long result = qdisc_l2t(rate, size);
+	return result;
 }
 
 struct htb_sched {
diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
index 8c2639a..b0d8109 100644
--- a/net/sched/sch_tbf.c
+++ b/net/sched/sch_tbf.c
@@ -115,8 +115,8 @@ struct tbf_sched_data
 	struct qdisc_watchdog watchdog;	/* Watchdog timer */
 };
 
-#define L2T(q,L)   ((q)->R_tab->data[(L)>>(q)->R_tab->rate.cell_log])
-#define L2T_P(q,L) ((q)->P_tab->data[(L)>>(q)->P_tab->rate.cell_log])
+#define L2T(q,L)   qdisc_l2t((q)->R_tab,L)
+#define L2T_P(q,L) qdisc_l2t((q)->P_tab,L)
 
 static int tbf_enqueue(struct sk_buff *skb, struct Qdisc* sch)
 {

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Patrick McHardy]

Jesper Dangaard Brouer wrote:
> commit 6fdc0f061be94f5e297650961360fb7a9d1cc85d
> Author: Jesper Dangaard Brouer <hawk@comx.dk>
> Date:   Thu Aug 30 17:53:42 2007 +0200
> 
>     [NET_SCHED]: Make all rate based scheduler work with TSO.
>     
>      Change L2T (length to time) macros, in all rate based schedulers, to
>      call a common function qdisc_l2t() that does the rate table lookup.
>      This function handles if the packet size lookup is larger than the
>      rate table, which often occurs with TSO enabled.


It still won't work properly with TSO (TBF for example already drops
oversized packets during ->enqueue), but its a good cleanup anyway.

 > +#define L2T(p,L)   ((p)->tcfp_R_tab, L)
 > +#define L2T_P(p,L) ((p)->tcfp_P_tab, L)


I'd prefer to get rid of these L2T macros completely.

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Jesper Dangaard Brouer]

On Sat, 1 Sep 2007, Patrick McHardy wrote:

> Jesper Dangaard Brouer wrote:
>> commit 6fdc0f061be94f5e297650961360fb7a9d1cc85d
>> Author: Jesper Dangaard Brouer <hawk@comx.dk>
>> Date:   Thu Aug 30 17:53:42 2007 +0200
>> 
>> [NET_SCHED]: Make all rate based scheduler work with TSO.
>> 
>> Change L2T (length to time) macros, in all rate based schedulers, to
>> call a common function qdisc_l2t() that does the rate table lookup.
>> This function handles if the packet size lookup is larger than the
>> rate table, which often occurs with TSO enabled.
>
>
> It still won't work properly with TSO (TBF for example already drops
> oversized packets during ->enqueue), but its a good cleanup anyway.

Then lets call it a cleanup of the L2T macros.  In the next step we will 
fix the different schedulers, to use the ability to lookup larger sized 
packets. (I did notice the TBF scheduler would drop oversized packets).


>> +#define L2T(p,L)   ((p)->tcfp_R_tab, L)
>> +#define L2T_P(p,L) ((p)->tcfp_P_tab, L)
>
>
> I'd prefer to get rid of these L2T macros completely.

Lets take it one step at a time, but I agree.


Will you ack the patch as a cleanup?

Hilsen
   Jesper Brouer

--
-------------------------------------------------------------------
MSc. Master of Computer Science
Dept. of Computer Science, University of Copenhagen
Author of http://www.adsl-optimizer.dk
-------------------------------------------------------------------

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Bill Fink]

On Sat, 1 Sep 2007, Jesper Dangaard Brouer wrote:

> On Sat, 1 Sep 2007, Patrick McHardy wrote:
> 
> > Jesper Dangaard Brouer wrote:
> >> commit 6fdc0f061be94f5e297650961360fb7a9d1cc85d
> >> Author: Jesper Dangaard Brouer <hawk@comx.dk>
> >> Date:   Thu Aug 30 17:53:42 2007 +0200
> >> 
> >> [NET_SCHED]: Make all rate based scheduler work with TSO.
> >> 
> >> Change L2T (length to time) macros, in all rate based schedulers, to
> >> call a common function qdisc_l2t() that does the rate table lookup.
> >> This function handles if the packet size lookup is larger than the
> >> rate table, which often occurs with TSO enabled.
> >
> >
> > It still won't work properly with TSO (TBF for example already drops
> > oversized packets during ->enqueue), but its a good cleanup anyway.
> 
> Then lets call it a cleanup of the L2T macros.  In the next step we will 
> fix the different schedulers, to use the ability to lookup larger sized 
> packets. (I did notice the TBF scheduler would drop oversized packets).

Hmmm.  I guess this is also why TBF doesn't seem to work with 9000 byte
jumbo frames.

[root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000
[root@lang4 ~]# tc qdisc show                                                   qdisc pfifo_fast 0: dev eth0 root bands 3 priomap  1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1
qdisc tbf 8002: dev eth2 rate 2000Mbit burst 5000000b lat 4.3s

With 9000 byte jumbo frames:

[root@lang4 ~]# ./nuttcp-5.5.5 -w10m 192.168.88.14
    0.0000 MB /   5.00 sec =    0.0000 Mbps 0 %TX 0 %RX

But reducing the MSS to 1460 to emulate a standard 1500 byte Ethernet MTU:

[root@lang4 ~]# ./nuttcp-5.5.5 -M1460 -w10m 192.168.88.14
 2335.7048 MB /  10.05 sec = 1950.3419 Mbps 62 %TX 22 %RX

This is on a 2.6.20.7 kernel.

						-Bill

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Patrick McHardy]

Bill Fink wrote:
> On Sat, 1 Sep 2007, Jesper Dangaard Brouer wrote:
>
>>On Sat, 1 Sep 2007, Patrick McHardy wrote:
>>
>>>
>>>It still won't work properly with TSO (TBF for example already drops
>>>oversized packets during ->enqueue), but its a good cleanup anyway.
>>
>>Then lets call it a cleanup of the L2T macros.  In the next step we will 
>>fix the different schedulers, to use the ability to lookup larger sized 
>>packets. (I did notice the TBF scheduler would drop oversized packets).
> 
> 
> Hmmm.  I guess this is also why TBF doesn't seem to work with 9000 byte
> jumbo frames.
> 
> [root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000


Yes, you need to specify the MTU on the command line for
jumbo frames.

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Bill Fink]

On Tue, 04 Sep 2007, Patrick McHardy wrote:

> Bill Fink wrote:
> > On Sat, 1 Sep 2007, Jesper Dangaard Brouer wrote:
> >
> >>On Sat, 1 Sep 2007, Patrick McHardy wrote:
> >>>
> >>>It still won't work properly with TSO (TBF for example already drops
> >>>oversized packets during ->enqueue), but its a good cleanup anyway.
> >>
> >>Then lets call it a cleanup of the L2T macros.  In the next step we will 
> >>fix the different schedulers, to use the ability to lookup larger sized 
> >>packets. (I did notice the TBF scheduler would drop oversized packets).
> > 
> > Hmmm.  I guess this is also why TBF doesn't seem to work with 9000 byte
> > jumbo frames.
> > 
> > [root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000
> 
> Yes, you need to specify the MTU on the command line for
> jumbo frames.

Thanks!  Works much better now, although it does slightly exceed
the specified rate.

[root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000 mtu 9000

[root@lang4 ~]# ./nuttcp-5.5.5 -w10m 192.168.88.14
 2465.6729 MB /  10.08 sec = 2051.8241 Mbps 19 %TX 13 %RX

[root@lang4 ~]# ./nuttcp-5.5.5 -M1460 -w10m 192.168.88.14
 2785.5000 MB /  10.00 sec = 2335.6569 Mbps 100 %TX 26 %RX

						-Bill

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Jesper Dangaard Brouer]

On Tue, 2007-09-04 at 13:40 -0400, Bill Fink wrote:
> On Tue, 04 Sep 2007, Patrick McHardy wrote:
> 
> > Bill Fink wrote:
> > > On Sat, 1 Sep 2007, Jesper Dangaard Brouer wrote:
> > >
> > Yes, you need to specify the MTU on the command line for
> > jumbo frames.
> 
> Thanks!  Works much better now, although it does slightly exceed
> the specified rate.

Thats what happens, with the current rate table system, as we use the
lower boundry (when doing the packet to time lookups). Especially with a
high MTU, as the "resolution" of the rate table diminish (mpu=9000 gives
cell_log=6, 2^6=64 bytes "resolution" buckets).

> [root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000 mtu 9000
> 
> [root@lang4 ~]# ./nuttcp-5.5.5 -w10m 192.168.88.14
>  2465.6729 MB /  10.08 sec = 2051.8241 Mbps 19 %TX 13 %RX
> 
> [root@lang4 ~]# ./nuttcp-5.5.5 -M1460 -w10m 192.168.88.14
>  2785.5000 MB /  10.00 sec = 2335.6569 Mbps 100 %TX 26 %RX

BTW, thats a fast connection you are having! :-)

-- 
Med venlig hilsen / Best regards
  Jesper Brouer
  ComX Networks A/S
  Linux Network developer
  Cand. Scient Datalog / MSc.
  Author of http://adsl-optimizer.dk

Re: [PATCH 1/2]: [NET_SCHED]: Make all rate based scheduler work with TSO.

[Bill Fink]

On Wed, 05 Sep 2007, Jesper Dangaard Brouer wrote:

> On Tue, 2007-09-04 at 13:40 -0400, Bill Fink wrote:
> > On Tue, 04 Sep 2007, Patrick McHardy wrote:
> > 
> > > Bill Fink wrote:
> > > > On Sat, 1 Sep 2007, Jesper Dangaard Brouer wrote:
> > > >
> > > Yes, you need to specify the MTU on the command line for
> > > jumbo frames.
> > 
> > Thanks!  Works much better now, although it does slightly exceed
> > the specified rate.
> 
> Thats what happens, with the current rate table system, as we use the
> lower boundry (when doing the packet to time lookups). Especially with a
> high MTU, as the "resolution" of the rate table diminish (mpu=9000 gives
> cell_log=6, 2^6=64 bytes "resolution" buckets).
> 
> > [root@lang4 ~]# tc qdisc add dev eth2 root tbf rate 2gbit buffer 5000000 limit 18000 mtu 9000
> > 
> > [root@lang4 ~]# ./nuttcp-5.5.5 -w10m 192.168.88.14
> >  2465.6729 MB /  10.08 sec = 2051.8241 Mbps 19 %TX 13 %RX

That doesn't seem to account for the magnitude of the rate exceeding.
In the worst case (rough calculation):

	(1+64/9000)*2000 = 2014.2222 Mbps

Now if that were 256 rather than 64:

	(1+256/9000)*2000 = 2056.8888 Mbps

Or maybe the packet overhead is calculated wrong for the 9000 MTU case
(just wild speculation on my part).

						-Bill