qdisc bulk dequeuing and utilizing delayed tailptr updates

This patchset uses DaveM's recent API changes to dev_hard_start_xmit(), from the qdisc layer, to implement dequeue bulking.

RFC V3: Keeping the ball rolling.

This patchset should now use BQL correctly. I've done lots of testing for Head-of-Line blocking issues that can occur due to requeue of a SKB bulk list. I've not been able to provoke any HoL blocking situation, simply because BQL is doing such a good job, thus I'm unable to "overshoot" HW/BQL limits with more than a single packet.

This patch chooses a very conservative approach, as by default only allowing dequeue of one extra packet, besides the normal dequeue.

Open questions:

- How do we expose tuning to userspace?
Patch adds /proc/sys/net/core/qdisc_bulk_dequeue_limit but I don't like it... Per device tunable?

- Can/should we limit dequeue bulking to devices supporting BQL?

Quoted 11 lines · Jesper Dangaard Brouer

This patchset uses DaveM's recent API changes to dev_hard_start_xmit(), from the qdisc layer, to implement dequeue bulking.

RFC V3: Keeping the ball rolling.

This patchset should now use BQL correctly. I've done lots of testing for Head-of-Line blocking issues that can occur due to requeue of a SKB bulk list. I've not been able to provoke any HoL blocking situation, simply because BQL is doing such a good job, thus I'm unable to "overshoot" HW/BQL limits with more than a single packet.

Quoted 9 lines

This patch chooses a very conservative approach, as by default only allowing dequeue of one extra packet, besides the normal dequeue.

Open questions:

- How do we expose tuning to userspace?
Patch adds /proc/sys/net/core/qdisc_bulk_dequeue_limit but I don't like it... Per device tunable?

bql is using /sys, of course ;)

# grep . /sys/class/net/eth1/queues/tx-0/byte_queue_limits/*
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/hold_time:1000
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/inflight:0
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit:113314
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_max:1879048192
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_min:0

Maybe you could simply reuse byte_queue_limits/limit, I am not sure we need a specific tunable.

Quoted 2 lines

- Can/should we limit dequeue bulking to devices supporting BQL?

Yes please. This will be an incentive to get BQL on drivers.

Quoted 11 lines · Jesper Dangaard Brouer

This patchset uses DaveM's recent API changes to dev_hard_start_xmit(), from the qdisc layer, to implement dequeue bulking.

RFC V3: Keeping the ball rolling.

This patchset should now use BQL correctly. I've done lots of testing for Head-of-Line blocking issues that can occur due to requeue of a SKB bulk list. I've not been able to provoke any HoL blocking situation, simply because BQL is doing such a good job, thus I'm unable to "overshoot" HW/BQL limits with more than a single packet.

Quoted 9 lines

This patch chooses a very conservative approach, as by default only allowing dequeue of one extra packet, besides the normal dequeue.

Open questions:

- How do we expose tuning to userspace?
Patch adds /proc/sys/net/core/qdisc_bulk_dequeue_limit but I don't like it... Per device tunable?

bql is using /sys, of course ;)

# grep . /sys/class/net/eth1/queues/tx-0/byte_queue_limits/*
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/hold_time:1000
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/inflight:0
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit:113314
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_max:1879048192
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_min:0

Maybe you could simply reuse byte_queue_limits/limit, I am not sure we need a specific tunable.

Quoted 3 lines

- Can/should we limit dequeue bulking to devices supporting BQL?

Yes please. This will be an incentive to get BQL on drivers.

How can I test if the dev supports BQL?

Quoted 1 line · Jesper Dangaard Brouer

How can I test if the dev supports BQL?

CONFIG_BQL should be enough.

If a device driver do nothing about BQL, byte_queue_limits/limit will be zero by default.

Then why commits like this one?

commit 7070ce0a6419a118842298bc967061ad6cea40db

Author: Alexander Duyck <[email protected]>
Date:   Sat Sep 28 06:00:37 2013 +0000

