[net-next,1/8] virtio-net: initially change the value of tx-frames

Commit Message

Heng Qi Aug. 11, 2023, 6:55 a.m. UTC

Background:
1. Commit 0c465be183c7 ("virtio_net: ethtool tx napi configuration") uses
   tx-frames to toggle napi_tx (0 off and 1 on) if notification coalescing
   is not supported.
2. Commit 31c03aef9bc2 ("virtio_net: enable napi_tx by default") enables
   napi_tx for all txqs by default.

Status:
When virtio-net supports notification coalescing, after initialization,
tx-frames is 0 and napi_tx is true.

Problem:
1. When the user only wants to set rx coalescing params using
           ethtool -C eth0 rx-usecs 10, or
	   ethtool -Q eth0 queue_mask 0x1 -C rx-usecs 10,
   these cmds will carry tx-frames as 0, causing the napi_tx switching condition
   is satisfied. Then the user gets:
           netlink error: Device or resource busy.
   The same happens when trying to set rx-frames, adaptive_{rx, tx}.

2. When notification coalescing is not supported, switching napi_tx is
   a way to achieve coalescing. But now coalescing is supported,
   we don't need tx-frames to toggle napi_tx anymore.

3. The value of supported tx-frames cannot be set arbitrarily.
   When setting 0 and non-zero value, if the device is up, the user will always
   get EBUSY warning. Not intuitive.

Result:
When notification coalescing feature is negotiated, initially change the
value of tx-frames to be consistent with napi_tx.

Reported-by: Xiaoming Zhao <[e-mail]>
Signed-off-by: Heng Qi <[e-mail]>
---
 drivers/net/virtio_net.c | 42 +++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 82f9b899b938..3b254f778e7e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4376,13 +4376,6 @@  static int virtnet_probe(struct virtio_device *vdev)
 		dev->xdp_features |= NETDEV_XDP_ACT_RX_SG;
 	}
 
-	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
-		vi->intr_coal_rx.max_usecs = 0;
-		vi->intr_coal_tx.max_usecs = 0;
-		vi->intr_coal_tx.max_packets = 0;
-		vi->intr_coal_rx.max_packets = 0;
-	}
-
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
 		vi->has_rss_hash_report = true;
 
@@ -4457,6 +4450,41 @@  static int virtnet_probe(struct virtio_device *vdev)
 	if (err)
 		goto free;
 
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
+		vi->intr_coal_rx.max_usecs = 0;
+		vi->intr_coal_tx.max_usecs = 0;
+		vi->intr_coal_rx.max_packets = 0;
+
+		/* Why is this needed?
+		 * If without this setting, consider that when VIRTIO_NET_F_NOTF_COAL is
+		 * negotiated and napi_tx is initially true: when the user sets non tx-frames
+		 * parameters, such as the following cmd or others,
+		 *		ethtool -C eth0 rx-usecs 10.
+		 * Then
+		 * 1. ethtool_set_coalesce() first calls virtnet_get_coalesce() to get
+		 *    the last parameters except rx-usecs. If tx-frames has never been set before,
+		 *    virtnet_get_coalesce() returns with tx-frames=0 in the parameters.
+		 * 2. virtnet_set_coalesce() is then called, according to 1:
+		 *    ec->tx_max_coalesced_frames=0. Now napi_tx switching condition is met.
+		 * 3. If the device is up, the user setting fails:
+		 *	       "netlink error: Device or resource busy"
+		 * This is not intuitive. Therefore, we keep napi_tx state consistent with
+		 * tx-frames when VIRTIO_NET_F_NOTF_COAL is negotiated. This behavior is
+		 * compatible with before.
+		 */
+		if (vi->sq[0].napi.weight)
+			vi->intr_coal_tx.max_packets = 1;
+		else
+			vi->intr_coal_tx.max_packets = 0;
+	}
+
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL)) {
+		/* The reason is the same as VIRTIO_NET_F_NOTF_COAL. */
+		for (i = 0; i < vi->max_queue_pairs; i++)
+			if (vi->sq[i].napi.weight)
+				vi->sq[i].intr_coal.max_packets = 1;
+	}
+
 #ifdef CONFIG_SYSFS
 	if (vi->mergeable_rx_bufs)
 		dev->sysfs_rx_queue_group = &virtio_net_mrg_rx_group;