virtio-net: interrupt related improvements

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +19 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
 
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
 
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
 
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}
 

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
 
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
 
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
 
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
 
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
 
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}
 
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)
+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
  			u64_stats_update_begin(&sq->stats.syncp);
  			sq->stats.kicks++;

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi.  It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 83 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------

  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +2 -0

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue 
 *rq, int budget,
       return stats.packets;
   }
   -static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct 
 netdev_queue *txq,
+                   bool use_napi)
   {

      struct sk_buff *skb;
      unsigned int len;

drivers/net/virtio_net.c +2 -0

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct 
 send_queue *sq)
       if (!packets)
           return;
   +    if (use_napi)
+        netdev_tx_completed_queue(txq, packets, bytes);
+
       u64_stats_update_begin(&sq->stats.syncp);
       sq->stats.bytes += bytes;

      sq->stats.packets += packets;

drivers/net/virtio_net.c +1 -1

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct 
 receive_queue *rq)
           return;
         if (__netif_tx_trylock(txq)) {
-        free_old_xmit_skbs(sq);
+        free_old_xmit_skbs(sq, txq, true);
           __netif_tx_unlock(txq);
       }
   @@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)

      struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); __netif_tx_lock(txq, raw_smp_processor_id());
-    free_old_xmit_skbs(sq);
+    free_old_xmit_skbs(sq, txq, true);
      __netif_tx_unlock(txq);
        virtqueue_napi_complete(napi, sq->vq, 0);

drivers/net/virtio_net.c +5 -2

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)
       struct send_queue *sq = &vi->sq[qnum];
       int err;
       struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-    bool kick = !skb->xmit_more;
+    bool more = skb->xmit_more;
       bool use_napi = sq->napi.weight;
+    unsigned int bytes = skb->len;
+    bool kick;
         /* Free up any pending old buffers before queueing new ones. */
-    free_old_xmit_skbs(sq);
+    free_old_xmit_skbs(sq, txq, use_napi);
   -    if (use_napi && kick)
+    if (use_napi && !more)
           virtqueue_enable_cb_delayed(sq->vq);
         /* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)

          if (!use_napi && unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ -            free_old_xmit_skbs(sq);
+            free_old_xmit_skbs(sq, txq, false);
              if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq);

drivers/net/virtio_net.c +6 -0

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)
           }
       }
   -    if (kick || netif_xmit_stopped(txq)) {
+    if (use_napi)
+        kick = __netdev_tx_sent_queue(txq, bytes, more);
+    else
+        kick = !more || netif_xmit_stopped(txq);
+
+    if (kick) {

Quoted 3 lines

drivers/net/virtio_net.c +0 -0

           if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
               u64_stats_update_begin(&sq->stats.syncp);

              sq->stats.kicks++;

_______________________________________________
Virtualization mailing list [email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
From f1c27543dc412778e682b63addbb0a471afc5153 Mon Sep 17 00:00:00 2001

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi.  It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 82 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------

  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +2 -0

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue 
 *rq, int budget,
       return stats.packets;
   }
   -static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct 
 netdev_queue *txq,
+                   bool use_napi)
   {

      struct sk_buff *skb;
      unsigned int len;

drivers/net/virtio_net.c +2 -0

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct 
 send_queue *sq)
       if (!packets)
           return;
   +    if (use_napi)
+        netdev_tx_completed_queue(txq, packets, bytes);
+
       u64_stats_update_begin(&sq->stats.syncp);
       sq->stats.bytes += bytes;

      sq->stats.packets += packets;

drivers/net/virtio_net.c +1 -1

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct 
 receive_queue *rq)
           return;
         if (__netif_tx_trylock(txq)) {
-        free_old_xmit_skbs(sq);
+        free_old_xmit_skbs(sq, txq, true);
           __netif_tx_unlock(txq);
       }
   @@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)

      struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq)); __netif_tx_lock(txq, raw_smp_processor_id());
-    free_old_xmit_skbs(sq);
+    free_old_xmit_skbs(sq, txq, true);
      __netif_tx_unlock(txq);
        virtqueue_napi_complete(napi, sq->vq, 0);

drivers/net/virtio_net.c +5 -2

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)
       struct send_queue *sq = &vi->sq[qnum];
       int err;
       struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-    bool kick = !skb->xmit_more;
+    bool more = skb->xmit_more;
       bool use_napi = sq->napi.weight;
+    unsigned int bytes = skb->len;
+    bool kick;
         /* Free up any pending old buffers before queueing new ones. */
