qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE

To avoid overshooting the HW limits, which results in requeuing, the patch limits the amount of bytes dequeued, based on the drivers BQL limits. In-effect bulking will only happen for BQL enabled drivers. Besides the bytelimit from BQL, also limit bulking to maximum 7 packets to avoid any issues with available descriptor in HW and any HoL issues measured at 100Mbit/s.

The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66% less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).

Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
* Disabled bulking: 8.30% soft 88.75% idle
* Enabled bulking: 7.80% soft 89.36% idle

Note, there are benefits as you have shown - but i would not consider those to be standard use cases (actully one which would have shown clear win is the VM thing Rusty was after).

For this reason my view is that i should be able to disable via ifdef bulking (yes, I know DaveM hates ifdefs ;->).

From: Jamal Hadi Salim <[email protected]>
Date: Tue, 30 Sep 2014 07:07:37 -0400

Quoted 3 lines

Note, there are benefits as you have shown - but i would not consider those to be standard use cases (actully one which would have shown clear win is the VM thing Rusty was after).

I completely disagree, you will see at least decreased cpu utilization for a very common case, bulk single stream transfers.

Quoted 4 lines · David Miller

From: Jamal Hadi Salim <[email protected]>
Date: Tue, 30 Sep 2014 07:07:37 -0400

Quoted 8 lines

Quoted 3 lines

Note, there are benefits as you have shown - but i would not consider those to be standard use cases (actully one which would have shown clear win is the VM thing Rusty was after).

I completely disagree, you will see at least decreased cpu utilization for a very common case, bulk single stream transfers.

So lets say the common use case is:
= modern day cpu (pick some random cpu)
= 1-10 Gbps ethernet (not 100mbps)
= 1-24 tcp or udp bulk (you said one, Jesper had 24 which sounds better)

Run with test cases:
a) unchanged (no bulking code added at all)
vs
b) bulking code added and used
vs
c) bulking code added and *not* used

Jesper's results are comparing #b and #c.

And if #b + #c are slightly worse or equal then we have a win;->

Again, I do believe things like traffic generators or the VM io or something like tuntap that crosses user space will have a clear benefit (but are those common use cases?).

cheers,
jamal

Quoted 27 lines · Jamal Hadi Salim

Quoted 4 lines · David Miller

From: Jamal Hadi Salim <[email protected]>
Date: Tue, 30 Sep 2014 07:07:37 -0400

Quoted 8 lines

Quoted 3 lines

Note, there are benefits as you have shown - but i would not consider those to be standard use cases (actully one which would have shown clear win is the VM thing Rusty was after).

I completely disagree, you will see at least decreased cpu utilization for a very common case, bulk single stream transfers.

So lets say the common use case is:
= modern day cpu (pick some random cpu)
= 1-10 Gbps ethernet (not 100mbps)
= 1-24 tcp or udp bulk (you said one, Jesper had 24 which sounds better)

Run with test cases:
a) unchanged (no bulking code added at all)
vs
b) bulking code added and used
vs
c) bulking code added and *not* used

Quoted 4 lines

Jesper's results are comparing #b and #c.

And if #b + #c are slightly worse or equal then we have a win;->

Quoted 4 lines

Again, I do believe things like traffic generators or the VM io or something like tuntap that crosses user space will have a clear benefit (but are those common use cases?).

You're making this much more complicated that it actually is. The algorithm is simple-- queue wakes up, finds out how exactly many bytes to dequeue, and performs dequeue of enough packets under one lock.

The
should be a benefit when transmitting high rate as we know that reducing locking is generally a win.

The issue is: if i am going to attempt to do a bulk transfer every single time (with new code) and for the common use case the result is "no need to do bulking" then you just added extra code that is unnecessary for that common case.
Even a single extra if statement at high packet rate is still costly and would be easy to observe.

That is if the added code ends up being hit meaningfully. Jesper said (and it was my experience as well) that it was _hard_ to achieve bulking in such a case.

