From: Willem de Bruijn <willemb at google.com> Add napi for virtio-net transmit completion processing. Changes: v1 -> v2: - disable by default - disable unless affinity_hint_set because cache misses add up to a third higher cycle cost, e.g., in TCP_RR tests. This is not limited to the patch that enables tx completion cleaning in rx napi. - use trylock to avoid contention between tx and rx napi - keep interrupts masked during xmit_more (new patch 5/5) this improves cycles especially for multi UDP_STREAM, which does not benefit from cleaning tx completions on rx napi. - move free_old_xmit_skbs (new patch 3/5) to avoid forward declaration not changed: - deduplicate virnet_poll_tx and virtnet_poll_txclean they look similar, but have differ too much to make it worthwhile. - delay netif_wake_subqueue for more than 2 + MAX_SKB_FRAGS evaluated, but made no difference - patch 1/5 RFC -> v1: - dropped vhost interrupt moderation patch: not needed and likely expensive at light load - remove tx napi weight - always clean all tx completions - use boolean to toggle tx-napi, instead - only clean tx in rx if tx-napi is enabled - then clean tx before rx - fix: add missing braces in virtnet_freeze_down - testing: add 4KB TCP_RR + UDP test results Based on previous patchsets by Jason Wang: [RFC V7 PATCH 0/7] enable tx interrupts for virtio-net http://lkml.iu.edu/hypermail/linux/kernel/1505.3/00245.html Before commit b0c39dbdc204 ("virtio_net: don't free buffers in xmit ring") the virtio-net driver would free transmitted packets on transmission of new packets in ndo_start_xmit and, to catch the edge case when no new packet is sent, also in a timer at 10HZ. A timer can cause long stalls. VIRTIO_F_NOTIFY_ON_EMPTY avoids stalls due to low free descriptor count. It does not address a stalls due to low socket SO_SNDBUF. Increasing timer frequency decreases that stall time, but increases interrupt rate and, thus, cycle count. Currently, with no timer, packets are freed only at ndo_start_xmit. Latency of consume_skb is now unbounded. To avoid a deadlock if a sock reaches SO_SNDBUF, packets are orphaned on tx. This breaks TCP small queues. Reenable TCP small queues by removing the orphan. Instead of using a timer, convert the driver to regular tx napi. This does not have the unresolved stall issue and does not have any frequency to tune. By keeping interrupts enabled by default, napi increases tx interrupt rate. VIRTIO_F_EVENT_IDX avoids sending an interrupt if one is already unacknowledged, so makes this more feasible today. Combine that with an optimization that brings interrupt rate back in line with the existing version for most workloads: Tx completion cleaning on rx interrupts elides most explicit tx interrupts by relying on the fact that many rx interrupts fire. Tested by running {1, 10, 100} {TCP, UDP} STREAM, RR, 4K_RR benchmarks from a guest to a server on the host, on an x86_64 Haswell. The guest runs 4 vCPUs pinned to 4 cores. vhost and the test server are pinned to a core each. All results are the median of 5 runs, with variance well < 10%. Used neper (github.com/google/neper) as test process. Napi increases single stream throughput, but increases cycle cost. The optimizations bring this down. The previous patchset saw a regression with UDP_STREAM, which does not benefit from cleaning tx interrupts in rx napi. This regression is now gone for 10x, 100x. Remaining difference is higher 1x TCP_STREAM, lower 1x UDP_STREAM. The latest results are with process, rx napi and tx napi affine to the same core. All numbers are lower than the previous patchset. upstream napi TCP_STREAM: 1x: Mbps 28985 34492 Gcycles 278 269 10x: Mbps 42267 42420 Gcycles 301 303 100x: Mbps 29710 31131 Gcycles 286 282 TCP_RR Latency (us): 1x: p50 22 21 p99 27 26 Gcycles 176 171 10x: p50 40 40 p99 51 51 Gcycles 215 211 100x: p50 272 235 p99 414 360 Gcycles 228 233 TCP_RR 4K: 1x: p50 27 29 p99 34 36 Gcycles 184 170 10x: p50 67 66 p99 82 82 Gcycles 217 218 100x: p50 435 485 p99 813 626 Gcycles 242 226 UDP_STREAM: 1x: Mbps 29802 25600 Gcycles 305 295 10x: Mbps 29901 29684 Gcycles 287 295 100x: Mbps 29952 29863 Gcycles 336 325 UDP_RR: 1x: p50 18 18 p99 23 22 Gcycles 183 181 10x: p50 35 38 p99 54 62 Gcycles 252 236 100x: p50 240 279 p99 462 519 Gcycles 228 219 Note that GSO is enabled, so 4K RR still translates to one packet per request. Lower throughput at 100x vs 10x can be (at least in part) explained by looking at bytes per packet sent (nstat). It likely also explains the lower throughput of 1x for some variants. upstream: N=1 bytes/pkt=16581 N=10 bytes/pkt=61513 N=100 bytes/pkt=51558 at_rx: N=1 bytes/pkt=65204 N=10 bytes/pkt=65148 N=100 bytes/pkt=56840 Willem de Bruijn (5): virtio-net: napi helper functions virtio-net: transmit napi virtio-net: move free_old_xmit_skbs virtio-net: clean tx descriptors from rx napi virtio-net: keep tx interrupts disabled unless kick drivers/net/virtio_net.c | 211 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 150 insertions(+), 61 deletions(-) -- 2.12.2.816.g2cccc81164-goog
Willem de Bruijn
2017-Apr-18 20:21 UTC
[PATCH net-next v2 1/5] virtio-net: napi helper functions
From: Willem de Bruijn <willemb at google.com> Prepare virtio-net for tx napi by converting existing napi code to use helper functions. This also deduplicates some logic. Signed-off-by: Willem de Bruijn <willemb at google.com> Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/net/virtio_net.c | 65 ++++++++++++++++++++++++++---------------------- 1 file changed, 35 insertions(+), 30 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 666ada6130ab..b9c1df29892c 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -239,6 +239,26 @@ static struct page *get_a_page(struct receive_queue *rq, gfp_t gfp_mask) return p; } +static void virtqueue_napi_schedule(struct napi_struct *napi, + struct virtqueue *vq) +{ + if (napi_schedule_prep(napi)) { + virtqueue_disable_cb(vq); + __napi_schedule(napi); + } +} + +static void virtqueue_napi_complete(struct napi_struct *napi, + struct virtqueue *vq, int processed) +{ + int opaque; + + opaque = virtqueue_enable_cb_prepare(vq); + if (napi_complete_done(napi, processed) && + unlikely(virtqueue_poll(vq, opaque))) + virtqueue_napi_schedule(napi, vq); +} + static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; @@ -936,27 +956,20 @@ static void skb_recv_done(struct virtqueue *rvq) struct virtnet_info *vi = rvq->vdev->priv; struct receive_queue *rq = &vi->rq[vq2rxq(rvq)]; - /* Schedule NAPI, Suppress further interrupts if successful. */ - if (napi_schedule_prep(&rq->napi)) { - virtqueue_disable_cb(rvq); - __napi_schedule(&rq->napi); - } + virtqueue_napi_schedule(&rq->napi, rvq); } -static void virtnet_napi_enable(struct receive_queue *rq) +static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi) { - napi_enable(&rq->napi); + napi_enable(napi); /* If all buffers were filled by other side before we napi_enabled, we - * won't get another interrupt, so process any outstanding packets - * now. virtnet_poll wants re-enable the queue, so we disable here. - * We synchronize against interrupts via NAPI_STATE_SCHED */ - if (napi_schedule_prep(&rq->napi)) { - virtqueue_disable_cb(rq->vq); - local_bh_disable(); - __napi_schedule(&rq->napi); - local_bh_enable(); - } + * won't get another interrupt, so process any outstanding packets now. + * Call local_bh_enable after to trigger softIRQ processing. + */ + local_bh_disable(); + virtqueue_napi_schedule(napi, vq); + local_bh_enable(); } static void refill_work(struct work_struct *work) @@ -971,7 +984,7 @@ static void refill_work(struct work_struct *work) napi_disable(&rq->napi); still_empty = !try_fill_recv(vi, rq, GFP_KERNEL); - virtnet_napi_enable(rq); + virtnet_napi_enable(rq->vq, &rq->napi); /* In theory, this can happen: if we don't get any buffers in * we will *never* try to fill again. @@ -1011,21 +1024,13 @@ static int virtnet_poll(struct napi_struct *napi, int budget) { struct receive_queue *rq container_of(napi, struct receive_queue, napi); - unsigned int r, received; + unsigned int received; received = virtnet_receive(rq, budget); /* Out of packets? */ - if (received < budget) { - r = virtqueue_enable_cb_prepare(rq->vq); - if (napi_complete_done(napi, received)) { - if (unlikely(virtqueue_poll(rq->vq, r)) && - napi_schedule_prep(napi)) { - virtqueue_disable_cb(rq->vq); - __napi_schedule(napi); - } - } - } + if (received < budget) + virtqueue_napi_complete(napi, rq->vq, received); return received; } @@ -1040,7 +1045,7 @@ static int virtnet_open(struct net_device *dev) /* Make sure we have some buffers: if oom use wq. */ if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) schedule_delayed_work(&vi->refill, 0); - virtnet_napi_enable(&vi->rq[i]); + virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); } return 0; @@ -1747,7 +1752,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) schedule_delayed_work(&vi->refill, 0); for (i = 0; i < vi->max_queue_pairs; i++) - virtnet_napi_enable(&vi->rq[i]); + virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); } netif_device_attach(vi->dev); -- 2.12.2.816.g2cccc81164-goog
From: Willem de Bruijn <willemb at google.com> Convert virtio-net to a standard napi tx completion path. This enables better TCP pacing using TCP small queues and increases single stream throughput. The virtio-net driver currently cleans tx descriptors on transmission of new packets in ndo_start_xmit. Latency depends on new traffic, so is unbounded. To avoid deadlock when a socket reaches its snd limit, packets are orphaned on tranmission. This breaks socket backpressure, including TSQ. Napi increases the number of interrupts generated compared to the current model, which keeps interrupts disabled as long as the ring has enough free descriptors. Keep tx napi optional and disabled for now. Follow-on patches will reduce the interrupt cost. Signed-off-by: Willem de Bruijn <willemb at google.com> Signed-off-by: Jason Wang <jasowang at redhat.com> --- drivers/net/virtio_net.c | 77 +++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 67 insertions(+), 10 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index b9c1df29892c..c173e85dc7b8 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -33,9 +33,10 @@ static int napi_weight = NAPI_POLL_WEIGHT; module_param(napi_weight, int, 0444); -static bool csum = true, gso = true; +static bool csum = true, gso = true, napi_tx; module_param(csum, bool, 0444); module_param(gso, bool, 0444); +module_param(napi_tx, bool, 0644); /* FIXME: MTU in config. */ #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN) @@ -86,6 +87,8 @@ struct send_queue { /* Name of the send queue: output.$index */ char name[40]; + + struct napi_struct napi; }; /* Internal representation of a receive virtqueue */ @@ -262,12 +265,16 @@ static void virtqueue_napi_complete(struct napi_struct *napi, static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; + struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; /* Suppress further interrupts. */ virtqueue_disable_cb(vq); - /* We were probably waiting for more output buffers. */ - netif_wake_subqueue(vi->dev, vq2txq(vq)); + if (napi->weight) + virtqueue_napi_schedule(napi, vq); + else + /* We were probably waiting for more output buffers. */ + netif_wake_subqueue(vi->dev, vq2txq(vq)); } static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx) @@ -972,6 +979,21 @@ static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi) local_bh_enable(); } +static void virtnet_napi_tx_enable(struct virtnet_info *vi, + struct virtqueue *vq, + struct napi_struct *napi) +{ + if (!napi->weight) + return; + + if (!vi->affinity_hint_set) { + napi->weight = 0; + return; + } + + return virtnet_napi_enable(vq, napi); +} + static void refill_work(struct work_struct *work) { struct virtnet_info *vi @@ -1046,6 +1068,7 @@ static int virtnet_open(struct net_device *dev) if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) schedule_delayed_work(&vi->refill, 0); virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); + virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi); } return 0; @@ -1081,6 +1104,25 @@ static void free_old_xmit_skbs(struct send_queue *sq) u64_stats_update_end(&stats->tx_syncp); } +static int virtnet_poll_tx(struct napi_struct *napi, int budget) +{ + struct send_queue *sq = container_of(napi, struct send_queue, napi); + struct virtnet_info *vi = sq->vq->vdev->priv; + struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); + + if (__netif_tx_trylock(txq)) { + free_old_xmit_skbs(sq); + __netif_tx_unlock(txq); + } + + virtqueue_napi_complete(napi, sq->vq, 0); + + if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) + netif_tx_wake_queue(txq); + + return 0; +} + static int xmit_skb(struct send_queue *sq, struct sk_buff *skb) { struct virtio_net_hdr_mrg_rxbuf *hdr; @@ -1130,9 +1172,11 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) int err; struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum); bool kick = !skb->xmit_more; + bool use_napi = sq->napi.weight; /* Free up any pending old buffers before queueing new ones. */ - free_old_xmit_skbs(sq); + if (!use_napi) + free_old_xmit_skbs(sq); /* timestamp packet in software */ skb_tx_timestamp(skb); @@ -1152,8 +1196,10 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) } /* Don't wait up for transmitted skbs to be freed. */ - skb_orphan(skb); - nf_reset(skb); + if (!use_napi) { + skb_orphan(skb); + nf_reset(skb); + } /* If running out of space, stop queue to avoid getting packets that we * are then unable to transmit. @@ -1167,7 +1213,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) */ if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { netif_stop_subqueue(dev, qnum); - if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { + if (!use_napi && + unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ free_old_xmit_skbs(sq); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { @@ -1371,8 +1418,10 @@ static int virtnet_close(struct net_device *dev) /* Make sure refill_work doesn't re-enable napi! */ cancel_delayed_work_sync(&vi->refill); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_queue_pairs; i++) { napi_disable(&vi->rq[i].napi); + napi_disable(&vi->sq[i].napi); + } return 0; } @@ -1727,8 +1776,10 @@ static void virtnet_freeze_down(struct virtio_device *vdev) cancel_delayed_work_sync(&vi->refill); if (netif_running(vi->dev)) { - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_queue_pairs; i++) { napi_disable(&vi->rq[i].napi); + napi_disable(&vi->sq[i].napi); + } } } @@ -1751,8 +1802,11 @@ static int virtnet_restore_up(struct virtio_device *vdev) if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) schedule_delayed_work(&vi->refill, 0); - for (i = 0; i < vi->max_queue_pairs; i++) + for (i = 0; i < vi->max_queue_pairs; i++) { virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); + virtnet_napi_tx_enable(vi, vi->sq[i].vq, + &vi->sq[i].napi); + } } netif_device_attach(vi->dev); @@ -1957,6 +2011,7 @@ static void virtnet_free_queues(struct virtnet_info *vi) for (i = 0; i < vi->max_queue_pairs; i++) { napi_hash_del(&vi->rq[i].napi); netif_napi_del(&vi->rq[i].napi); + netif_napi_del(&vi->sq[i].napi); } /* We called napi_hash_del() before netif_napi_del(), @@ -2142,6 +2197,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi) vi->rq[i].pages = NULL; netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll, napi_weight); + netif_napi_add(vi->dev, &vi->sq[i].napi, virtnet_poll_tx, + napi_tx ? napi_weight : 0); sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg)); ewma_pkt_len_init(&vi->rq[i].mrg_avg_pkt_len); -- 2.12.2.816.g2cccc81164-goog
Willem de Bruijn
2017-Apr-18 20:21 UTC
[PATCH net-next v2 3/5] virtio-net: move free_old_xmit_skbs
From: Willem de Bruijn <willemb at google.com> An upcoming patch will call free_old_xmit_skbs indirectly from virtnet_poll. Move the function above this to avoid having to introduce a forward declaration. This is a pure move: no code changes. Signed-off-by: Willem de Bruijn <willemb at google.com> --- drivers/net/virtio_net.c | 60 ++++++++++++++++++++++++------------------------ 1 file changed, 30 insertions(+), 30 deletions(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index c173e85dc7b8..81a98de864e6 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1042,6 +1042,36 @@ static int virtnet_receive(struct receive_queue *rq, int budget) return received; } +static void free_old_xmit_skbs(struct send_queue *sq) +{ + struct sk_buff *skb; + unsigned int len; + struct virtnet_info *vi = sq->vq->vdev->priv; + struct virtnet_stats *stats = this_cpu_ptr(vi->stats); + unsigned int packets = 0; + unsigned int bytes = 0; + + while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) { + pr_debug("Sent skb %p\n", skb); + + bytes += skb->len; + packets++; + + dev_kfree_skb_any(skb); + } + + /* Avoid overhead when no packets have been processed + * happens when called speculatively from start_xmit. + */ + if (!packets) + return; + + u64_stats_update_begin(&stats->tx_syncp); + stats->tx_bytes += bytes; + stats->tx_packets += packets; + u64_stats_update_end(&stats->tx_syncp); +} + static int virtnet_poll(struct napi_struct *napi, int budget) { struct receive_queue *rq @@ -1074,36 +1104,6 @@ static int virtnet_open(struct net_device *dev) return 0; } -static void free_old_xmit_skbs(struct send_queue *sq) -{ - struct sk_buff *skb; - unsigned int len; - struct virtnet_info *vi = sq->vq->vdev->priv; - struct virtnet_stats *stats = this_cpu_ptr(vi->stats); - unsigned int packets = 0; - unsigned int bytes = 0; - - while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) { - pr_debug("Sent skb %p\n", skb); - - bytes += skb->len; - packets++; - - dev_kfree_skb_any(skb); - } - - /* Avoid overhead when no packets have been processed - * happens when called speculatively from start_xmit. - */ - if (!packets) - return; - - u64_stats_update_begin(&stats->tx_syncp); - stats->tx_bytes += bytes; - stats->tx_packets += packets; - u64_stats_update_end(&stats->tx_syncp); -} - static int virtnet_poll_tx(struct napi_struct *napi, int budget) { struct send_queue *sq = container_of(napi, struct send_queue, napi); -- 2.12.2.816.g2cccc81164-goog
Willem de Bruijn
2017-Apr-18 20:21 UTC
[PATCH net-next v2 4/5] virtio-net: clean tx descriptors from rx napi
From: Willem de Bruijn <willemb at google.com> Amortize the cost of virtual interrupts by doing both rx and tx work on reception of a receive interrupt if tx napi is enabled. With VIRTIO_F_EVENT_IDX, this suppresses most explicit tx completion interrupts for bidirectional workloads. Signed-off-by: Willem de Bruijn <willemb at google.com> --- drivers/net/virtio_net.c | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 81a98de864e6..b14c82ce0032 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1072,12 +1072,33 @@ static void free_old_xmit_skbs(struct send_queue *sq) u64_stats_update_end(&stats->tx_syncp); } +static void virtnet_poll_cleantx(struct receive_queue *rq) +{ + struct virtnet_info *vi = rq->vq->vdev->priv; + unsigned int index = vq2rxq(rq->vq); + struct send_queue *sq = &vi->sq[index]; + struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, index); + + if (!sq->napi.weight) + return; + + if (__netif_tx_trylock(txq)) { + free_old_xmit_skbs(sq); + __netif_tx_unlock(txq); + } + + if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) + netif_tx_wake_queue(txq); +} + static int virtnet_poll(struct napi_struct *napi, int budget) { struct receive_queue *rq container_of(napi, struct receive_queue, napi); unsigned int received; + virtnet_poll_cleantx(rq); + received = virtnet_receive(rq, budget); /* Out of packets? */ -- 2.12.2.816.g2cccc81164-goog
Willem de Bruijn
2017-Apr-18 20:21 UTC
[PATCH net-next v2 5/5] virtio-net: keep tx interrupts disabled unless kick
From: Willem de Bruijn <willemb at google.com> Tx napi mode increases the rate of transmit interrupts. Suppress some by masking interrupts while more packets are expected. The interrupts will be reenabled before the last packet is sent. This optimization reduces the througput drop with tx napi for unidirectional flows such as UDP_STREAM that do not benefit from cleaning tx completions in the the receive napi handler. Signed-off-by: Willem de Bruijn <willemb at google.com> --- drivers/net/virtio_net.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index b14c82ce0032..b107ae011632 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -1196,8 +1196,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) bool use_napi = sq->napi.weight; /* Free up any pending old buffers before queueing new ones. */ - if (!use_napi) + if (use_napi) { + if (kick) + virtqueue_enable_cb_delayed(sq->vq); + else + virtqueue_disable_cb(sq->vq); + } else { free_old_xmit_skbs(sq); + } /* timestamp packet in software */ skb_tx_timestamp(skb); -- 2.12.2.816.g2cccc81164-goog
On 2017?04?19? 04:21, Willem de Bruijn wrote:> From: Willem de Bruijn <willemb at google.com> > > Convert virtio-net to a standard napi tx completion path. This enables > better TCP pacing using TCP small queues and increases single stream > throughput. > > The virtio-net driver currently cleans tx descriptors on transmission > of new packets in ndo_start_xmit. Latency depends on new traffic, so > is unbounded. To avoid deadlock when a socket reaches its snd limit, > packets are orphaned on tranmission. This breaks socket backpressure, > including TSQ. > > Napi increases the number of interrupts generated compared to the > current model, which keeps interrupts disabled as long as the ring > has enough free descriptors. Keep tx napi optional and disabled for > now. Follow-on patches will reduce the interrupt cost. > > Signed-off-by: Willem de Bruijn <willemb at google.com> > Signed-off-by: Jason Wang <jasowang at redhat.com> > --- > drivers/net/virtio_net.c | 77 +++++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 67 insertions(+), 10 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index b9c1df29892c..c173e85dc7b8 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -33,9 +33,10 @@ > static int napi_weight = NAPI_POLL_WEIGHT; > module_param(napi_weight, int, 0444); > > -static bool csum = true, gso = true; > +static bool csum = true, gso = true, napi_tx; > module_param(csum, bool, 0444); > module_param(gso, bool, 0444); > +module_param(napi_tx, bool, 0644); > > /* FIXME: MTU in config. */ > #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN) > @@ -86,6 +87,8 @@ struct send_queue { > > /* Name of the send queue: output.$index */ > char name[40]; > + > + struct napi_struct napi; > }; > > /* Internal representation of a receive virtqueue */ > @@ -262,12 +265,16 @@ static void virtqueue_napi_complete(struct napi_struct *napi, > static void skb_xmit_done(struct virtqueue *vq) > { > struct virtnet_info *vi = vq->vdev->priv; > + struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; > > /* Suppress further interrupts. */ > virtqueue_disable_cb(vq); > > - /* We were probably waiting for more output buffers. */ > - netif_wake_subqueue(vi->dev, vq2txq(vq)); > + if (napi->weight) > + virtqueue_napi_schedule(napi, vq); > + else > + /* We were probably waiting for more output buffers. */ > + netif_wake_subqueue(vi->dev, vq2txq(vq)); > } > > static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx) > @@ -972,6 +979,21 @@ static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi) > local_bh_enable(); > } > > +static void virtnet_napi_tx_enable(struct virtnet_info *vi, > + struct virtqueue *vq, > + struct napi_struct *napi) > +{ > + if (!napi->weight) > + return; > + > + if (!vi->affinity_hint_set) { > + napi->weight = 0; > + return; > + } > + > + return virtnet_napi_enable(vq, napi); > +} > + > static void refill_work(struct work_struct *work) > { > struct virtnet_info *vi > @@ -1046,6 +1068,7 @@ static int virtnet_open(struct net_device *dev) > if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL)) > schedule_delayed_work(&vi->refill, 0); > virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi); > + virtnet_napi_tx_enable(vi, vi->sq[i].vq, &vi->sq[i].napi); > } > > return 0; > @@ -1081,6 +1104,25 @@ static void free_old_xmit_skbs(struct send_queue *sq) > u64_stats_update_end(&stats->tx_syncp); > } > > +static int virtnet_poll_tx(struct napi_struct *napi, int budget) > +{ > + struct send_queue *sq = container_of(napi, struct send_queue, napi); > + struct virtnet_info *vi = sq->vq->vdev->priv; > + struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); > + > + if (__netif_tx_trylock(txq)) { > + free_old_xmit_skbs(sq); > + __netif_tx_unlock(txq); > + } > + > + virtqueue_napi_complete(napi, sq->vq, 0); > + > + if (sq->vq->num_free >= 2 + MAX_SKB_FRAGS) > + netif_tx_wake_queue(txq); > + > + return 0; > +} > + > static int xmit_skb(struct send_queue *sq, struct sk_buff *skb) > { > struct virtio_net_hdr_mrg_rxbuf *hdr; > @@ -1130,9 +1172,11 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > int err; > struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum); > bool kick = !skb->xmit_more; > + bool use_napi = sq->napi.weight; > > /* Free up any pending old buffers before queueing new ones. */ > - free_old_xmit_skbs(sq); > + if (!use_napi) > + free_old_xmit_skbs(sq);I'm not sure this is best or even correct. Consider we clean xmit packets speculatively in virtnet_poll_tx(), we need call free_old_xmit_skbs() unconditionally. This can also help to reduce the possible of napi rescheduling in virtnet_poll_tx(). Thanks
Jason Wang
2017-Apr-20 06:17 UTC
[PATCH net-next v2 5/5] virtio-net: keep tx interrupts disabled unless kick
On 2017?04?19? 04:21, Willem de Bruijn wrote:> From: Willem de Bruijn <willemb at google.com> > > Tx napi mode increases the rate of transmit interrupts. Suppress some > by masking interrupts while more packets are expected. The interrupts > will be reenabled before the last packet is sent. > > This optimization reduces the througput drop with tx napi for > unidirectional flows such as UDP_STREAM that do not benefit from > cleaning tx completions in the the receive napi handler. > > Signed-off-by: Willem de Bruijn <willemb at google.com> > --- > drivers/net/virtio_net.c | 8 +++++++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index b14c82ce0032..b107ae011632 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -1196,8 +1196,14 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) > bool use_napi = sq->napi.weight; > > /* Free up any pending old buffers before queueing new ones. */ > - if (!use_napi) > + if (use_napi) { > + if (kick) > + virtqueue_enable_cb_delayed(sq->vq); > + else > + virtqueue_disable_cb(sq->vq);Since virtqueue_disable_cb() do nothing for event idx. I wonder whether or not just calling enable_cb_dealyed() is ok here. Btw, it does not disable interrupt at all, I propose a patch in the past which can do more than this: https://patchwork.kernel.org/patch/6472601/ Thanks> + } else { > free_old_xmit_skbs(sq); > + } > > /* timestamp packet in software */ > skb_tx_timestamp(skb);
On 2017?04?19? 04:21, Willem de Bruijn wrote:> +static void virtnet_napi_tx_enable(struct virtnet_info *vi, > + struct virtqueue *vq, > + struct napi_struct *napi) > +{ > + if (!napi->weight) > + return; > + > + if (!vi->affinity_hint_set) { > + napi->weight = 0; > + return; > + } > + > + return virtnet_napi_enable(vq, napi); > +} > + > static void refill_work(struct work_struct *work)Maybe I was wrong, but according to Michael's comment it looks like he want check affinity_hint_set just for speculative tx polling on rx napi instead of disabling it at all. And I'm not convinced this is really needed, driver only provide affinity hint instead of affinity, so it's not guaranteed that tx and rx interrupt are in the same vcpus. Thanks