net: batch skb dequeueing from softnet input_pkt_queue
Subsystems (2)
PATCH v3 1/1 net: batch skb dequeueing from softnet input_pkt_queue
batch skb dequeueing from softnet input_pkt_queue
batch skb dequeueing from softnet input_pkt_queue to reduce potential lock contention and irq disabling/enabling.
----
include/linux/netdevice.h | 1 net/core/dev.c | 56 ++++++++++++++++++++++++++++++++-------------- 2 files changed, 40 insertions(+), 17 deletions(-)
include/linux/netdevice.h +1 -0
@@ -1335,6 +1335,7 @@ struct softnet_data {struct call_single_data csd____cacheline_aligned_in_smp ;#endif struct sk_buff_head input_pkt_queue; +struct sk_buff_head processing_queue;struct napi_struct backlog; };
net/core/dev.c +39 -17
117 lines not on this page · raw message
Quoted 6 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.Signed-off-by Changli Gao <[email protected]>
It seems we are now going to generate a lot more IPIs with such a change. At least this is what i am imagining.
CPU0: packet comes in,queue empty, generate an IPI to CPU1
CPU0: second packet comes in, enqueue
CPU1: grab two packets to process and run with them
CPU0: packet comes in,queue empty, generate an IPI to CPU1 ..
...
.....
IPIs add to latency (refer to my other email). Did you test this to reach some conclusion that it improves thing or was it just by inspection?
cheers,
jamal
Quoted 7 lines · jamal
It seems we are now going to generate a lot more IPIs with such a change. At least this is what i am imagining.
CPU0: packet comes in,queue empty, generate an IPI to CPU1
CPU0: second packet comes in, enqueue
CPU1: grab two packets to process and run with them
CPU0: packet comes in,queue empty, generate an IPI to CPU1
No extra IPI is needed.
+ qlen = queue->input_pkt_queue.qlen + queue->processing_queue.qlen;
+ if (qlen <= netdev_max_backlog) {
+ if (qlen) {
the packets in processing_queue are counted too.
Quoted 5 lines
IPIs add to latency (refer to my other email). Did you test this to reach some conclusion that it improves thing or was it just by inspection?
:( only insepection.
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 9 lines · Changli Gao
On Wed, Apr 14, 2010 at 7:58 PM, jamal <[email protected]> wrote:
No extra IPI is needed.
net/core/dev.c +3
-0+ qlen = queue->input_pkt_queue.qlen + queue->processing_queue.qlen; +if (qlen <= netdev_max_backlog) { +if (qlen) {the packets in
processing_queueare counted too.
Ok - Looks reasonable.
Quoted 6 lines
Quoted 4 lines
IPIs add to latency (refer to my other email). Did you test this to reach some conclusion that it improves thing or was it just by inspection?
:( only insepection.
I am probably being pushy, but one simple test for latency of single flow is:
from machine 1, send ping -f
on rps machine:
Base test: no rps on ( a fresh boot with no sysctls should do fine) Test 1: irq affinity on cpuX, rps to cpuY
Test 2: repeat test1 with your change.
It should show no difference between test1 and 2. If it shows improvement better - but showing worse latency is bad.
cheers,
jamal
Quoted 6 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.Signed-off-by Changli Gao <[email protected]>
Adding stop_machine() with no explanation ?
No ack from my previous comments, suggestions, and still same logic ?
Are we supposed to read patch, test it, make some benches, correct bugs, say Amen ?
This is becoming silly, if you ask me.
This is a NACK of this patch, obviously.
Quoted 9 lines · Eric Dumazet
Quoted 6 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.Signed-off-by Changli Gao <[email protected]>Adding
stop_machine()with no explanation ?
stop_machine() is added to flush the processing_queue. Because the old flush_backlog() runs in IRQ context, it can't touch the things, which are only valid in softirq context.
Quoted 2 lines
No ack from my previous comments, suggestions, and still same logic ?
In this patch, the volatile variable flush_processing_queue is removed, and the corresponding lines are removed too.
Oh, I should splice the old message back, as stop_machine is used instead. So, the processing queue will be removed from softnet_data, and a single int counter will be used instead to count the packets which are being processing.
one issue you concern is potential cache miss when summing in enqueue function. It won't happen all the time, in fact, I think it should happen seldom, and it is the responsibility of hardware to cache the data frequently used.
Quoted 4 lines
Are we supposed to read patch, test it, make some benches, correct bugs, say Amen ?
OK, I'll test it.
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 7 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.Signed-off-by Changli Gao <[email protected]>----
lock contention _is_ a problem, Jamal tests can show it.
irq disabling/enabling is not, and force to use stop_machine() killer.
I suggest something very simple, like a small buffer (16 slots), so that process_backlog() can batch 16 buffers at once.
Following patch not tested, but its late here and I need to sleep ;)
This is a RFC, not for inclusion, and based on current net-next-2.6 tree
[RFC] net: introduce a batch mode in process_backlog()
We see a lock contention on input_pkt_queue.lock in RPS benches.
As suggested by Changli Gao, we can batch several skbs at once in process_backlog(), so that we dirty input_pkt_queue less often.
I chose to batch at most 16 skbs per round, and place them in softnet_data zone where flush_backlog() can find them and eventually free this skbs at device dismantle.
--- include/linux/netdevice.h | 2 + net/core/dev.c | 48 +++++++++++++++++++++++++++--------- 2 files changed, 38 insertions(+), 12 deletions(-)
include/linux/netdevice.h +2 -0
@@ -1383,11 +1383,13 @@ static inline int unregister_gifconf(unsigned int family)/* * Incoming packets are placed on per-cpu queues */ +#define SD_BATCH_SZ 16struct softnet_data {struct Qdisc *output_queue;struct list_head poll_list;struct sk_buff *completion_queue; +struct sk_buff *batch[SD_BATCH_SZ ];/* process_backlog() & flush_backlog() */ #ifdef CONFIG_RPS struct softnet_data *rps_ipi_list;
net/core/dev.c +38 -12
79 lines not on this page · raw message
Quoted 7 lines
do {
- structsk_buff*skb;
+ lim = quota - work;
+ if (lim >ARRAY_SIZE(sd->batch))
+ lim =ARRAY_SIZE(sd->batch);
+ /* batch at most 16 buffers */
How about just using two input_pkt_queue's (define input_pkt_queue[2])? One that is used to enqueue from RPS, and one that is being processed by process_backlog. Then the only thing that needs to be done under lock in process_backlog is to switch the queues; something like sd->current_input_pkt_queue ^= 1
Tom
Quoted 44 lines
-
local_irq_disable();
rps_lock(sd);
- skb =__skb_dequeue(&sd->input_pkt_queue);
- if (!skb) {
+ for (n = 0; n < lim; n++) {
+ sd->batch[n] =__skb_dequeue(&sd->input_pkt_queue);
+ if (!sd->batch[n])
+ break;
+ }
+ if (!sd->input_pkt_queue.qlen) {__napi_complete(napi);
-rps_unlock(sd);
-local_irq_enable();
- break;
+ quota = 0;
}
-input_queue_head_incr(sd);
rps_unlock(sd);
-local_irq_enable();-
__netif_receive_skb(skb);
- } while (++work < quota);
+ /* Now process our batch */
+ for (i = 0; i < n; i++) {
+ skb = sd->batch[i];
+ /*flush_backlog()might have stolen this skb */
+input_queue_head_incr(sd);
+ if (likely(skb)) {
+ sd->batch[i] = NULL;
+local_irq_enable();
+__netif_receive_skb(skb);
+local_irq_disable();
+ }
+ }
+ work += n;
+ } while (work < quota);+
local_irq_enable();
return work;
}
Quoted 7 lines · Tom Herbert
How about just using two input_pkt_queue's (define input_pkt_queue[2])? One that is used to enqueue from RPS, and one that is being processed by
process_backlog. Then the only thing that needs to be done under lock inprocess_backlogis to switch the queues; something likesd->current_input_pkt_queue^= 1
It is a better idea, IMO.
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 6 lines · Eric Dumazet
Quoted 5 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.
Quoted 7 lines
Quoted 2 lines
Signed-off-by Changli Gao <[email protected]>----
lock contention _is_ a problem, Jamal tests can show it.
irq disabling/enabling is not, and force to use
stop_machine()killer.
Although irq_disabling/enabling is not, we should do our best to make fast path as quickly as possible, and because stop_machine() is used in slow patch, I think we can afford its weight.
Signature · 2 lines
Regards， Changli Gao([email protected])
Quoted 10 lines · Changli Gao
Quoted 9 lines · Eric Dumazet
Quoted 7 lines · Changli Gao
batch skb dequeueing from softnet
input_pkt_queuebatch skb dequeueing from softnet
input_pkt_queueto reduce potential lock contention and irq disabling/enabling.Signed-off-by Changli Gao <[email protected]>----
Quoted 10 lines
Quoted 4 lines
lock contention _is_ a problem, Jamal tests can show it.
irq disabling/enabling is not, and force to use
stop_machine()killer.Although irq_disabling/enabling is not, we should do our best to make fast path as quickly as possible, and because
stop_machine()is used in slow patch, I think we can afford its weight.
No thanks, this is out of the question.
Talk to ixiacom guys, some people settle/dismantle dozens of network device per second, on production machines.
David Miller
maintainerFrom: Eric Dumazet <[email protected]>
Date: Thu, 22 Apr 2010 09:13:22 +0200
Quoted 4 lines
No thanks, this is out of the question.
Talk to ixiacom guys, some people settle/dismantle dozens of network device per second, on production machines.
Yes, ifup/ifdown performance is very important these days.
Recently we've had to do a lot of work to increase scalability and latency in this area, let's not undo that.
Quoted 7 lines · Eric Dumazet
[RFC] net: introduce a batch mode in
process_backlog()We see a lock contention on input_pkt_queue.lock in RPS benches.
As suggested by Changli Gao, we can batch several skbs at once in
process_backlog(), so that we dirtyinput_pkt_queueless often.
Ok, so i grab the latest and greatest net-next and apply this before testing? Let me know..
cheers,
jamal