[PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Daniel Jurgens]

Currently the buffer used for control VQ commands is protected by the
RTNL lock. Previously this wasn't a major concern because the control
VQ was only used during device setup and user interaction. With the
recent addition of dynamic interrupt moderation the control VQ may be
used frequently during normal operation.

This series removes the RNTL lock dependancy by introducing a spin lock
to protect the control buffer and writing SGs to the control VQ.

Daniel Jurgens (4):
  virtio_net: Store RSS setting in virtnet_info
  virtio_net: Remove command data from control_buf
  virtio_net: Add a lock for the command VQ.
  virtio_net: Remove rtnl lock protection of command buffers

 drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
 1 file changed, 104 insertions(+), 81 deletions(-)

-- 
2.42.0

[PATCH net-next 1/4] virtio_net: Store RSS setting in virtnet_info

[Daniel Jurgens]

Stop storing RSS setting in the control buffer. This is prep work for
removing RTNL lock protection of the control buffer.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/virtio_net.c | 40 ++++++++++++++++++++--------------------
 1 file changed, 20 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d7ce4a1011ea..7419a68cf6e2 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -240,7 +240,6 @@ struct control_buf {
 	u8 allmulti;
 	__virtio16 vid;
 	__virtio64 offloads;
-	struct virtio_net_ctrl_rss rss;
 	struct virtio_net_ctrl_coal_tx coal_tx;
 	struct virtio_net_ctrl_coal_rx coal_rx;
 	struct virtio_net_ctrl_coal_vq coal_vq;
@@ -282,6 +281,7 @@ struct virtnet_info {
 	u16 rss_indir_table_size;
 	u32 rss_hash_types_supported;
 	u32 rss_hash_types_saved;
+	struct virtio_net_ctrl_rss rss;
 
 	/* Has control virtqueue */
 	bool has_cvq;
@@ -3048,17 +3048,17 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
 	sg_init_table(sgs, 4);
 
 	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, indirection_table);
-	sg_set_buf(&sgs[0], &vi->ctrl->rss, sg_buf_size);
+	sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
 
-	sg_buf_size = sizeof(uint16_t) * (vi->ctrl->rss.indirection_table_mask + 1);
-	sg_set_buf(&sgs[1], vi->ctrl->rss.indirection_table, sg_buf_size);
+	sg_buf_size = sizeof(uint16_t) * (vi->rss.indirection_table_mask + 1);
+	sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
 
 	sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
 			- offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
-	sg_set_buf(&sgs[2], &vi->ctrl->rss.max_tx_vq, sg_buf_size);
+	sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
 
 	sg_buf_size = vi->rss_key_size;
-	sg_set_buf(&sgs[3], vi->ctrl->rss.key, sg_buf_size);
+	sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
 				  vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
@@ -3074,21 +3074,21 @@ static void virtnet_init_default_rss(struct virtnet_info *vi)
 	u32 indir_val = 0;
 	int i = 0;
 
-	vi->ctrl->rss.hash_types = vi->rss_hash_types_supported;
+	vi->rss.hash_types = vi->rss_hash_types_supported;
 	vi->rss_hash_types_saved = vi->rss_hash_types_supported;
-	vi->ctrl->rss.indirection_table_mask = vi->rss_indir_table_size
+	vi->rss.indirection_table_mask = vi->rss_indir_table_size
 						? vi->rss_indir_table_size - 1 : 0;
-	vi->ctrl->rss.unclassified_queue = 0;
+	vi->rss.unclassified_queue = 0;
 
 	for (; i < vi->rss_indir_table_size; ++i) {
 		indir_val = ethtool_rxfh_indir_default(i, vi->curr_queue_pairs);
-		vi->ctrl->rss.indirection_table[i] = indir_val;
+		vi->rss.indirection_table[i] = indir_val;
 	}
 
-	vi->ctrl->rss.max_tx_vq = vi->has_rss ? vi->curr_queue_pairs : 0;
-	vi->ctrl->rss.hash_key_length = vi->rss_key_size;
+	vi->rss.max_tx_vq = vi->has_rss ? vi->curr_queue_pairs : 0;
+	vi->rss.hash_key_length = vi->rss_key_size;
 
-	netdev_rss_key_fill(vi->ctrl->rss.key, vi->rss_key_size);
+	netdev_rss_key_fill(vi->rss.key, vi->rss_key_size);
 }
 
 static void virtnet_get_hashflow(const struct virtnet_info *vi, struct ethtool_rxnfc *info)
@@ -3199,7 +3199,7 @@ static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *
 
 	if (new_hashtypes != vi->rss_hash_types_saved) {
 		vi->rss_hash_types_saved = new_hashtypes;
-		vi->ctrl->rss.hash_types = vi->rss_hash_types_saved;
+		vi->rss.hash_types = vi->rss_hash_types_saved;
 		if (vi->dev->features & NETIF_F_RXHASH)
 			return virtnet_commit_rss_command(vi);
 	}