i40e: Add support for Tx byte queue limits

Implement BQL (byte queue limit) support in i40e.

Quoted 4 lines

If a device driver do nothing about BQL, byte_queue_limits/limit will be zero by default.

Also note that for some virtual device, BQL may make no sense at all, at least veth doesn't even use a TX queue (I know it has tx-0 mostly for qdisc). Perhaps all NETIF_F_LLTX ones?

Based on DaveM's recent API work on dev_hard_start_xmit(), that allows sending/processing an entire skb list.

This patch implements qdisc bulk dequeue, by allowing multiple packets to be dequeued in dequeue_skb().

The optimization principle for this is two fold, (1) to amortize locking cost and (2) avoid expensive tailptr update for notifying HW.
(1) Several packets are dequeued while holding the qdisc root_lock,
amortizing locking cost over several packet. The dequeued SKB list is processed under the TXQ lock in dev_hard_start_xmit(), thus also amortizing the cost of the TXQ lock.
(2) Further more, dev_hard_start_xmit() will utilize the skb->xmit_more
API to delay HW tailptr update, which also reduces the cost per packet.

One restriction of the new API is that every SKB must belong to the same TXQ. This patch takes the easy way out, by restricting bulk dequeue to qdisc's with the TCQ_F_ONETXQUEUE flag, that specifies the qdisc only have attached a single TXQ.

Some detail about the flow; dev_hard_start_xmit() will process the skb list, and transmit packets individually towards the driver (see xmit_one()). In case the driver stops midway in the list, the remaining skb list is returned by dev_hard_start_xmit(). In sch_direct_xmit() this returned list is requeued by dev_requeue_skb().

To avoid overshooting the HW limits, which results in requeuing, the patch limits the amount of bytes dequeued, based on the drivers BQL limits.

It also see GSO and segmented GSO packets, as a seperate kind of SKB bulking, and thus tries to avoid and stop dequeuing when seeing a GSO packet (both real GSO and segmented GSO skb lists).

Choosing a very conservative initial default value of 1 extra packet dequeue, if bulking is allowed.

Signed-off-by Jesper Dangaard Brouer <[email protected]>

---
V2:
- Restruct functions, split out functionality
- Use BQL bytelimit to avoid overshooting driver limits

V3:
- Correct use of BQL
- Some minor adjustments based on feedback.
- Default setting only bulk dequeue 1 extra packet (2 packets).

 include/net/sch_generic.h  |    2 +
 net/core/sysctl_net_core.c |    9 ++++++
 net/sched/sch_generic.c    |   70 ++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 78 insertions(+), 3 deletions(-)

include/net/sch_generic.h +2 -0

index 1e89b9a..da9324f 100644
@@ -14,6 +14,8 @@ struct qdisc_walker;
 struct tcf_walker;
 struct module;
 
+extern int qdisc_bulk_dequeue_limit;
+
 struct qdisc_rate_table {
        struct tc_ratespec rate;
        u32             data[256];

net/core/sysctl_net_core.c +9 -0

index cf9cd13..5505841 100644
@@ -361,6 +361,15 @@ static struct ctl_table net_core_table[] = {
                .mode           = 0644,
                .proc_handler   = proc_dointvec
        },
+       {
+               .procname       = "qdisc_bulk_dequeue_limit",
+               .data           = &qdisc_bulk_dequeue_limit,
+               .maxlen         = sizeof(int),
+               .mode           = 0644,
+               .extra1         = &zero,
+               .extra2         = &ushort_max,
+               .proc_handler   = proc_dointvec_minmax
+       },
        { }
 };
 

net/sched/sch_generic.c +64 -0

index 346ef85..eee2280 100644
@@ -34,6 +34,9 @@
 const struct Qdisc_ops *default_qdisc_ops = &pfifo_fast_ops;
 EXPORT_SYMBOL(default_qdisc_ops);
 
