[PATCH RFC v3 0/3] virtio_net: enabling tx interrupts

[PATCH RFC v3 0/3] virtio_net: enabling tx interrupts

[Michael S. Tsirkin]

RFC patches to enable tx interrupts.
This is to demonstrate how this can be done without
core virtio changes, and to make sure I understand
the new APIs correctly.

Testing TBD, I was asked for a version for early testing.

Applies on top of patch: "virtio_net: fix use after free"
that I recently sent.

Changes from v3:
	clean up code, address issues raised by Jason
Changes from v1:
        address comments by Jason Wang, use delayed cb everywhere
        rebased Jason's patch on top of mine and include it (with some tweaks)

Jason Wang (1):
  virtio-net: optimize free_old_xmit_skbs stats

Michael S. Tsirkin (2):
  virtio_net: enable tx interrupt
  virtio_net: bql

 drivers/net/virtio_net.c | 144 +++++++++++++++++++++++++++++++++--------------
 1 file changed, 101 insertions(+), 43 deletions(-)

-- 
MST

[PATCH RFC v3 1/3] virtio_net: enable tx interrupt

[Michael S. Tsirkin]

On newer hosts that support delayed tx interrupts,
we probably don't have much to gain from orphaning
packets early.

Based on patch by Jason Wang.

Note: this might degrade performance for
hosts without event idx support.
Should be addressed by the next patch.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 133 +++++++++++++++++++++++++++++++----------------
 1 file changed, 89 insertions(+), 44 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 13d0a8b..14f4cda 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -72,6 +72,8 @@ struct send_queue {
 
 	/* Name of the send queue: output.$index */
 	char name[40];
+
+	struct napi_struct napi;
 };
 
 /* Internal representation of a receive virtqueue */
@@ -217,15 +219,41 @@ static struct page *get_a_page(struct receive_queue *rq, gfp_t gfp_mask)
 	return p;
 }
 
+static unsigned int free_old_xmit_skbs(struct netdev_queue *txq,
+				       struct send_queue *sq, int budget)
+{
+	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;
+
+	while (packets < budget &&
+	       (skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
+		pr_debug("Sent skb %p\n", skb);
+
+		u64_stats_update_begin(&stats->tx_syncp);
+		stats->tx_bytes += skb->len;
+		stats->tx_packets++;
+		u64_stats_update_end(&stats->tx_syncp);
+
+		dev_kfree_skb_any(skb);
+		packets++;
+	}
+
+	if (sq->vq->num_free >= 2+MAX_SKB_FRAGS)
+		netif_tx_start_queue(txq);
+
+	return packets;
+}
+
 static void skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->vdev->priv;
+	struct send_queue *sq = &vi->sq[vq2txq(vq)];
 
-	/* Suppress further interrupts. */
-	virtqueue_disable_cb(vq);
-
-	/* We were probably waiting for more output buffers. */
-	netif_wake_subqueue(vi->dev, vq2txq(vq));
+	virtqueue_disable_cb(sq->vq);
+	napi_schedule(&sq->napi);
 }
 
 static unsigned int mergeable_ctx_to_buf_truesize(unsigned long mrg_ctx)
@@ -774,6 +802,31 @@ again:
 	return received;
 }
 
+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));
+	unsigned int sent;
+
+	__netif_tx_lock(txq, smp_processor_id());
+	sent = free_old_xmit_skbs(txq, sq, budget);
+	if (sent < budget) {
+		napi_complete(napi);
+		/* Note: we must enable cb *after* napi_complete, because
+		 * napi_schedule calls from callbacks that trigger before
+		 * napi_complete are ignored.
+		 */
+		if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
+			virtqueue_disable_cb(sq->vq);
+			napi_schedule(&sq->napi);
+		}
+	}
+	__netif_tx_unlock(txq);
+	return sent;
+}
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
 /* must be called with local_bh_disable()d */
 static int virtnet_busy_poll(struct napi_struct *napi)
@@ -822,30 +875,12 @@ static int virtnet_open(struct net_device *dev)
 			if (!try_fill_recv(&vi->rq[i], GFP_KERNEL))
 				schedule_delayed_work(&vi->refill, 0);
 		virtnet_napi_enable(&vi->rq[i]);
+		napi_enable(&vi->sq[i].napi);
 	}
 
 	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);
-
-	while ((skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
-		pr_debug("Sent skb %p\n", skb);
-
-		u64_stats_update_begin(&stats->tx_syncp);
-		stats->tx_bytes += skb->len;
-		stats->tx_packets++;
-		u64_stats_update_end(&stats->tx_syncp);
-
-		dev_kfree_skb_any(skb);
-	}
-}
-
 static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 {
 	struct skb_vnet_hdr *hdr;
@@ -911,7 +946,9 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
 		sg_set_buf(sq->sg, hdr, hdr_len);
 		num_sg = skb_to_sgvec(skb, sq->sg + 1, 0, skb->len) + 1;
 	}