@@ -3752,11 +3752,11 @@ static int virtnet_get_rxfh(struct net_device *dev,
 
 	if (rxfh->indir) {
 		for (i = 0; i < vi->rss_indir_table_size; ++i)
-			rxfh->indir[i] = vi->ctrl->rss.indirection_table[i];
+			rxfh->indir[i] = vi->rss.indirection_table[i];
 	}
 
 	if (rxfh->key)
-		memcpy(rxfh->key, vi->ctrl->rss.key, vi->rss_key_size);
+		memcpy(rxfh->key, vi->rss.key, vi->rss_key_size);
 
 	rxfh->hfunc = ETH_RSS_HASH_TOP;
 
@@ -3776,10 +3776,10 @@ static int virtnet_set_rxfh(struct net_device *dev,
 
 	if (rxfh->indir) {
 		for (i = 0; i < vi->rss_indir_table_size; ++i)
-			vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
+			vi->rss.indirection_table[i] = rxfh->indir[i];
 	}
 	if (rxfh->key)
-		memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
+		memcpy(vi->rss.key, rxfh->key, vi->rss_key_size);
 
 	virtnet_commit_rss_command(vi);
 
@@ -4098,9 +4098,9 @@ static int virtnet_set_features(struct net_device *dev,
 
 	if ((dev->features ^ features) & NETIF_F_RXHASH) {
 		if (features & NETIF_F_RXHASH)
-			vi->ctrl->rss.hash_types = vi->rss_hash_types_saved;
+			vi->rss.hash_types = vi->rss_hash_types_saved;
 		else
-			vi->ctrl->rss.hash_types = VIRTIO_NET_HASH_REPORT_NONE;
+			vi->rss.hash_types = VIRTIO_NET_HASH_REPORT_NONE;
 
 		if (!virtnet_commit_rss_command(vi))
 			return -EINVAL;
-- 
2.42.0

[PATCH net-next 2/4] virtio_net: Remove command data from control_buf

[Daniel Jurgens]

Allocate memory for the data when it's used. Ideally the could be on the
stack, but we can't DMA stack memory. With this change only the header
and status memory are shared between commands, which will allow using a
tighter lock than RTNL.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/virtio_net.c | 110 ++++++++++++++++++++++++++-------------
 1 file changed, 74 insertions(+), 36 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7419a68cf6e2..6780479a1e6c 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -235,14 +235,6 @@ struct virtio_net_ctrl_rss {
 struct control_buf {
 	struct virtio_net_ctrl_hdr hdr;
 	virtio_net_ctrl_ack status;
-	struct virtio_net_ctrl_mq mq;
-	u8 promisc;
-	u8 allmulti;
-	__virtio16 vid;
-	__virtio64 offloads;
-	struct virtio_net_ctrl_coal_tx coal_tx;
-	struct virtio_net_ctrl_coal_rx coal_rx;
-	struct virtio_net_ctrl_coal_vq coal_vq;
 };
 
 struct virtnet_info {
@@ -2654,14 +2646,19 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
 
 static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 {
+	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
 	struct scatterlist sg;
 	struct net_device *dev = vi->dev;
 
 	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
 		return 0;
 
-	vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
-	sg_init_one(&sg, &vi->ctrl->mq, sizeof(vi->ctrl->mq));
+	mq = kzalloc(sizeof(*mq), GFP_KERNEL);
+	if (!mq)
+		return -ENOMEM;
+
+	mq->virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
+	sg_init_one(&sg, mq, sizeof(*mq));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
 				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) {
@@ -2708,6 +2705,7 @@ static int virtnet_close(struct net_device *dev)
 
 static void virtnet_set_rx_mode(struct net_device *dev)
 {
+	u8 *promisc_allmulti  __free(kfree) = NULL;
 	struct virtnet_info *vi = netdev_priv(dev);
 	struct scatterlist sg[2];
 	struct virtio_net_ctrl_mac *mac_data;
@@ -2721,22 +2719,27 @@ static void virtnet_set_rx_mode(struct net_device *dev)
 	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
 		return;
 
-	vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
-	vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
+	promisc_allmulti = kzalloc(sizeof(*promisc_allmulti), GFP_ATOMIC);
+	if (!promisc_allmulti) {
+		dev_warn(&dev->dev, "Failed to set RX mode, no memory.\n");
+		return;
+	}
 
-	sg_init_one(sg, &vi->ctrl->promisc, sizeof(vi->ctrl->promisc));
+	*promisc_allmulti = !!(dev->flags & IFF_PROMISC);
+	sg_init_one(sg, promisc_allmulti, sizeof(*promisc_allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
 		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
-			 vi->ctrl->promisc ? "en" : "dis");
+			 *promisc_allmulti ? "en" : "dis");
 
-	sg_init_one(sg, &vi->ctrl->allmulti, sizeof(vi->ctrl->allmulti));
+	*promisc_allmulti = !!(dev->flags & IFF_ALLMULTI);
+	sg_init_one(sg, promisc_allmulti, sizeof(*promisc_allmulti));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
 				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
 		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
-			 vi->ctrl->allmulti ? "en" : "dis");
+			 *promisc_allmulti ? "en" : "dis");
 
 	uc_count = netdev_uc_count(dev);
 	mc_count = netdev_mc_count(dev);
@@ -2780,10 +2783,15 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev,
 				   __be16 proto, u16 vid)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	__virtio16 *_vid __free(kfree) = NULL;
 	struct scatterlist sg;
 
-	vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
-	sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
+	_vid = kzalloc(sizeof(*_vid), GFP_KERNEL);
+	if (!_vid)
+		return -ENOMEM;
+
+	*_vid = cpu_to_virtio16(vi->vdev, vid);
+	sg_init_one(&sg, _vid, sizeof(*_vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_ADD, &sg))
@@ -2795,10 +2803,15 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev,
 				    __be16 proto, u16 vid)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
+	__virtio16 *_vid __free(kfree) = NULL;
 	struct scatterlist sg;
 
-	vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
-	sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
+	_vid = kzalloc(sizeof(*_vid), GFP_KERNEL);
+	if (!_vid)
+		return -ENOMEM;
+
+	*_vid = cpu_to_virtio16(vi->vdev, vid);
+	sg_init_one(&sg, _vid, sizeof(*_vid));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
 				  VIRTIO_NET_CTRL_VLAN_DEL, &sg))
@@ -2911,12 +2924,17 @@ static void virtnet_cpu_notif_remove(struct virtnet_info *vi)
 static int virtnet_send_ctrl_coal_vq_cmd(struct virtnet_info *vi,
 					 u16 vqn, u32 max_usecs, u32 max_packets)
 {
+	struct virtio_net_ctrl_coal_vq *coal_vq __free(kfree) = NULL;
 	struct scatterlist sgs;
 
-	vi->ctrl->coal_vq.vqn = cpu_to_le16(vqn);
-	vi->ctrl->coal_vq.coal.max_usecs = cpu_to_le32(max_usecs);
-	vi->ctrl->coal_vq.coal.max_packets = cpu_to_le32(max_packets);
-	sg_init_one(&sgs, &vi->ctrl->coal_vq, sizeof(vi->ctrl->coal_vq));
+	coal_vq = kzalloc(sizeof(*coal_vq), GFP_KERNEL);
+	if (!coal_vq)
+		return -ENOMEM;
+
+	coal_vq->vqn = cpu_to_le16(vqn);
+	coal_vq->coal.max_usecs = cpu_to_le32(max_usecs);
+	coal_vq->coal.max_packets = cpu_to_le32(max_packets);
+	sg_init_one(&sgs, coal_vq, sizeof(*coal_vq));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
 				  VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
@@ -3062,11 +3080,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
 				  vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
-				  : VIRTIO_NET_CTRL_MQ_HASH_CONFIG, sgs)) {
-		dev_warn(&dev->dev, "VIRTIONET issue with committing RSS sgs\n");
-		return false;
-	}
+				  : VIRTIO_NET_CTRL_MQ_HASH_CONFIG, sgs))
+		goto err;
+
 	return true;
+
+err:
+	dev_warn(&dev->dev, "VIRTIONET issue with committing RSS sgs\n");
+	return false;
+
 }
 
 static void virtnet_init_default_rss(struct virtnet_info *vi)