+// FIXME: Still undecided where to put this parameter...
+int qdisc_bulk_dequeue_limit __read_mostly = 1;
+
 /* Main transmission queue. */
 
 /* Modifications to data participating in scheduling must be protected with

@@ -56,6 +59,67 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
        return 0;
 }
 
+static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
+{
+       return qdisc->flags & TCQ_F_ONETXQUEUE;
+}
+
+static inline struct sk_buff *qdisc_dequeue_validate(struct Qdisc *qdisc)
+{
+       struct sk_buff *skb = qdisc->dequeue(qdisc);
+
+       if (skb != NULL)
+               skb = validate_xmit_skb(skb, qdisc_dev(qdisc));
+
+       return skb;
+}
+
+static inline struct sk_buff *qdisc_bulk_dequeue_skb(struct Qdisc *q,
+                                                    struct sk_buff *head)
+{
+       struct sk_buff *new, *skb = head;
+       struct netdev_queue *txq = q->dev_queue;
+       int bytelimit = netdev_tx_avail_queue(txq);
+       int limit = qdisc_bulk_dequeue_limit;
+
+       bytelimit -= skb->len; /* incorrect len if skb->next, but exits below */
+
+       // if (bytelimit < psched_mtu(qdisc_dev(q)) //proposed by fw
+       if (bytelimit < 0)
+               return head;
+
+       do {
+               if (skb->next || skb_is_gso(skb)) {
+                       /* Stop processing if the skb is already a skb
+                        * list (e.g a segmented GSO packet, from
+                        * below or func caller) or a real GSO packet
+                        */
+                       break;
+               }
+               new = q->dequeue(q);
+               if (new) {
+                       bytelimit -= new->len; /* covers GSO len */
+                       new = validate_xmit_skb(new, qdisc_dev(q));
+                       if (!new)
+                               break;
+                       /* "new" can be a skb list after validate call
+                        * above (GSO segmented), but it is okay to
+                        * append it to current skb->next, because
+                        * next round will exit in-case "new" were a
+                        * skb list.
+                        */
+                       skb->next = new;
+                       skb = new;
+               }
+       } while (new && --limit && (bytelimit >= 0));
+       skb = head;
+
+       return skb;
+}
+
+/* Note that dequeue_skb can possibly return a SKB list (via skb->next).
+ * A requeued skb (via q->gso_skb) can also be a SKB list.
+ */
 static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 {
        struct sk_buff *skb = q->gso_skb;

net/sched/sch_generic.c +0 -0

        }
 

Quoted 11 lines · Jesper Dangaard Brouer

This patchset uses DaveM's recent API changes to dev_hard_start_xmit(), from the qdisc layer, to implement dequeue bulking.

RFC V3: Keeping the ball rolling.

This patchset should now use BQL correctly. I've done lots of testing for Head-of-Line blocking issues that can occur due to requeue of a SKB bulk list. I've not been able to provoke any HoL blocking situation, simply because BQL is doing such a good job, thus I'm unable to "overshoot" HW/BQL limits with more than a single packet.

Quoted 9 lines

This patch chooses a very conservative approach, as by default only allowing dequeue of one extra packet, besides the normal dequeue.

Open questions:

- How do we expose tuning to userspace?
Patch adds /proc/sys/net/core/qdisc_bulk_dequeue_limit but I don't like it... Per device tunable?

bql is using /sys, of course ;)

# grep . /sys/class/net/eth1/queues/tx-0/byte_queue_limits/*
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/hold_time:1000
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/inflight:0
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit:113314
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_max:1879048192
/sys/class/net/eth1/queues/tx-0/byte_queue_limits/limit_min:0

Maybe you could simply reuse byte_queue_limits/limit, I am not sure we need a specific tunable.

Quoted 2 lines

- Can/should we limit dequeue bulking to devices supporting BQL?

Yes please. This will be an incentive to get BQL on drivers.