-    free_old_xmit_skbs(sq);
+    free_old_xmit_skbs(sq, txq, use_napi);
   -    if (use_napi && kick)
+    if (use_napi && !more)
           virtqueue_enable_cb_delayed(sq->vq);
         /* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)

          if (!use_napi && unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ -            free_old_xmit_skbs(sq);
+            free_old_xmit_skbs(sq, txq, false);
              if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq);

drivers/net/virtio_net.c +5 -0

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff 
 *skb, struct net_device *dev)
           }
       }
   -    if (kick || netif_xmit_stopped(txq)) {
+    if (use_napi)
+        kick = __netdev_tx_sent_queue(txq, bytes, more);
+    else
+        kick = !more || netif_xmit_stopped(txq);
+

Quoted 4 lines

drivers/net/virtio_net.c +1 -0

+    if (kick) {
           if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
               u64_stats_update_begin(&sq->stats.syncp);

              sq->stats.kicks++;

_______________________________________________
Virtualization mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 72 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
  			u64_stats_update_begin(&sq->stats.syncp);
  			sq->stats.kicks++;

Quoted 17 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Quoted 7 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

I don't see how it's for correctness to be frank.

What if we just do the bulk free? Does that fix the regression?

Quoted 73 lines

Quoted 73 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance,

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
  			u64_stats_update_begin(&sq->stats.syncp);
  			sq->stats.kicks++;

Quoted 6 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

Quoted 7 lines

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

Quoted 5 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

Quoted 81 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 13 lines · Michael S. Tsirkin

Quoted 12 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Quoted 9 lines

Quoted 6 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Thanks

Quoted 82 lines

Quoted 82 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 13 lines · Michael S. Tsirkin

Quoted 12 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Quoted 9 lines

Quoted 6 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Thanks

Quoted 90 lines

Quoted 90 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 79 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +19 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)
+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 5 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 12 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 81 lines

Quoted 81 lines

Quoted 81 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
    drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
    1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
    	return stats.packets;
    }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
    {
    	struct sk_buff *skb;
    	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
    	if (!packets)
    		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
    	u64_stats_update_begin(&sq->stats.syncp);
    	sq->stats.bytes += bytes;
    	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
    		return;
    	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
    		__netif_tx_unlock(txq);
    	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
    	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
    	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
    	__netif_tx_unlock(txq);
    	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    	struct send_queue *sq = &vi->sq[qnum];
    	int err;
    	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
    	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
    	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
    		virtqueue_enable_cb_delayed(sq->vq);
    	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    		if (!use_napi &&
    		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
    			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
    			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
    				netif_start_subqueue(dev, qnum);
    				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    		}
    	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
    		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
    			u64_stats_update_begin(&sq->stats.syncp);
    			sq->stats.kicks++;

Quoted 34 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 5 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 12 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 3 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Quoted 82 lines

Quoted 82 lines

Quoted 82 lines

Quoted 82 lines

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
    drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
    1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
    	return stats.packets;
    }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
    {
    	struct sk_buff *skb;
    	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
    	if (!packets)
    		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
    	u64_stats_update_begin(&sq->stats.syncp);
    	sq->stats.bytes += bytes;
    	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
    		return;
    	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
    		__netif_tx_unlock(txq);
    	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
    	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
    	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
    	__netif_tx_unlock(txq);
    	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    	struct send_queue *sq = &vi->sq[qnum];
    	int err;
    	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
    	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
    	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
    		virtqueue_enable_cb_delayed(sq->vq);
    	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    		if (!use_napi &&
    		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
    			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
    			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
    				netif_start_subqueue(dev, qnum);
    				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
    		}
    	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
    		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
    			u64_stats_update_begin(&sq->stats.syncp);
    			sq->stats.kicks++;

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 1 line

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Thanks

Quoted 2 lines

Quoted 60 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

Quoted 3 lines

I don't know. But how a "more" flag can help here?

Thanks

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

Quoted 63 lines · Michael S. Tsirkin

Quoted 62 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Quoted 4 lines

Quoted 1 line

Thanks

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

I agree but the problem is to balance the PPS and throughput. More batching helps for PPS but may damage TCP throughput.

Quoted 4 lines

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

It's under the control of guest, so I'm afraid we still need some more guard (e.g time/bytes counters) on host.

Thanks

Quoted 2 lines

Quoted 64 lines · Jason Wang

Quoted 63 lines · Michael S. Tsirkin

Quoted 62 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Thanks

Quoted 6 lines

Quoted 3 lines

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

I agree but the problem is to balance the PPS and throughput. More batching helps for PPS but may damage TCP throughput.

That is what more flag is supposed to be I think - it is only set if there's a socket that actually needs the skb freed in order to go on.

Quoted 4 lines

Quoted 3 lines

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

Quoted 4 lines

It's under the control of guest, so I'm afraid we still need some more guard (e.g time/bytes counters) on host.

Thanks

Point is if guest does not care about the skb being freed, then there is no rush host side to mark buffer used.

Quoted 65 lines · Michael S. Tsirkin

Quoted 64 lines · Jason Wang

Quoted 63 lines · Michael S. Tsirkin

Quoted 62 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Thanks

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

I agree but the problem is to balance the PPS and throughput. More batching helps for PPS but may damage TCP throughput.

That is what more flag is supposed to be I think - it is only set if there's a socket that actually needs the skb freed in order to go on.

I'm not quite sure I get, but is this something similar to what you want?

https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027667.html

Which enables tx interrupt for TCP packets, and you want to add used more aggressively for those sockets?

Thanks

Quoted 13 lines

Quoted 9 lines

Quoted 4 lines

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

It's under the control of guest, so I'm afraid we still need some more guard (e.g time/bytes counters) on host.

Thanks

Point is if guest does not care about the skb being freed, then there is no rush host side to mark buffer used.

Quoted 82 lines · Jason Wang

Quoted 64 lines · Michael S. Tsirkin

Quoted 63 lines · Jason Wang

Quoted 62 lines · Michael S. Tsirkin

Quoted 61 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are
similar to wifi. It's worth considering whether something similar to
commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be
benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

Thanks

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

I agree but the problem is to balance the PPS and throughput. More batching helps for PPS but may damage TCP throughput.

That is what more flag is supposed to be I think - it is only set if there's a socket that actually needs the skb freed in order to go on.

I'm not quite sure I get, but is this something similar to what you want?

https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027667.html

Which enables tx interrupt for TCP packets, and you want to add used more aggressively for those sockets?

Quoted 1 line

Thanks

That's the idea.
But then you said we can just play with event index
instead. I think the answer to why not do that is that it's tricky to do without races.

We need to think about the exact semantics: e.g. I think it is better to keep interrupts on and then saying "I promise sending more buffers even if you do not use any buffers so using this one is not urgent" rather than as your patches do keeping them off and then saying "this one is urgent".

The reason being is that "I promise to send more" is more informative and can allow better batching for the host.

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

Quoted 8 lines

Quoted 8 lines

Quoted 4 lines

It's under the control of guest, so I'm afraid we still need some more guard (e.g time/bytes counters) on host.

Thanks

Point is if guest does not care about the skb being freed, then there is no rush host side to mark buffer used.

Quoted 65 lines · Michael S. Tsirkin

Quoted 64 lines · Jason Wang

Quoted 63 lines · Michael S. Tsirkin

Quoted 62 lines · Jason Wang

Quoted 45 lines · Michael S. Tsirkin

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 7 lines

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

I'm not sure, it might be too big.

Quoted 3 lines

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

I don't know. But how a "more" flag can help here?

Thanks

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

It sounds like we should be a bit more aggressive in updating used ring. But if we just do it naively we will harm performance for sure as that is how we are doing batching right now.

I agree but the problem is to balance the PPS and throughput. More batching helps for PPS but may damage TCP throughput.

That is what more flag is supposed to be I think - it is only set if there's a socket that actually needs the skb freed in order to go on.

I'm not quite sure I get, but is this something similar to what you want?

https://lists.linuxfoundation.org/pipermail/virtualization/2014-October/027667.html

Which enables tx interrupt for TCP packets, and you want to add used more aggressively for those sockets?

Thanks

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

Instead we could make guest
control batching using the more flag - if that's not set we write out the used ring.

Quoted 8 lines

Quoted 4 lines

It's under the control of guest, so I'm afraid we still need some more guard (e.g time/bytes counters) on host.

Thanks

Point is if guest does not care about the skb being freed, then there is no rush host side to mark buffer used.

Quoted 44 lines · Jason Wang

Quoted 32 lines · Michael S. Tsirkin

Quoted 13 lines · Jason Wang

Quoted 12 lines · Michael S. Tsirkin

Quoted 11 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well. I did it by tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

Quoted 4 lines

Quoted 2 lines

Quoted 1 line

But when testing with vhost-net, I don't very a stable performance,

So how about increasing TSQ pacing shift then?

I can test this. But changing default TCP value is much more than a virtio-net specific thing.

Well same logic as wifi applies. Unpredictable latencies related to radio in one case, to host scheduler in the other.

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

it was
probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

I don't think it's reasonable to expect userspace to be that smart ... Why do we need time bounded? used ring is always updated when ring becomes empty.

We don't add used when means BQL may not see the consumed packet in time. And the delay varies based on the workload since we count packets not bytes or time before doing the batched updating.

Quoted 5 lines

Quoted 1 line

Thanks

Sorry I still don't get it.
When nothing is outstanding then we do update the used. So if BQL stops userspace from sending packets then we get an interrupt and packets start flowing again.

Yes, but how about the cases of multiple flows. That's where I see unstable results.

Quoted 2 lines

It might be suboptimal, we might need to tune it but I doubt running timers is a solution, timer interrupts cause VM exits.

Quoted 6 lines

Probably not a timer but a time counter (or event byte counter) in vhost to add used and signal guest if it exceeds a value instead of waiting the number of packets.

Thanks

Well we already have VHOST_NET_WEIGHT - is it too big then?

And maybe we should expose the "MORE" flag in the descriptor - do you think that will help?

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 79 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +19 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)
+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
  			u64_stats_update_begin(&sq->stats.syncp);
  			sq->stats.kicks++;

Quoted 9 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Quoted 99 lines

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 79 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +19 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)
+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

BQL try to consume those packets when lead WARN for dql.

Thanks

Quoted 101 lines

Quoted 101 lines

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 79 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +19 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)
+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 27 lines · Jason Wang

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

Quoted 4 lines

BQL try to consume those packets when lead WARN for dql.

Thanks

Can one really switch tx napi on and off? How?
While root can change the napi_tx module parameter, I don't think that has any effect outside device probe time. What did I miss?

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Jason Wang

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

Quoted 10 lines

Quoted 4 lines

BQL try to consume those packets when lead WARN for dql.

Thanks

Can one really switch tx napi on and off? How?
While root can change the napi_tx module parameter, I don't think that has any effect outside device probe time. What did I miss?

We support switch the mode through ethtool recently. See

commit 0c465be183c7c57a26446df6ea96d8676b865f92
Author: Jason Wang <[email protected]>
Date:   Tue Oct 9 10:06:26 2018 +0800

    virtio_net: ethtool tx napi configuration

    Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers. Interrupt moderation is currently not supported, so these accept and display the default settings of 0 usec and 1 frame.

    Toggle tx napi through setting tx-frames. So as to not interfere with possible future interrupt moderation, value 1 means tx napi while value 0 means not.

    Only allow the switching when device is down for simplicity.

Link https://patchwork.ozlabs.org/patch/948149/

Suggested-by Jason Wang <[email protected]>

Signed-off-by Willem de Bruijn <[email protected]>

Signed-off-by Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Thanks

Quoted 63 lines · Jason Wang

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Jason Wang

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

Quoted 10 lines

Quoted 4 lines

BQL try to consume those packets when lead WARN for dql.

Thanks

Can one really switch tx napi on and off? How?
While root can change the napi_tx module parameter, I don't think that has any effect outside device probe time. What did I miss?

We support switch the mode through ethtool recently. See

commit 0c465be183c7c57a26446df6ea96d8676b865f92
Author: Jason Wang <[email protected]>
Date:   Tue Oct 9 10:06:26 2018 +0800

    virtio_net: ethtool tx napi configuration

    Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers. Interrupt moderation is currently not supported, so these accept and display the default settings of 0 usec and 1 frame.

    Toggle tx napi through setting tx-frames. So as to not interfere with possible future interrupt moderation, value 1 means tx napi while value 0 means not.

    Only allow the switching when device is down for simplicity.

Link https://patchwork.ozlabs.org/patch/948149/

Suggested-by Jason Wang <[email protected]>

Signed-off-by Willem de Bruijn <[email protected]>

Signed-off-by Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Quoted 1 line

Thanks

It's disabled when device is up - isn't that enough?

Quoted 63 lines · Jason Wang

Quoted 28 lines · Michael S. Tsirkin

Quoted 27 lines · Jason Wang

Quoted 9 lines · Michael S. Tsirkin

Quoted 8 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

Quoted 10 lines

Quoted 4 lines

BQL try to consume those packets when lead WARN for dql.

Thanks

Can one really switch tx napi on and off? How?
While root can change the napi_tx module parameter, I don't think that has any effect outside device probe time. What did I miss?

We support switch the mode through ethtool recently. See

commit 0c465be183c7c57a26446df6ea96d8676b865f92
Author: Jason Wang <[email protected]>
Date:   Tue Oct 9 10:06:26 2018 +0800

    virtio_net: ethtool tx napi configuration

    Implement ethtool .set_coalesce (-C) and .get_coalesce (-c) handlers. Interrupt moderation is currently not supported, so these accept and display the default settings of 0 usec and 1 frame.

    Toggle tx napi through setting tx-frames. So as to not interfere with possible future interrupt moderation, value 1 means tx napi while value 0 means not.

    Only allow the switching when device is down for simplicity.

Link https://patchwork.ozlabs.org/patch/948149/

Suggested-by Jason Wang <[email protected]>

Signed-off-by Willem de Bruijn <[email protected]>

Signed-off-by Jason Wang <[email protected]>

Signed-off-by David S. Miller <[email protected]>

Quoted 1 line

Thanks

It's disabled when device is up - isn't that enough?

Quoted 11 lines · Michael S. Tsirkin

Quoted 9 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

Quoted 6 lines

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Consider the case:

TX NAPI is disabled:

send N packets

turn TX NAPI on:

get tx interrupt

BQL try to consume those packets when lead WARN for dql.

Thanks

Quoted 93 lines

Quoted 93 lines

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 9 lines · Jason Wang

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

I just went over the patch again and I don't understand this comment. This patch only enabled BQL with tx napi.

Thus there's no mode switching.

What did I miss?

Quoted 91 lines

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
   drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
   1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
   	return stats.packets;
   }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
   {
   	struct sk_buff *skb;
   	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
   	if (!packets)
   		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
   	u64_stats_update_begin(&sq->stats.syncp);
   	sq->stats.bytes += bytes;
   	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
   		return;
   	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
   		__netif_tx_unlock(txq);
   	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
   	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
   	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
   	__netif_tx_unlock(txq);
   	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   	struct send_queue *sq = &vi->sq[qnum];
   	int err;
   	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
   	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
   	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
   		virtqueue_enable_cb_delayed(sq->vq);
   	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		if (!use_napi &&
   		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
   			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
   			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
   				netif_start_subqueue(dev, qnum);
   				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
   		}
   	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

Quoted 8 lines

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
   		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
   			u64_stats_update_begin(&sq->stats.syncp);
   			sq->stats.kicks++;

Quoted 4 lines · Michael S. Tsirkin

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

I've played a similar patch several days before. The tricky part is the mode switching between napi and no napi. We should make sure when the packet is sent and trakced by BQL,  it should be consumed by BQL as well.

I did it by
tracking it through skb->cb.  And deal with the freeze by reset the BQL status. Patch attached.

But when testing with vhost-net, I don't very a stable performance, it was probably because we batch the used ring updating so tx interrupt may come randomly. We probably need to implement time bounded coalescing mechanism which could be configured from userspace.

Btw, maybe it's time just enable napi TX by default. I get ~10% TCP_RR regression on machine without APICv, (haven't found time to test APICv machine). But consider it was for correctness, I think it's acceptable? Then we can do optimization on top?

Thanks

Quoted 71 lines

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +14 -8

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}
-	if (kick || netif_xmit_stopped(txq)) {
+	if (use_napi)

drivers/net/virtio_net.c static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev) · line 1577

Quoted 8 lines

drivers/net/virtio_net.c +5 -0

+		kick = __netdev_tx_sent_queue(txq, bytes, more);
+	else
+		kick = !more || netif_xmit_stopped(txq);
+
+	if (kick) {
  		if (virtqueue_kick_prepare(sq->vq) && virtqueue_notify(sq->vq)) {
  			u64_stats_update_begin(&sq->stats.syncp);
  			sq->stats.kicks++;

When use_napi is set, let's enable BQLs. Note: some of the issues are similar to wifi. It's worth considering whether something similar to commit 36148c2bbfbe ("mac80211: Adjust TSQ pacing shift") might be benefitial.

Signed-off-by Michael S. Tsirkin <[email protected]>

---
  drivers/net/virtio_net.c | 27 +++++++++++++++++++--------
  1 file changed, 19 insertions(+), 8 deletions(-)

drivers/net/virtio_net.c +13 -7

index cecfd77c9f3c..b657bde6b94b 100644
@@ -1325,7 +1325,8 @@ static int virtnet_receive(struct receive_queue *rq, int budget,
  	return stats.packets;
  }
 
-static void free_old_xmit_skbs(struct send_queue *sq)
+static void free_old_xmit_skbs(struct send_queue *sq, struct netdev_queue *txq,
+			       bool use_napi)
  {
  	struct sk_buff *skb;
  	unsigned int len;

@@ -1347,6 +1348,9 @@ static void free_old_xmit_skbs(struct send_queue *sq)
  	if (!packets)
  		return;
 
+	if (use_napi)
+		netdev_tx_completed_queue(txq, packets, bytes);
+
  	u64_stats_update_begin(&sq->stats.syncp);
  	sq->stats.bytes += bytes;
  	sq->stats.packets += packets;

@@ -1364,7 +1368,7 @@ static void virtnet_poll_cleantx(struct receive_queue *rq)
  		return;
 
  	if (__netif_tx_trylock(txq)) {
-		free_old_xmit_skbs(sq);
+		free_old_xmit_skbs(sq, txq, true);
  		__netif_tx_unlock(txq);
  	}
 

@@ -1440,7 +1444,7 @@ static int virtnet_poll_tx(struct napi_struct *napi, int budget)
  	struct netdev_queue *txq = netdev_get_tx_queue(vi->dev, vq2txq(sq->vq));
 
  	__netif_tx_lock(txq, raw_smp_processor_id());
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, true);
  	__netif_tx_unlock(txq);
 
  	virtqueue_napi_complete(napi, sq->vq, 0);

@@ -1505,13 +1509,15 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  	struct send_queue *sq = &vi->sq[qnum];
  	int err;
  	struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
-	bool kick = !skb->xmit_more;
+	bool more = skb->xmit_more;
  	bool use_napi = sq->napi.weight;
+	unsigned int bytes = skb->len;
+	bool kick;
 
  	/* Free up any pending old buffers before queueing new ones. */
-	free_old_xmit_skbs(sq);
+	free_old_xmit_skbs(sq, txq, use_napi);
 
-	if (use_napi && kick)
+	if (use_napi && !more)
  		virtqueue_enable_cb_delayed(sq->vq);
 
  	/* timestamp packet in software */

@@ -1552,7 +1558,7 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		if (!use_napi &&
  		    unlikely(!virtqueue_enable_cb_delayed(sq->vq))) {
  			/* More just got used, free them then recheck. */
-			free_old_xmit_skbs(sq);
+			free_old_xmit_skbs(sq, txq, false);
  			if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) {
  				netif_start_subqueue(dev, qnum);
  				virtqueue_disable_cb(sq->vq);

@@ -1560,7 +1566,12 @@ static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
  		}
  	}