Thus, code is activated only when q->qlen is >= 1. And I have already shown that we see a win with just bulking 2 packets:

Quoted 2 lines · Jesper Dangaard Brouer

Thus, code is activated only when q->qlen is >= 1. And I have already shown that we see a win with just bulking 2 packets:

If you can get 2 packets, indeed you win. If you can on average get >1 over a long period, you still win.
You have clearly demonstrated you can do that with traffic generators (udp or in kernel pktgen). I was more worried about the common use case scenario (handwaved as 1-24 TCP streams).

The key here is: *if you never hit bulking* then the cost is _per packet_ for sch_direct_xmit bypass.
Question is what is that cost for the common case as defined above? Can you hit a bulk level >1 on 1-24 TCP streams?
I would be happy if your answer is *yes*.

If your answer is no (since
it is hard to achieve) - then how far off is it from before your patches (since now you have added at minimal a branch check). I think it is fair for you to quantify that, no? Feature is still useful for the other cases.

Note:
This is what i referred to as the "no animals were hurt during the making of these patches" statement. I am sorry again for raining on the parade.

Answer is yes. It is very easy with simple netperf TCP_STREAM to cause queueing >1 packet in the qdisc layer.

If tuned (according to my blog,
unloading netfilter etc.) then a single netperf TCP_STREAM will max out 10Gbit/s and cause a standing queue.

