[PATCH net-next v4 0/4] virtio-net: Link queues to NAPIs

[PATCH net-next v4 0/4] virtio-net: Link queues to NAPIs

[Joe Damato]

Greetings:

Welcome to v4.

Jakub recently commented [1] that I should not hold this series on
virtio-net linking queues to NAPIs behind other important work that is
on-going and suggested I re-spin, so here we are :)

This is a significant refactor from the rfcv3 and as such I've dropped
almost all of the tags from reviewers except for patch 4 (sorry Gerhard
and Jason; the changes are significant so I think patches 1-3 need to be
re-reviewed).

As per the discussion on the v3 [2], now both RX and TX NAPIs use the
API to link queues to NAPIs. Since TX-only NAPIs don't have a NAPI ID,
commit 6597e8d35851 ("netdev-genl: Elide napi_id when not present") now
correctly elides the TX-only NAPIs (instead of printing zero) when the
queues and NAPIs are linked.

See the commit message of patch 3 for an example of how to get the NAPI
to queue mapping information.

See the commit message of patch 4 for an example of how NAPI IDs are
persistent despite queue count changes.

Thanks,
Joe

v4:
  - Dropped Jakub's patch (previously patch 1).
  - Significant refactor from v3 affecting patches 1-3.
  - Patch 4 added tags from Jason and Gerhard.

rfcv3: https://lore.kernel.org/netdev/20250121191047.269844-1-jdamato@fastly.com/
  - patch 3:
    - Removed the xdp checks completely, as Gerhard Engleder pointed
      out, they are likely not necessary.

  - patch 4:
    - Added Xuan Zhuo's Reviewed-by.

v2: https://lore.kernel.org/netdev/20250116055302.14308-1-jdamato@fastly.com/
  - patch 1:
    - New in the v2 from Jakub.

  - patch 2:
    - Previously patch 1, unchanged from v1.
    - Added Gerhard Engleder's Reviewed-by.
    - Added Lei Yang's Tested-by.

  - patch 3:
    - Introduced virtnet_napi_disable to eliminate duplicated code
      in virtnet_xdp_set, virtnet_rx_pause, virtnet_disable_queue_pair,
      refill_work as suggested by Jason Wang.
    - As a result of the above refactor, dropped Reviewed-by and
      Tested-by from patch 3.

  - patch 4:
    - New in v2. Adds persistent NAPI configuration. See commit message
      for more details.

v1: https://lore.kernel.org/netdev/20250110202605.429475-1-jdamato@fastly.com/

[1]: https://lore.kernel.org/netdev/20250221142650.3c74dcac@kernel.org/
[2]: https://lore.kernel.org/netdev/20250127142400.24eca319@kernel.org/

Joe Damato (4):
  virtio-net: Refactor napi_enable paths
  virtio-net: Refactor napi_disable paths
  virtio-net: Map NAPIs to queues
  virtio_net: Use persistent NAPI config

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


base-commit: 7183877d6853801258b7a8d3b51b415982e5097e
-- 
2.45.2

[PATCH net-next v4 1/4] virtio-net: Refactor napi_enable paths

[Joe Damato]

Refactor virtnet_napi_enable and virtnet_napi_tx_enable to take a struct
receive_queue. Create a helper, virtnet_napi_do_enable, which contains
the logic to enable a NAPI.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/virtio_net.c | 37 +++++++++++++++++++++----------------
 1 file changed, 21 insertions(+), 16 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ac26a6201c44..133b004c7a9a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2807,7 +2807,8 @@ static void skb_recv_done(struct virtqueue *rvq)
 	virtqueue_napi_schedule(&rq->napi, rvq);
 }
 
-static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi)
+static void virtnet_napi_do_enable(struct virtqueue *vq,
+				   struct napi_struct *napi)
 {
 	napi_enable(napi);
 
@@ -2820,10 +2821,16 @@ static void virtnet_napi_enable(struct virtqueue *vq, struct napi_struct *napi)
 	local_bh_enable();
 }
 
-static void virtnet_napi_tx_enable(struct virtnet_info *vi,
-				   struct virtqueue *vq,
-				   struct napi_struct *napi)
+static void virtnet_napi_enable(struct receive_queue *rq)
 {
+	virtnet_napi_do_enable(rq->vq, &rq->napi);
+}
+
+static void virtnet_napi_tx_enable(struct send_queue *sq)
+{
+	struct virtnet_info *vi = sq->vq->vdev->priv;
+	struct napi_struct *napi = &sq->napi;
+
 	if (!napi->weight)
 		return;
 
@@ -2835,7 +2842,7 @@ static void virtnet_napi_tx_enable(struct virtnet_info *vi,
 		return;
 	}
 
-	return virtnet_napi_enable(vq, napi);
+	virtnet_napi_do_enable(sq->vq, napi);
 }
 
 static void virtnet_napi_tx_disable(struct napi_struct *napi)
@@ -2856,7 +2863,7 @@ static void refill_work(struct work_struct *work)
 
 		napi_disable(&rq->napi);
 		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
-		virtnet_napi_enable(rq->vq, &rq->napi);
+		virtnet_napi_enable(rq);
 
 		/* In theory, this can happen: if we don't get any buffers in
 		 * we will *never* try to fill again.
@@ -3073,8 +3080,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
 	if (err < 0)
 		goto err_xdp_reg_mem_model;
 
-	virtnet_napi_enable(vi->rq[qp_index].vq, &vi->rq[qp_index].napi);
-	virtnet_napi_tx_enable(vi, vi->sq[qp_index].vq, &vi->sq[qp_index].napi);
+	virtnet_napi_enable(&vi->rq[qp_index]);
+	virtnet_napi_tx_enable(&vi->sq[qp_index]);
 
 	return 0;
 
@@ -3339,7 +3346,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
 		schedule_delayed_work(&vi->refill, 0);
 
 	if (running)
-		virtnet_napi_enable(rq->vq, &rq->napi);
+		virtnet_napi_enable(rq);
 }
 
 static int virtnet_rx_resize(struct virtnet_info *vi,
@@ -3402,7 +3409,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
 	__netif_tx_unlock_bh(txq);
 
 	if (running)
-		virtnet_napi_tx_enable(vi, sq->vq, &sq->napi);
+		virtnet_napi_tx_enable(sq);
 }
 
 static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
@@ -5983,9 +5990,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		if (old_prog)
 			bpf_prog_put(old_prog);
 		if (netif_running(dev)) {
-			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
-			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
-					       &vi->sq[i].napi);
+			virtnet_napi_enable(&vi->rq[i]);
+			virtnet_napi_tx_enable(&vi->sq[i]);
 		}
 	}
 
@@ -6000,9 +6006,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
-			virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
-			virtnet_napi_tx_enable(vi, vi->sq[i].vq,
-					       &vi->sq[i].napi);
+			virtnet_napi_enable(&vi->rq[i]);
+			virtnet_napi_tx_enable(&vi->sq[i]);
 		}
 	}
 	if (prog)
-- 
2.45.2

[PATCH net-next v4 2/4] virtio-net: Refactor napi_disable paths

[Joe Damato]

Create virtnet_napi_disable helper and refactor virtnet_napi_tx_disable
to take a struct send_queue.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/virtio_net.c | 25 +++++++++++++++++--------
 1 file changed, 17 insertions(+), 8 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 133b004c7a9a..e578885c1093 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2845,12 +2845,21 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
 	virtnet_napi_do_enable(sq->vq, napi);
 }
 
-static void virtnet_napi_tx_disable(struct napi_struct *napi)
+static void virtnet_napi_tx_disable(struct send_queue *sq)
 {
+	struct napi_struct *napi = &sq->napi;
+
 	if (napi->weight)
 		napi_disable(napi);
 }
 
+static void virtnet_napi_disable(struct receive_queue *rq)
+{
+	struct napi_struct *napi = &rq->napi;
+
+	napi_disable(napi);
+}
+
 static void refill_work(struct work_struct *work)
 {
 	struct virtnet_info *vi =
@@ -2861,7 +2870,7 @@ static void refill_work(struct work_struct *work)
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		struct receive_queue *rq = &vi->rq[i];
 
-		napi_disable(&rq->napi);
+		virtnet_napi_disable(rq);
 		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
 		virtnet_napi_enable(rq);
 
@@ -3060,8 +3069,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 
 static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
 {
-	virtnet_napi_tx_disable(&vi->sq[qp_index].napi);
-	napi_disable(&vi->rq[qp_index].napi);
+	virtnet_napi_tx_disable(&vi->sq[qp_index]);
+	virtnet_napi_disable(&vi->rq[qp_index]);
 	xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
 }
 
@@ -3333,7 +3342,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
 	bool running = netif_running(vi->dev);
 
 	if (running) {
-		napi_disable(&rq->napi);
+		virtnet_napi_disable(rq);
 		virtnet_cancel_dim(vi, &rq->dim);
 	}
 }
@@ -3375,7 +3384,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
 	qindex = sq - vi->sq;
 
 	if (running)
-		virtnet_napi_tx_disable(&sq->napi);
+		virtnet_napi_tx_disable(sq);
 
 	txq = netdev_get_tx_queue(vi->dev, qindex);
 
@@ -5952,8 +5961,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	/* Make sure NAPI is not using any XDP TX queues for RX. */
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
-			napi_disable(&vi->rq[i].napi);
-			virtnet_napi_tx_disable(&vi->sq[i].napi);
+			virtnet_napi_disable(&vi->rq[i]);
+			virtnet_napi_tx_disable(&vi->sq[i]);
 		}
 	}
 
-- 
2.45.2

[PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Joe Damato]

Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
can be accessed by user apps, taking care to hold RTNL as needed.

$ ethtool -i ens4 | grep driver
driver: virtio_net

$ sudo ethtool -L ens4 combined 4

$ ./tools/net/ynl/pyynl/cli.py \
       --spec Documentation/netlink/specs/netdev.yaml \
       --dump queue-get --json='{"ifindex": 2}'
[{'id': 0, 'ifindex': 2, 'napi-id': 8289, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8290, 'type': 'rx'},
 {'id': 2, 'ifindex': 2, 'napi-id': 8291, 'type': 'rx'},
 {'id': 3, 'ifindex': 2, 'napi-id': 8292, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'type': 'tx'},
 {'id': 1, 'ifindex': 2, 'type': 'tx'},
 {'id': 2, 'ifindex': 2, 'type': 'tx'},
 {'id': 3, 'ifindex': 2, 'type': 'tx'}]

Note that virtio_net has TX-only NAPIs which do not have NAPI IDs, so
the lack of 'napi-id' in the above output is expected.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 drivers/net/virtio_net.c | 73 ++++++++++++++++++++++++++++------------
 1 file changed, 52 insertions(+), 21 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index e578885c1093..13bb4a563073 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2807,6 +2807,20 @@ static void skb_recv_done(struct virtqueue *rvq)
 	virtqueue_napi_schedule(&rq->napi, rvq);
 }
 