-	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
+
+	return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb,
+				    GFP_ATOMIC);
 }
 
 static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
@@ -923,8 +960,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
 	bool kick = !skb->xmit_more;
 
-	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	virtqueue_disable_cb(sq->vq);
 
 	/* Try to transmit */
 	err = xmit_skb(sq, skb);
@@ -940,27 +976,26 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_OK;
 	}
 
-	/* Don't wait up for transmitted skbs to be freed. */
-	skb_orphan(skb);
-	nf_reset(skb);
-
 	/* Apparently nice girls don't return TX_BUSY; stop the queue
 	 * before it gets out of hand.  Naturally, this wastes entries. */
-	if (sq->vq->num_free < 2+MAX_SKB_FRAGS) {
+	if (sq->vq->num_free < 2+MAX_SKB_FRAGS)
 		netif_stop_subqueue(dev, qnum);
-		if (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) {
-				netif_start_subqueue(dev, qnum);
-				virtqueue_disable_cb(sq->vq);
-			}
-		}
-	}
 
 	if (kick || netif_xmit_stopped(txq))
 		virtqueue_kick(sq->vq);
 
+	/* Try to pop off some buffers before we re-enable callbacks.
+	 * It makes sense to do it after kick, since that causes
+	 * device to process packets.
+	 */
+	if (free_old_xmit_skbs(txq, sq, NAPI_POLL_WEIGHT) < NAPI_POLL_WEIGHT) {
+		if (unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
+			virtqueue_disable_cb(sq->vq);
+			napi_schedule(&sq->napi);
+		}
+	} else {
+		napi_schedule(&sq->napi);
+	}
 	return NETDEV_TX_OK;
 }
 
@@ -1137,8 +1172,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;
 }
@@ -1457,8 +1494,10 @@ static void virtnet_free_queues(struct virtnet_info *vi)
 {
 	int i;
 
-	for (i = 0; i < vi->max_queue_pairs; i++)
+	for (i = 0; i < vi->max_queue_pairs; i++) {
 		netif_napi_del(&vi->rq[i].napi);
+		netif_napi_del(&vi->sq[i].napi);
+	}
 
 	kfree(vi->rq);
 	kfree(vi->sq);
@@ -1612,6 +1651,8 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
 		netif_napi_add(vi->dev, &vi->rq[i].napi, virtnet_poll,
 			       napi_weight);
 		napi_hash_add(&vi->rq[i].napi);
+		netif_napi_add(vi->dev, &vi->sq[i].napi, virtnet_poll_tx,
+			       napi_weight);
 
 		sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
 		ewma_init(&vi->rq[i].mrg_avg_pkt_len, 1, RECEIVE_AVG_WEIGHT);
@@ -1916,8 +1957,10 @@ static int virtnet_freeze(struct virtio_device *vdev)
 	if (netif_running(vi->dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
 			napi_disable(&vi->rq[i].napi);
+			napi_disable(&vi->sq[i].napi);
 			napi_hash_del(&vi->rq[i].napi);
 			netif_napi_del(&vi->rq[i].napi);
+			netif_napi_del(&vi->sq[i].napi);
 		}
 	}
 
@@ -1942,8 +1985,10 @@ static int virtnet_restore(struct virtio_device *vdev)
 			if (!try_fill_recv(&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]);
+			napi_enable(&vi->sq[i].napi);
+		}
 	}
 
 	netif_device_attach(vi->dev);
-- 
MST

[PATCH RFC v3 2/3] virtio_net: bql

[Michael S. Tsirkin]