I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never saw a standing queue of 1 packet in my testing. Either the backlog area is high 100-200 packets, or 0 backlog. (With fake pktgen/trafgen style tests, it's possible to cause 1000 backlog).

Quoted 3 lines · Jesper Dangaard Brouer

Answer is yes. It is very easy with simple netperf TCP_STREAM to cause queueing >1 packet in the qdisc layer.

If that is the case, I withdraw any doubts i had. Can you please specify this in your commit logs for patch 0?

Quoted 4 lines

If tuned (according to my blog,
unloading netfilter etc.) then a single netperf TCP_STREAM will max out 10Gbit/s and cause a standing queue.

You should describe such tuning in the patch log (hard to read blogs for more than 30 seconds; write a paper if you want to provide more details).

Quoted 4 lines

I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never saw a standing queue of 1 packet in my testing. Either the backlog area is high 100-200 packets, or 0 backlog. (With fake pktgen/trafgen style tests, it's possible to cause 1000 backlog).

It would be nice to actually collect such stats. Monitoring the backlog via dumping qdisc stats is a good start - but actually keeping traces of average bulk size is more useful.

Quoted 8 lines

Quoted 4 lines

I'm monitoring backlog of qdiscs, and I always see >1 backlog, I never saw a standing queue of 1 packet in my testing. Either the backlog area is high 100-200 packets, or 0 backlog. (With fake pktgen/trafgen style tests, it's possible to cause 1000 backlog).

It would be nice to actually collect such stats. Monitoring the backlog via dumping qdisc stats is a good start - but actually keeping traces of average bulk size is more useful.

I usually also monitors the BQL limits during these tests.

grep -H . /sys/class/net/eth4/queues/tx-*/byte_queue_limits/{inflight,limit}

To Toke:
Perhaps we could convince Toke, to add a netperf-wrapper recorder for
the BQL inflight and limit? (It would be really cool to plot together)

Quoted 7 lines

I usually also monitors the BQL limits during these tests.

grep -H . /sys/class/net/eth4/queues/tx-*/byte_queue_limits/{inflight,limit}

To Toke:
Perhaps we could convince Toke, to add a netperf-wrapper recorder for
the BQL inflight and limit? (It would be really cool to plot together)

I just added a command line mode and support for timestamped limit and inflight output to bqlmon

Get it here:

https://github.com/dtaht/bqlmon

That should be sufficient for netperf-wrapper to poll it efficiently.

Perhaps Toke have an opinion on the output formatting from bqlmon?

I am not huge on averages. Network theorists tend to think about things in terms of fluid models. Van Jacobson's analogy of a water fountain's operation is very profound...

While it is nearly impossible for a conventional Van Neuman time sliced CPU + network to actually act that way, things like BQL and dedicated pipelining systems like those in DPDK are getting closer to that ideal.

An example of where averages let you down is on the classic 5 minute data reduction things like mrtg do, where you might see `60% of the bandwidth (capacity/5 minutes) in use, yet still see drops because over shorter intervals (capacity/10ms) you have bursts arriving.

On Wed, 01 Oct 2014 16:05:31 -0400

I'll try to make it more explicit.
Will resubmit patchset shortly...

Notice it is not difficult cause a queue to form, but it is tricky (not difficult) to correctly test this patchset. Perhaps you misread my statement earlier as "it was difficult to test and cause a queue to form"?

I think you could read this blog in 30 sec:
http://netoptimizer.blogspot.dk/2014/04/basic-tuning-for-network-overload.html

My cover letter and testing section... will take you longer that 30 sec, it have grown quite large (and Eric will not even read it :-P ;-))

Believe or not, I've actually restricted and reduced the testing section.

Thus, code is activated only when q->qlen is >= 1. And I have already shown that we see a win with just bulking 2 packets:
"Subject: qdisc/UDP_STREAM: measuring effect of qdisc bulk dequeue"
http://thread.gmane.org/gmane.linux.network/331152/focus=331154

I actually believe, that I have already measured and shown that going though the bulk dequeue is a win, this should show direct_xmit vs. bulking:
"Subject: qdisc/trafgen: Measuring effect of qdisc bulk dequeue, with trafgen"
http://thread.gmane.org/gmane.linux.network/331152

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. In-effect bulking will only happen for BQL enabled drivers. Besides the bytelimit from BQL, also limit bulking to maximum 7 packets to avoid any issues with available descriptor in HW and any HoL issues measured at 100Mbit/s.

For now, as a conservative approach, don't bulk dequeue GSO and segmented GSO packets, because testing showed requeuing occuring with segmented GSO packets.

Jointed work with Hannes, Daniel and Florian.

Testing:
--------
Demonstrating the performance improvement of qdisc dequeue bulking, is
tricky because the effect only "kicks-in" once the qdisc system have a backlog. Thus, for a backlog to form, we need either 1) to exceed wirespeed of the link or 2) exceed the capability of the device driver.

For practical use-cases, the measureable effect of this will be a reduction in CPU usage

01-TCP_STREAM:
--------------
Testing effect for TCP involves disabling TSO and GSO, because TCP already benefit from bulking, via TSO and especially for GSO segmented packets. This patch view TSO/GSO as a seperate kind of bulking, and avoid further bulking of these packet types.

The measured perf diff benefit (at 10Gbit/s) for TCP_STREAM were 4.66% less CPU used on calls to _raw_spin_lock() (mostly from sch_direct_xmit).

Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
* Disabled bulking: 8.30% soft 88.75% idle
* Enabled bulking: 7.80% soft 89.36% idle

02-UDP_STREAM
-------------
The measured perf diff benefit for UDP_STREAM were 5.26% less CPU used on calls to _raw_spin_lock(). UDP_STREAM with packet size -m 1472 (to avoid sending UDP/IP fragments).

03-trafgen driver test
----------------------
The performance of the 10Gbit/s ixgbe driver is limited due to updating the HW ring-queue tail-pointer on every packet. As previously demonstrated with pktgen.

Using trafgen to send RAW frames from userspace (via AF_PACKET), and forcing it through qdisc path (with option --qdisc-path and -t0), sending with 12 CPUs.

I can demonstrate this driver layer limitation:
* 12.8 Mpps with no qdisc bulking
* 14.8 Mpps with qdisc bulking (full 10G-wirespeed)

04-netperf-wrapper
------------------
I've designed some tests for the tool "netperf-wrapper" that tries to measure the Head-of-Line (HoL) blocking effect.

The bulking "budget" limit of 7 packets are choosen, based on testing with netperf-wrapper. With driver igb at 100Mbit/s and a budget of max 7 packets I cannot measure any added HoL latency. Increasing this value to 8 packets, I observed a fluctuating added 0.24ms delay corrosponding to 3000 bytes at 100Mbit/s.

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

Signed-off-by Hannes Frederic Sowa <[email protected]>

Signed-off-by Daniel Borkmann <[email protected]>

Signed-off-by Florian Westphal <[email protected]>

---
V5:
- Changed "budget" limit to 7 packets (from 8)
- Choosing to use skb->len, because BQL uses that
- Added testing results to patch desc

V4:
- Patch rewritten in the Red Hat Neuchatel office jointed work with Hannes, Daniel and Florian.
- Conservative approach of only using on BQL enabled drivers
- No user tunable parameter, but limit bulking to 8 packets.
- For now, avoid bulking GSO packets packets.

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

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

 include/net/sch_generic.h |   16 +++++++++++++++
 net/sched/sch_generic.c   |   47 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 61 insertions(+), 2 deletions(-)

include/net/sch_generic.h +16 -0

index e65b8e0..0654dfa 100644
@@ -6,6 +6,7 @@
 #include <linux/rcupdate.h>
 #include <linux/pkt_sched.h>
 #include <linux/pkt_cls.h>
+#include <linux/dynamic_queue_limits.h>
 #include <net/gen_stats.h>
 #include <net/rtnetlink.h>
 

@@ -111,6 +112,21 @@ static inline void qdisc_run_end(struct Qdisc *qdisc)
 	qdisc->__state &= ~__QDISC___STATE_RUNNING;
 }
 
+static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
+{
+	return qdisc->flags & TCQ_F_ONETXQUEUE;
+}
+
+static inline int qdisc_avail_bulklimit(const struct netdev_queue *txq)
+{
+#ifdef CONFIG_BQL
+	/* Non-BQL migrated drivers will return 0, too. */
+	return dql_avail(&txq->dql);
+#else
+	return 0;
+#endif
+}
+
 static inline bool qdisc_is_throttled(const struct Qdisc *qdisc)
 {
 	return test_bit(__QDISC_STATE_THROTTLED, &qdisc->state) ? true : false;

net/sched/sch_generic.c +40 -1

index 11b28f6..0fa8f0e 100644
@@ -56,6 +56,42 @@ static inline int dev_requeue_skb(struct sk_buff *skb, struct Qdisc *q)
 	return 0;
 }
 
+static struct sk_buff *try_bulk_dequeue_skb(struct Qdisc *q,
+					    struct sk_buff *head_skb,
+					    int bytelimit)
+{
+	struct sk_buff *skb, *tail_skb = head_skb;
+	int budget = 7; /* For now, conservatively choosen limit */
+
+	while (bytelimit > 0 && --budget > 0) {
+		/* For now, don't bulk dequeue GSO (or GSO segmented) pkts */
+		if (tail_skb->next || skb_is_gso(tail_skb))
+			break;
+
+		skb = q->dequeue(q);
+		if (!skb)
+			break;
+
+		bytelimit -= skb->len; /* covers GSO len */
+		skb = validate_xmit_skb(skb, qdisc_dev(q));
+		if (!skb)
+			break;
+
+		/* "skb" can be a skb list after validate call above
+		 * (GSO segmented), but it is okay to append it to
+		 * current tail_skb->next, because next round will exit
+		 * in-case "tail_skb->next" is a skb list.
+		 */
+		tail_skb->next = skb;
+		tail_skb = skb;
+	}
+
+	return head_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;

@@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 		} else
 			skb = NULL;
 	} else {
-		if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
+		if (!(q->flags & TCQ_F_ONETXQUEUE) ||
+		    !netif_xmit_frozen_or_stopped(txq)) {
+			int bytelimit = qdisc_avail_bulklimit(txq);
+

Quoted 10 lines

net/sched/sch_generic.c +4 -1

 	struct sk_buff *skb = q->gso_skb;

@@ -70,10 +106,17 @@ static inline struct sk_buff *dequeue_skb(struct Qdisc *q)
 		} else
 			skb = NULL;
 	} else {
-		if (!(q->flags & TCQ_F_ONETXQUEUE) || !netif_xmit_frozen_or_stopped(txq)) {
+		if (!(q->flags & TCQ_F_ONETXQUEUE) ||
+		    !netif_xmit_frozen_or_stopped(txq)) {
+			int bytelimit = qdisc_avail_bulklimit(txq);
+

net/sched/sch_generic.c static inline struct sk_buff *dequeue_skb(struct Qdisc *q) · line 121

Quoted 10 lines

net/sched/sch_generic.c +5 -1

 			skb = q->dequeue(q);
-			if (skb)
+			if (skb) {
+				bytelimit -= skb->len;
 				skb = validate_xmit_skb(skb, qdisc_dev(q));
+			}
+			if (skb && qdisc_may_bulk(q))
+				skb = try_bulk_dequeue_skb(q, skb, bytelimit);
 		}
 	}

Okay...

In my opinion, you guys missed some opportunities :

1) Ideally the algo should try to call validate_xmit_skb() outside of the locks (qdisc lock or device lock), because eventually doing checksum computation or full segmentation should allow other cpus doing enqueues.

2) TSO support. Have you ideas how to perform this ?

You're right, but this is not related to this patch specifically. But sure, we can look into moving validate_xmit_skb calls outside of the qdisc root locked sections.

Quoted 2 lines

2) TSO support. Have you ideas how to perform this ?

Yes, the idea was to just remove the skb_is_gso() test and limit dql_avail() result to e.g. 128k to avoid a '8-fat-gso-skbs-in-a-row' scenario.

Did you have any other ideas in mind?

I guess my question was more like : Should I wait you sending patches, or should I do this ?

In my opinion, you guys missed some opportunities :

1) Ideally the algo should try to call validate_xmit_skb() outside of the locks (qdisc lock or device lock), because eventually doing checksum computation or full segmentation should allow other cpus doing enqueues.

2) TSO support. Have you ideas how to perform this ?

Quoted 1 line · On Tue, 30 Sep 2014 04:48:56 -0700 Eric Dumazet <[email protected]> wrote:

In my opinion, you guys missed some opportunities :

This is a conservative first approach. The merge window is approaching fast, is is annoying you are blocking a save conservative approach.

For now, as a conservative approach, don't bulk dequeue GSO and segmented GSO packets, because testing showed requeuing occuring with segmented GSO packets.

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

Quoted 4 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

I understand the motivation for this, but I find it a bit out-of-place to have a 'packet' type counter in bql context?

Would it perhaps make more sense to restrict bulk dequeues by an upper (possibly changeable from userspace) byte counter limit?

Quoted 11 lines · Florian Westphal

Quoted 4 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

I understand the motivation for this, but I find it a bit out-of-place to have a 'packet' type counter in bql context?

Would it perhaps make more sense to restrict bulk dequeues by an upper (possibly changeable from userspace) byte counter limit?

The byte count is already provided : its the BQL limit.
We already have ways to tune it (limit_min & limit_max)
We do not think we need something else here.
That was the whole point of reusing BQL in the first place : Its already powerful and implemented.

This new attribute is a packet burst.

When I suggested 'pburst' attribute for pktgen to Alexei, he sent a patch with "burst". Nobody complained.

Now I am reusing what apparently was OK for pktgen, you seem to object.

If you feel not comfortable with "burst", rename it to whatever you think is best.

But please, do not hard code magic 7 in your code.

Thanks

Yes, this is the wrong place to put it. DQL knows nothing about packets or technically even bytes, it's a generic library that deal with "objects" on a queue. I don't see how burst relates to this.

Quoted 3 lines · Tom Herbert

Yes, this is the wrong place to put it. DQL knows nothing about packets or technically even bytes, it's a generic library that deal with "objects" on a queue. I don't see how burst relates to this.

I wonder then why you put :

netdev_tx_completed_queue(struct netdev_queue *dev_queue,
unsigned int pkts, unsigned int bytes)

Really, I don't care _where_ the attribute is, as long we dont access yet another cache line in fast path.

If you feel not comfortable with "burst", rename it to whatever you think is best.
But please, do not hard code magic 7 in your code.

Quoted 3 lines · Eric Dumazet

If you feel not comfortable with "burst", rename it to whatever you think is best.
But please, do not hard code magic 7 in your code.

I had hoped that this 'magic' value could be removed completely, only using bql data for bulking decisions.

Quoted 7 lines · Florian Westphal

Quoted 3 lines · Eric Dumazet

If you feel not comfortable with "burst", rename it to whatever you think is best.
But please, do not hard code magic 7 in your code.

I had hoped that this 'magic' value could be removed completely, only using bql data for bulking decisions.

But it is apparently not the case, since you guys decided to had it set to 8, then to 7 later, based on experiments.

Quoted 11 lines · On Tue, 30 Sep 2014 07:55:36 -0700 Eric Dumazet <[email protected]> wrote:

Quoted 7 lines · Florian Westphal

Quoted 3 lines · Eric Dumazet

If you feel not comfortable with "burst", rename it to whatever you think is best.
But please, do not hard code magic 7 in your code.

I had hoped that this 'magic' value could be removed completely, only using bql data for bulking decisions.

But it is apparently not the case, since you guys decided to had it set to 8, then to 7 later, based on experiments.

The "magic" limit is only a conservative save guard. We do plan to remove this completely. That is the reason for not exporting this, as we want free-hands to remove this completely.

Guess, I'll remove it completely now, so we can move on.

I would like some off/on switch, exported to userspace, for disabling this bulking (then we don't need this conservative magic number). How would that be done best?

--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer

Quoted 9 lines · Florian Westphal

Quoted 4 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

I understand the motivation for this, but I find it a bit out-of-place to have a 'packet' type counter in bql context?

Quoted 2 lines

Would it perhaps make more sense to restrict bulk dequeues by an upper (possibly changeable from userspace) byte counter limit?

The byte count is already provided : its the BQL limit. We already have ways to tune it (limit_min & limit_max) We do not think we need something else here.

Quoted 10 lines · Eric Dumazet

Quoted 9 lines · Florian Westphal

Quoted 4 lines · Eric Dumazet

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

I understand the motivation for this, but I find it a bit out-of-place to have a 'packet' type counter in bql context?

Quoted 6 lines

Quoted 2 lines

Would it perhaps make more sense to restrict bulk dequeues by an upper (possibly changeable from userspace) byte counter limit?

The byte count is already provided : its the BQL limit. We already have ways to tune it (limit_min & limit_max) We do not think we need something else here.

So you're saying that a bulk dequeue should just grab as many skbs as possible until no more available or dql_avail exhausted?

The "magic" value was just to be conservative and not induce any hol blocking, which is also why Jesper reduced it again in the latest submission.

Then, we might later be able to remove the TSO restriction and switch to a pure byte-based limit.

(I don't think having a packet-based count makes sense once tso/gso
skbs can be bulk dequeued).

Sorry for the confusion.

So you're saying that a bulk dequeue should just grab as many skbs as possible until no more available or dql_avail exhausted?

The "magic" value was just to be conservative and not induce any hol blocking, which is also why Jesper reduced it again in the latest submission.

Then, we might later be able to remove the TSO restriction and switch to a pure byte-based limit

.

(I don't think having a packet-based count makes sense once tso/gso
skbs can be bulk dequeued).

From: Eric Dumazet <[email protected]>

Add a new dql attribute, to control how much packets we are allowed to burst from qdisc to device.

Signed-off-by Eric Dumazet <[email protected]>

---
Jesper, please consider using this instead of 7 or 8 values you hard coded.