+static void virtnet_queue_set_napi(struct net_device *dev,
+				   struct napi_struct *napi,
+				   enum netdev_queue_type q_type, int qidx,
+				   bool need_rtnl)
+{
+	if (need_rtnl)
+		rtnl_lock();
+
+	netif_queue_set_napi(dev, qidx, q_type, napi);
+
+	if (need_rtnl)
+		rtnl_unlock();
+}
+
 static void virtnet_napi_do_enable(struct virtqueue *vq,
 				   struct napi_struct *napi)
 {
@@ -2821,15 +2835,21 @@ static void virtnet_napi_do_enable(struct virtqueue *vq,
 	local_bh_enable();
 }
 
-static void virtnet_napi_enable(struct receive_queue *rq)
+static void virtnet_napi_enable(struct receive_queue *rq, bool need_rtnl)
 {
+	struct virtnet_info *vi = rq->vq->vdev->priv;
+	int qidx = vq2rxq(rq->vq);
+
 	virtnet_napi_do_enable(rq->vq, &rq->napi);
+	virtnet_queue_set_napi(vi->dev, &rq->napi,
+			       NETDEV_QUEUE_TYPE_RX, qidx, need_rtnl);
 }
 
-static void virtnet_napi_tx_enable(struct send_queue *sq)
+static void virtnet_napi_tx_enable(struct send_queue *sq, bool need_rtnl)
 {
 	struct virtnet_info *vi = sq->vq->vdev->priv;
 	struct napi_struct *napi = &sq->napi;
+	int qidx = vq2txq(sq->vq);
 
 	if (!napi->weight)
 		return;
@@ -2843,20 +2863,31 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
 	}
 
 	virtnet_napi_do_enable(sq->vq, napi);
+	virtnet_queue_set_napi(vi->dev, napi, NETDEV_QUEUE_TYPE_TX, qidx,
+			       need_rtnl);
 }
 
-static void virtnet_napi_tx_disable(struct send_queue *sq)
+static void virtnet_napi_tx_disable(struct send_queue *sq, bool need_rtnl)
 {
+	struct virtnet_info *vi = sq->vq->vdev->priv;
 	struct napi_struct *napi = &sq->napi;
+	int qidx = vq2txq(sq->vq);
 
-	if (napi->weight)
+	if (napi->weight) {
+		virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX,
+				       qidx, need_rtnl);
 		napi_disable(napi);
+	}
 }
 
-static void virtnet_napi_disable(struct receive_queue *rq)
+static void virtnet_napi_disable(struct receive_queue *rq, bool need_rtnl)
 {
+	struct virtnet_info *vi = rq->vq->vdev->priv;
 	struct napi_struct *napi = &rq->napi;
+	int qidx = vq2rxq(rq->vq);
 
+	virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX, qidx,
+			       need_rtnl);
 	napi_disable(napi);
 }
 
@@ -2870,9 +2901,9 @@ static void refill_work(struct work_struct *work)
 	for (i = 0; i < vi->curr_queue_pairs; i++) {
 		struct receive_queue *rq = &vi->rq[i];
 
-		virtnet_napi_disable(rq);
+		virtnet_napi_disable(rq, true);
 		still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
-		virtnet_napi_enable(rq);
+		virtnet_napi_enable(rq, true);
 
 		/* In theory, this can happen: if we don't get any buffers in
 		 * we will *never* try to fill again.
@@ -3069,8 +3100,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 
 static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
 {
-	virtnet_napi_tx_disable(&vi->sq[qp_index]);
-	virtnet_napi_disable(&vi->rq[qp_index]);
+	virtnet_napi_tx_disable(&vi->sq[qp_index], false);
+	virtnet_napi_disable(&vi->rq[qp_index], false);
 	xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
 }
 
@@ -3089,8 +3120,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
 	if (err < 0)
 		goto err_xdp_reg_mem_model;
 
-	virtnet_napi_enable(&vi->rq[qp_index]);
-	virtnet_napi_tx_enable(&vi->sq[qp_index]);
+	virtnet_napi_enable(&vi->rq[qp_index], false);
+	virtnet_napi_tx_enable(&vi->sq[qp_index], false);
 
 	return 0;
 
@@ -3342,7 +3373,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
 	bool running = netif_running(vi->dev);
 
 	if (running) {
-		virtnet_napi_disable(rq);
+		virtnet_napi_disable(rq, true);
 		virtnet_cancel_dim(vi, &rq->dim);
 	}
 }
@@ -3355,7 +3386,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
 		schedule_delayed_work(&vi->refill, 0);
 
 	if (running)
-		virtnet_napi_enable(rq);
+		virtnet_napi_enable(rq, true);
 }
 
 static int virtnet_rx_resize(struct virtnet_info *vi,
@@ -3384,7 +3415,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
 	qindex = sq - vi->sq;
 
 	if (running)
-		virtnet_napi_tx_disable(sq);
+		virtnet_napi_tx_disable(sq, true);
 
 	txq = netdev_get_tx_queue(vi->dev, qindex);
 
@@ -3418,7 +3449,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
 	__netif_tx_unlock_bh(txq);
 
 	if (running)
-		virtnet_napi_tx_enable(sq);
+		virtnet_napi_tx_enable(sq, true);
 }
 
 static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
@@ -5961,8 +5992,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 	/* Make sure NAPI is not using any XDP TX queues for RX. */
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
-			virtnet_napi_disable(&vi->rq[i]);
-			virtnet_napi_tx_disable(&vi->sq[i]);
+			virtnet_napi_disable(&vi->rq[i], false);
+			virtnet_napi_tx_disable(&vi->sq[i], false);
 		}
 	}
 
@@ -5999,8 +6030,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 		if (old_prog)
 			bpf_prog_put(old_prog);
 		if (netif_running(dev)) {
-			virtnet_napi_enable(&vi->rq[i]);
-			virtnet_napi_tx_enable(&vi->sq[i]);
+			virtnet_napi_enable(&vi->rq[i], false);
+			virtnet_napi_tx_enable(&vi->sq[i], false);
 		}
 	}
 
@@ -6015,8 +6046,8 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
 
 	if (netif_running(dev)) {
 		for (i = 0; i < vi->max_queue_pairs; i++) {
-			virtnet_napi_enable(&vi->rq[i]);
-			virtnet_napi_tx_enable(&vi->sq[i]);
+			virtnet_napi_enable(&vi->rq[i], false);
+			virtnet_napi_tx_enable(&vi->sq[i], false);
 		}
 	}
 	if (prog)
-- 
2.45.2

[PATCH net-next v4 4/4] virtio-net: Use persistent NAPI config

[Joe Damato]

Use persistent NAPI config so that NAPI IDs are not renumbered as queue
counts change.

$ sudo ethtool -l ens4  | tail -5 | egrep -i '(current|combined)'
Current hardware settings:
Combined:       4

$ ./tools/net/ynl/pyynl/cli.py \
    --spec Documentation/netlink/specs/netdev.yaml \
    --dump queue-get --json='{"ifindex": 2}'