Improve tx batching using byte queue limits.
Should be especially effective for MQ.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 14f4cda..b83d39d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -227,6 +227,7 @@ static unsigned int free_old_xmit_skbs(struct netdev_queue *txq,
 	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 (packets < budget &&
 	       (skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
@@ -234,6 +235,7 @@ static unsigned int free_old_xmit_skbs(struct netdev_queue *txq,
 
 		u64_stats_update_begin(&stats->tx_syncp);
 		stats->tx_bytes += skb->len;
+		bytes += skb->len;
 		stats->tx_packets++;
 		u64_stats_update_end(&stats->tx_syncp);
 
@@ -241,6 +243,8 @@ static unsigned int free_old_xmit_skbs(struct netdev_queue *txq,
 		packets++;
 	}
 
+	netdev_tx_completed_queue(txq, packets, bytes);
+
 	if (sq->vq->num_free >= 2+MAX_SKB_FRAGS)
 		netif_tx_start_queue(txq);
 
@@ -959,6 +963,7 @@ 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;
+	unsigned int bytes = skb->len;
 
 	virtqueue_disable_cb(sq->vq);
 
@@ -976,6 +981,8 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
 		return NETDEV_TX_OK;
 	}
 
+	netdev_tx_sent_queue(txq, bytes);
+
 	/* Apparently nice girls don't return TX_BUSY; stop the queue
 	 * before it gets out of hand.  Naturally, this wastes entries. */
 	if (sq->vq->num_free < 2+MAX_SKB_FRAGS)
-- 
MST

[PATCH RFC v3 3/3] virtio-net: optimize free_old_xmit_skbs stats

[Michael S. Tsirkin]

From: Jason Wang <jasowang@redhat.com>

We already have counters for sent packets and sent bytes.
Use them to reduce the number of u64_stats_update_begin/end().

Take care not to bother with stats update when called
speculatively.

Based on a patch by Jason Wang.

Cc: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
 drivers/net/virtio_net.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b83d39d..c2b69f8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -233,16 +233,22 @@ static unsigned int free_old_xmit_skbs(struct netdev_queue *txq,
 	       (skb = virtqueue_get_buf(sq->vq, &len)) != NULL) {
 		pr_debug("Sent skb %p\n", skb);
 
-		u64_stats_update_begin(&stats->tx_syncp);
-		stats->tx_bytes += skb->len;
 		bytes += skb->len;
-		stats->tx_packets++;
-		u64_stats_update_end(&stats->tx_syncp);
+		packets++;
 
 		dev_kfree_skb_any(skb);
-		packets++;
 	}
 
+	/* Avoid overhead when no packets have been processed
+	 * happens when called speculatively from start_xmit. */
+	if (!packets)
+		return 0;
+
+	u64_stats_update_begin(&stats->tx_syncp);
+	stats->tx_bytes += bytes;
+	stats->tx_packets += packets;
+	u64_stats_update_end(&stats->tx_syncp);
+
 	netdev_tx_completed_queue(txq, packets, bytes);
 
 	if (sq->vq->num_free >= 2+MAX_SKB_FRAGS)
-- 
MST

Re: [PATCH RFC v3 0/3] virtio_net: enabling tx interrupts

[Jason Wang]

On 10/20/2014 02:52 PM, Michael S. Tsirkin wrote:
> RFC patches to enable tx interrupts.
> This is to demonstrate how this can be done without
> core virtio changes, and to make sure I understand
> the new APIs correctly.
>
> Testing TBD, I was asked for a version for early testing.
>
> Applies on top of patch: "virtio_net: fix use after free"
> that I recently sent.
>
> Changes from v3:
> 	clean up code, address issues raised by Jason
> Changes from v1:
>          address comments by Jason Wang, use delayed cb everywhere
>          rebased Jason's patch on top of mine and include it (with some tweaks)
>
> Jason Wang (1):
>    virtio-net: optimize free_old_xmit_skbs stats
>
> Michael S. Tsirkin (2):
>    virtio_net: enable tx interrupt
>    virtio_net: bql
>
>   drivers/net/virtio_net.c | 144 +++++++++++++++++++++++++++++++++--------------
>   1 file changed, 101 insertions(+), 43 deletions(-)
>

I've run a full tests on this series and see huge regression when 
zerocopy is disabled. Looks like the reason is zerocopy could coalescing 
tx completion which greatly reduce the number of tx interrupts.

I will post RFC V4 shortly with interrupt coalescing support. In this 
version I remove the tx packet cleanup in ndo_start_xmit() since it may 
reduce the effects of interrupt coalescing.

Re: [PATCH RFC v3 0/3] virtio_net: enabling tx interrupts

[Michael S. Tsirkin]

On Mon, Dec 01, 2014 at 06:14:36PM +0800, Jason Wang wrote:
> 
> 
> On 10/20/2014 02:52 PM, Michael S. Tsirkin wrote:
> >RFC patches to enable tx interrupts.
> >This is to demonstrate how this can be done without
> >core virtio changes, and to make sure I understand
> >the new APIs correctly.
> >
> >Testing TBD, I was asked for a version for early testing.
> >
> >Applies on top of patch: "virtio_net: fix use after free"
> >that I recently sent.
> >
> >Changes from v3:
> >	clean up code, address issues raised by Jason
> >Changes from v1:
> >         address comments by Jason Wang, use delayed cb everywhere
> >         rebased Jason's patch on top of mine and include it (with some tweaks)
> >
> >Jason Wang (1):
> >   virtio-net: optimize free_old_xmit_skbs stats
> >
> >Michael S. Tsirkin (2):
> >   virtio_net: enable tx interrupt
> >   virtio_net: bql
> >
> >  drivers/net/virtio_net.c | 144 +++++++++++++++++++++++++++++++++--------------
> >  1 file changed, 101 insertions(+), 43 deletions(-)
> >
> 
> I've run a full tests on this series and see huge regression when zerocopy
> is disabled. Looks like the reason is zerocopy could coalescing tx
> completion which greatly reduce the number of tx interrupts.

I think you refer to this code:

        /*
         * Trigger polling thread if guest stopped submitting new
         * buffers:
         * in this case, the refcount after decrement will eventually
         * reach 1.
         * We also trigger polling periodically after each 16 packets
         * (the value 16 here is more or less arbitrary, it's tuned to
         * trigger
         * less than 10% of times).
         */
        if (cnt <= 1 || !(cnt % 16))
                vhost_poll_queue(&vq->poll);

?
This seems unrelated to interrupt coalescing.
We can easily enable something similar for all tx
packets, without need for guest configuration.

If it's not clear how to do this, let me know, I'll try to put out a
patch like this in a couple of days.


> I will post RFC V4 shortly with interrupt coalescing support. In this
> version I remove the tx packet cleanup in ndo_start_xmit() since it may
> reduce the effects of interrupt coalescing.

Maybe split this in a separate patch?

Re: [PATCH RFC v3 0/3] virtio_net: enabling tx interrupts

[Jason Wang]

On Mon, Dec 1, 2014 at 6:48 PM, Michael S. Tsirkin <mst@redhat.com> 
wrote:
> On Mon, Dec 01, 2014 at 06:14:36PM +0800, Jason Wang wrote:
>>  
>>  
>>  On 10/20/2014 02:52 PM, Michael S. Tsirkin wrote:
>>  >RFC patches to enable tx interrupts.
>>  >This is to demonstrate how this can be done without
>>  >core virtio changes, and to make sure I understand
>>  >the new APIs correctly.
>>  >
>>  >Testing TBD, I was asked for a version for early testing.
>>  >
>>  >Applies on top of patch: "virtio_net: fix use after free"
>>  >that I recently sent.
>>  >
>>  >Changes from v3:
>>  >	clean up code, address issues raised by Jason
>>  >Changes from v1:
>>  >         address comments by Jason Wang, use delayed cb everywhere
>>  >         rebased Jason's patch on top of mine and include it (with 
>> some tweaks)
>>  >
>>  >Jason Wang (1):
>>  >   virtio-net: optimize free_old_xmit_skbs stats
>>  >
>>  >Michael S. Tsirkin (2):
>>  >   virtio_net: enable tx interrupt
>>  >   virtio_net: bql
>>  >
>>  >  drivers/net/virtio_net.c | 144 
>> +++++++++++++++++++++++++++++++++--------------
>>  >  1 file changed, 101 insertions(+), 43 deletions(-)
>>  >
>>  
>>  I've run a full tests on this series and see huge regression when 
>> zerocopy
>>  is disabled. Looks like the reason is zerocopy could coalescing tx
>>  completion which greatly reduce the number of tx interrupts.
> 
> I think you refer to this code:
> 
>         /*
>          * Trigger polling thread if guest stopped submitting new
>          * buffers:
>          * in this case, the refcount after decrement will eventually
>          * reach 1.
>          * We also trigger polling periodically after each 16 packets
>          * (the value 16 here is more or less arbitrary, it's tuned to
>          * trigger
>          * less than 10% of times).
>          */
>         if (cnt <= 1 || !(cnt % 16))
>                 vhost_poll_queue(&vq->poll);
> 
> ?
> This seems unrelated to interrupt coalescing.

Well, this in fact tries to coalesce 16 packets per tx irq.

More important, zerocopy depends on host nics tx completion.
This means, if host nic coalesces several packets per irq,
zerocopy will probably also do this. vhost_zerocopy_signal_used()
will try to coalesce the tx intrs.
> 
> We can easily enable something similar for all tx
> packets, without need for guest configuration.

We can, but we lose the an interface for user to tune for 
their applications.
> 
> If it's not clear how to do this, let me know, I'll try to put out a
> patch like this in a couple of days.

We can just do this through harding coding the tx-frames to 16 
through interrupt coalescing. I don't see obvious differences.
And in my test of RFCv4, I just use tx-frames 16 to get the result.

Without a timer for coalescing, I suspect how much this can help
for e.g 1 session of TCP_RR. It just has at most 1 packet pending
during the test. So in fact no tx completion could be coalesced 
in this case.
> 
> 
>>  I will post RFC V4 shortly with interrupt coalescing support. In 
>> this
>>  version I remove the tx packet cleanup in ndo_start_xmit() since it 
>> may
>>  reduce the effects of interrupt coalescing.
> 
> Maybe split this in a separate patch?

I can, but since just two minor changes compared to V3. Maybe just
document the differences is ok for you?