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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20230811065512.22190-2-hengqi@linux.alibaba.com>
Date: Fri, 11 Aug 2023 14:55:05 +0800
From: Heng Qi <hengqi@...ux.alibaba.com>
To: "Michael S . Tsirkin" <mst@...hat.com>,
	Jason Wang <jasowang@...hat.com>,
	netdev@...r.kernel.org,
	virtualization@...ts.linux-foundation.org
Cc: "David S . Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Paolo Abeni <pabeni@...hat.com>,
	Alexei Starovoitov <ast@...nel.org>,
	Daniel Borkmann <daniel@...earbox.net>,
	Jesper Dangaard Brouer <hawk@...nel.org>,
	John Fastabend <john.fastabend@...il.com>,
	Xuan Zhuo <xuanzhuo@...ux.alibaba.com>
Subject: [PATCH net-next 1/8] virtio-net: initially change the value of tx-frames

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 <zxm377917@...baba-inc.com>
Signed-off-by: Heng Qi <hengqi@...ux.alibaba.com>
---
 drivers/net/virtio_net.c | 42 +++++++++++++++++++++++++++++++++-------
 1 file changed, 35 insertions(+), 7 deletions(-)

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;
-- 
2.19.1.6.gb485710b