@@ -3371,12 +3393,17 @@ static int virtnet_get_link_ksettings(struct net_device *dev,
 static int virtnet_send_tx_notf_coal_cmds(struct virtnet_info *vi,
 					  struct ethtool_coalesce *ec)
 {
+	struct virtio_net_ctrl_coal_tx *coal_tx __free(kfree) = NULL;
 	struct scatterlist sgs_tx;
 	int i;
 
-	vi->ctrl->coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
-	vi->ctrl->coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
-	sg_init_one(&sgs_tx, &vi->ctrl->coal_tx, sizeof(vi->ctrl->coal_tx));
+	coal_tx = kzalloc(sizeof(*coal_tx), GFP_KERNEL);
+	if (!coal_tx)
+		return -ENOMEM;
+
+	coal_tx->tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
+	coal_tx->tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
+	sg_init_one(&sgs_tx, coal_tx, sizeof(*coal_tx));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
 				  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
@@ -3396,6 +3423,7 @@ static int virtnet_send_tx_notf_coal_cmds(struct virtnet_info *vi,
 static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
 					  struct ethtool_coalesce *ec)
 {
+	struct virtio_net_ctrl_coal_rx *coal_rx __free(kfree) = NULL;
 	bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce;
 	struct scatterlist sgs_rx;
 	int i;
@@ -3414,6 +3442,10 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
 		return 0;
 	}
 
+	coal_rx = kzalloc(sizeof(*coal_rx), GFP_KERNEL);
+	if (!coal_rx)
+		return -ENOMEM;
+
 	if (!rx_ctrl_dim_on && vi->rx_dim_enabled) {
 		vi->rx_dim_enabled = false;
 		for (i = 0; i < vi->max_queue_pairs; i++)
@@ -3424,9 +3456,9 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
 	 * we need apply the global new params even if they
 	 * are not updated.
 	 */
-	vi->ctrl->coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
-	vi->ctrl->coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
-	sg_init_one(&sgs_rx, &vi->ctrl->coal_rx, sizeof(vi->ctrl->coal_rx));
+	coal_rx->rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
+	coal_rx->rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
+	sg_init_one(&sgs_rx, coal_rx, sizeof(*coal_rx));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
 				  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
@@ -3893,10 +3925,16 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 
 static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
 {
+	u64 *_offloads __free(kfree) = NULL;
 	struct scatterlist sg;
-	vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads);
 
-	sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads));
+	_offloads = kzalloc(sizeof(*_offloads), GFP_KERNEL);
+	if (!_offloads)
+		return -ENOMEM;
+
+	*_offloads = cpu_to_virtio64(vi->vdev, offloads);
+
+	sg_init_one(&sg, _offloads, sizeof(*_offloads));
 
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
 				  VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
-- 
2.42.0

[PATCH net-next 3/4] virtio_net: Add a lock for the command VQ.

[Daniel Jurgens]

The command VQ will no longer be protected by the RTNL lock. Use a
spinlock to protect the control buffer header and the VQ.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/virtio_net.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 6780479a1e6c..41f8dc16ff38 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -277,6 +277,7 @@ struct virtnet_info {
 
 	/* Has control virtqueue */
 	bool has_cvq;
+	spinlock_t cvq_lock;
 
 	/* Host can handle any s/g split between our header and packet data */
 	bool any_header_sg;
@@ -2513,6 +2514,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	/* Caller should know better */
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
+	guard(spinlock)(&vi->cvq_lock);
 	vi->ctrl->status = ~0;
 	vi->ctrl->hdr.class = class;
 	vi->ctrl->hdr.cmd = cmd;
@@ -4756,8 +4758,10 @@ static int virtnet_probe(struct virtio_device *vdev)
 	    virtio_has_feature(vdev, VIRTIO_F_VERSION_1))
 		vi->any_header_sg = true;
 
-	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ))
+	if (virtio_has_feature(vdev, VIRTIO_NET_F_CTRL_VQ)) {
 		vi->has_cvq = true;
+		spin_lock_init(&vi->cvq_lock);
+	}
 
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MTU)) {
 		mtu = virtio_cread16(vdev,
-- 
2.42.0

[PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of command buffers

[Daniel Jurgens]

The rtnl lock is no longer needed to protect the control buffer and
command VQ.

Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
Reviewed-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/net/virtio_net.c | 27 +++++----------------------
 1 file changed, 5 insertions(+), 22 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 41f8dc16ff38..d09ea20b16be 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device *dev,
 
 static void virtnet_ack_link_announce(struct virtnet_info *vi)
 {
-	rtnl_lock();
 	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
 				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
 		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
-	rtnl_unlock();
 }
 
-static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
+static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 {
 	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
 	struct scatterlist sg;
@@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	return 0;
 }
 
-static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
-{
-	int err;
-
-	rtnl_lock();
-	err = _virtnet_set_queues(vi, queue_pairs);
-	rtnl_unlock();
-	return err;
-}
-
 static int virtnet_close(struct net_device *dev)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -3268,7 +3256,7 @@ static int virtnet_set_channels(struct net_device *dev,
 		return -EINVAL;
 
 	cpus_read_lock();
-	err = _virtnet_set_queues(vi, queue_pairs);
+	err = virtnet_set_queues(vi, queue_pairs);
 	if (err) {
 		cpus_read_unlock();
 		goto err;
@@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct work_struct *work)
 	struct dim_cq_moder update_moder;
 	int i, qnum, err;
 
-	if (!rtnl_trylock())
-		return;
-
 	/* Each rxq's work is queued by "net_dim()->schedule_work()"
 	 * in response to NAPI traffic changes. Note that dim->profile_ix
 	 * for each rxq is updated prior to the queuing action.
 	 * So we only need to traverse and update profiles for all rxqs
-	 * in the work which is holding rtnl_lock.
+	 * in the work.
 	 */
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		rq = &vi->rq[i];
@@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct work_struct *work)
 			dim->state = DIM_START_MEASURE;
 		}
 	}
-
-	rtnl_unlock();
 }
 
 static int virtnet_coal_params_supported(struct ethtool_coalesce *ec)
@@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		synchronize_net();
 	}
 
-	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
 	if (err)
 		goto err;
 	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
@@ -4852,7 +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 	virtio_device_ready(vdev);
 