[{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
 {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
 {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'type': 'tx'},
 {'id': 1, 'ifindex': 2, 'type': 'tx'},
 {'id': 2, 'ifindex': 2, 'type': 'tx'},
 {'id': 3, 'ifindex': 2, 'type': 'tx'}]

Now adjust the queue count, note that the NAPI IDs are not renumbered:

$ sudo ethtool -L ens4 combined 1
$ ./tools/net/ynl/pyynl/cli.py \
    --spec Documentation/netlink/specs/netdev.yaml \
    --dump queue-get --json='{"ifindex": 2}'
[{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'},
 {'id': 0, 'ifindex': 2, 'type': 'tx'}]

$ sudo ethtool -L ens4 combined 8
$ ./tools/net/ynl/pyynl/cli.py \
    --spec Documentation/netlink/specs/netdev.yaml \
    --dump queue-get --json='{"ifindex": 2}'
[{'id': 0, 'ifindex': 2, 'napi-id': 8193, 'type': 'rx'},
 {'id': 1, 'ifindex': 2, 'napi-id': 8194, 'type': 'rx'},
 {'id': 2, 'ifindex': 2, 'napi-id': 8195, 'type': 'rx'},
 {'id': 3, 'ifindex': 2, 'napi-id': 8196, 'type': 'rx'},
 {'id': 4, 'ifindex': 2, 'napi-id': 8197, 'type': 'rx'},
 {'id': 5, 'ifindex': 2, 'napi-id': 8198, 'type': 'rx'},
 {'id': 6, 'ifindex': 2, 'napi-id': 8199, 'type': 'rx'},
 {'id': 7, 'ifindex': 2, 'napi-id': 8200, 'type': 'rx'},
 [...]

Signed-off-by: Joe Damato <jdamato@fastly.com>
Reviewed-by: Xuan Zhuo <xuanzhuo@linux.alibaba.com>
Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>
Acked-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 13bb4a563073..186030693eeb 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -6454,8 +6454,9 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
 	INIT_DELAYED_WORK(&vi->refill, refill_work);
 	for (i = 0; i < vi->max_queue_pairs; i++) {
 		vi->rq[i].pages = NULL;
-		netif_napi_add_weight(vi->dev, &vi->rq[i].napi, virtnet_poll,
-				      napi_weight);
+		netif_napi_add_config(vi->dev, &vi->rq[i].napi, virtnet_poll,
+				      i);
+		vi->rq[i].napi.weight = napi_weight;
 		netif_napi_add_tx_weight(vi->dev, &vi->sq[i].napi,
 					 virtnet_poll_tx,
 					 napi_tx ? napi_weight : 0);
-- 
2.45.2

Re: [PATCH net-next v4 0/4] virtio-net: Link queues to NAPIs

[Lei Yang]

I tested this series of patches with virtio-net regression tests,
everything works fine.

Tested-by: Lei Yang <leiyang@redhat.com>


On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Greetings:
>
> Welcome to v4.
>
> Jakub recently commented [1] that I should not hold this series on
> virtio-net linking queues to NAPIs behind other important work that is
> on-going and suggested I re-spin, so here we are :)
>
> This is a significant refactor from the rfcv3 and as such I've dropped
> almost all of the tags from reviewers except for patch 4 (sorry Gerhard
> and Jason; the changes are significant so I think patches 1-3 need to be
> re-reviewed).
>
> As per the discussion on the v3 [2], now both RX and TX NAPIs use the
> API to link queues to NAPIs. Since TX-only NAPIs don't have a NAPI ID,
> commit 6597e8d35851 ("netdev-genl: Elide napi_id when not present") now
> correctly elides the TX-only NAPIs (instead of printing zero) when the
> queues and NAPIs are linked.
>
> See the commit message of patch 3 for an example of how to get the NAPI
> to queue mapping information.
>
> See the commit message of patch 4 for an example of how NAPI IDs are
> persistent despite queue count changes.
>
> Thanks,
> Joe
>
> v4:
>   - Dropped Jakub's patch (previously patch 1).
>   - Significant refactor from v3 affecting patches 1-3.
>   - Patch 4 added tags from Jason and Gerhard.
>
> rfcv3: https://lore.kernel.org/netdev/20250121191047.269844-1-jdamato@fastly.com/
>   - patch 3:
>     - Removed the xdp checks completely, as Gerhard Engleder pointed
>       out, they are likely not necessary.
>
>   - patch 4:
>     - Added Xuan Zhuo's Reviewed-by.
>
> v2: https://lore.kernel.org/netdev/20250116055302.14308-1-jdamato@fastly.com/
>   - patch 1:
>     - New in the v2 from Jakub.
>
>   - patch 2:
>     - Previously patch 1, unchanged from v1.
>     - Added Gerhard Engleder's Reviewed-by.
>     - Added Lei Yang's Tested-by.
>
>   - patch 3:
>     - Introduced virtnet_napi_disable to eliminate duplicated code
>       in virtnet_xdp_set, virtnet_rx_pause, virtnet_disable_queue_pair,
>       refill_work as suggested by Jason Wang.
>     - As a result of the above refactor, dropped Reviewed-by and
>       Tested-by from patch 3.
>
>   - patch 4:
>     - New in v2. Adds persistent NAPI configuration. See commit message
>       for more details.
>
> v1: https://lore.kernel.org/netdev/20250110202605.429475-1-jdamato@fastly.com/
>
> [1]: https://lore.kernel.org/netdev/20250221142650.3c74dcac@kernel.org/
> [2]: https://lore.kernel.org/netdev/20250127142400.24eca319@kernel.org/
>
> Joe Damato (4):
>   virtio-net: Refactor napi_enable paths
>   virtio-net: Refactor napi_disable paths
>   virtio-net: Map NAPIs to queues
>   virtio_net: Use persistent NAPI config
>
>  drivers/net/virtio_net.c | 100 ++++++++++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 27 deletions(-)
>
>
> base-commit: 7183877d6853801258b7a8d3b51b415982e5097e
> --
> 2.45.2
>
>

Re: [PATCH net-next v4 0/4] virtio-net: Link queues to NAPIs

[Michael S. Tsirkin]

On Tue, Feb 25, 2025 at 02:04:47AM +0000, Joe Damato wrote:
> Greetings:
> 
> Welcome to v4.
> 
> Jakub recently commented [1] that I should not hold this series on
> virtio-net linking queues to NAPIs behind other important work that is
> on-going and suggested I re-spin, so here we are :)
> 
> This is a significant refactor from the rfcv3 and as such I've dropped
> almost all of the tags from reviewers except for patch 4 (sorry Gerhard
> and Jason; the changes are significant so I think patches 1-3 need to be
> re-reviewed).
> 
> As per the discussion on the v3 [2], now both RX and TX NAPIs use the
> API to link queues to NAPIs. Since TX-only NAPIs don't have a NAPI ID,
> commit 6597e8d35851 ("netdev-genl: Elide napi_id when not present") now
> correctly elides the TX-only NAPIs (instead of printing zero) when the
> queues and NAPIs are linked.
> 
> See the commit message of patch 3 for an example of how to get the NAPI
> to queue mapping information.
> 
> See the commit message of patch 4 for an example of how NAPI IDs are
> persistent despite queue count changes.
> 
> Thanks,
> Joe


Acked-by: Michael S. Tsirkin <mst@redhat.com>

> v4:
>   - Dropped Jakub's patch (previously patch 1).
>   - Significant refactor from v3 affecting patches 1-3.
>   - Patch 4 added tags from Jason and Gerhard.
> 
> rfcv3: https://lore.kernel.org/netdev/20250121191047.269844-1-jdamato@fastly.com/
>   - patch 3:
>     - Removed the xdp checks completely, as Gerhard Engleder pointed
>       out, they are likely not necessary.
> 
>   - patch 4:
>     - Added Xuan Zhuo's Reviewed-by.
> 
> v2: https://lore.kernel.org/netdev/20250116055302.14308-1-jdamato@fastly.com/
>   - patch 1:
>     - New in the v2 from Jakub.
> 
>   - patch 2:
>     - Previously patch 1, unchanged from v1.
>     - Added Gerhard Engleder's Reviewed-by.
>     - Added Lei Yang's Tested-by.
> 
>   - patch 3:
>     - Introduced virtnet_napi_disable to eliminate duplicated code
>       in virtnet_xdp_set, virtnet_rx_pause, virtnet_disable_queue_pair,
>       refill_work as suggested by Jason Wang.
>     - As a result of the above refactor, dropped Reviewed-by and
>       Tested-by from patch 3.
> 
>   - patch 4:
>     - New in v2. Adds persistent NAPI configuration. See commit message
>       for more details.
> 
> v1: https://lore.kernel.org/netdev/20250110202605.429475-1-jdamato@fastly.com/
> 
> [1]: https://lore.kernel.org/netdev/20250221142650.3c74dcac@kernel.org/
> [2]: https://lore.kernel.org/netdev/20250127142400.24eca319@kernel.org/
> 
> Joe Damato (4):
>   virtio-net: Refactor napi_enable paths
>   virtio-net: Refactor napi_disable paths
>   virtio-net: Map NAPIs to queues
>   virtio_net: Use persistent NAPI config
> 
>  drivers/net/virtio_net.c | 100 ++++++++++++++++++++++++++++-----------
>  1 file changed, 73 insertions(+), 27 deletions(-)
> 
> 
> base-commit: 7183877d6853801258b7a8d3b51b415982e5097e
> -- 
> 2.45.2

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Jason Wang]

On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> can be accessed by user apps, taking care to hold RTNL as needed.

I may miss something but I wonder whether letting the caller hold the
lock is better.

More below.

>
> $ ethtool -i ens4 | grep driver
> driver: virtio_net
>
> $ sudo ethtool -L ens4 combined 4
>
> $ ./tools/net/ynl/pyynl/cli.py \
>        --spec Documentation/netlink/specs/netdev.yaml \
>        --dump queue-get --json='{"ifindex": 2}'
> [{'id': 0, 'ifindex': 2, 'napi-id': 8289, 'type': 'rx'},
>  {'id': 1, 'ifindex': 2, 'napi-id': 8290, 'type': 'rx'},
>  {'id': 2, 'ifindex': 2, 'napi-id': 8291, 'type': 'rx'},
>  {'id': 3, 'ifindex': 2, 'napi-id': 8292, 'type': 'rx'},
>  {'id': 0, 'ifindex': 2, 'type': 'tx'},
>  {'id': 1, 'ifindex': 2, 'type': 'tx'},
>  {'id': 2, 'ifindex': 2, 'type': 'tx'},
>  {'id': 3, 'ifindex': 2, 'type': 'tx'}]
>
> Note that virtio_net has TX-only NAPIs which do not have NAPI IDs, so
> the lack of 'napi-id' in the above output is expected.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  drivers/net/virtio_net.c | 73 ++++++++++++++++++++++++++++------------
>  1 file changed, 52 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e578885c1093..13bb4a563073 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -2807,6 +2807,20 @@ static void skb_recv_done(struct virtqueue *rvq)
>         virtqueue_napi_schedule(&rq->napi, rvq);
>  }
>
> +static void virtnet_queue_set_napi(struct net_device *dev,
> +                                  struct napi_struct *napi,
> +                                  enum netdev_queue_type q_type, int qidx,
> +                                  bool need_rtnl)
> +{
> +       if (need_rtnl)
> +               rtnl_lock();
> +
> +       netif_queue_set_napi(dev, qidx, q_type, napi);
> +
> +       if (need_rtnl)
> +               rtnl_unlock();
> +}
> +
>  static void virtnet_napi_do_enable(struct virtqueue *vq,
>                                    struct napi_struct *napi)
>  {
> @@ -2821,15 +2835,21 @@ static void virtnet_napi_do_enable(struct virtqueue *vq,
>         local_bh_enable();
>  }
>
> -static void virtnet_napi_enable(struct receive_queue *rq)
> +static void virtnet_napi_enable(struct receive_queue *rq, bool need_rtnl)
>  {
> +       struct virtnet_info *vi = rq->vq->vdev->priv;
> +       int qidx = vq2rxq(rq->vq);
> +
>         virtnet_napi_do_enable(rq->vq, &rq->napi);
> +       virtnet_queue_set_napi(vi->dev, &rq->napi,
> +                              NETDEV_QUEUE_TYPE_RX, qidx, need_rtnl);
>  }
>
> -static void virtnet_napi_tx_enable(struct send_queue *sq)
> +static void virtnet_napi_tx_enable(struct send_queue *sq, bool need_rtnl)
>  {
>         struct virtnet_info *vi = sq->vq->vdev->priv;
>         struct napi_struct *napi = &sq->napi;
> +       int qidx = vq2txq(sq->vq);
>
>         if (!napi->weight)
>                 return;
> @@ -2843,20 +2863,31 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
>         }
>
>         virtnet_napi_do_enable(sq->vq, napi);
> +       virtnet_queue_set_napi(vi->dev, napi, NETDEV_QUEUE_TYPE_TX, qidx,
> +                              need_rtnl);
>  }
>
> -static void virtnet_napi_tx_disable(struct send_queue *sq)
> +static void virtnet_napi_tx_disable(struct send_queue *sq, bool need_rtnl)
>  {
> +       struct virtnet_info *vi = sq->vq->vdev->priv;
>         struct napi_struct *napi = &sq->napi;
> +       int qidx = vq2txq(sq->vq);
>
> -       if (napi->weight)
> +       if (napi->weight) {
> +               virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX,
> +                                      qidx, need_rtnl);
>                 napi_disable(napi);
> +       }
>  }
>
> -static void virtnet_napi_disable(struct receive_queue *rq)
> +static void virtnet_napi_disable(struct receive_queue *rq, bool need_rtnl)
>  {
> +       struct virtnet_info *vi = rq->vq->vdev->priv;
>         struct napi_struct *napi = &rq->napi;
> +       int qidx = vq2rxq(rq->vq);
>
> +       virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX, qidx,
> +                              need_rtnl);
>         napi_disable(napi);
>  }
>
> @@ -2870,9 +2901,9 @@ static void refill_work(struct work_struct *work)
>         for (i = 0; i < vi->curr_queue_pairs; i++) {
>                 struct receive_queue *rq = &vi->rq[i];
>
> -               virtnet_napi_disable(rq);
> +               virtnet_napi_disable(rq, true);
>                 still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> -               virtnet_napi_enable(rq);
> +               virtnet_napi_enable(rq, true);
>
>                 /* In theory, this can happen: if we don't get any buffers in
>                  * we will *never* try to fill again.
> @@ -3069,8 +3100,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>
>  static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
>  {
> -       virtnet_napi_tx_disable(&vi->sq[qp_index]);
> -       virtnet_napi_disable(&vi->rq[qp_index]);
> +       virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> +       virtnet_napi_disable(&vi->rq[qp_index], false);
>         xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
>  }
>
> @@ -3089,8 +3120,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
>         if (err < 0)
>                 goto err_xdp_reg_mem_model;
>
> -       virtnet_napi_enable(&vi->rq[qp_index]);
> -       virtnet_napi_tx_enable(&vi->sq[qp_index]);
> +       virtnet_napi_enable(&vi->rq[qp_index], false);
> +       virtnet_napi_tx_enable(&vi->sq[qp_index], false);
>
>         return 0;
>
> @@ -3342,7 +3373,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
>         bool running = netif_running(vi->dev);
>
>         if (running) {
> -               virtnet_napi_disable(rq);
> +               virtnet_napi_disable(rq, true);

During the resize, the rtnl lock has been held on the ethtool path

        rtnl_lock();
        rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
        rtnl_unlock();

virtnet_rx_resize()
    virtnet_rx_pause()

and in the case of XSK binding, I see ASSERT_RTNL in xp_assign_dev() at least.

Thanks

Re: [PATCH net-next v4 1/4] virtio-net: Refactor napi_enable paths

[Jason Wang]

On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Refactor virtnet_napi_enable and virtnet_napi_tx_enable to take a struct
> receive_queue. Create a helper, virtnet_napi_do_enable, which contains
> the logic to enable a NAPI.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH net-next v4 2/4] virtio-net: Refactor napi_disable paths

[Jason Wang]

On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Create virtnet_napi_disable helper and refactor virtnet_napi_tx_disable
> to take a struct send_queue.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---

Acked-by: Jason Wang <jasowang@redhat.com>

Thanks

Re: [PATCH net-next v4 0/4] virtio-net: Link queues to NAPIs

[Jason Wang]

On Tue, Feb 25, 2025 at 11:47 PM Lei Yang <leiyang@redhat.com> wrote:
>
> I tested this series of patches with virtio-net regression tests,
> everything works fine.
>
> Tested-by: Lei Yang <leiyang@redhat.com>

If it's possible, I would like to add the resize (via ethtool) support
in the regression test.

Thanks

>
>
> On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Greetings:
> >
> > Welcome to v4.
> >
> > Jakub recently commented [1] that I should not hold this series on
> > virtio-net linking queues to NAPIs behind other important work that is
> > on-going and suggested I re-spin, so here we are :)
> >
> > This is a significant refactor from the rfcv3 and as such I've dropped
> > almost all of the tags from reviewers except for patch 4 (sorry Gerhard
> > and Jason; the changes are significant so I think patches 1-3 need to be
> > re-reviewed).
> >
> > As per the discussion on the v3 [2], now both RX and TX NAPIs use the
> > API to link queues to NAPIs. Since TX-only NAPIs don't have a NAPI ID,
> > commit 6597e8d35851 ("netdev-genl: Elide napi_id when not present") now
> > correctly elides the TX-only NAPIs (instead of printing zero) when the
> > queues and NAPIs are linked.
> >
> > See the commit message of patch 3 for an example of how to get the NAPI
> > to queue mapping information.
> >
> > See the commit message of patch 4 for an example of how NAPI IDs are
> > persistent despite queue count changes.
> >
> > Thanks,
> > Joe
> >
> > v4:
> >   - Dropped Jakub's patch (previously patch 1).
> >   - Significant refactor from v3 affecting patches 1-3.
> >   - Patch 4 added tags from Jason and Gerhard.
> >
> > rfcv3: https://lore.kernel.org/netdev/20250121191047.269844-1-jdamato@fastly.com/
> >   - patch 3:
> >     - Removed the xdp checks completely, as Gerhard Engleder pointed
> >       out, they are likely not necessary.
> >
> >   - patch 4:
> >     - Added Xuan Zhuo's Reviewed-by.
> >
> > v2: https://lore.kernel.org/netdev/20250116055302.14308-1-jdamato@fastly.com/
> >   - patch 1:
> >     - New in the v2 from Jakub.
> >
> >   - patch 2:
> >     - Previously patch 1, unchanged from v1.
> >     - Added Gerhard Engleder's Reviewed-by.
> >     - Added Lei Yang's Tested-by.
> >
> >   - patch 3:
> >     - Introduced virtnet_napi_disable to eliminate duplicated code
> >       in virtnet_xdp_set, virtnet_rx_pause, virtnet_disable_queue_pair,
> >       refill_work as suggested by Jason Wang.
> >     - As a result of the above refactor, dropped Reviewed-by and
> >       Tested-by from patch 3.
> >
> >   - patch 4:
> >     - New in v2. Adds persistent NAPI configuration. See commit message
> >       for more details.
> >
> > v1: https://lore.kernel.org/netdev/20250110202605.429475-1-jdamato@fastly.com/
> >
> > [1]: https://lore.kernel.org/netdev/20250221142650.3c74dcac@kernel.org/
> > [2]: https://lore.kernel.org/netdev/20250127142400.24eca319@kernel.org/
> >
> > Joe Damato (4):
> >   virtio-net: Refactor napi_enable paths
> >   virtio-net: Refactor napi_disable paths
> >   virtio-net: Map NAPIs to queues
> >   virtio_net: Use persistent NAPI config
> >
> >  drivers/net/virtio_net.c | 100 ++++++++++++++++++++++++++++-----------
> >  1 file changed, 73 insertions(+), 27 deletions(-)
> >
> >
> > base-commit: 7183877d6853801258b7a8d3b51b415982e5097e
> > --
> > 2.45.2
> >
> >
>

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Joe Damato]

On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > can be accessed by user apps, taking care to hold RTNL as needed.
> 
> I may miss something but I wonder whether letting the caller hold the
> lock is better.

Hmm...

Double checking all the paths over again, here's what I see:
  - refill_work, delayed work that needs RTNL so this change seems
    right?

  - virtnet_disable_queue_pair, called from virtnet_open and
    virtnet_close. When called via NDO these are safe and hold RTNL,
    but they can be called from power management and need RTNL.

  - virtnet_enable_queue_pair called from virtnet_open, safe when
    used via NDO but needs RTNL when used via power management.

  - virtnet_rx_pause called in both paths as you mentioned, one
    which needs RTNL and one which doesn't.

I think there are a couple ways to fix this:

  1. Edit this patch to remove the virtnet_queue_set_napi helper,
     and call netif_queue_set_napi from the napi_enable and
     napi_disable helpers directly. Modify code calling into these
     paths to hold rtnl (or not) as described above.

  2. Modify virtnet_enable_queue_pair, virtnet_disable_queue_pair,
     and virtnet_rx_pause to take a "bool need_rtnl" as an a
     function argument and pass that through.

I'm not sure which is cleaner and I do not have a preference.

Can you let me know which you prefer? I am happy to implement either
one for the next revision.

[...]

> > ---
> >  drivers/net/virtio_net.c | 73 ++++++++++++++++++++++++++++------------
> >  1 file changed, 52 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index e578885c1093..13bb4a563073 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -2807,6 +2807,20 @@ static void skb_recv_done(struct virtqueue *rvq)
> >         virtqueue_napi_schedule(&rq->napi, rvq);
> >  }
> >
> > +static void virtnet_queue_set_napi(struct net_device *dev,
> > +                                  struct napi_struct *napi,
> > +                                  enum netdev_queue_type q_type, int qidx,
> > +                                  bool need_rtnl)
> > +{
> > +       if (need_rtnl)
> > +               rtnl_lock();
> > +
> > +       netif_queue_set_napi(dev, qidx, q_type, napi);
> > +
> > +       if (need_rtnl)
> > +               rtnl_unlock();
> > +}
> > +
> >  static void virtnet_napi_do_enable(struct virtqueue *vq,
> >                                    struct napi_struct *napi)
> >  {
> > @@ -2821,15 +2835,21 @@ static void virtnet_napi_do_enable(struct virtqueue *vq,
> >         local_bh_enable();
> >  }
> >
> > -static void virtnet_napi_enable(struct receive_queue *rq)
> > +static void virtnet_napi_enable(struct receive_queue *rq, bool need_rtnl)
> >  {
> > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> > +       int qidx = vq2rxq(rq->vq);
> > +
> >         virtnet_napi_do_enable(rq->vq, &rq->napi);
> > +       virtnet_queue_set_napi(vi->dev, &rq->napi,
> > +                              NETDEV_QUEUE_TYPE_RX, qidx, need_rtnl);
> >  }
> >
> > -static void virtnet_napi_tx_enable(struct send_queue *sq)
> > +static void virtnet_napi_tx_enable(struct send_queue *sq, bool need_rtnl)
> >  {
> >         struct virtnet_info *vi = sq->vq->vdev->priv;
> >         struct napi_struct *napi = &sq->napi;
> > +       int qidx = vq2txq(sq->vq);
> >
> >         if (!napi->weight)
> >                 return;
> > @@ -2843,20 +2863,31 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
> >         }
> >
> >         virtnet_napi_do_enable(sq->vq, napi);
> > +       virtnet_queue_set_napi(vi->dev, napi, NETDEV_QUEUE_TYPE_TX, qidx,
> > +                              need_rtnl);
> >  }
> >
> > -static void virtnet_napi_tx_disable(struct send_queue *sq)
> > +static void virtnet_napi_tx_disable(struct send_queue *sq, bool need_rtnl)
> >  {
> > +       struct virtnet_info *vi = sq->vq->vdev->priv;
> >         struct napi_struct *napi = &sq->napi;
> > +       int qidx = vq2txq(sq->vq);
> >
> > -       if (napi->weight)
> > +       if (napi->weight) {
> > +               virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX,
> > +                                      qidx, need_rtnl);
> >                 napi_disable(napi);
> > +       }
> >  }
> >
> > -static void virtnet_napi_disable(struct receive_queue *rq)
> > +static void virtnet_napi_disable(struct receive_queue *rq, bool need_rtnl)
> >  {
> > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> >         struct napi_struct *napi = &rq->napi;
> > +       int qidx = vq2rxq(rq->vq);
> >
> > +       virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX, qidx,
> > +                              need_rtnl);
> >         napi_disable(napi);
> >  }
> >
> > @@ -2870,9 +2901,9 @@ static void refill_work(struct work_struct *work)
> >         for (i = 0; i < vi->curr_queue_pairs; i++) {
> >                 struct receive_queue *rq = &vi->rq[i];
> >
> > -               virtnet_napi_disable(rq);
> > +               virtnet_napi_disable(rq, true);
> >                 still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> > -               virtnet_napi_enable(rq);
> > +               virtnet_napi_enable(rq, true);
> >
> >                 /* In theory, this can happen: if we don't get any buffers in
> >                  * we will *never* try to fill again.
> > @@ -3069,8 +3100,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> >
> >  static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> >  {
> > -       virtnet_napi_tx_disable(&vi->sq[qp_index]);
> > -       virtnet_napi_disable(&vi->rq[qp_index]);
> > +       virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> > +       virtnet_napi_disable(&vi->rq[qp_index], false);
> >         xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
> >  }
> >
> > @@ -3089,8 +3120,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> >         if (err < 0)
> >                 goto err_xdp_reg_mem_model;
> >
> > -       virtnet_napi_enable(&vi->rq[qp_index]);
> > -       virtnet_napi_tx_enable(&vi->sq[qp_index]);
> > +       virtnet_napi_enable(&vi->rq[qp_index], false);
> > +       virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> >
> >         return 0;
> >
> > @@ -3342,7 +3373,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
> >         bool running = netif_running(vi->dev);
> >
> >         if (running) {
> > -               virtnet_napi_disable(rq);
> > +               virtnet_napi_disable(rq, true);
> 
> During the resize, the rtnl lock has been held on the ethtool path
> 
>         rtnl_lock();
>         rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
>         rtnl_unlock();
> 
> virtnet_rx_resize()
>     virtnet_rx_pause()
> 
> and in the case of XSK binding, I see ASSERT_RTNL in xp_assign_dev() at least.

Thanks for catching this. I re-read all the paths and I think I've
outlined a few other issues above.

Please let me know which of the proposed methods above you'd like me
to implement to get this merged.

Thanks.

---
pw-bot: cr

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Joe Damato]

On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > >
> > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > can be accessed by user apps, taking care to hold RTNL as needed.
> > 
> > I may miss something but I wonder whether letting the caller hold the
> > lock is better.
> 
> Hmm...
> 
> Double checking all the paths over again, here's what I see:
>   - refill_work, delayed work that needs RTNL so this change seems
>     right?
> 
>   - virtnet_disable_queue_pair, called from virtnet_open and
>     virtnet_close. When called via NDO these are safe and hold RTNL,
>     but they can be called from power management and need RTNL.
> 
>   - virtnet_enable_queue_pair called from virtnet_open, safe when
>     used via NDO but needs RTNL when used via power management.
> 
>   - virtnet_rx_pause called in both paths as you mentioned, one
>     which needs RTNL and one which doesn't.

Sorry, I missed more paths:

    - virtnet_rx_resume
    - virtnet_tx_pause and virtnet_tx_resume

which are similar to path you mentioned (virtnet_rx_pause) and need
rtnl in one of two different paths.

Let me know if I missed any paths and what your preferred way to fix
this would be?

I think both options below are possible and I have no strong
preference.

> 
> I think there are a couple ways to fix this:
> 
>   1. Edit this patch to remove the virtnet_queue_set_napi helper,
>      and call netif_queue_set_napi from the napi_enable and
>      napi_disable helpers directly. Modify code calling into these
>      paths to hold rtnl (or not) as described above.
> 
>   2. Modify virtnet_enable_queue_pair, virtnet_disable_queue_pair,
>      and virtnet_rx_pause to take a "bool need_rtnl" as an a
>      function argument and pass that through.
> 
> I'm not sure which is cleaner and I do not have a preference.
> 
> Can you let me know which you prefer? I am happy to implement either
> one for the next revision.
> 
> [...]
> 
> > > ---
> > >  drivers/net/virtio_net.c | 73 ++++++++++++++++++++++++++++------------
> > >  1 file changed, 52 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index e578885c1093..13bb4a563073 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -2807,6 +2807,20 @@ static void skb_recv_done(struct virtqueue *rvq)
> > >         virtqueue_napi_schedule(&rq->napi, rvq);
> > >  }
> > >
> > > +static void virtnet_queue_set_napi(struct net_device *dev,
> > > +                                  struct napi_struct *napi,
> > > +                                  enum netdev_queue_type q_type, int qidx,
> > > +                                  bool need_rtnl)
> > > +{
> > > +       if (need_rtnl)
> > > +               rtnl_lock();
> > > +
> > > +       netif_queue_set_napi(dev, qidx, q_type, napi);
> > > +
> > > +       if (need_rtnl)
> > > +               rtnl_unlock();
> > > +}
> > > +
> > >  static void virtnet_napi_do_enable(struct virtqueue *vq,
> > >                                    struct napi_struct *napi)
> > >  {
> > > @@ -2821,15 +2835,21 @@ static void virtnet_napi_do_enable(struct virtqueue *vq,
> > >         local_bh_enable();
> > >  }
> > >
> > > -static void virtnet_napi_enable(struct receive_queue *rq)
> > > +static void virtnet_napi_enable(struct receive_queue *rq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> > > +       int qidx = vq2rxq(rq->vq);
> > > +
> > >         virtnet_napi_do_enable(rq->vq, &rq->napi);
> > > +       virtnet_queue_set_napi(vi->dev, &rq->napi,
> > > +                              NETDEV_QUEUE_TYPE_RX, qidx, need_rtnl);
> > >  }
> > >
> > > -static void virtnet_napi_tx_enable(struct send_queue *sq)
> > > +static void virtnet_napi_tx_enable(struct send_queue *sq, bool need_rtnl)
> > >  {
> > >         struct virtnet_info *vi = sq->vq->vdev->priv;
> > >         struct napi_struct *napi = &sq->napi;
> > > +       int qidx = vq2txq(sq->vq);
> > >
> > >         if (!napi->weight)
> > >                 return;
> > > @@ -2843,20 +2863,31 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
> > >         }
> > >
> > >         virtnet_napi_do_enable(sq->vq, napi);
> > > +       virtnet_queue_set_napi(vi->dev, napi, NETDEV_QUEUE_TYPE_TX, qidx,
> > > +                              need_rtnl);
> > >  }
> > >
> > > -static void virtnet_napi_tx_disable(struct send_queue *sq)
> > > +static void virtnet_napi_tx_disable(struct send_queue *sq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = sq->vq->vdev->priv;
> > >         struct napi_struct *napi = &sq->napi;
> > > +       int qidx = vq2txq(sq->vq);
> > >
> > > -       if (napi->weight)
> > > +       if (napi->weight) {
> > > +               virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX,
> > > +                                      qidx, need_rtnl);
> > >                 napi_disable(napi);
> > > +       }
> > >  }
> > >
> > > -static void virtnet_napi_disable(struct receive_queue *rq)
> > > +static void virtnet_napi_disable(struct receive_queue *rq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> > >         struct napi_struct *napi = &rq->napi;
> > > +       int qidx = vq2rxq(rq->vq);
> > >
> > > +       virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX, qidx,
> > > +                              need_rtnl);
> > >         napi_disable(napi);
> > >  }
> > >
> > > @@ -2870,9 +2901,9 @@ static void refill_work(struct work_struct *work)
> > >         for (i = 0; i < vi->curr_queue_pairs; i++) {
> > >                 struct receive_queue *rq = &vi->rq[i];
> > >
> > > -               virtnet_napi_disable(rq);
> > > +               virtnet_napi_disable(rq, true);
> > >                 still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> > > -               virtnet_napi_enable(rq);
> > > +               virtnet_napi_enable(rq, true);
> > >
> > >                 /* In theory, this can happen: if we don't get any buffers in
> > >                  * we will *never* try to fill again.
> > > @@ -3069,8 +3100,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > >
> > >  static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >  {
> > > -       virtnet_napi_tx_disable(&vi->sq[qp_index]);
> > > -       virtnet_napi_disable(&vi->rq[qp_index]);
> > > +       virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> > > +       virtnet_napi_disable(&vi->rq[qp_index], false);
> > >         xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
> > >  }
> > >
> > > @@ -3089,8 +3120,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >         if (err < 0)
> > >                 goto err_xdp_reg_mem_model;
> > >
> > > -       virtnet_napi_enable(&vi->rq[qp_index]);
> > > -       virtnet_napi_tx_enable(&vi->sq[qp_index]);
> > > +       virtnet_napi_enable(&vi->rq[qp_index], false);
> > > +       virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> > >
> > >         return 0;
> > >
> > > @@ -3342,7 +3373,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
> > >         bool running = netif_running(vi->dev);
> > >
> > >         if (running) {
> > > -               virtnet_napi_disable(rq);
> > > +               virtnet_napi_disable(rq, true);
> > 
> > During the resize, the rtnl lock has been held on the ethtool path
> > 
> >         rtnl_lock();
> >         rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
> >         rtnl_unlock();
> > 
> > virtnet_rx_resize()
> >     virtnet_rx_pause()
> > 
> > and in the case of XSK binding, I see ASSERT_RTNL in xp_assign_dev() at least.
> 
> Thanks for catching this. I re-read all the paths and I think I've
> outlined a few other issues above.
> 
> Please let me know which of the proposed methods above you'd like me
> to implement to get this merged.
> 
> Thanks.
> 
> ---
> pw-bot: cr

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Joe Damato]

On Wed, Feb 26, 2025 at 01:08:49PM -0500, Joe Damato wrote:
> On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> > On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > > >
> > > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > > can be accessed by user apps, taking care to hold RTNL as needed.
> > > 
> > > I may miss something but I wonder whether letting the caller hold the
> > > lock is better.
> > 
> > Hmm...
> > 
> > Double checking all the paths over again, here's what I see:
> >   - refill_work, delayed work that needs RTNL so this change seems
> >     right?
> > 
> >   - virtnet_disable_queue_pair, called from virtnet_open and
> >     virtnet_close. When called via NDO these are safe and hold RTNL,
> >     but they can be called from power management and need RTNL.
> > 
> >   - virtnet_enable_queue_pair called from virtnet_open, safe when
> >     used via NDO but needs RTNL when used via power management.
> > 
> >   - virtnet_rx_pause called in both paths as you mentioned, one
> >     which needs RTNL and one which doesn't.
> 
> Sorry, I missed more paths:
> 
>     - virtnet_rx_resume
>     - virtnet_tx_pause and virtnet_tx_resume
> 
> which are similar to path you mentioned (virtnet_rx_pause) and need
> rtnl in one of two different paths.
> 
> Let me know if I missed any paths and what your preferred way to fix
> this would be?
> 
> I think both options below are possible and I have no strong
> preference.

OK, my apologies. I read your message and the code wrong. Sorry for
the back-to-back emails from me.

Please ignore my message above... I think after re-reading the code,
here's where I've arrived:

  - refill_work needs to hold RTNL (as in the existing patch)

  - virtnet_rx_pause, virtnet_rx_resume, virtnet_tx_pause,
    virtnet_tx_resume -- all do NOT need to hold RTNL because it is
    already held in the ethtool resize path and the XSK path, as you
    explained, but I mis-read (sorry).

  - virtnet_disable_queue_pair and virtnet_enable_queue_pair both
    need to hold RTNL only when called via power management, but not
    when called via ndo_open or ndo_close

Is my understanding correct and does it match your understanding?

If so, that means there are two issues:

  1. Fixing the hardcoded bools in rx_pause, rx_resume, tx_pause,
     tx_resume (all should be false, RTNL is not needed).

  2. Handling the power management case which calls virtnet_open and
     virtnet_close.

I made a small diff included below as an example of a possible
solution:

  1. Modify virtnet_disable_queue_pair and virtnet_enable_queue_pair
     to take a "bool need_rtnl" and pass it through to the helpers
     they call.

  2. Create two helpers, virtnet_do_open and virt_do_close both of
     which take struct net_device *dev, bool need_rtnl. virtnet_open
     and virtnet_close are modified to call the helpers and pass
     false for need_rtnl. The power management paths call the
     helpers and pass true for need_rtnl. (fixes issue 2 above)

  3. Fix the bools for rx_pause, rx_resume, tx_pause, tx_resume to
     pass false since all paths that I could find that lead to these
     functions hold RTNL. (fixes issue 1 above)

See the diff below (which can be applied on top of patch 3) to see
what it looks like.

If you are OK with this approach, I will send a v5 where patch 3
includes the changes shown in this diff.

Please let me know what you think:

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 13bb4a563073..76ecb8f3ce9a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3098,14 +3098,16 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
 	return received;
 }
 
-static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
+static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index,
+				       bool need_rtnl)
 {
-	virtnet_napi_tx_disable(&vi->sq[qp_index], false);
-	virtnet_napi_disable(&vi->rq[qp_index], false);
+	virtnet_napi_tx_disable(&vi->sq[qp_index], need_rtnl);
+	virtnet_napi_disable(&vi->rq[qp_index], need_rtnl);
 	xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
 }
 
-static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
+static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index,
+				     bool need_rtnl)
 {
 	struct net_device *dev = vi->dev;
 	int err;
@@ -3120,8 +3122,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
 	if (err < 0)
 		goto err_xdp_reg_mem_model;
 
-	virtnet_napi_enable(&vi->rq[qp_index], false);
-	virtnet_napi_tx_enable(&vi->sq[qp_index], false);
+	virtnet_napi_enable(&vi->rq[qp_index], need_rtnl);
+	virtnet_napi_tx_enable(&vi->sq[qp_index], need_rtnl);
 
 	return 0;
 
@@ -3156,7 +3158,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
 		vi->duplex = duplex;
 }
 
-static int virtnet_open(struct net_device *dev)
+static int virtnet_do_open(struct net_device *dev, bool need_rtnl)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i, err;
@@ -3169,7 +3171,7 @@ static int virtnet_open(struct net_device *dev)
 			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
 				schedule_delayed_work(&vi->refill, 0);
 
-		err = virtnet_enable_queue_pair(vi, i);
+		err = virtnet_enable_queue_pair(vi, i, need_rtnl);
 		if (err < 0)
 			goto err_enable_qp;
 	}
@@ -3190,13 +3192,18 @@ static int virtnet_open(struct net_device *dev)
 	cancel_delayed_work_sync(&vi->refill);
 
 	for (i--; i >= 0; i--) {
-		virtnet_disable_queue_pair(vi, i);
+		virtnet_disable_queue_pair(vi, i, need_rtnl);
 		virtnet_cancel_dim(vi, &vi->rq[i].dim);
 	}
 
 	return err;
 }
 
+static int virtnet_open(struct net_device *dev)
+{
+	return virtnet_do_open(dev, false);
+}
+
 static int virtnet_poll_tx(struct napi_struct *napi, int budget)
 {
 	struct send_queue *sq = container_of(napi, struct send_queue, napi);
@@ -3373,7 +3380,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
 	bool running = netif_running(vi->dev);
 
 	if (running) {
-		virtnet_napi_disable(rq, true);
+		virtnet_napi_disable(rq, false);
 		virtnet_cancel_dim(vi, &rq->dim);
 	}
 }
@@ -3386,7 +3393,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
 		schedule_delayed_work(&vi->refill, 0);
 
 	if (running)
-		virtnet_napi_enable(rq, true);
+		virtnet_napi_enable(rq, false);
 }
 
 static int virtnet_rx_resize(struct virtnet_info *vi,
@@ -3415,7 +3422,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
 	qindex = sq - vi->sq;
 
 	if (running)
-		virtnet_napi_tx_disable(sq, true);
+		virtnet_napi_tx_disable(sq, false);
 
 	txq = netdev_get_tx_queue(vi->dev, qindex);
 
@@ -3449,7 +3456,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
 	__netif_tx_unlock_bh(txq);
 
 	if (running)
-		virtnet_napi_tx_enable(sq, true);
+		virtnet_napi_tx_enable(sq, false);
 }
 
 static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
@@ -3708,7 +3715,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
 	return 0;
 }
 
-static int virtnet_close(struct net_device *dev)
+static int virtnet_do_close(struct net_device *dev, bool need_rtnl)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i;
@@ -3727,7 +3734,7 @@ static int virtnet_close(struct net_device *dev)
 	cancel_work_sync(&vi->config_work);
 
 	for (i = 0; i < vi->max_queue_pairs; i++) {
-		virtnet_disable_queue_pair(vi, i);
+		virtnet_disable_queue_pair(vi, i, need_rtnl);
 		virtnet_cancel_dim(vi, &vi->rq[i].dim);
 	}
 
@@ -3736,6 +3743,11 @@ static int virtnet_close(struct net_device *dev)
 	return 0;
 }
 
+static int virtnet_close(struct net_device *dev)
+{
+	return virtnet_do_close(dev, false);
+}
+
 static void virtnet_rx_mode_work(struct work_struct *work)
 {
 	struct virtnet_info *vi =
@@ -5682,7 +5694,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
 	netif_device_detach(vi->dev);
 	netif_tx_unlock_bh(vi->dev);
 	if (netif_running(vi->dev))
-		virtnet_close(vi->dev);
+		virtnet_do_close(vi->dev, true);
 }
 
 static int init_vqs(struct virtnet_info *vi);
@@ -5702,7 +5714,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 	enable_rx_mode_work(vi);
 
 	if (netif_running(vi->dev)) {
-		err = virtnet_open(vi->dev);
+		err = virtnet_do_open(vi->dev, false);
 		if (err)
 			return err;
 	}

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Michael S. Tsirkin]

On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > >
> > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > can be accessed by user apps, taking care to hold RTNL as needed.
> > 
> > I may miss something but I wonder whether letting the caller hold the
> > lock is better.
> 
> Hmm...
> 
> Double checking all the paths over again, here's what I see:
>   - refill_work, delayed work that needs RTNL so this change seems
>     right?
> 
>   - virtnet_disable_queue_pair, called from virtnet_open and
>     virtnet_close. When called via NDO these are safe and hold RTNL,
>     but they can be called from power management and need RTNL.
> 
>   - virtnet_enable_queue_pair called from virtnet_open, safe when
>     used via NDO but needs RTNL when used via power management.
> 
>   - virtnet_rx_pause called in both paths as you mentioned, one
>     which needs RTNL and one which doesn't.
> 
> I think there are a couple ways to fix this:
> 
>   1. Edit this patch to remove the virtnet_queue_set_napi helper,
>      and call netif_queue_set_napi from the napi_enable and
>      napi_disable helpers directly. Modify code calling into these
>      paths to hold rtnl (or not) as described above.
> 
>   2. Modify virtnet_enable_queue_pair, virtnet_disable_queue_pair,
>      and virtnet_rx_pause to take a "bool need_rtnl" as an a
>      function argument and pass that through.
> 
> I'm not sure which is cleaner and I do not have a preference.
> 
> Can you let me know which you prefer? I am happy to implement either
> one for the next revision.


1  seems cleaner.
taking locks depending on paths is confusing

> [...]
> 
> > > ---
> > >  drivers/net/virtio_net.c | 73 ++++++++++++++++++++++++++++------------
> > >  1 file changed, 52 insertions(+), 21 deletions(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index e578885c1093..13bb4a563073 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -2807,6 +2807,20 @@ static void skb_recv_done(struct virtqueue *rvq)
> > >         virtqueue_napi_schedule(&rq->napi, rvq);
> > >  }
> > >
> > > +static void virtnet_queue_set_napi(struct net_device *dev,
> > > +                                  struct napi_struct *napi,
> > > +                                  enum netdev_queue_type q_type, int qidx,
> > > +                                  bool need_rtnl)
> > > +{
> > > +       if (need_rtnl)
> > > +               rtnl_lock();
> > > +
> > > +       netif_queue_set_napi(dev, qidx, q_type, napi);
> > > +
> > > +       if (need_rtnl)
> > > +               rtnl_unlock();
> > > +}
> > > +
> > >  static void virtnet_napi_do_enable(struct virtqueue *vq,
> > >                                    struct napi_struct *napi)
> > >  {
> > > @@ -2821,15 +2835,21 @@ static void virtnet_napi_do_enable(struct virtqueue *vq,
> > >         local_bh_enable();
> > >  }
> > >
> > > -static void virtnet_napi_enable(struct receive_queue *rq)
> > > +static void virtnet_napi_enable(struct receive_queue *rq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> > > +       int qidx = vq2rxq(rq->vq);
> > > +
> > >         virtnet_napi_do_enable(rq->vq, &rq->napi);
> > > +       virtnet_queue_set_napi(vi->dev, &rq->napi,
> > > +                              NETDEV_QUEUE_TYPE_RX, qidx, need_rtnl);
> > >  }
> > >
> > > -static void virtnet_napi_tx_enable(struct send_queue *sq)
> > > +static void virtnet_napi_tx_enable(struct send_queue *sq, bool need_rtnl)
> > >  {
> > >         struct virtnet_info *vi = sq->vq->vdev->priv;
> > >         struct napi_struct *napi = &sq->napi;
> > > +       int qidx = vq2txq(sq->vq);
> > >
> > >         if (!napi->weight)
> > >                 return;
> > > @@ -2843,20 +2863,31 @@ static void virtnet_napi_tx_enable(struct send_queue *sq)
> > >         }
> > >
> > >         virtnet_napi_do_enable(sq->vq, napi);
> > > +       virtnet_queue_set_napi(vi->dev, napi, NETDEV_QUEUE_TYPE_TX, qidx,
> > > +                              need_rtnl);
> > >  }
> > >
> > > -static void virtnet_napi_tx_disable(struct send_queue *sq)
> > > +static void virtnet_napi_tx_disable(struct send_queue *sq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = sq->vq->vdev->priv;
> > >         struct napi_struct *napi = &sq->napi;
> > > +       int qidx = vq2txq(sq->vq);
> > >
> > > -       if (napi->weight)
> > > +       if (napi->weight) {
> > > +               virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX,
> > > +                                      qidx, need_rtnl);
> > >                 napi_disable(napi);
> > > +       }
> > >  }
> > >
> > > -static void virtnet_napi_disable(struct receive_queue *rq)
> > > +static void virtnet_napi_disable(struct receive_queue *rq, bool need_rtnl)
> > >  {
> > > +       struct virtnet_info *vi = rq->vq->vdev->priv;
> > >         struct napi_struct *napi = &rq->napi;
> > > +       int qidx = vq2rxq(rq->vq);
> > >
> > > +       virtnet_queue_set_napi(vi->dev, NULL, NETDEV_QUEUE_TYPE_TX, qidx,
> > > +                              need_rtnl);
> > >         napi_disable(napi);
> > >  }
> > >
> > > @@ -2870,9 +2901,9 @@ static void refill_work(struct work_struct *work)
> > >         for (i = 0; i < vi->curr_queue_pairs; i++) {
> > >                 struct receive_queue *rq = &vi->rq[i];
> > >
> > > -               virtnet_napi_disable(rq);
> > > +               virtnet_napi_disable(rq, true);
> > >                 still_empty = !try_fill_recv(vi, rq, GFP_KERNEL);
> > > -               virtnet_napi_enable(rq);
> > > +               virtnet_napi_enable(rq, true);
> > >
> > >                 /* In theory, this can happen: if we don't get any buffers in
> > >                  * we will *never* try to fill again.
> > > @@ -3069,8 +3100,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > >
> > >  static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >  {
> > > -       virtnet_napi_tx_disable(&vi->sq[qp_index]);
> > > -       virtnet_napi_disable(&vi->rq[qp_index]);
> > > +       virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> > > +       virtnet_napi_disable(&vi->rq[qp_index], false);
> > >         xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
> > >  }
> > >
> > > @@ -3089,8 +3120,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >         if (err < 0)
> > >                 goto err_xdp_reg_mem_model;
> > >
> > > -       virtnet_napi_enable(&vi->rq[qp_index]);
> > > -       virtnet_napi_tx_enable(&vi->sq[qp_index]);
> > > +       virtnet_napi_enable(&vi->rq[qp_index], false);
> > > +       virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> > >
> > >         return 0;
> > >
> > > @@ -3342,7 +3373,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
> > >         bool running = netif_running(vi->dev);
> > >
> > >         if (running) {
> > > -               virtnet_napi_disable(rq);
> > > +               virtnet_napi_disable(rq, true);
> > 
> > During the resize, the rtnl lock has been held on the ethtool path
> > 
> >         rtnl_lock();
> >         rc = __dev_ethtool(net, ifr, useraddr, ethcmd, state);
> >         rtnl_unlock();
> > 
> > virtnet_rx_resize()
> >     virtnet_rx_pause()
> > 
> > and in the case of XSK binding, I see ASSERT_RTNL in xp_assign_dev() at least.
> 
> Thanks for catching this. I re-read all the paths and I think I've
> outlined a few other issues above.
> 
> Please let me know which of the proposed methods above you'd like me
> to implement to get this merged.
> 
> Thanks.
> 
> ---
> pw-bot: cr

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Michael S. Tsirkin]

On Wed, Feb 26, 2025 at 03:27:42PM -0500, Joe Damato wrote:
> On Wed, Feb 26, 2025 at 01:08:49PM -0500, Joe Damato wrote:
> > On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> > > On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > > > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > > > >
> > > > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > > > can be accessed by user apps, taking care to hold RTNL as needed.
> > > > 
> > > > I may miss something but I wonder whether letting the caller hold the
> > > > lock is better.
> > > 
> > > Hmm...
> > > 
> > > Double checking all the paths over again, here's what I see:
> > >   - refill_work, delayed work that needs RTNL so this change seems
> > >     right?
> > > 
> > >   - virtnet_disable_queue_pair, called from virtnet_open and
> > >     virtnet_close. When called via NDO these are safe and hold RTNL,
> > >     but they can be called from power management and need RTNL.
> > > 
> > >   - virtnet_enable_queue_pair called from virtnet_open, safe when
> > >     used via NDO but needs RTNL when used via power management.
> > > 
> > >   - virtnet_rx_pause called in both paths as you mentioned, one
> > >     which needs RTNL and one which doesn't.
> > 
> > Sorry, I missed more paths:
> > 
> >     - virtnet_rx_resume
> >     - virtnet_tx_pause and virtnet_tx_resume
> > 
> > which are similar to path you mentioned (virtnet_rx_pause) and need
> > rtnl in one of two different paths.
> > 
> > Let me know if I missed any paths and what your preferred way to fix
> > this would be?
> > 
> > I think both options below are possible and I have no strong
> > preference.
> 
> OK, my apologies. I read your message and the code wrong. Sorry for
> the back-to-back emails from me.
> 
> Please ignore my message above... I think after re-reading the code,
> here's where I've arrived:
> 
>   - refill_work needs to hold RTNL (as in the existing patch)
> 
>   - virtnet_rx_pause, virtnet_rx_resume, virtnet_tx_pause,
>     virtnet_tx_resume -- all do NOT need to hold RTNL because it is
>     already held in the ethtool resize path and the XSK path, as you
>     explained, but I mis-read (sorry).
> 
>   - virtnet_disable_queue_pair and virtnet_enable_queue_pair both
>     need to hold RTNL only when called via power management, but not
>     when called via ndo_open or ndo_close
> 
> Is my understanding correct and does it match your understanding?
> 
> If so, that means there are two issues:
> 
>   1. Fixing the hardcoded bools in rx_pause, rx_resume, tx_pause,
>      tx_resume (all should be false, RTNL is not needed).
> 
>   2. Handling the power management case which calls virtnet_open and
>      virtnet_close.
> 
> I made a small diff included below as an example of a possible
> solution:
> 
>   1. Modify virtnet_disable_queue_pair and virtnet_enable_queue_pair
>      to take a "bool need_rtnl" and pass it through to the helpers
>      they call.
> 
>   2. Create two helpers, virtnet_do_open and virt_do_close both of
>      which take struct net_device *dev, bool need_rtnl. virtnet_open
>      and virtnet_close are modified to call the helpers and pass
>      false for need_rtnl. The power management paths call the
>      helpers and pass true for need_rtnl. (fixes issue 2 above)
> 
>   3. Fix the bools for rx_pause, rx_resume, tx_pause, tx_resume to
>      pass false since all paths that I could find that lead to these
>      functions hold RTNL. (fixes issue 1 above)
> 
> See the diff below (which can be applied on top of patch 3) to see
> what it looks like.
> 
> If you are OK with this approach, I will send a v5 where patch 3
> includes the changes shown in this diff.
> 
> Please let me know what you think:



Looks ok I think.

> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 13bb4a563073..76ecb8f3ce9a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3098,14 +3098,16 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
>  	return received;
>  }
>  
> -static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> +static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index,
> +				       bool need_rtnl)
>  {
> -	virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> -	virtnet_napi_disable(&vi->rq[qp_index], false);
> +	virtnet_napi_tx_disable(&vi->sq[qp_index], need_rtnl);
> +	virtnet_napi_disable(&vi->rq[qp_index], need_rtnl);
>  	xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
>  }
>  
> -static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> +static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index,
> +				     bool need_rtnl)
>  {
>  	struct net_device *dev = vi->dev;
>  	int err;
> @@ -3120,8 +3122,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
>  	if (err < 0)
>  		goto err_xdp_reg_mem_model;
>  
> -	virtnet_napi_enable(&vi->rq[qp_index], false);
> -	virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> +	virtnet_napi_enable(&vi->rq[qp_index], need_rtnl);
> +	virtnet_napi_tx_enable(&vi->sq[qp_index], need_rtnl);
>  
>  	return 0;
>  
> @@ -3156,7 +3158,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
>  		vi->duplex = duplex;
>  }
>  
> -static int virtnet_open(struct net_device *dev)
> +static int virtnet_do_open(struct net_device *dev, bool need_rtnl)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	int i, err;
> @@ -3169,7 +3171,7 @@ static int virtnet_open(struct net_device *dev)
>  			if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
>  				schedule_delayed_work(&vi->refill, 0);
>  
> -		err = virtnet_enable_queue_pair(vi, i);
> +		err = virtnet_enable_queue_pair(vi, i, need_rtnl);
>  		if (err < 0)
>  			goto err_enable_qp;
>  	}
> @@ -3190,13 +3192,18 @@ static int virtnet_open(struct net_device *dev)
>  	cancel_delayed_work_sync(&vi->refill);
>  
>  	for (i--; i >= 0; i--) {
> -		virtnet_disable_queue_pair(vi, i);
> +		virtnet_disable_queue_pair(vi, i, need_rtnl);
>  		virtnet_cancel_dim(vi, &vi->rq[i].dim);
>  	}
>  
>  	return err;
>  }
>  
> +static int virtnet_open(struct net_device *dev)
> +{
> +	return virtnet_do_open(dev, false);
> +}
> +
>  static int virtnet_poll_tx(struct napi_struct *napi, int budget)
>  {
>  	struct send_queue *sq = container_of(napi, struct send_queue, napi);
> @@ -3373,7 +3380,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
>  	bool running = netif_running(vi->dev);
>  
>  	if (running) {
> -		virtnet_napi_disable(rq, true);
> +		virtnet_napi_disable(rq, false);
>  		virtnet_cancel_dim(vi, &rq->dim);
>  	}
>  }
> @@ -3386,7 +3393,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
>  		schedule_delayed_work(&vi->refill, 0);
>  
>  	if (running)
> -		virtnet_napi_enable(rq, true);
> +		virtnet_napi_enable(rq, false);
>  }
>  
>  static int virtnet_rx_resize(struct virtnet_info *vi,
> @@ -3415,7 +3422,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
>  	qindex = sq - vi->sq;
>  
>  	if (running)
> -		virtnet_napi_tx_disable(sq, true);
> +		virtnet_napi_tx_disable(sq, false);
>  
>  	txq = netdev_get_tx_queue(vi->dev, qindex);
>  
> @@ -3449,7 +3456,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
>  	__netif_tx_unlock_bh(txq);
>  
>  	if (running)
> -		virtnet_napi_tx_enable(sq, true);
> +		virtnet_napi_tx_enable(sq, false);
>  }
>  
>  static int virtnet_tx_resize(struct virtnet_info *vi, struct send_queue *sq,
> @@ -3708,7 +3715,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>  	return 0;
>  }
>  
> -static int virtnet_close(struct net_device *dev)
> +static int virtnet_do_close(struct net_device *dev, bool need_rtnl)
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	int i;
> @@ -3727,7 +3734,7 @@ static int virtnet_close(struct net_device *dev)
>  	cancel_work_sync(&vi->config_work);
>  
>  	for (i = 0; i < vi->max_queue_pairs; i++) {
> -		virtnet_disable_queue_pair(vi, i);
> +		virtnet_disable_queue_pair(vi, i, need_rtnl);
>  		virtnet_cancel_dim(vi, &vi->rq[i].dim);
>  	}
>  
> @@ -3736,6 +3743,11 @@ static int virtnet_close(struct net_device *dev)
>  	return 0;
>  }
>  
> +static int virtnet_close(struct net_device *dev)
> +{
> +	return virtnet_do_close(dev, false);
> +}
> +
>  static void virtnet_rx_mode_work(struct work_struct *work)
>  {
>  	struct virtnet_info *vi =
> @@ -5682,7 +5694,7 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
>  	netif_device_detach(vi->dev);
>  	netif_tx_unlock_bh(vi->dev);
>  	if (netif_running(vi->dev))
> -		virtnet_close(vi->dev);
> +		virtnet_do_close(vi->dev, true);
>  }
>  
>  static int init_vqs(struct virtnet_info *vi);
> @@ -5702,7 +5714,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>  	enable_rx_mode_work(vi);
>  
>  	if (netif_running(vi->dev)) {
> -		err = virtnet_open(vi->dev);
> +		err = virtnet_do_open(vi->dev, false);
>  		if (err)
>  			return err;
>  	}

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Jason Wang]

On Thu, Feb 27, 2025 at 6:13 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Wed, Feb 26, 2025 at 03:27:42PM -0500, Joe Damato wrote:
> > On Wed, Feb 26, 2025 at 01:08:49PM -0500, Joe Damato wrote:
> > > On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> > > > On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > > > > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > > > > >
> > > > > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > > > > can be accessed by user apps, taking care to hold RTNL as needed.
> > > > >
> > > > > I may miss something but I wonder whether letting the caller hold the
> > > > > lock is better.
> > > >
> > > > Hmm...
> > > >
> > > > Double checking all the paths over again, here's what I see:
> > > >   - refill_work, delayed work that needs RTNL so this change seems
> > > >     right?
> > > >
> > > >   - virtnet_disable_queue_pair, called from virtnet_open and
> > > >     virtnet_close. When called via NDO these are safe and hold RTNL,
> > > >     but they can be called from power management and need RTNL.
> > > >
> > > >   - virtnet_enable_queue_pair called from virtnet_open, safe when
> > > >     used via NDO but needs RTNL when used via power management.
> > > >
> > > >   - virtnet_rx_pause called in both paths as you mentioned, one
> > > >     which needs RTNL and one which doesn't.
> > >
> > > Sorry, I missed more paths:
> > >
> > >     - virtnet_rx_resume
> > >     - virtnet_tx_pause and virtnet_tx_resume
> > >
> > > which are similar to path you mentioned (virtnet_rx_pause) and need
> > > rtnl in one of two different paths.
> > >
> > > Let me know if I missed any paths and what your preferred way to fix
> > > this would be?
> > >
> > > I think both options below are possible and I have no strong
> > > preference.
> >
> > OK, my apologies. I read your message and the code wrong. Sorry for
> > the back-to-back emails from me.
> >
> > Please ignore my message above... I think after re-reading the code,
> > here's where I've arrived:
> >
> >   - refill_work needs to hold RTNL (as in the existing patch)
> >
> >   - virtnet_rx_pause, virtnet_rx_resume, virtnet_tx_pause,
> >     virtnet_tx_resume -- all do NOT need to hold RTNL because it is
> >     already held in the ethtool resize path and the XSK path, as you
> >     explained, but I mis-read (sorry).
> >
> >   - virtnet_disable_queue_pair and virtnet_enable_queue_pair both
> >     need to hold RTNL only when called via power management, but not
> >     when called via ndo_open or ndo_close
> >
> > Is my understanding correct and does it match your understanding?
> >
> > If so, that means there are two issues:
> >
> >   1. Fixing the hardcoded bools in rx_pause, rx_resume, tx_pause,
> >      tx_resume (all should be false, RTNL is not needed).
> >
> >   2. Handling the power management case which calls virtnet_open and
> >      virtnet_close.
> >
> > I made a small diff included below as an example of a possible
> > solution:
> >
> >   1. Modify virtnet_disable_queue_pair and virtnet_enable_queue_pair
> >      to take a "bool need_rtnl" and pass it through to the helpers
> >      they call.
> >
> >   2. Create two helpers, virtnet_do_open and virt_do_close both of
> >      which take struct net_device *dev, bool need_rtnl. virtnet_open
> >      and virtnet_close are modified to call the helpers and pass
> >      false for need_rtnl. The power management paths call the
> >      helpers and pass true for need_rtnl. (fixes issue 2 above)
> >
> >   3. Fix the bools for rx_pause, rx_resume, tx_pause, tx_resume to
> >      pass false since all paths that I could find that lead to these
> >      functions hold RTNL. (fixes issue 1 above)
> >
> > See the diff below (which can be applied on top of patch 3) to see
> > what it looks like.
> >
> > If you are OK with this approach, I will send a v5 where patch 3
> > includes the changes shown in this diff.
> >
> > Please let me know what you think:
>
>
>
> Looks ok I think.
>
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 13bb4a563073..76ecb8f3ce9a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -3098,14 +3098,16 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> >       return received;
> >  }
> >
> > -static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > +static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index,
> > +                                    bool need_rtnl)
> >  {
> > -     virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> > -     virtnet_napi_disable(&vi->rq[qp_index], false);
> > +     virtnet_napi_tx_disable(&vi->sq[qp_index], need_rtnl);
> > +     virtnet_napi_disable(&vi->rq[qp_index], need_rtnl);
> >       xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
> >  }
> >
> > -static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > +static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index,
> > +                                  bool need_rtnl)
> >  {
> >       struct net_device *dev = vi->dev;
> >       int err;
> > @@ -3120,8 +3122,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> >       if (err < 0)
> >               goto err_xdp_reg_mem_model;
> >
> > -     virtnet_napi_enable(&vi->rq[qp_index], false);
> > -     virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> > +     virtnet_napi_enable(&vi->rq[qp_index], need_rtnl);
> > +     virtnet_napi_tx_enable(&vi->sq[qp_index], need_rtnl);
> >
> >       return 0;
> >
> > @@ -3156,7 +3158,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
> >               vi->duplex = duplex;
> >  }
> >
> > -static int virtnet_open(struct net_device *dev)
> > +static int virtnet_do_open(struct net_device *dev, bool need_rtnl)
> >  {
> >       struct virtnet_info *vi = netdev_priv(dev);
> >       int i, err;
> > @@ -3169,7 +3171,7 @@ static int virtnet_open(struct net_device *dev)
> >                       if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> >                               schedule_delayed_work(&vi->refill, 0);
> >
> > -             err = virtnet_enable_queue_pair(vi, i);
> > +             err = virtnet_enable_queue_pair(vi, i, need_rtnl);
> >               if (err < 0)
> >                       goto err_enable_qp;
> >       }
> > @@ -3190,13 +3192,18 @@ static int virtnet_open(struct net_device *dev)
> >       cancel_delayed_work_sync(&vi->refill);
> >
> >       for (i--; i >= 0; i--) {
> > -             virtnet_disable_queue_pair(vi, i);
> > +             virtnet_disable_queue_pair(vi, i, need_rtnl);
> >               virtnet_cancel_dim(vi, &vi->rq[i].dim);
> >       }
> >
> >       return err;
> >  }
> >
> > +static int virtnet_open(struct net_device *dev)
> > +{
> > +     return virtnet_do_open(dev, false);
> > +}
> > +
> >  static int virtnet_poll_tx(struct napi_struct *napi, int budget)
> >  {
> >       struct send_queue *sq = container_of(napi, struct send_queue, napi);
> > @@ -3373,7 +3380,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
> >       bool running = netif_running(vi->dev);
> >
> >       if (running) {
> > -             virtnet_napi_disable(rq, true);
> > +             virtnet_napi_disable(rq, false);
> >               virtnet_cancel_dim(vi, &rq->dim);
> >       }
> >  }
> > @@ -3386,7 +3393,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
> >               schedule_delayed_work(&vi->refill, 0);
> >
> >       if (running)
> > -             virtnet_napi_enable(rq, true);
> > +             virtnet_napi_enable(rq, false);
> >  }
> >
> >  static int virtnet_rx_resize(struct virtnet_info *vi,
> > @@ -3415,7 +3422,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
> >       qindex = sq - vi->sq;
> >
> >       if (running)
> > -             virtnet_napi_tx_disable(sq, true);
> > +             virtnet_napi_tx_disable(sq, false);
> >
> >       txq = netdev_get_tx_queue(vi->dev, qindex);
> >
> > @@ -3449,7 +3456,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
> >       __netif_tx_unlock_bh(txq);
> >
> >       if (running)
> > -             virtnet_napi_tx_enable(sq, true);
> > +             virtnet_napi_tx_enable(sq, false);

Instead of this, it looks to me it would be much simpler if we can
just hold the rtnl lock in freeze/restore.

Thanks

Re: [PATCH net-next v4 3/4] virtio-net: Map NAPIs to queues

[Joe Damato]

On Thu, Feb 27, 2025 at 12:18:33PM +0800, Jason Wang wrote:
> On Thu, Feb 27, 2025 at 6:13 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Wed, Feb 26, 2025 at 03:27:42PM -0500, Joe Damato wrote:
> > > On Wed, Feb 26, 2025 at 01:08:49PM -0500, Joe Damato wrote:
> > > > On Wed, Feb 26, 2025 at 01:03:09PM -0500, Joe Damato wrote:
> > > > > On Wed, Feb 26, 2025 at 01:48:50PM +0800, Jason Wang wrote:
> > > > > > On Tue, Feb 25, 2025 at 10:05 AM Joe Damato <jdamato@fastly.com> wrote:
> > > > > > >
> > > > > > > Use netif_queue_set_napi to map NAPIs to queue IDs so that the mapping
> > > > > > > can be accessed by user apps, taking care to hold RTNL as needed.
> > > > > >
> > > > > > I may miss something but I wonder whether letting the caller hold the
> > > > > > lock is better.
> > > > >
> > > > > Hmm...
> > > > >
> > > > > Double checking all the paths over again, here's what I see:
> > > > >   - refill_work, delayed work that needs RTNL so this change seems
> > > > >     right?
> > > > >
> > > > >   - virtnet_disable_queue_pair, called from virtnet_open and
> > > > >     virtnet_close. When called via NDO these are safe and hold RTNL,
> > > > >     but they can be called from power management and need RTNL.
> > > > >
> > > > >   - virtnet_enable_queue_pair called from virtnet_open, safe when
> > > > >     used via NDO but needs RTNL when used via power management.
> > > > >
> > > > >   - virtnet_rx_pause called in both paths as you mentioned, one
> > > > >     which needs RTNL and one which doesn't.
> > > >
> > > > Sorry, I missed more paths:
> > > >
> > > >     - virtnet_rx_resume
> > > >     - virtnet_tx_pause and virtnet_tx_resume
> > > >
> > > > which are similar to path you mentioned (virtnet_rx_pause) and need
> > > > rtnl in one of two different paths.
> > > >
> > > > Let me know if I missed any paths and what your preferred way to fix
> > > > this would be?
> > > >
> > > > I think both options below are possible and I have no strong
> > > > preference.
> > >
> > > OK, my apologies. I read your message and the code wrong. Sorry for
> > > the back-to-back emails from me.
> > >
> > > Please ignore my message above... I think after re-reading the code,
> > > here's where I've arrived:
> > >
> > >   - refill_work needs to hold RTNL (as in the existing patch)
> > >
> > >   - virtnet_rx_pause, virtnet_rx_resume, virtnet_tx_pause,
> > >     virtnet_tx_resume -- all do NOT need to hold RTNL because it is
> > >     already held in the ethtool resize path and the XSK path, as you
> > >     explained, but I mis-read (sorry).
> > >
> > >   - virtnet_disable_queue_pair and virtnet_enable_queue_pair both
> > >     need to hold RTNL only when called via power management, but not
> > >     when called via ndo_open or ndo_close
> > >
> > > Is my understanding correct and does it match your understanding?
> > >
> > > If so, that means there are two issues:
> > >
> > >   1. Fixing the hardcoded bools in rx_pause, rx_resume, tx_pause,
> > >      tx_resume (all should be false, RTNL is not needed).
> > >
> > >   2. Handling the power management case which calls virtnet_open and
> > >      virtnet_close.
> > >
> > > I made a small diff included below as an example of a possible
> > > solution:
> > >
> > >   1. Modify virtnet_disable_queue_pair and virtnet_enable_queue_pair
> > >      to take a "bool need_rtnl" and pass it through to the helpers
> > >      they call.
> > >
> > >   2. Create two helpers, virtnet_do_open and virt_do_close both of
> > >      which take struct net_device *dev, bool need_rtnl. virtnet_open
> > >      and virtnet_close are modified to call the helpers and pass
> > >      false for need_rtnl. The power management paths call the
> > >      helpers and pass true for need_rtnl. (fixes issue 2 above)
> > >
> > >   3. Fix the bools for rx_pause, rx_resume, tx_pause, tx_resume to
> > >      pass false since all paths that I could find that lead to these
> > >      functions hold RTNL. (fixes issue 1 above)
> > >
> > > See the diff below (which can be applied on top of patch 3) to see
> > > what it looks like.
> > >
> > > If you are OK with this approach, I will send a v5 where patch 3
> > > includes the changes shown in this diff.
> > >
> > > Please let me know what you think:
> >
> >
> >
> > Looks ok I think.
> >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 13bb4a563073..76ecb8f3ce9a 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -3098,14 +3098,16 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
> > >       return received;
> > >  }
> > >
> > > -static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > +static void virtnet_disable_queue_pair(struct virtnet_info *vi, int qp_index,
> > > +                                    bool need_rtnl)
> > >  {
> > > -     virtnet_napi_tx_disable(&vi->sq[qp_index], false);
> > > -     virtnet_napi_disable(&vi->rq[qp_index], false);
> > > +     virtnet_napi_tx_disable(&vi->sq[qp_index], need_rtnl);
> > > +     virtnet_napi_disable(&vi->rq[qp_index], need_rtnl);
> > >       xdp_rxq_info_unreg(&vi->rq[qp_index].xdp_rxq);
> > >  }
> > >
> > > -static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > > +static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index,
> > > +                                  bool need_rtnl)
> > >  {
> > >       struct net_device *dev = vi->dev;
> > >       int err;
> > > @@ -3120,8 +3122,8 @@ static int virtnet_enable_queue_pair(struct virtnet_info *vi, int qp_index)
> > >       if (err < 0)
> > >               goto err_xdp_reg_mem_model;
> > >
> > > -     virtnet_napi_enable(&vi->rq[qp_index], false);
> > > -     virtnet_napi_tx_enable(&vi->sq[qp_index], false);
> > > +     virtnet_napi_enable(&vi->rq[qp_index], need_rtnl);
> > > +     virtnet_napi_tx_enable(&vi->sq[qp_index], need_rtnl);
> > >
> > >       return 0;
> > >
> > > @@ -3156,7 +3158,7 @@ static void virtnet_update_settings(struct virtnet_info *vi)
> > >               vi->duplex = duplex;
> > >  }
> > >
> > > -static int virtnet_open(struct net_device *dev)
> > > +static int virtnet_do_open(struct net_device *dev, bool need_rtnl)
> > >  {
> > >       struct virtnet_info *vi = netdev_priv(dev);
> > >       int i, err;
> > > @@ -3169,7 +3171,7 @@ static int virtnet_open(struct net_device *dev)
> > >                       if (!try_fill_recv(vi, &vi->rq[i], GFP_KERNEL))
> > >                               schedule_delayed_work(&vi->refill, 0);
> > >
> > > -             err = virtnet_enable_queue_pair(vi, i);
> > > +             err = virtnet_enable_queue_pair(vi, i, need_rtnl);
> > >               if (err < 0)
> > >                       goto err_enable_qp;
> > >       }
> > > @@ -3190,13 +3192,18 @@ static int virtnet_open(struct net_device *dev)
> > >       cancel_delayed_work_sync(&vi->refill);
> > >
> > >       for (i--; i >= 0; i--) {
> > > -             virtnet_disable_queue_pair(vi, i);
> > > +             virtnet_disable_queue_pair(vi, i, need_rtnl);
> > >               virtnet_cancel_dim(vi, &vi->rq[i].dim);
> > >       }
> > >
> > >       return err;
> > >  }
> > >
> > > +static int virtnet_open(struct net_device *dev)
> > > +{
> > > +     return virtnet_do_open(dev, false);
> > > +}
> > > +
> > >  static int virtnet_poll_tx(struct napi_struct *napi, int budget)
> > >  {
> > >       struct send_queue *sq = container_of(napi, struct send_queue, napi);
> > > @@ -3373,7 +3380,7 @@ static void virtnet_rx_pause(struct virtnet_info *vi, struct receive_queue *rq)
> > >       bool running = netif_running(vi->dev);
> > >
> > >       if (running) {
> > > -             virtnet_napi_disable(rq, true);
> > > +             virtnet_napi_disable(rq, false);
> > >               virtnet_cancel_dim(vi, &rq->dim);
> > >       }
> > >  }
> > > @@ -3386,7 +3393,7 @@ static void virtnet_rx_resume(struct virtnet_info *vi, struct receive_queue *rq)
> > >               schedule_delayed_work(&vi->refill, 0);
> > >
> > >       if (running)
> > > -             virtnet_napi_enable(rq, true);
> > > +             virtnet_napi_enable(rq, false);
> > >  }
> > >
> > >  static int virtnet_rx_resize(struct virtnet_info *vi,
> > > @@ -3415,7 +3422,7 @@ static void virtnet_tx_pause(struct virtnet_info *vi, struct send_queue *sq)
> > >       qindex = sq - vi->sq;
> > >
> > >       if (running)
> > > -             virtnet_napi_tx_disable(sq, true);
> > > +             virtnet_napi_tx_disable(sq, false);
> > >
> > >       txq = netdev_get_tx_queue(vi->dev, qindex);
> > >
> > > @@ -3449,7 +3456,7 @@ static void virtnet_tx_resume(struct virtnet_info *vi, struct send_queue *sq)
> > >       __netif_tx_unlock_bh(txq);
> > >
> > >       if (running)
> > > -             virtnet_napi_tx_enable(sq, true);
> > > +             virtnet_napi_tx_enable(sq, false);
> 
> Instead of this, it looks to me it would be much simpler if we can
> just hold the rtnl lock in freeze/restore.

I disagree.

Holding RTNL for all of open and close instead of just the 1 API
call that needs it has the possibility of introducing other lock
ordering bugs now or in the future.

We only need RTNL for 1 API, why hold it for all of open or close?