-	_virtnet_set_queues(vi, vi->curr_queue_pairs);
+	virtnet_set_queues(vi, vi->curr_queue_pairs);
 
 	/* a random MAC address has been assigned, notify the device.
 	 * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there
-- 
2.42.0

Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Xuan Zhuo]

For series:

Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>

On Mon, 25 Mar 2024 16:49:07 -0500, Daniel Jurgens <danielj@nvidia.com> wrote:
> Currently the buffer used for control VQ commands is protected by the
> RTNL lock. Previously this wasn't a major concern because the control
> VQ was only used during device setup and user interaction. With the
> recent addition of dynamic interrupt moderation the control VQ may be
> used frequently during normal operation.
>
> This series removes the RNTL lock dependancy by introducing a spin lock
> to protect the control buffer and writing SGs to the control VQ.
>
> Daniel Jurgens (4):
>   virtio_net: Store RSS setting in virtnet_info
>   virtio_net: Remove command data from control_buf
>   virtio_net: Add a lock for the command VQ.
>   virtio_net: Remove rtnl lock protection of command buffers
>
>  drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
>  1 file changed, 104 insertions(+), 81 deletions(-)
>
> --
> 2.42.0
>

Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Heng Qi]

在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> Currently the buffer used for control VQ commands is protected by the
> RTNL lock. Previously this wasn't a major concern because the control
> VQ was only used during device setup and user interaction. With the
> recent addition of dynamic interrupt moderation the control VQ may be
> used frequently during normal operation.
>
> This series removes the RNTL lock dependancy by introducing a spin lock
> to protect the control buffer and writing SGs to the control VQ.

Hi Daniel.

It's a nice piece of work, but now that we're talking about ctrlq adding 
interrupts, spin lock has some
conflicts with its goals. For example, we expect the ethtool command to 
be blocked.
Therefore, a mutex lock may be more suitable.

Any how, the final conclusion may require some waiting.

Regards,
Heng

>
> Daniel Jurgens (4):
>    virtio_net: Store RSS setting in virtnet_info
>    virtio_net: Remove command data from control_buf
>    virtio_net: Add a lock for the command VQ.
>    virtio_net: Remove rtnl lock protection of command buffers
>
>   drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
>   1 file changed, 104 insertions(+), 81 deletions(-)
>

RE: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Dan Jurgens]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Monday, March 25, 2024 9:54 PM
> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
> virtualization@lists.linux.dev; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
> <jiri@nvidia.com>
> Subject: Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ
> 
> 
> 
> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> > Currently the buffer used for control VQ commands is protected by the
> > RTNL lock. Previously this wasn't a major concern because the control
> > VQ was only used during device setup and user interaction. With the
> > recent addition of dynamic interrupt moderation the control VQ may be
> > used frequently during normal operation.
> >
> > This series removes the RNTL lock dependancy by introducing a spin
> > lock to protect the control buffer and writing SGs to the control VQ.
> 
> Hi Daniel.
> 
> It's a nice piece of work, but now that we're talking about ctrlq adding
> interrupts, spin lock has some conflicts with its goals. For example, we expect
> the ethtool command to be blocked.
> Therefore, a mutex lock may be more suitable.
> 
> Any how, the final conclusion may require some waiting.

Thanks, Heng

I took this a step further and made the ctrlq interrupt driven, but an internal reviewer pointed me to this:
https://lore.kernel.org/lkml/20230413064027.13267-1-jasowang@redhat.com/ (sorry if it gets safelinked)

It seemed there was little appetite to go that route last year, because of set RX mode behavior change, and consumption of an additional IRQ.

Either way, I think the spin lock is still needed. In my interrupt driven implantation I was allocating a new control buffer instead of just the data fields. The spin lock was tighter around virtqueue_add_sgs, after the kick it would unlock and wait for a completion that would be triggered from the cvq callback. 


> 
> Regards,
> Heng
> 
> >
> > Daniel Jurgens (4):
> >    virtio_net: Store RSS setting in virtnet_info
> >    virtio_net: Remove command data from control_buf
> >    virtio_net: Add a lock for the command VQ.
> >    virtio_net: Remove rtnl lock protection of command buffers
> >
> >   drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
> >   1 file changed, 104 insertions(+), 81 deletions(-)
> >

Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ

[Heng Qi]

在 2024/3/26 下午12:11, Dan Jurgens 写道:
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Monday, March 25, 2024 9:54 PM
>> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
>> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
>> virtualization@lists.linux.dev; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
>> <jiri@nvidia.com>
>> Subject: Re: [PATCH net-next 0/4] Remove RTNL lock protection of CVQ
>>
>>
>>
>> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
>>> Currently the buffer used for control VQ commands is protected by the
>>> RTNL lock. Previously this wasn't a major concern because the control
>>> VQ was only used during device setup and user interaction. With the
>>> recent addition of dynamic interrupt moderation the control VQ may be
>>> used frequently during normal operation.
>>>
>>> This series removes the RNTL lock dependancy by introducing a spin
>>> lock to protect the control buffer and writing SGs to the control VQ.
>> Hi Daniel.
>>
>> It's a nice piece of work, but now that we're talking about ctrlq adding
>> interrupts, spin lock has some conflicts with its goals. For example, we expect
>> the ethtool command to be blocked.
>> Therefore, a mutex lock may be more suitable.
>>
>> Any how, the final conclusion may require some waiting.
> Thanks, Heng
>
> I took this a step further and made the ctrlq interrupt driven, but an internal reviewer pointed me to this:
> https://lore.kernel.org/lkml/20230413064027.13267-1-jasowang@redhat.com/ (sorry if it gets safelinked)
>
> It seemed there was little appetite to go that route last year, because of set RX mode behavior change, and consumption of an additional IRQ.

Hi DanielJ.

Jason now supports this and wants to make changes to ctrlq.

Yes, our requirements for ctrlq have become higher and we need to make 
updates as expected.

So your patches look good:

         Reviewed-by: Heng Qi <hengqi@linux.alibaba.com>

I will make further modifications on top of these.

Regards,
Heng

>
> Either way, I think the spin lock is still needed. In my interrupt driven implantation I was allocating a new control buffer instead of just the data fields. The spin lock was tighter around virtqueue_add_sgs, after the kick it would unlock and wait for a completion that would be triggered from the cvq callback.
>
>
>> Regards,
>> Heng
>>
>>> Daniel Jurgens (4):
>>>     virtio_net: Store RSS setting in virtnet_info
>>>     virtio_net: Remove command data from control_buf
>>>     virtio_net: Add a lock for the command VQ.
>>>     virtio_net: Remove rtnl lock protection of command buffers
>>>
>>>    drivers/net/virtio_net.c | 185 ++++++++++++++++++++++-----------------
>>>    1 file changed, 104 insertions(+), 81 deletions(-)
>>>

Re: [PATCH net-next 2/4] virtio_net: Remove command data from control_buf

[Heng Qi]

在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> Allocate memory for the data when it's used. Ideally the could be on the
> stack, but we can't DMA stack memory. With this change only the header
> and status memory are shared between commands, which will allow using a
> tighter lock than RTNL.
>
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> ---
>   drivers/net/virtio_net.c | 110 ++++++++++++++++++++++++++-------------
>   1 file changed, 74 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7419a68cf6e2..6780479a1e6c 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -235,14 +235,6 @@ struct virtio_net_ctrl_rss {
>   struct control_buf {
>   	struct virtio_net_ctrl_hdr hdr;
>   	virtio_net_ctrl_ack status;
> -	struct virtio_net_ctrl_mq mq;
> -	u8 promisc;
> -	u8 allmulti;
> -	__virtio16 vid;
> -	__virtio64 offloads;
> -	struct virtio_net_ctrl_coal_tx coal_tx;
> -	struct virtio_net_ctrl_coal_rx coal_rx;
> -	struct virtio_net_ctrl_coal_vq coal_vq;
>   };
>   
>   struct virtnet_info {
> @@ -2654,14 +2646,19 @@ static void virtnet_ack_link_announce(struct virtnet_info *vi)
>   
>   static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>   {
> +	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>   	struct scatterlist sg;
>   	struct net_device *dev = vi->dev;
>   
>   	if (!vi->has_cvq || !virtio_has_feature(vi->vdev, VIRTIO_NET_F_MQ))
>   		return 0;
>   
> -	vi->ctrl->mq.virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> -	sg_init_one(&sg, &vi->ctrl->mq, sizeof(vi->ctrl->mq));
> +	mq = kzalloc(sizeof(*mq), GFP_KERNEL);
> +	if (!mq)
> +		return -ENOMEM;
> +
> +	mq->virtqueue_pairs = cpu_to_virtio16(vi->vdev, queue_pairs);
> +	sg_init_one(&sg, mq, sizeof(*mq));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
>   				  VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET, &sg)) {
> @@ -2708,6 +2705,7 @@ static int virtnet_close(struct net_device *dev)
>   
>   static void virtnet_set_rx_mode(struct net_device *dev)

You need to rebase next branch in the next version,
.ndo_set_rx_mode has been completed using workqueue in that.

Regards,
Heng

>   {
> +	u8 *promisc_allmulti  __free(kfree) = NULL;
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	struct scatterlist sg[2];
>   	struct virtio_net_ctrl_mac *mac_data;
> @@ -2721,22 +2719,27 @@ static void virtnet_set_rx_mode(struct net_device *dev)
>   	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX))
>   		return;
>   
> -	vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0);
> -	vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0);
> +	promisc_allmulti = kzalloc(sizeof(*promisc_allmulti), GFP_ATOMIC);
> +	if (!promisc_allmulti) {
> +		dev_warn(&dev->dev, "Failed to set RX mode, no memory.\n");
> +		return;
> +	}
>   
> -	sg_init_one(sg, &vi->ctrl->promisc, sizeof(vi->ctrl->promisc));
> +	*promisc_allmulti = !!(dev->flags & IFF_PROMISC);
> +	sg_init_one(sg, promisc_allmulti, sizeof(*promisc_allmulti));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>   				  VIRTIO_NET_CTRL_RX_PROMISC, sg))
>   		dev_warn(&dev->dev, "Failed to %sable promisc mode.\n",
> -			 vi->ctrl->promisc ? "en" : "dis");
> +			 *promisc_allmulti ? "en" : "dis");
>   
> -	sg_init_one(sg, &vi->ctrl->allmulti, sizeof(vi->ctrl->allmulti));
> +	*promisc_allmulti = !!(dev->flags & IFF_ALLMULTI);
> +	sg_init_one(sg, promisc_allmulti, sizeof(*promisc_allmulti));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_RX,
>   				  VIRTIO_NET_CTRL_RX_ALLMULTI, sg))
>   		dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n",
> -			 vi->ctrl->allmulti ? "en" : "dis");
> +			 *promisc_allmulti ? "en" : "dis");
>   
>   	uc_count = netdev_uc_count(dev);
>   	mc_count = netdev_mc_count(dev);
> @@ -2780,10 +2783,15 @@ static int virtnet_vlan_rx_add_vid(struct net_device *dev,
>   				   __be16 proto, u16 vid)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> +	__virtio16 *_vid __free(kfree) = NULL;
>   	struct scatterlist sg;
>   
> -	vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
> -	sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
> +	_vid = kzalloc(sizeof(*_vid), GFP_KERNEL);
> +	if (!_vid)
> +		return -ENOMEM;
> +
> +	*_vid = cpu_to_virtio16(vi->vdev, vid);
> +	sg_init_one(&sg, _vid, sizeof(*_vid));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
>   				  VIRTIO_NET_CTRL_VLAN_ADD, &sg))
> @@ -2795,10 +2803,15 @@ static int virtnet_vlan_rx_kill_vid(struct net_device *dev,
>   				    __be16 proto, u16 vid)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> +	__virtio16 *_vid __free(kfree) = NULL;
>   	struct scatterlist sg;
>   
> -	vi->ctrl->vid = cpu_to_virtio16(vi->vdev, vid);
> -	sg_init_one(&sg, &vi->ctrl->vid, sizeof(vi->ctrl->vid));
> +	_vid = kzalloc(sizeof(*_vid), GFP_KERNEL);
> +	if (!_vid)
> +		return -ENOMEM;
> +
> +	*_vid = cpu_to_virtio16(vi->vdev, vid);
> +	sg_init_one(&sg, _vid, sizeof(*_vid));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_VLAN,
>   				  VIRTIO_NET_CTRL_VLAN_DEL, &sg))
> @@ -2911,12 +2924,17 @@ static void virtnet_cpu_notif_remove(struct virtnet_info *vi)
>   static int virtnet_send_ctrl_coal_vq_cmd(struct virtnet_info *vi,
>   					 u16 vqn, u32 max_usecs, u32 max_packets)
>   {
> +	struct virtio_net_ctrl_coal_vq *coal_vq __free(kfree) = NULL;
>   	struct scatterlist sgs;
>   
> -	vi->ctrl->coal_vq.vqn = cpu_to_le16(vqn);
> -	vi->ctrl->coal_vq.coal.max_usecs = cpu_to_le32(max_usecs);
> -	vi->ctrl->coal_vq.coal.max_packets = cpu_to_le32(max_packets);
> -	sg_init_one(&sgs, &vi->ctrl->coal_vq, sizeof(vi->ctrl->coal_vq));
> +	coal_vq = kzalloc(sizeof(*coal_vq), GFP_KERNEL);
> +	if (!coal_vq)
> +		return -ENOMEM;
> +
> +	coal_vq->vqn = cpu_to_le16(vqn);
> +	coal_vq->coal.max_usecs = cpu_to_le32(max_usecs);
> +	coal_vq->coal.max_packets = cpu_to_le32(max_packets);
> +	sg_init_one(&sgs, coal_vq, sizeof(*coal_vq));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
>   				  VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
> @@ -3062,11 +3080,15 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
>   				  vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
> -				  : VIRTIO_NET_CTRL_MQ_HASH_CONFIG, sgs)) {
> -		dev_warn(&dev->dev, "VIRTIONET issue with committing RSS sgs\n");
> -		return false;
> -	}
> +				  : VIRTIO_NET_CTRL_MQ_HASH_CONFIG, sgs))
> +		goto err;
> +
>   	return true;
> +
> +err:
> +	dev_warn(&dev->dev, "VIRTIONET issue with committing RSS sgs\n");
> +	return false;
> +
>   }
>   
>   static void virtnet_init_default_rss(struct virtnet_info *vi)
> @@ -3371,12 +3393,17 @@ static int virtnet_get_link_ksettings(struct net_device *dev,
>   static int virtnet_send_tx_notf_coal_cmds(struct virtnet_info *vi,
>   					  struct ethtool_coalesce *ec)
>   {
> +	struct virtio_net_ctrl_coal_tx *coal_tx __free(kfree) = NULL;
>   	struct scatterlist sgs_tx;
>   	int i;
>   
> -	vi->ctrl->coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> -	vi->ctrl->coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> -	sg_init_one(&sgs_tx, &vi->ctrl->coal_tx, sizeof(vi->ctrl->coal_tx));
> +	coal_tx = kzalloc(sizeof(*coal_tx), GFP_KERNEL);
> +	if (!coal_tx)
> +		return -ENOMEM;
> +
> +	coal_tx->tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> +	coal_tx->tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> +	sg_init_one(&sgs_tx, coal_tx, sizeof(*coal_tx));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
>   				  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> @@ -3396,6 +3423,7 @@ static int virtnet_send_tx_notf_coal_cmds(struct virtnet_info *vi,
>   static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   					  struct ethtool_coalesce *ec)
>   {
> +	struct virtio_net_ctrl_coal_rx *coal_rx __free(kfree) = NULL;
>   	bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce;
>   	struct scatterlist sgs_rx;
>   	int i;
> @@ -3414,6 +3442,10 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   		return 0;
>   	}
>   
> +	coal_rx = kzalloc(sizeof(*coal_rx), GFP_KERNEL);
> +	if (!coal_rx)
> +		return -ENOMEM;
> +
>   	if (!rx_ctrl_dim_on && vi->rx_dim_enabled) {
>   		vi->rx_dim_enabled = false;
>   		for (i = 0; i < vi->max_queue_pairs; i++)
> @@ -3424,9 +3456,9 @@ static int virtnet_send_rx_notf_coal_cmds(struct virtnet_info *vi,
>   	 * we need apply the global new params even if they
>   	 * are not updated.
>   	 */
> -	vi->ctrl->coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> -	vi->ctrl->coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> -	sg_init_one(&sgs_rx, &vi->ctrl->coal_rx, sizeof(vi->ctrl->coal_rx));
> +	coal_rx->rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> +	coal_rx->rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> +	sg_init_one(&sgs_rx, coal_rx, sizeof(*coal_rx));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
>   				  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> @@ -3893,10 +3925,16 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>   
>   static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>   {
> +	u64 *_offloads __free(kfree) = NULL;
>   	struct scatterlist sg;
> -	vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads);
>   
> -	sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads));
> +	_offloads = kzalloc(sizeof(*_offloads), GFP_KERNEL);
> +	if (!_offloads)
> +		return -ENOMEM;
> +
> +	*_offloads = cpu_to_virtio64(vi->vdev, offloads);
> +
> +	sg_init_one(&sg, _offloads, sizeof(*_offloads));
>   
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>   				  VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {

Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of command buffers

[Heng Qi]

在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> The rtnl lock is no longer needed to protect the control buffer and
> command VQ.
>
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> ---
>   drivers/net/virtio_net.c | 27 +++++----------------------
>   1 file changed, 5 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 41f8dc16ff38..d09ea20b16be 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device *dev,
>   
>   static void virtnet_ack_link_announce(struct virtnet_info *vi)
>   {
> -	rtnl_lock();
>   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
>   				  VIRTIO_NET_CTRL_ANNOUNCE_ACK, NULL))
>   		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> -	rtnl_unlock();
>   }
>   
> -static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> +static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>   {
>   	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>   	struct scatterlist sg;
> @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>   	return 0;
>   }
>   
> -static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> -{
> -	int err;
> -
> -	rtnl_lock();
> -	err = _virtnet_set_queues(vi, queue_pairs);
> -	rtnl_unlock();
> -	return err;
> -}
> -
>   static int virtnet_close(struct net_device *dev)
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
> @@ -3268,7 +3256,7 @@ static int virtnet_set_channels(struct net_device *dev,
>   		return -EINVAL;
>   
>   	cpus_read_lock();
> -	err = _virtnet_set_queues(vi, queue_pairs);
> +	err = virtnet_set_queues(vi, queue_pairs);
>   	if (err) {
>   		cpus_read_unlock();
>   		goto err;
> @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct work_struct *work)
>   	struct dim_cq_moder update_moder;
>   	int i, qnum, err;
>   
> -	if (!rtnl_trylock())
> -		return;
> -

Does this guarantee that the synchronization is completely correct?

The purpose of this patch set is to add a separate lock for ctrlq rather 
than reusing the RTNL lock.
But for dim workers, it not only involves the use of ctrlq, but also 
involves reading shared variables
in interfaces such as .set_coalesce, .get_coalesce, etc.

In addition, assuming there are 10 queues, each queue is scheduled with 
its own dim worker at the
same time, then these 10 workers may issue parameters to rxq0 10 times 
in parallel, just because
the RTNL lock is removed here.

Therefore, when the RTNL lock is removed, a 'for loop' is no longer 
needed in virtnet_rx_dim_work,
and the dim worker of each queue only configures its own parameters.

Alternatively, please keep RTNL lock here.

Regards,
Heng

>   	/* Each rxq's work is queued by "net_dim()->schedule_work()"
>   	 * in response to NAPI traffic changes. Note that dim->profile_ix
>   	 * for each rxq is updated prior to the queuing action.
>   	 * So we only need to traverse and update profiles for all rxqs
> -	 * in the work which is holding rtnl_lock.
> +	 * in the work.
>   	 */
>   	for (i = 0; i < vi->curr_queue_pairs; i++) {
>   		rq = &vi->rq[i];
> @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct work_struct *work)
>   			dim->state = DIM_START_MEASURE;
>   		}
>   	}
> -
> -	rtnl_unlock();
>   }
>   
>   static int virtnet_coal_params_supported(struct ethtool_coalesce *ec)
> @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>   		synchronize_net();
>   	}
>   
> -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> +	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
>   	if (err)
>   		goto err;
>   	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> @@ -4852,7 +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>   
>   	virtio_device_ready(vdev);
>   
> -	_virtnet_set_queues(vi, vi->curr_queue_pairs);
> +	virtnet_set_queues(vi, vi->curr_queue_pairs);
>   
>   	/* a random MAC address has been assigned, notify the device.
>   	 * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not there

RE: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of command buffers

[Dan Jurgens]

> From: Heng Qi <hengqi@linux.alibaba.com>
> Sent: Tuesday, March 26, 2024 3:55 AM
> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
> virtualization@lists.linux.dev; davem@davemloft.net;
> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
> <jiri@nvidia.com>
> Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of
> command buffers
> 
> 
> 
> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
> > The rtnl lock is no longer needed to protect the control buffer and
> > command VQ.
> >
> > Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> > ---
> >   drivers/net/virtio_net.c | 27 +++++----------------------
> >   1 file changed, 5 insertions(+), 22 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
> > 41f8dc16ff38..d09ea20b16be 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device
> > *dev,
> >
> >   static void virtnet_ack_link_announce(struct virtnet_info *vi)
> >   {
> > -	rtnl_lock();
> >   	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
> >   				  VIRTIO_NET_CTRL_ANNOUNCE_ACK,
> NULL))
> >   		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
> > -	rtnl_unlock();
> >   }
> >
> > -static int _virtnet_set_queues(struct virtnet_info *vi, u16
> > queue_pairs)
> > +static int virtnet_set_queues(struct virtnet_info *vi, u16
> > +queue_pairs)
> >   {
> >   	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> >   	struct scatterlist sg;
> > @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct
> virtnet_info *vi, u16 queue_pairs)
> >   	return 0;
> >   }
> >
> > -static int virtnet_set_queues(struct virtnet_info *vi, u16
> > queue_pairs) -{
> > -	int err;
> > -
> > -	rtnl_lock();
> > -	err = _virtnet_set_queues(vi, queue_pairs);
> > -	rtnl_unlock();
> > -	return err;
> > -}
> > -
> >   static int virtnet_close(struct net_device *dev)
> >   {
> >   	struct virtnet_info *vi = netdev_priv(dev); @@ -3268,7 +3256,7 @@
> > static int virtnet_set_channels(struct net_device *dev,
> >   		return -EINVAL;
> >
> >   	cpus_read_lock();
> > -	err = _virtnet_set_queues(vi, queue_pairs);
> > +	err = virtnet_set_queues(vi, queue_pairs);
> >   	if (err) {
> >   		cpus_read_unlock();
> >   		goto err;
> > @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct
> work_struct *work)
> >   	struct dim_cq_moder update_moder;
> >   	int i, qnum, err;
> >
> > -	if (!rtnl_trylock())
> > -		return;
> > -
> 
> Does this guarantee that the synchronization is completely correct?
> 
> The purpose of this patch set is to add a separate lock for ctrlq rather than
> reusing the RTNL lock.
> But for dim workers, it not only involves the use of ctrlq, but also involves
> reading shared variables in interfaces such as .set_coalesce, .get_coalesce,
> etc.

It looks like there is a risk of a dirty read in the get (usecs updated, but not max_packets). In the set it will return -EINVAL if trying to adjust the settings aside from DIM enabled.  I can add a lock for this if you think it's needed, but it doesn't seem like a major problem for debug info.


> 
> In addition, assuming there are 10 queues, each queue is scheduled with its
> own dim worker at the same time, then these 10 workers may issue
> parameters to rxq0 10 times in parallel, just because the RTNL lock is
> removed here.
> 
> Therefore, when the RTNL lock is removed, a 'for loop' is no longer needed in
> virtnet_rx_dim_work, and the dim worker of each queue only configures its
> own parameters.
> 

Good point. I'll add a new patch to remove the for loop.

> Alternatively, please keep RTNL lock here.
> 
> Regards,
> Heng
> 
> >   	/* Each rxq's work is queued by "net_dim()->schedule_work()"
> >   	 * in response to NAPI traffic changes. Note that dim->profile_ix
> >   	 * for each rxq is updated prior to the queuing action.
> >   	 * So we only need to traverse and update profiles for all rxqs
> > -	 * in the work which is holding rtnl_lock.
> > +	 * in the work.
> >   	 */
> >   	for (i = 0; i < vi->curr_queue_pairs; i++) {
> >   		rq = &vi->rq[i];
> > @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct
> work_struct *work)
> >   			dim->state = DIM_START_MEASURE;
> >   		}
> >   	}
> > -
> > -	rtnl_unlock();
> >   }
> >
> >   static int virtnet_coal_params_supported(struct ethtool_coalesce
> > *ec) @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device
> *dev, struct bpf_prog *prog,
> >   		synchronize_net();
> >   	}
> >
> > -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> > +	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
> >   	if (err)
> >   		goto err;
> >   	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); @@ -
> 4852,7
> > +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> >
> >   	virtio_device_ready(vdev);
> >
> > -	_virtnet_set_queues(vi, vi->curr_queue_pairs);
> > +	virtnet_set_queues(vi, vi->curr_queue_pairs);
> >
> >   	/* a random MAC address has been assigned, notify the device.
> >   	 * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not
> there

Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of command buffers

[Heng Qi]

在 2024/3/26 下午11:18, Dan Jurgens 写道:
>> From: Heng Qi <hengqi@linux.alibaba.com>
>> Sent: Tuesday, March 26, 2024 3:55 AM
>> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
>> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
>> virtualization@lists.linux.dev; davem@davemloft.net;
>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
>> <jiri@nvidia.com>
>> Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of
>> command buffers
>>
>>
>>
>> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
>>> The rtnl lock is no longer needed to protect the control buffer and
>>> command VQ.
>>>
>>> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>> ---
>>>    drivers/net/virtio_net.c | 27 +++++----------------------
>>>    1 file changed, 5 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
>>> 41f8dc16ff38..d09ea20b16be 100644
>>> --- a/drivers/net/virtio_net.c
>>> +++ b/drivers/net/virtio_net.c
>>> @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device
>>> *dev,
>>>
>>>    static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>>    {
>>> -	rtnl_lock();
>>>    	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
>>>    				  VIRTIO_NET_CTRL_ANNOUNCE_ACK,
>> NULL))
>>>    		dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>> -	rtnl_unlock();
>>>    }
>>>
>>> -static int _virtnet_set_queues(struct virtnet_info *vi, u16
>>> queue_pairs)
>>> +static int virtnet_set_queues(struct virtnet_info *vi, u16
>>> +queue_pairs)
>>>    {
>>>    	struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>>>    	struct scatterlist sg;
>>> @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct
>> virtnet_info *vi, u16 queue_pairs)
>>>    	return 0;
>>>    }
>>>
>>> -static int virtnet_set_queues(struct virtnet_info *vi, u16
>>> queue_pairs) -{
>>> -	int err;
>>> -
>>> -	rtnl_lock();
>>> -	err = _virtnet_set_queues(vi, queue_pairs);
>>> -	rtnl_unlock();
>>> -	return err;
>>> -}
>>> -
>>>    static int virtnet_close(struct net_device *dev)
>>>    {
>>>    	struct virtnet_info *vi = netdev_priv(dev); @@ -3268,7 +3256,7 @@
>>> static int virtnet_set_channels(struct net_device *dev,
>>>    		return -EINVAL;
>>>
>>>    	cpus_read_lock();
>>> -	err = _virtnet_set_queues(vi, queue_pairs);
>>> +	err = virtnet_set_queues(vi, queue_pairs);
>>>    	if (err) {
>>>    		cpus_read_unlock();
>>>    		goto err;
>>> @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct
>> work_struct *work)
>>>    	struct dim_cq_moder update_moder;
>>>    	int i, qnum, err;
>>>
>>> -	if (!rtnl_trylock())
>>> -		return;
>>> -
>> Does this guarantee that the synchronization is completely correct?
>>
>> The purpose of this patch set is to add a separate lock for ctrlq rather than
>> reusing the RTNL lock.
>> But for dim workers, it not only involves the use of ctrlq, but also involves
>> reading shared variables in interfaces such as .set_coalesce, .get_coalesce,
>> etc.
> It looks like there is a risk of a dirty read in the get (usecs updated, but not max_packets).

Also dim_enabled.

And later I need to asynchronousize the dim cmds, which means that
different dim workers will operate a shared linked list.

So we need a lock.

>   In the set it will return -EINVAL if trying to adjust the settings aside from DIM enabled.  I can add a lock for this if you think it's needed, but it doesn't seem like a major problem for debug info.

Not just for debug info, but future extensions as well.

These desynchronizations can introduce more trouble in the future.

Regards,
Heng

>
>
>> In addition, assuming there are 10 queues, each queue is scheduled with its
>> own dim worker at the same time, then these 10 workers may issue
>> parameters to rxq0 10 times in parallel, just because the RTNL lock is
>> removed here.
>>
>> Therefore, when the RTNL lock is removed, a 'for loop' is no longer needed in
>> virtnet_rx_dim_work, and the dim worker of each queue only configures its
>> own parameters.
>>
> Good point. I'll add a new patch to remove the for loop.
>
>> Alternatively, please keep RTNL lock here.
>>
>> Regards,
>> Heng
>>
>>>    	/* Each rxq's work is queued by "net_dim()->schedule_work()"
>>>    	 * in response to NAPI traffic changes. Note that dim->profile_ix
>>>    	 * for each rxq is updated prior to the queuing action.
>>>    	 * So we only need to traverse and update profiles for all rxqs
>>> -	 * in the work which is holding rtnl_lock.
>>> +	 * in the work.
>>>    	 */
>>>    	for (i = 0; i < vi->curr_queue_pairs; i++) {
>>>    		rq = &vi->rq[i];
>>> @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct
>> work_struct *work)
>>>    			dim->state = DIM_START_MEASURE;
>>>    		}
>>>    	}
>>> -
>>> -	rtnl_unlock();
>>>    }
>>>
>>>    static int virtnet_coal_params_supported(struct ethtool_coalesce
>>> *ec) @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct net_device
>> *dev, struct bpf_prog *prog,
>>>    		synchronize_net();
>>>    	}
>>>
>>> -	err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>>> +	err = virtnet_set_queues(vi, curr_qp + xdp_qp);
>>>    	if (err)
>>>    		goto err;
>>>    	netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); @@ -
>> 4852,7
>>> +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>
>>>    	virtio_device_ready(vdev);
>>>
>>> -	_virtnet_set_queues(vi, vi->curr_queue_pairs);
>>> +	virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>
>>>    	/* a random MAC address has been assigned, notify the device.
>>>    	 * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not
>> there

Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock protection of command buffers

[Heng Qi]

在 2024/3/27 上午10:10, Heng Qi 写道:
>
>
> 在 2024/3/26 下午11:18, Dan Jurgens 写道:
>>> From: Heng Qi <hengqi@linux.alibaba.com>
>>> Sent: Tuesday, March 26, 2024 3:55 AM
>>> To: Dan Jurgens <danielj@nvidia.com>; netdev@vger.kernel.org
>>> Cc: mst@redhat.com; jasowang@redhat.com; xuanzhuo@linux.alibaba.com;
>>> virtualization@lists.linux.dev; davem@davemloft.net;
>>> edumazet@google.com; kuba@kernel.org; pabeni@redhat.com; Jiri Pirko
>>> <jiri@nvidia.com>
>>> Subject: Re: [PATCH net-next 4/4] virtio_net: Remove rtnl lock 
>>> protection of
>>> command buffers
>>>
>>>
>>>
>>> 在 2024/3/26 上午5:49, Daniel Jurgens 写道:
>>>> The rtnl lock is no longer needed to protect the control buffer and
>>>> command VQ.
>>>>
>>>> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
>>>> Reviewed-by: Jiri Pirko <jiri@nvidia.com>
>>>> ---
>>>>    drivers/net/virtio_net.c | 27 +++++----------------------
>>>>    1 file changed, 5 insertions(+), 22 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index
>>>> 41f8dc16ff38..d09ea20b16be 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -2639,14 +2639,12 @@ static void virtnet_stats(struct net_device
>>>> *dev,
>>>>
>>>>    static void virtnet_ack_link_announce(struct virtnet_info *vi)
>>>>    {
>>>> -    rtnl_lock();
>>>>        if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_ANNOUNCE,
>>>>                      VIRTIO_NET_CTRL_ANNOUNCE_ACK,
>>> NULL))
>>>> dev_warn(&vi->dev->dev, "Failed to ack link announce.\n");
>>>> -    rtnl_unlock();
>>>>    }
>>>>
>>>> -static int _virtnet_set_queues(struct virtnet_info *vi, u16
>>>> queue_pairs)
>>>> +static int virtnet_set_queues(struct virtnet_info *vi, u16
>>>> +queue_pairs)
>>>>    {
>>>>        struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>>>>        struct scatterlist sg;
>>>> @@ -2677,16 +2675,6 @@ static int _virtnet_set_queues(struct
>>> virtnet_info *vi, u16 queue_pairs)
>>>>        return 0;
>>>>    }
>>>>
>>>> -static int virtnet_set_queues(struct virtnet_info *vi, u16
>>>> queue_pairs) -{
>>>> -    int err;
>>>> -
>>>> -    rtnl_lock();
>>>> -    err = _virtnet_set_queues(vi, queue_pairs);
>>>> -    rtnl_unlock();
>>>> -    return err;
>>>> -}
>>>> -
>>>>    static int virtnet_close(struct net_device *dev)
>>>>    {
>>>>        struct virtnet_info *vi = netdev_priv(dev); @@ -3268,7 
>>>> +3256,7 @@
>>>> static int virtnet_set_channels(struct net_device *dev,
>>>>            return -EINVAL;
>>>>
>>>>        cpus_read_lock();
>>>> -    err = _virtnet_set_queues(vi, queue_pairs);
>>>> +    err = virtnet_set_queues(vi, queue_pairs);
>>>>        if (err) {
>>>>            cpus_read_unlock();
>>>>            goto err;
>>>> @@ -3558,14 +3546,11 @@ static void virtnet_rx_dim_work(struct
>>> work_struct *work)
>>>>        struct dim_cq_moder update_moder;
>>>>        int i, qnum, err;
>>>>
>>>> -    if (!rtnl_trylock())
>>>> -        return;
>>>> -
>>> Does this guarantee that the synchronization is completely correct?
>>>
>>> The purpose of this patch set is to add a separate lock for ctrlq 
>>> rather than
>>> reusing the RTNL lock.
>>> But for dim workers, it not only involves the use of ctrlq, but also 
>>> involves
>>> reading shared variables in interfaces such as .set_coalesce, 
>>> .get_coalesce,
>>> etc.
>> It looks like there is a risk of a dirty read in the get (usecs 
>> updated, but not max_packets).
>
> Also dim_enabled.
>
> And later I need to asynchronousize the dim cmds, which means that
> different dim workers will operate a shared linked list.
>
> So we need a lock.

After removing the loop, maybe READ_ONCE/WRITE_ONCE will be enough?

>
>>   In the set it will return -EINVAL if trying to adjust the settings 
>> aside from DIM enabled.  I can add a lock for this if you think it's 
>> needed, but it doesn't seem like a major problem for debug info.
>
> Not just for debug info, but future extensions as well.
>
> These desynchronizations can introduce more trouble in the future.
>
> Regards,
> Heng
>
>>
>>
>>> In addition, assuming there are 10 queues, each queue is scheduled 
>>> with its
>>> own dim worker at the same time, then these 10 workers may issue
>>> parameters to rxq0 10 times in parallel, just because the RTNL lock is
>>> removed here.
>>>
>>> Therefore, when the RTNL lock is removed, a 'for loop' is no longer 
>>> needed in
>>> virtnet_rx_dim_work, and the dim worker of each queue only 
>>> configures its
>>> own parameters.
>>>
>> Good point. I'll add a new patch to remove the for loop.
>>
>>> Alternatively, please keep RTNL lock here.
>>>
>>> Regards,
>>> Heng
>>>
>>>>        /* Each rxq's work is queued by "net_dim()->schedule_work()"
>>>>         * in response to NAPI traffic changes. Note that 
>>>> dim->profile_ix
>>>>         * for each rxq is updated prior to the queuing action.
>>>>         * So we only need to traverse and update profiles for all rxqs
>>>> -     * in the work which is holding rtnl_lock.
>>>> +     * in the work.
>>>>         */
>>>>        for (i = 0; i < vi->curr_queue_pairs; i++) {
>>>>            rq = &vi->rq[i];
>>>> @@ -3587,8 +3572,6 @@ static void virtnet_rx_dim_work(struct
>>> work_struct *work)
>>>>                dim->state = DIM_START_MEASURE;
>>>>            }
>>>>        }
>>>> -
>>>> -    rtnl_unlock();
>>>>    }
>>>>
>>>>    static int virtnet_coal_params_supported(struct ethtool_coalesce
>>>> *ec) @@ -4036,7 +4019,7 @@ static int virtnet_xdp_set(struct 
>>>> net_device
>>> *dev, struct bpf_prog *prog,
>>>>            synchronize_net();
>>>>        }
>>>>
>>>> -    err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>>>> +    err = virtnet_set_queues(vi, curr_qp + xdp_qp);
>>>>        if (err)
>>>>            goto err;
>>>>        netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp); @@ -
>>> 4852,7
>>>> +4835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>>>>
>>>>        virtio_device_ready(vdev);
>>>>
>>>> -    _virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>> +    virtnet_set_queues(vi, vi->curr_queue_pairs);
>>>>
>>>>        /* a random MAC address has been assigned, notify the device.
>>>>         * We don't fail probe if VIRTIO_NET_F_CTRL_MAC_ADDR is not
>>> there
>

Re: [PATCH net-next 2/4] virtio_net: Remove command data from control_buf

[Simon Horman]

On Mon, Mar 25, 2024 at 04:49:09PM -0500, Daniel Jurgens wrote:
> Allocate memory for the data when it's used. Ideally the could be on the
> stack, but we can't DMA stack memory. With this change only the header
> and status memory are shared between commands, which will allow using a
> tighter lock than RTNL.
> 
> Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> Reviewed-by: Jiri Pirko <jiri@nvidia.com>

...

> @@ -3893,10 +3925,16 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>  
>  static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>  {
> +	u64 *_offloads __free(kfree) = NULL;
>  	struct scatterlist sg;
> -	vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads);
>  
> -	sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads));
> +	_offloads = kzalloc(sizeof(*_offloads), GFP_KERNEL);
> +	if (!_offloads)
> +		return -ENOMEM;
> +
> +	*_offloads = cpu_to_virtio64(vi->vdev, offloads);

Hi Daniel,

There is a type mismatch between *_offloads and cpu_to_virtio64
which is flagged by Sparse as follows:

 .../virtio_net.c:3978:20: warning: incorrect type in assignment (different base types)
 .../virtio_net.c:3978:20:    expected unsigned long long [usertype]
 .../virtio_net.c:3978:20:    got restricted __virtio64

I think this can be addressed by changing the type of *_offloads to
__virtio64 *.

> +
> +	sg_init_one(&sg, _offloads, sizeof(*_offloads));
>  
>  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>  				  VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
> -- 
> 2.42.0
> 
> 

Re: [PATCH net-next 2/4] virtio_net: Remove command data from control_buf

[Michael S. Tsirkin]

On Thu, Mar 28, 2024 at 01:35:16PM +0000, Simon Horman wrote:
> On Mon, Mar 25, 2024 at 04:49:09PM -0500, Daniel Jurgens wrote:
> > Allocate memory for the data when it's used. Ideally the could be on the
> > stack, but we can't DMA stack memory. With this change only the header
> > and status memory are shared between commands, which will allow using a
> > tighter lock than RTNL.
> > 
> > Signed-off-by: Daniel Jurgens <danielj@nvidia.com>
> > Reviewed-by: Jiri Pirko <jiri@nvidia.com>
> 
> ...
> 
> > @@ -3893,10 +3925,16 @@ static int virtnet_restore_up(struct virtio_device *vdev)
> >  
> >  static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> >  {
> > +	u64 *_offloads __free(kfree) = NULL;
> >  	struct scatterlist sg;
> > -	vi->ctrl->offloads = cpu_to_virtio64(vi->vdev, offloads);
> >  
> > -	sg_init_one(&sg, &vi->ctrl->offloads, sizeof(vi->ctrl->offloads));
> > +	_offloads = kzalloc(sizeof(*_offloads), GFP_KERNEL);
> > +	if (!_offloads)
> > +		return -ENOMEM;
> > +
> > +	*_offloads = cpu_to_virtio64(vi->vdev, offloads);
> 
> Hi Daniel,
> 
> There is a type mismatch between *_offloads and cpu_to_virtio64
> which is flagged by Sparse as follows:
> 
>  .../virtio_net.c:3978:20: warning: incorrect type in assignment (different base types)
>  .../virtio_net.c:3978:20:    expected unsigned long long [usertype]
>  .../virtio_net.c:3978:20:    got restricted __virtio64
> 
> I think this can be addressed by changing the type of *_offloads to
> __virtio64 *.


Yes pls, endian-ness is easier to get right 1st time than fix
afterwards.

> > +
> > +	sg_init_one(&sg, _offloads, sizeof(*_offloads));
> >  
> >  	if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> >  				  VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
> > -- 
> > 2.42.0
> > 
> >