[PATCH net-next v4 0/5] virtio_net: enable the irq for ctrlq

[PATCH net-next v4 0/5] virtio_net: enable the irq for ctrlq

[Heng Qi]

Ctrlq in polling mode may cause the virtual machine to hang and
occupy additional CPU resources. Enabling the irq for ctrlq
alleviates this problem and allows commands to be requested
concurrently.

Changelog
=========
v3->v4:
  - Turn off the switch before flush the get_cvq work.
  - Add interrupt suppression.

v2->v3:
  - Use the completion for dim cmds.

v1->v2:
  - Refactor the patch 1 and rephase the commit log.

Heng Qi (5):
  virtio_net: passing control_buf explicitly
  virtio_net: enable irq for the control vq
  virtio_net: change the command token to completion
  virtio_net: refactor command sending and response handling
  virtio_net: improve dim command request efficiency

 drivers/net/virtio_net.c | 309 ++++++++++++++++++++++++++++++++-------
 1 file changed, 260 insertions(+), 49 deletions(-)

-- 
2.32.0.3.g01195cf9f

[PATCH net-next v4 1/5] virtio_net: passing control_buf explicitly

[Heng Qi]

In a later patch, the driver may send requests concurrently, in
which case each command will have its own control buffer, so we
refactor virtnet_send_command_reply() to pass the control buffer
explicitly as a parameter.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 61a57d134544..b45f58a902e3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2680,7 +2680,9 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
  * supported by the hypervisor, as indicated by feature bits, should
  * never fail unless improperly formatted.
  */
-static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd,
+static bool virtnet_send_command_reply(struct virtnet_info *vi,
+				       u8 class, u8 cmd,
+				       struct control_buf *ctrl,
 				       struct scatterlist *out,
 				       struct scatterlist *in)
 {
@@ -2693,18 +2695,18 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 	BUG_ON(!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_VQ));
 
 	mutex_lock(&vi->cvq_lock);
-	vi->ctrl->status = ~0;
-	vi->ctrl->hdr.class = class;
-	vi->ctrl->hdr.cmd = cmd;
+	ctrl->status = ~0;
+	ctrl->hdr.class = class;
+	ctrl->hdr.cmd = cmd;
 	/* Add header */
-	sg_init_one(&hdr, &vi->ctrl->hdr, sizeof(vi->ctrl->hdr));
+	sg_init_one(&hdr, &ctrl->hdr, sizeof(ctrl->hdr));
 	sgs[out_num++] = &hdr;
 
 	if (out)
 		sgs[out_num++] = out;
 
 	/* Add return status. */
-	sg_init_one(&stat, &vi->ctrl->status, sizeof(vi->ctrl->status));
+	sg_init_one(&stat, &ctrl->status, sizeof(ctrl->status));
 	sgs[out_num + in_num++] = &stat;
 
 	if (in)
@@ -2732,7 +2734,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 	}
 
 unlock:
-	ok = vi->ctrl->status == VIRTIO_NET_OK;
+	ok = ctrl->status == VIRTIO_NET_OK;
 	mutex_unlock(&vi->cvq_lock);
 	return ok;
 }
@@ -2740,7 +2742,7 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi, u8 class, u8 cmd
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
-	return virtnet_send_command_reply(vi, class, cmd, out, NULL);
+	return virtnet_send_command_reply(vi, class, cmd, vi->ctrl, out, NULL);
 }
 
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
@@ -4020,7 +4022,7 @@ static int __virtnet_get_hw_stats(struct virtnet_info *vi,
 
 	ok = virtnet_send_command_reply(vi, VIRTIO_NET_CTRL_STATS,
 					VIRTIO_NET_CTRL_STATS_GET,
-					&sgs_out, &sgs_in);
+					vi->ctrl, &sgs_out, &sgs_in);
 
 	if (!ok)
 		return ok;
@@ -5880,7 +5882,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 
 		if (!virtnet_send_command_reply(vi, VIRTIO_NET_CTRL_STATS,
 						VIRTIO_NET_CTRL_STATS_QUERY,
-						NULL, &sg)) {
+						vi->ctrl, NULL, &sg)) {
 			pr_debug("virtio_net: fail to get stats capability\n");
 			rtnl_unlock();
 			err = -EINVAL;
-- 
2.32.0.3.g01195cf9f

[PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

If the device does not respond to a request for a long time,
then control vq polling elevates CPU utilization, a problem that
exacerbates with more command requests.

Enabling control vq's irq is advantageous for the guest, and
this still doesn't support concurrent requests.

Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b45f58a902e3..ed10084997d3 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -372,6 +372,8 @@ struct virtio_net_ctrl_rss {
 struct control_buf {
 	struct virtio_net_ctrl_hdr hdr;
 	virtio_net_ctrl_ack status;
+	/* Wait for the device to complete the cvq request. */
+	struct completion completion;
 };
 
 struct virtnet_info {
@@ -664,6 +666,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
 	return false;
 }
 
+static void virtnet_cvq_done(struct virtqueue *cvq)
+{
+	struct virtnet_info *vi = cvq->vdev->priv;
+
+	complete(&vi->ctrl->completion);
+}
+
 static void skb_xmit_done(struct virtqueue *vq)
 {
 	struct virtnet_info *vi = vq->vdev->priv;
@@ -2724,14 +2733,8 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi,
 	if (unlikely(!virtqueue_kick(vi->cvq)))
 		goto unlock;
 
-	/* Spin for a response, the kick causes an ioport write, trapping
-	 * into the hypervisor, so the request should be handled immediately.
-	 */
-	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
-	       !virtqueue_is_broken(vi->cvq)) {
-		cond_resched();
-		cpu_relax();
-	}
+	wait_for_completion(&ctrl->completion);
+	virtqueue_get_buf(vi->cvq, &tmp);
 
 unlock:
 	ok = ctrl->status == VIRTIO_NET_OK;
@@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
 
 	/* Parameters for control virtqueue, if any */
 	if (vi->has_cvq) {
-		callbacks[total_vqs - 1] = NULL;
+		callbacks[total_vqs - 1] = virtnet_cvq_done;
 		names[total_vqs - 1] = "control";
 	}
 
@@ -5832,6 +5835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (vi->has_rss || vi->has_rss_hash_report)
 		virtnet_init_default_rss(vi);
 
+	init_completion(&vi->ctrl->completion);
 	enable_rx_mode_work(vi);
 
 	/* serialize netdev register + virtio_device_ready() with ndo_open() */
-- 
2.32.0.3.g01195cf9f

[PATCH net-next v4 3/5] virtio_net: change the command token to completion

[Heng Qi]

Previously, control vq only allowed a single request to be sent,
so using virtnet_info as a global token was fine. To support
concurrent requests, the driver needs to use a command-level token
to distinguish between requests that have been sent.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ed10084997d3..4256b1eda043 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2722,7 +2722,8 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi,
 		sgs[out_num + in_num++] = in;
 
 	BUG_ON(out_num + in_num > ARRAY_SIZE(sgs));
-	ret = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num, vi, GFP_ATOMIC);
+	ret = virtqueue_add_sgs(vi->cvq, sgs, out_num, in_num,
+				&ctrl->completion, GFP_ATOMIC);
 	if (ret < 0) {
 		dev_warn(&vi->vdev->dev,
 			 "Failed to add sgs for command vq: %d\n.", ret);
-- 
2.32.0.3.g01195cf9f

[PATCH net-next v4 4/5] virtio_net: refactor command sending and response handling

[Heng Qi]

Refactor the command handling logic by splitting
virtnet_send_command_reply into virtnet_add_command_reply
and virtnet_wait_command_response for better clarity and
subsequent use.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 53 ++++++++++++++++++++++++++++------------
 1 file changed, 38 insertions(+), 15 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4256b1eda043..807724772bcf 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -2684,20 +2684,14 @@ static int virtnet_tx_resize(struct virtnet_info *vi,
 	return err;
 }
 
-/*
- * Send command via the control virtqueue and check status.  Commands
- * supported by the hypervisor, as indicated by feature bits, should
- * never fail unless improperly formatted.
- */
-static bool virtnet_send_command_reply(struct virtnet_info *vi,
-				       u8 class, u8 cmd,
-				       struct control_buf *ctrl,
-				       struct scatterlist *out,
-				       struct scatterlist *in)
+static bool virtnet_add_command_reply(struct virtnet_info *vi,
+				      u8 class, u8 cmd,
+				      struct control_buf *ctrl,
+				      struct scatterlist *out,
+				      struct scatterlist *in)
 {
 	struct scatterlist *sgs[5], hdr, stat;
-	u32 out_num = 0, tmp, in_num = 0;
-	bool ok;
+	u32 out_num = 0, in_num = 0;
 	int ret;
 
 	/* Caller should know better */
@@ -2731,18 +2725,47 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi,
 		return false;
 	}
 
-	if (unlikely(!virtqueue_kick(vi->cvq)))
-		goto unlock;
+	if (unlikely(!virtqueue_kick(vi->cvq))) {
+		mutex_unlock(&vi->cvq_lock);
+		return false;
+	}
+
+	return true;
+}
+
+static bool virtnet_wait_command_response(struct virtnet_info *vi,
+					  struct control_buf *ctrl)
+{
+	unsigned int tmp;
+	bool ok;
 
 	wait_for_completion(&ctrl->completion);
 	virtqueue_get_buf(vi->cvq, &tmp);
 
-unlock:
 	ok = ctrl->status == VIRTIO_NET_OK;
 	mutex_unlock(&vi->cvq_lock);
 	return ok;
 }
 
+/* Send command via the control virtqueue and check status. Commands
+ * supported by the hypervisor, as indicated by feature bits, should
+ * never fail unless improperly formatted.
+ */
+static bool virtnet_send_command_reply(struct virtnet_info *vi,
+				       u8 class, u8 cmd,
+				       struct control_buf *ctrl,
+				       struct scatterlist *out,
+				       struct scatterlist *in)
+{
+	bool ret;
+
+	ret = virtnet_add_command_reply(vi, class, cmd, ctrl, out, in);
+	if (!ret)
+		return ret;
+
+	return virtnet_wait_command_response(vi, ctrl);
+}
+
 static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 				 struct scatterlist *out)
 {
-- 
2.32.0.3.g01195cf9f

[PATCH net-next v4 5/5] virtio_net: improve dim command request efficiency

[Heng Qi]

Currently, control vq handles commands synchronously,
leading to increased delays for dim commands during multi-queue
VM configuration and directly impacting dim performance.

To address this, we are shifting to asynchronous processing of
ctrlq's dim commands.

Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
---
 drivers/net/virtio_net.c | 221 +++++++++++++++++++++++++++++++++++----
 1 file changed, 201 insertions(+), 20 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 807724772bcf..bd224e282f9b 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -376,6 +376,12 @@ struct control_buf {
 	struct completion completion;
 };
 
+struct virtnet_coal_node {
+	struct control_buf ctrl;
+	struct virtio_net_ctrl_coal_vq coal_vqs;
+	struct list_head list;
+};
+
 struct virtnet_info {
 	struct virtio_device *vdev;
 	struct virtqueue *cvq;
@@ -420,6 +426,12 @@ struct virtnet_info {
 	/* Lock to protect the control VQ */
 	struct mutex cvq_lock;
 
+	/* Work struct for acquisition of cvq processing results. */
+	struct work_struct get_cvq;
+
+	/* OK to queue work getting cvq response? */
+	bool get_cvq_work_enabled;
+
 	/* Host can handle any s/g split between our header and packet data */
 	bool any_header_sg;
 
@@ -464,6 +476,10 @@ struct virtnet_info {
 	struct virtnet_interrupt_coalesce intr_coal_tx;
 	struct virtnet_interrupt_coalesce intr_coal_rx;
 
+	/* Free nodes used for concurrent delivery */
+	struct mutex coal_free_lock;
+	struct list_head coal_free_list;
+
 	unsigned long guest_offloads;
 	unsigned long guest_offloads_capable;
 
@@ -666,11 +682,26 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
 	return false;
 }
 
+static void enable_get_cvq_work(struct virtnet_info *vi)
+{
+	rtnl_lock();
+	vi->get_cvq_work_enabled = true;
+	rtnl_unlock();
+}
+
+static void disable_get_cvq_work(struct virtnet_info *vi)
+{
+	rtnl_lock();
+	vi->get_cvq_work_enabled = false;
+	rtnl_unlock();
+}
+
 static void virtnet_cvq_done(struct virtqueue *cvq)
 {
 	struct virtnet_info *vi = cvq->vdev->priv;
 
-	complete(&vi->ctrl->completion);
+	virtqueue_disable_cb(cvq);
+	schedule_work(&vi->get_cvq);
 }
 
 static void skb_xmit_done(struct virtqueue *vq)
@@ -2730,6 +2761,7 @@ static bool virtnet_add_command_reply(struct virtnet_info *vi,
 		return false;
 	}
 
+	mutex_unlock(&vi->cvq_lock);
 	return true;
 }
 
@@ -2740,10 +2772,8 @@ static bool virtnet_wait_command_response(struct virtnet_info *vi,
 	bool ok;
 
 	wait_for_completion(&ctrl->completion);
-	virtqueue_get_buf(vi->cvq, &tmp);
 
 	ok = ctrl->status == VIRTIO_NET_OK;
-	mutex_unlock(&vi->cvq_lock);
 	return ok;
 }
 
@@ -2772,6 +2802,89 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd,
 	return virtnet_send_command_reply(vi, class, cmd, vi->ctrl, out, NULL);
 }
 
+static bool virtnet_process_dim_cmd(struct virtnet_info *vi,
+				    struct virtnet_coal_node *node)
+{
+	u16 qnum = le16_to_cpu(node->coal_vqs.vqn) / 2;
+	bool ret;
+
+	ret = virtnet_wait_command_response(vi, &node->ctrl);
+	if (ret) {
+		mutex_lock(&vi->rq[qnum].dim_lock);
+		vi->rq[qnum].intr_coal.max_usecs =
+			le32_to_cpu(node->coal_vqs.coal.max_usecs);
+		vi->rq[qnum].intr_coal.max_packets =
+			le32_to_cpu(node->coal_vqs.coal.max_packets);
+		mutex_unlock(&vi->rq[qnum].dim_lock);
+	}
+
+	vi->rq[qnum].dim.state = DIM_START_MEASURE;
+
+	mutex_lock(&vi->coal_free_lock);
+	list_add(&node->list, &vi->coal_free_list);
+	mutex_unlock(&vi->coal_free_lock);
+	return ret;
+}
+
+static bool virtnet_add_dim_command(struct virtnet_info *vi,
+				    struct receive_queue *rq,
+				    struct dim_cq_moder update_moder,
+				    struct virtnet_coal_node **avail_coal)
+{
+	struct virtnet_coal_node *node;
+	struct scatterlist sg;
+	bool ret;
+
+	mutex_lock(&vi->coal_free_lock);
+	if (list_empty(&vi->coal_free_list)) {
+		mutex_unlock(&vi->coal_free_lock);
+		return false;
+	}
+
+	node = list_first_entry(&vi->coal_free_list,
+				struct virtnet_coal_node, list);
+	node->coal_vqs.vqn = cpu_to_le16(rxq2vq(rq - vi->rq));
+	node->coal_vqs.coal.max_usecs = cpu_to_le32(update_moder.usec);
+	node->coal_vqs.coal.max_packets = cpu_to_le32(update_moder.pkts);
+
+	sg_init_one(&sg, &node->coal_vqs, sizeof(node->coal_vqs));
+	ret = virtnet_add_command_reply(vi, VIRTIO_NET_CTRL_NOTF_COAL,
+					VIRTIO_NET_CTRL_NOTF_COAL_VQ_SET,
+					&node->ctrl, &sg, NULL);
+	if (!ret) {
+		dev_warn(&vi->dev->dev,
+			 "Failed to change coalescing params.\n");
+		mutex_unlock(&vi->coal_free_lock);
+		return ret;
+	}
+
+	*avail_coal = node;
+	list_del(&node->list);
+	mutex_unlock(&vi->coal_free_lock);
+
+	return true;
+}
+
+static void virtnet_get_cvq_work(struct work_struct *work)
+{
+	struct virtnet_info *vi =
+		container_of(work, struct virtnet_info, get_cvq);
+	unsigned int tmp;
+	int opaque;
+	void *res;
+
+again:
+	mutex_lock(&vi->cvq_lock);
+	while ((res = virtqueue_get_buf(vi->cvq, &tmp)) != NULL)
+		complete((struct completion *)res);
+	mutex_unlock(&vi->cvq_lock);
+
+	opaque = virtqueue_enable_cb_prepare(vi->cvq);
+	if (unlikely(virtqueue_poll(vi->cvq, opaque))) {
+		virtqueue_disable_cb(vi->cvq);
+		goto again;
+	}
+}
 static int virtnet_set_mac_address(struct net_device *dev, void *p)
 {
 	struct virtnet_info *vi = netdev_priv(dev);
@@ -4419,35 +4532,54 @@ static int virtnet_send_notf_coal_vq_cmds(struct virtnet_info *vi,
 	return 0;
 }
 
+static void virtnet_wait_space(struct virtnet_info *vi)
+{
+	bool no_coal_free = true;
+
+	while (READ_ONCE(vi->cvq->num_free) < 3)
+		usleep_range(1000, 2000);
+
+	while (no_coal_free) {
+		mutex_lock(&vi->coal_free_lock);
+		if (!list_empty(&vi->coal_free_list))
+			no_coal_free = false;
+		mutex_unlock(&vi->coal_free_lock);
+		if (no_coal_free)
+			usleep_range(1000, 2000);
+	}
+}
+
 static void virtnet_rx_dim_work(struct work_struct *work)
 {
 	struct dim *dim = container_of(work, struct dim, work);
 	struct receive_queue *rq = container_of(dim,
 			struct receive_queue, dim);
 	struct virtnet_info *vi = rq->vq->vdev->priv;
-	struct net_device *dev = vi->dev;
+	struct virtnet_coal_node *avail_coal;
 	struct dim_cq_moder update_moder;
-	int qnum, err;
 
-	qnum = rq - vi->rq;
+	update_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
 
 	mutex_lock(&rq->dim_lock);
-	if (!rq->dim_enabled)
-		goto out;
-
-	update_moder = net_dim_get_rx_moderation(dim->mode, dim->profile_ix);
-	if (update_moder.usec != rq->intr_coal.max_usecs ||
-	    update_moder.pkts != rq->intr_coal.max_packets) {
-		err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, qnum,
-						       update_moder.usec,
-						       update_moder.pkts);
-		if (err)
-			pr_debug("%s: Failed to send dim parameters on rxq%d\n",
-				 dev->name, qnum);
+	if (!rq->dim_enabled ||
+	    (update_moder.usec == rq->intr_coal.max_usecs &&
+	     update_moder.pkts == rq->intr_coal.max_packets)) {
+		rq->dim.state = DIM_START_MEASURE;
+		mutex_unlock(&rq->dim_lock);
+		return;
 	}
-out:
-	dim->state = DIM_START_MEASURE;
 	mutex_unlock(&rq->dim_lock);
+
+again:
+	virtnet_wait_space(vi);
+
+	if (!virtnet_add_dim_command(vi, rq, update_moder, &avail_coal)) {
+		if (virtqueue_is_broken(vi->cvq))
+			return;
+		goto again;
+	}
+
+	virtnet_process_dim_cmd(vi, avail_coal);
 }
 
 static int virtnet_coal_params_supported(struct ethtool_coalesce *ec)
@@ -4860,6 +4992,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
 	flush_work(&vi->config_work);
 	disable_rx_mode_work(vi);
 	flush_work(&vi->rx_mode_work);
+	disable_get_cvq_work(vi);
+	flush_work(&vi->get_cvq);
 
 	netif_tx_lock_bh(vi->dev);
 	netif_device_detach(vi->dev);
@@ -4883,6 +5017,7 @@ static int virtnet_restore_up(struct virtio_device *vdev)
 
 	enable_delayed_refill(vi);
 	enable_rx_mode_work(vi);
+	enable_get_cvq_work(vi);
 
 	if (netif_running(vi->dev)) {
 		err = virtnet_open(vi->dev);
@@ -5633,6 +5768,43 @@ static const struct xdp_metadata_ops virtnet_xdp_metadata_ops = {
 	.xmo_rx_hash			= virtnet_xdp_rx_hash,
 };
 
+static void virtnet_del_coal_free_list(struct virtnet_info *vi)
+{
+	struct virtnet_coal_node *coal_node, *tmp;
+
+	list_for_each_entry_safe(coal_node, tmp,  &vi->coal_free_list, list) {
+		list_del(&coal_node->list);
+		kfree(coal_node);
+	}
+}
+
+static int virtnet_init_coal_list(struct virtnet_info *vi)
+{
+	struct virtnet_coal_node *coal_node;
+	int batch_dim_nums;
+	int i;
+
+	INIT_LIST_HEAD(&vi->coal_free_list);
+	mutex_init(&vi->coal_free_lock);
+
+	if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_VQ_NOTF_COAL))
+		return 0;
+
+	batch_dim_nums = min((unsigned int)vi->max_queue_pairs,
+			     virtqueue_get_vring_size(vi->cvq) / 3);
+	for (i = 0; i < batch_dim_nums; i++) {
+		coal_node = kzalloc(sizeof(*coal_node), GFP_KERNEL);
+		if (!coal_node) {
+			virtnet_del_coal_free_list(vi);
+			return -ENOMEM;
+		}
+		init_completion(&coal_node->ctrl.completion);
+		list_add(&coal_node->list, &vi->coal_free_list);
+	}
+
+	return 0;
+}
+
 static int virtnet_probe(struct virtio_device *vdev)
 {
 	int i, err = -ENOMEM;
@@ -5818,6 +5990,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (err)
 		goto free;
 
+	if (virtnet_init_coal_list(vi))
+		goto free;
+
 	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
 		vi->intr_coal_rx.max_usecs = 0;
 		vi->intr_coal_tx.max_usecs = 0;
@@ -5859,7 +6034,9 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (vi->has_rss || vi->has_rss_hash_report)
 		virtnet_init_default_rss(vi);
 
+	INIT_WORK(&vi->get_cvq, virtnet_get_cvq_work);
 	init_completion(&vi->ctrl->completion);
+	enable_get_cvq_work(vi);
 	enable_rx_mode_work(vi);
 
 	/* serialize netdev register + virtio_device_ready() with ndo_open() */
@@ -5988,11 +6165,15 @@ static void virtnet_remove(struct virtio_device *vdev)
 	flush_work(&vi->config_work);
 	disable_rx_mode_work(vi);
 	flush_work(&vi->rx_mode_work);
+	disable_get_cvq_work(vi);
+	flush_work(&vi->get_cvq);
 
 	unregister_netdev(vi->dev);
 
 	net_failover_destroy(vi->failover);
 
+	virtnet_del_coal_free_list(vi);
+
 	remove_vq_common(vi);
 
 	free_netdev(vi->dev);
-- 
2.32.0.3.g01195cf9f

Re: [PATCH net-next v4 0/5] virtio_net: enable the irq for ctrlq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 12:19:03AM +0800, Heng Qi wrote:
> Ctrlq in polling mode may cause the virtual machine to hang and
> occupy additional CPU resources. Enabling the irq for ctrlq
> alleviates this problem and allows commands to be requested
> concurrently.

Any patch that is supposed to be a performance improvement
has to come with actual before/after testing restults, not
vague "may cause".



> Changelog
> =========
> v3->v4:
>   - Turn off the switch before flush the get_cvq work.
>   - Add interrupt suppression.
> 
> v2->v3:
>   - Use the completion for dim cmds.
> 
> v1->v2:
>   - Refactor the patch 1 and rephase the commit log.
> 
> Heng Qi (5):
>   virtio_net: passing control_buf explicitly
>   virtio_net: enable irq for the control vq
>   virtio_net: change the command token to completion
>   virtio_net: refactor command sending and response handling
>   virtio_net: improve dim command request efficiency
> 
>  drivers/net/virtio_net.c | 309 ++++++++++++++++++++++++++++++++-------
>  1 file changed, 260 insertions(+), 49 deletions(-)
> 
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  
>  	/* Parameters for control virtqueue, if any */
>  	if (vi->has_cvq) {
> -		callbacks[total_vqs - 1] = NULL;
> +		callbacks[total_vqs - 1] = virtnet_cvq_done;
>  		names[total_vqs - 1] = "control";
>  	}
>  

If the # of MSIX vectors is exactly for data path VQs,
this will cause irq sharing between VQs which will degrade
performance significantly.

So no, you can not just do it unconditionally.

The correct fix probably requires virtio core/API extensions.

-- 
MST

Re: [PATCH net-next v4 5/5] virtio_net: improve dim command request efficiency

[kernel test robot]

Hi Heng,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Heng-Qi/virtio_net-passing-control_buf-explicitly/20240620-002212
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240619161908.82348-6-hengqi%40linux.alibaba.com
patch subject: [PATCH net-next v4 5/5] virtio_net: improve dim command request efficiency
config: x86_64-rhel-8.3 (https://download.01.org/0day-ci/archive/20240620/202406201437.JuUEpbZL-lkp@intel.com/config)
compiler: gcc-13 (Ubuntu 13.2.0-4ubuntu3) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240620/202406201437.JuUEpbZL-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202406201437.JuUEpbZL-lkp@intel.com/

All warnings (new ones prefixed by >>):

   drivers/net/virtio_net.c: In function 'virtnet_wait_command_response':
>> drivers/net/virtio_net.c:2771:22: warning: unused variable 'tmp' [-Wunused-variable]
    2771 |         unsigned int tmp;
         |                      ^~~


vim +/tmp +2771 drivers/net/virtio_net.c

2bef89a476bb44 Heng Qi   2024-06-20  2767  
2bef89a476bb44 Heng Qi   2024-06-20  2768  static bool virtnet_wait_command_response(struct virtnet_info *vi,
2bef89a476bb44 Heng Qi   2024-06-20  2769  					  struct control_buf *ctrl)
2bef89a476bb44 Heng Qi   2024-06-20  2770  {
2bef89a476bb44 Heng Qi   2024-06-20 @2771  	unsigned int tmp;
2bef89a476bb44 Heng Qi   2024-06-20  2772  	bool ok;
40cbfc37075a2a Amos Kong 2013-01-21  2773  
ff0de8d3002c75 Heng Qi   2024-06-20  2774  	wait_for_completion(&ctrl->completion);
40cbfc37075a2a Amos Kong 2013-01-21  2775  
74b2bc860d5cb5 Heng Qi   2024-06-20  2776  	ok = ctrl->status == VIRTIO_NET_OK;
30636258a7c917 Heng Qi   2024-05-30  2777  	return ok;
40cbfc37075a2a Amos Kong 2013-01-21  2778  }
40cbfc37075a2a Amos Kong 2013-01-21  2779  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v4 0/5] virtio_net: enable the irq for ctrlq

[Heng Qi]

On Wed, 19 Jun 2024 17:16:57 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 12:19:03AM +0800, Heng Qi wrote:
> > Ctrlq in polling mode may cause the virtual machine to hang and
> > occupy additional CPU resources. Enabling the irq for ctrlq
> > alleviates this problem and allows commands to be requested
> > concurrently.
> 
> Any patch that is supposed to be a performance improvement
> has to come with actual before/after testing restults, not
> vague "may cause".

1. If the device does not respond in time, the CPU usage for ctrlq in the polling mode is
   ~100%, and in irq mode is ~0%;
2. If there are concurrent requests, the situation in 1 will be even worse;
3. On 64 queues with dim on, use nginx + wrk with the number of connections is 500:
   a. If ctrlq is in polling mode and concurrent requests are not supported:
      seeing that the dim worker occupies 20%+ CPU usage. Because of the
      large number of queues, dim requests cannot be concurrent, and the
      performance is unstable;
   b. If ctrlq is in irq mode and concurrent requests are supported: the overhead
      of the dim worker is not visible, and the pps increases by ~13%
      compared to a.

Thanks.

> 
> 
> 
> > Changelog
> > =========
> > v3->v4:
> >   - Turn off the switch before flush the get_cvq work.
> >   - Add interrupt suppression.
> > 
> > v2->v3:
> >   - Use the completion for dim cmds.
> > 
> > v1->v2:
> >   - Refactor the patch 1 and rephase the commit log.
> > 
> > Heng Qi (5):
> >   virtio_net: passing control_buf explicitly
> >   virtio_net: enable irq for the control vq
> >   virtio_net: change the command token to completion
> >   virtio_net: refactor command sending and response handling
> >   virtio_net: improve dim command request efficiency
> > 
> >  drivers/net/virtio_net.c | 309 ++++++++++++++++++++++++++++++++-------
> >  1 file changed, 260 insertions(+), 49 deletions(-)
> > 
> > -- 
> > 2.32.0.3.g01195cf9f
> 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >  
> >  	/* Parameters for control virtqueue, if any */
> >  	if (vi->has_cvq) {
> > -		callbacks[total_vqs - 1] = NULL;
> > +		callbacks[total_vqs - 1] = virtnet_cvq_done;
> >  		names[total_vqs - 1] = "control";
> >  	}
> >  
> 
> If the # of MSIX vectors is exactly for data path VQs,
> this will cause irq sharing between VQs which will degrade
> performance significantly.
> 
> So no, you can not just do it unconditionally.
> 
> The correct fix probably requires virtio core/API extensions.

If the introduction of cvq irq causes interrupts to become shared, then
ctrlq need to fall back to polling mode and keep the status quo.

Thanks.

> 
> -- 
> MST
> 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jason Wang]

On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>
> On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > >
> > >     /* Parameters for control virtqueue, if any */
> > >     if (vi->has_cvq) {
> > > -           callbacks[total_vqs - 1] = NULL;
> > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > >             names[total_vqs - 1] = "control";
> > >     }
> > >
> >
> > If the # of MSIX vectors is exactly for data path VQs,
> > this will cause irq sharing between VQs which will degrade
> > performance significantly.
> >

Why do we need to care about buggy management? I think libvirt has
been teached to use 2N+2 since the introduction of the multiqueue[1].

> > So no, you can not just do it unconditionally.
> >
> > The correct fix probably requires virtio core/API extensions.
>
> If the introduction of cvq irq causes interrupts to become shared, then
> ctrlq need to fall back to polling mode and keep the status quo.

Having to path sounds a burden.

>
> Thanks.
>


Thanks

[1] https://www.linux-kvm.org/page/Multiqueue

> >
> > --
> > MST
> >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jason Wang]

On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>
> On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >
> > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > >
> > > >     /* Parameters for control virtqueue, if any */
> > > >     if (vi->has_cvq) {
> > > > -           callbacks[total_vqs - 1] = NULL;
> > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > >             names[total_vqs - 1] = "control";
> > > >     }
> > > >
> > >
> > > If the # of MSIX vectors is exactly for data path VQs,
> > > this will cause irq sharing between VQs which will degrade
> > > performance significantly.
> > >
>
> Why do we need to care about buggy management? I think libvirt has
> been teached to use 2N+2 since the introduction of the multiqueue[1].

And Qemu can calculate it correctly automatically since:

commit 51a81a2118df0c70988f00d61647da9e298483a4
Author: Jason Wang <jasowang@redhat.com>
Date:   Mon Mar 8 12:49:19 2021 +0800

    virtio-net: calculating proper msix vectors on init

    Currently, the default msix vectors for virtio-net-pci is 3 which is
    obvious not suitable for multiqueue guest, so we depends on the user
    or management tools to pass a correct vectors parameter. In fact, we
    can simplifying this by calculating the number of vectors on realize.

    Consider we have N queues, the number of vectors needed is 2*N + 2
    (#queue pairs + plus one config interrupt and control vq). We didn't
    check whether or not host support control vq because it was added
    unconditionally by qemu to avoid breaking legacy guests such as Minix.

    Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
    Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
    Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
    Signed-off-by: Jason Wang <jasowang@redhat.com>

Thanks

>
> > > So no, you can not just do it unconditionally.
> > >
> > > The correct fix probably requires virtio core/API extensions.
> >
> > If the introduction of cvq irq causes interrupts to become shared, then
> > ctrlq need to fall back to polling mode and keep the status quo.
>
> Having to path sounds a burden.
>
> >
> > Thanks.
> >
>
>
> Thanks
>
> [1] https://www.linux-kvm.org/page/Multiqueue
>
> > >
> > > --
> > > MST
> > >
> >

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 03:29:15PM +0800, Heng Qi wrote:
> On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > >  
> > >  	/* Parameters for control virtqueue, if any */
> > >  	if (vi->has_cvq) {
> > > -		callbacks[total_vqs - 1] = NULL;
> > > +		callbacks[total_vqs - 1] = virtnet_cvq_done;
> > >  		names[total_vqs - 1] = "control";
> > >  	}
> > >  
> > 
> > If the # of MSIX vectors is exactly for data path VQs,
> > this will cause irq sharing between VQs which will degrade
> > performance significantly.
> > 
> > So no, you can not just do it unconditionally.
> > 
> > The correct fix probably requires virtio core/API extensions.
> 
> If the introduction of cvq irq causes interrupts to become shared, then
> ctrlq need to fall back to polling mode and keep the status quo.
> 
> Thanks.

I don't see that in the code.

I guess we'll need more info in find vqs about what can and what can't share irqs?
Sharing between ctrl vq and config irq can also be an option.




> > 
> > -- 
> > MST
> > 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jason Wang]

On Thu, Jun 20, 2024 at 4:32 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 20, 2024 at 03:29:15PM +0800, Heng Qi wrote:
> > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > >
> > > >   /* Parameters for control virtqueue, if any */
> > > >   if (vi->has_cvq) {
> > > > -         callbacks[total_vqs - 1] = NULL;
> > > > +         callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > >           names[total_vqs - 1] = "control";
> > > >   }
> > > >
> > >
> > > If the # of MSIX vectors is exactly for data path VQs,
> > > this will cause irq sharing between VQs which will degrade
> > > performance significantly.
> > >
> > > So no, you can not just do it unconditionally.
> > >
> > > The correct fix probably requires virtio core/API extensions.
> >
> > If the introduction of cvq irq causes interrupts to become shared, then
> > ctrlq need to fall back to polling mode and keep the status quo.
> >
> > Thanks.
>
> I don't see that in the code.
>
> I guess we'll need more info in find vqs about what can and what can't share irqs?
> Sharing between ctrl vq and config irq can also be an option.

One way is to allow the driver to specify the group of virtqueues. So
the core can request irq per group instead of pre vq. I used to post a
series like this about 10 years ago (but fail to find it).

It might also help for the case for NAPI.

Thanks

>
>
>
>
> > >
> > > --
> > > MST
> > >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

On Thu, 20 Jun 2024 04:32:15 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 03:29:15PM +0800, Heng Qi wrote:
> > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > >  
> > > >  	/* Parameters for control virtqueue, if any */
> > > >  	if (vi->has_cvq) {
> > > > -		callbacks[total_vqs - 1] = NULL;
> > > > +		callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > >  		names[total_vqs - 1] = "control";
> > > >  	}
> > > >  
> > > 
> > > If the # of MSIX vectors is exactly for data path VQs,
> > > this will cause irq sharing between VQs which will degrade
> > > performance significantly.
> > > 
> > > So no, you can not just do it unconditionally.
> > > 
> > > The correct fix probably requires virtio core/API extensions.
> > 
> > If the introduction of cvq irq causes interrupts to become shared, then
> > ctrlq need to fall back to polling mode and keep the status quo.
> > 
> > Thanks.
> 
> I don't see that in the code.
> 
> I guess we'll need more info in find vqs about what can and what can't share irqs?

I mean we should add fallback code, for example if allocating interrupt for ctrlq
fails, we should clear the callback of ctrlq.

> Sharing between ctrl vq and config irq can also be an option.
> 

Not sure if this violates the spec. In the spec, used buffer notification and
configuration change notification are clearly defined - ctrlq is a virtqueue
and used buffer notification should be used.

Thanks.

> 
> 
> 
> > > 
> > > -- 
> > > MST
> > > 
> 
> 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > >
> > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > >
> > > > >     /* Parameters for control virtqueue, if any */
> > > > >     if (vi->has_cvq) {
> > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > >             names[total_vqs - 1] = "control";
> > > > >     }
> > > > >
> > > >
> > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > this will cause irq sharing between VQs which will degrade
> > > > performance significantly.
> > > >
> >
> > Why do we need to care about buggy management? I think libvirt has
> > been teached to use 2N+2 since the introduction of the multiqueue[1].
> 
> And Qemu can calculate it correctly automatically since:
> 
> commit 51a81a2118df0c70988f00d61647da9e298483a4
> Author: Jason Wang <jasowang@redhat.com>
> Date:   Mon Mar 8 12:49:19 2021 +0800
> 
>     virtio-net: calculating proper msix vectors on init
> 
>     Currently, the default msix vectors for virtio-net-pci is 3 which is
>     obvious not suitable for multiqueue guest, so we depends on the user
>     or management tools to pass a correct vectors parameter. In fact, we
>     can simplifying this by calculating the number of vectors on realize.
> 
>     Consider we have N queues, the number of vectors needed is 2*N + 2
>     (#queue pairs + plus one config interrupt and control vq). We didn't
>     check whether or not host support control vq because it was added
>     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> 
>     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>     Signed-off-by: Jason Wang <jasowang@redhat.com>

Yes, devices designed according to the spec need to reserve an interrupt
vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?

Thanks.

> 
> Thanks
> 
> >
> > > > So no, you can not just do it unconditionally.
> > > >
> > > > The correct fix probably requires virtio core/API extensions.
> > >
> > > If the introduction of cvq irq causes interrupts to become shared, then
> > > ctrlq need to fall back to polling mode and keep the status quo.
> >
> > Having to path sounds a burden.
> >
> > >
> > > Thanks.
> > >
> >
> >
> > Thanks
> >
> > [1] https://www.linux-kvm.org/page/Multiqueue
> >
> > > >
> > > > --
> > > > MST
> > > >
> > >
> 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 05:38:22PM +0800, Heng Qi wrote:
> On Thu, 20 Jun 2024 04:32:15 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 03:29:15PM +0800, Heng Qi wrote:
> > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > >  
> > > > >  	/* Parameters for control virtqueue, if any */
> > > > >  	if (vi->has_cvq) {
> > > > > -		callbacks[total_vqs - 1] = NULL;
> > > > > +		callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > >  		names[total_vqs - 1] = "control";
> > > > >  	}
> > > > >  
> > > > 
> > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > this will cause irq sharing between VQs which will degrade
> > > > performance significantly.
> > > > 
> > > > So no, you can not just do it unconditionally.
> > > > 
> > > > The correct fix probably requires virtio core/API extensions.
> > > 
> > > If the introduction of cvq irq causes interrupts to become shared, then
> > > ctrlq need to fall back to polling mode and keep the status quo.
> > > 
> > > Thanks.
> > 
> > I don't see that in the code.
> > 
> > I guess we'll need more info in find vqs about what can and what can't share irqs?
> 
> I mean we should add fallback code, for example if allocating interrupt for ctrlq
> fails, we should clear the callback of ctrlq.

I have no idea how you plan to do that. interrupts are allocated in
virtio core, callbacks enabled in drivers.

> > Sharing between ctrl vq and config irq can also be an option.
> > 
> 
> Not sure if this violates the spec. In the spec, used buffer notification and
> configuration change notification are clearly defined - ctrlq is a virtqueue
> and used buffer notification should be used.
> 
> Thanks.

It is up to driver to choose which msix vector will trigger.
Nothing says same vector can't be reused.
Whether devices made assumptions based on current driver
behaviour is another matter.


> > 
> > 
> > 
> > > > 
> > > > -- 
> > > > MST
> > > > 
> > 
> > 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > >
> > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > >
> > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > >
> > > > > >     /* Parameters for control virtqueue, if any */
> > > > > >     if (vi->has_cvq) {
> > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > >             names[total_vqs - 1] = "control";
> > > > > >     }
> > > > > >
> > > > >
> > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > this will cause irq sharing between VQs which will degrade
> > > > > performance significantly.
> > > > >
> > >
> > > Why do we need to care about buggy management? I think libvirt has
> > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > 
> > And Qemu can calculate it correctly automatically since:
> > 
> > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > Author: Jason Wang <jasowang@redhat.com>
> > Date:   Mon Mar 8 12:49:19 2021 +0800
> > 
> >     virtio-net: calculating proper msix vectors on init
> > 
> >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> >     obvious not suitable for multiqueue guest, so we depends on the user
> >     or management tools to pass a correct vectors parameter. In fact, we
> >     can simplifying this by calculating the number of vectors on realize.
> > 
> >     Consider we have N queues, the number of vectors needed is 2*N + 2
> >     (#queue pairs + plus one config interrupt and control vq). We didn't
> >     check whether or not host support control vq because it was added
> >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > 
> >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> 
> Yes, devices designed according to the spec need to reserve an interrupt
> vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> 
> Thanks.

These aren't buggy, the spec allows this. So don't fail, but
I'm fine with using polling if not enough vectors.

> > 
> > Thanks
> > 
> > >
> > > > > So no, you can not just do it unconditionally.
> > > > >
> > > > > The correct fix probably requires virtio core/API extensions.
> > > >
> > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > ctrlq need to fall back to polling mode and keep the status quo.
> > >
> > > Having to path sounds a burden.
> > >
> > > >
> > > > Thanks.
> > > >
> > >
> > >
> > > Thanks
> > >
> > > [1] https://www.linux-kvm.org/page/Multiqueue
> > >
> > > > >
> > > > > --
> > > > > MST
> > > > >
> > > >
> > 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > >
> > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > >
> > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > >
> > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > >     if (vi->has_cvq) {
> > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > >             names[total_vqs - 1] = "control";
> > > > > > >     }
> > > > > > >
> > > > > >
> > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > performance significantly.
> > > > > >
> > > >
> > > > Why do we need to care about buggy management? I think libvirt has
> > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > 
> > > And Qemu can calculate it correctly automatically since:
> > > 
> > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > Author: Jason Wang <jasowang@redhat.com>
> > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > 
> > >     virtio-net: calculating proper msix vectors on init
> > > 
> > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > >     obvious not suitable for multiqueue guest, so we depends on the user
> > >     or management tools to pass a correct vectors parameter. In fact, we
> > >     can simplifying this by calculating the number of vectors on realize.
> > > 
> > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > >     check whether or not host support control vq because it was added
> > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > 
> > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > 
> > Yes, devices designed according to the spec need to reserve an interrupt
> > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > 
> > Thanks.
> 
> These aren't buggy, the spec allows this. So don't fail, but
> I'm fine with using polling if not enough vectors.

sharing with config interrupt is easier code-wise though, FWIW -
we don't need to maintain two code-paths.

> > > 
> > > Thanks
> > > 
> > > >
> > > > > > So no, you can not just do it unconditionally.
> > > > > >
> > > > > > The correct fix probably requires virtio core/API extensions.
> > > > >
> > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > >
> > > > Having to path sounds a burden.
> > > >
> > > > >
> > > > > Thanks.
> > > > >
> > > >
> > > >
> > > > Thanks
> > > >
> > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > >
> > > > > >
> > > > > > --
> > > > > > MST
> > > > > >
> > > > >
> > > 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > > >
> > > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > > >     if (vi->has_cvq) {
> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > > >             names[total_vqs - 1] = "control";
> > > > > > > >     }
> > > > > > > >
> > > > > > >
> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > > performance significantly.
> > > > > > >
> > > > >
> > > > > Why do we need to care about buggy management? I think libvirt has
> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > > 
> > > > And Qemu can calculate it correctly automatically since:
> > > > 
> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > > Author: Jason Wang <jasowang@redhat.com>
> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > > 
> > > >     virtio-net: calculating proper msix vectors on init
> > > > 
> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> > > >     or management tools to pass a correct vectors parameter. In fact, we
> > > >     can simplifying this by calculating the number of vectors on realize.
> > > > 
> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > > >     check whether or not host support control vq because it was added
> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > > 
> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > 
> > > Yes, devices designed according to the spec need to reserve an interrupt
> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > > 
> > > Thanks.
> > 
> > These aren't buggy, the spec allows this. So don't fail, but
> > I'm fine with using polling if not enough vectors.
> 
> sharing with config interrupt is easier code-wise though, FWIW -
> we don't need to maintain two code-paths.

Yes, it works well - config change irq is used less before - and will not fail.

Thanks.

> 
> > > > 
> > > > Thanks
> > > > 
> > > > >
> > > > > > > So no, you can not just do it unconditionally.
> > > > > > >
> > > > > > > The correct fix probably requires virtio core/API extensions.
> > > > > >
> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > > >
> > > > > Having to path sounds a burden.
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > > >
> > > > > > >
> > > > > > > --
> > > > > > > MST
> > > > > > >
> > > > > >
> > > > 
> 

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Xuan Zhuo]

On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > > >
> > > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > > >     if (vi->has_cvq) {
> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > > >             names[total_vqs - 1] = "control";
> > > > > > > >     }
> > > > > > > >
> > > > > > >
> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > > performance significantly.
> > > > > > >
> > > > >
> > > > > Why do we need to care about buggy management? I think libvirt has
> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > >
> > > > And Qemu can calculate it correctly automatically since:
> > > >
> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > > Author: Jason Wang <jasowang@redhat.com>
> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > >
> > > >     virtio-net: calculating proper msix vectors on init
> > > >
> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> > > >     or management tools to pass a correct vectors parameter. In fact, we
> > > >     can simplifying this by calculating the number of vectors on realize.
> > > >
> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > > >     check whether or not host support control vq because it was added
> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > >
> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > Yes, devices designed according to the spec need to reserve an interrupt
> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > >
> > > Thanks.
> >
> > These aren't buggy, the spec allows this. So don't fail, but
> > I'm fine with using polling if not enough vectors.
>
> sharing with config interrupt is easier code-wise though, FWIW -
> we don't need to maintain two code-paths.


If we do that, we should introduce a new helper, not to add new function to
find_vqs().

if ->share_irq_with_config:
	->share_irq_with_config(..., vq_idx)

Thanks.


>
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > > > So no, you can not just do it unconditionally.
> > > > > > >
> > > > > > > The correct fix probably requires virtio core/API extensions.
> > > > > >
> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > > >
> > > > > Having to path sounds a burden.
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > > >
> > > > > > >
> > > > > > > --
> > > > > > > MST
> > > > > > >
> > > > > >
> > > >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Fri, Jun 21, 2024 at 03:41:46PM +0800, Xuan Zhuo wrote:
> On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > > > >
> > > > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > > > >     if (vi->has_cvq) {
> > > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > > > >             names[total_vqs - 1] = "control";
> > > > > > > > >     }
> > > > > > > > >
> > > > > > > >
> > > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > > > performance significantly.
> > > > > > > >
> > > > > >
> > > > > > Why do we need to care about buggy management? I think libvirt has
> > > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > > >
> > > > > And Qemu can calculate it correctly automatically since:
> > > > >
> > > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > > >
> > > > >     virtio-net: calculating proper msix vectors on init
> > > > >
> > > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > > > >     obvious not suitable for multiqueue guest, so we depends on the user
> > > > >     or management tools to pass a correct vectors parameter. In fact, we
> > > > >     can simplifying this by calculating the number of vectors on realize.
> > > > >
> > > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > > > >     check whether or not host support control vq because it was added
> > > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > > >
> > > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > >
> > > > Yes, devices designed according to the spec need to reserve an interrupt
> > > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > > >
> > > > Thanks.
> > >
> > > These aren't buggy, the spec allows this. So don't fail, but
> > > I'm fine with using polling if not enough vectors.
> >
> > sharing with config interrupt is easier code-wise though, FWIW -
> > we don't need to maintain two code-paths.
> 
> 
> If we do that, we should introduce a new helper, not to add new function to
> find_vqs().
> 
> if ->share_irq_with_config:
> 	->share_irq_with_config(..., vq_idx)
> 
> Thanks.

Generally, I'd like to avoid something very narrow and single-use
in the API. Just telling virtio core that a vq is slow-path
sounds generic enough, and we should be able to extend that
later to support sharing between VQs.

> 
> >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > > > So no, you can not just do it unconditionally.
> > > > > > > >
> > > > > > > > The correct fix probably requires virtio core/API extensions.
> > > > > > >
> > > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > > > >
> > > > > > Having to path sounds a burden.
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > MST
> > > > > > > >
> > > > > > >
> > > > >
> >

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> If the device does not respond to a request for a long time,
> then control vq polling elevates CPU utilization, a problem that
> exacerbates with more command requests.
> 
> Enabling control vq's irq is advantageous for the guest, and
> this still doesn't support concurrent requests.
> 
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Heng Qi <hengqi@linux.alibaba.com>
> ---
>  drivers/net/virtio_net.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index b45f58a902e3..ed10084997d3 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -372,6 +372,8 @@ struct virtio_net_ctrl_rss {
>  struct control_buf {
>  	struct virtio_net_ctrl_hdr hdr;
>  	virtio_net_ctrl_ack status;
> +	/* Wait for the device to complete the cvq request. */
> +	struct completion completion;
>  };
>  
>  struct virtnet_info {
> @@ -664,6 +666,13 @@ static bool virtqueue_napi_complete(struct napi_struct *napi,
>  	return false;
>  }
>  
> +static void virtnet_cvq_done(struct virtqueue *cvq)
> +{
> +	struct virtnet_info *vi = cvq->vdev->priv;
> +
> +	complete(&vi->ctrl->completion);
> +}
> +
>  static void skb_xmit_done(struct virtqueue *vq)
>  {
>  	struct virtnet_info *vi = vq->vdev->priv;

> @@ -2724,14 +2733,8 @@ static bool virtnet_send_command_reply(struct virtnet_info *vi,
>  	if (unlikely(!virtqueue_kick(vi->cvq)))
>  		goto unlock;
>  
> -	/* Spin for a response, the kick causes an ioport write, trapping
> -	 * into the hypervisor, so the request should be handled immediately.
> -	 */
> -	while (!virtqueue_get_buf(vi->cvq, &tmp) &&
> -	       !virtqueue_is_broken(vi->cvq)) {
> -		cond_resched();
> -		cpu_relax();
> -	}
> +	wait_for_completion(&ctrl->completion);
> +	virtqueue_get_buf(vi->cvq, &tmp);
>  
>  unlock:
>  	ok = ctrl->status == VIRTIO_NET_OK;

Hmm no this is not a good idea, code should be robust in case
of spurious interrupts.

Also suprise removal is now broken ...


> @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  
>  	/* Parameters for control virtqueue, if any */
>  	if (vi->has_cvq) {
> -		callbacks[total_vqs - 1] = NULL;
> +		callbacks[total_vqs - 1] = virtnet_cvq_done;
>  		names[total_vqs - 1] = "control";
>  	}
>  
> @@ -5832,6 +5835,7 @@ static int virtnet_probe(struct virtio_device *vdev)
>  	if (vi->has_rss || vi->has_rss_hash_report)
>  		virtnet_init_default_rss(vi);
>  
> +	init_completion(&vi->ctrl->completion);
>  	enable_rx_mode_work(vi);
>  
>  	/* serialize netdev register + virtio_device_ready() with ndo_open() */
> -- 
> 2.32.0.3.g01195cf9f

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jason Wang]

On Thu, Jun 20, 2024 at 6:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > >
> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > >
> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > > >
> > > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > > >     if (vi->has_cvq) {
> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > > >             names[total_vqs - 1] = "control";
> > > > > > > >     }
> > > > > > > >
> > > > > > >
> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > > performance significantly.
> > > > > > >
> > > > >
> > > > > Why do we need to care about buggy management? I think libvirt has
> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > >
> > > > And Qemu can calculate it correctly automatically since:
> > > >
> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > > Author: Jason Wang <jasowang@redhat.com>
> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > >
> > > >     virtio-net: calculating proper msix vectors on init
> > > >
> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> > > >     or management tools to pass a correct vectors parameter. In fact, we
> > > >     can simplifying this by calculating the number of vectors on realize.
> > > >
> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > > >     check whether or not host support control vq because it was added
> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > >
> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > Yes, devices designed according to the spec need to reserve an interrupt
> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > >
> > > Thanks.
> >
> > These aren't buggy, the spec allows this.

So it doesn't differ from the case when we are lacking sufficient msix
vectors in the case of multiqueue. In that case we just fallback to
share one msix for all queues and another for config and we don't
bother at that time.

Any reason to bother now?

Thanks

> >  So don't fail, but
> > I'm fine with using polling if not enough vectors.
>
> sharing with config interrupt is easier code-wise though, FWIW -
> we don't need to maintain two code-paths.
>
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > > > So no, you can not just do it unconditionally.
> > > > > > >
> > > > > > > The correct fix probably requires virtio core/API extensions.
> > > > > >
> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > > >
> > > > > Having to path sounds a burden.
> > > > >
> > > > > >
> > > > > > Thanks.
> > > > > >
> > > > >
> > > > >
> > > > > Thanks
> > > > >
> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > > >
> > > > > > >
> > > > > > > --
> > > > > > > MST
> > > > > > >
> > > > > >
> > > >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 09:27:24AM +0800, Jason Wang wrote:
> On Thu, Jun 20, 2024 at 6:12 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> > > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> > > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> > > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> > > > > >
> > > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> > > > > > >
> > > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> > > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> > > > > > > > >
> > > > > > > > >     /* Parameters for control virtqueue, if any */
> > > > > > > > >     if (vi->has_cvq) {
> > > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> > > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> > > > > > > > >             names[total_vqs - 1] = "control";
> > > > > > > > >     }
> > > > > > > > >
> > > > > > > >
> > > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> > > > > > > > this will cause irq sharing between VQs which will degrade
> > > > > > > > performance significantly.
> > > > > > > >
> > > > > >
> > > > > > Why do we need to care about buggy management? I think libvirt has
> > > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> > > > >
> > > > > And Qemu can calculate it correctly automatically since:
> > > > >
> > > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> > > > > Author: Jason Wang <jasowang@redhat.com>
> > > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> > > > >
> > > > >     virtio-net: calculating proper msix vectors on init
> > > > >
> > > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> > > > >     obvious not suitable for multiqueue guest, so we depends on the user
> > > > >     or management tools to pass a correct vectors parameter. In fact, we
> > > > >     can simplifying this by calculating the number of vectors on realize.
> > > > >
> > > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> > > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> > > > >     check whether or not host support control vq because it was added
> > > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> > > > >
> > > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> > > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> > > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> > > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > >
> > > > Yes, devices designed according to the spec need to reserve an interrupt
> > > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> > > >
> > > > Thanks.
> > >
> > > These aren't buggy, the spec allows this.
> 
> So it doesn't differ from the case when we are lacking sufficient msix
> vectors in the case of multiqueue. In that case we just fallback to
> share one msix for all queues and another for config and we don't
> bother at that time.

sharing queues is exactly "bothering".

> Any reason to bother now?
> 
> Thanks

This patch can make datapath slower for such configs by switching to
sharing msix for a control path benefit. Not a tradeoff many users want
to make.


> > >  So don't fail, but
> > > I'm fine with using polling if not enough vectors.
> >
> > sharing with config interrupt is easier code-wise though, FWIW -
> > we don't need to maintain two code-paths.
> >
> > > > >
> > > > > Thanks
> > > > >
> > > > > >
> > > > > > > > So no, you can not just do it unconditionally.
> > > > > > > >
> > > > > > > > The correct fix probably requires virtio core/API extensions.
> > > > > > >
> > > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> > > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> > > > > >
> > > > > > Having to path sounds a burden.
> > > > > >
> > > > > > >
> > > > > > > Thanks.
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Thanks
> > > > > >
> > > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> > > > > >
> > > > > > > >
> > > > > > > > --
> > > > > > > > MST
> > > > > > > >
> > > > > > >
> > > > >
> >

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jiri Pirko]

Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
>On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
>> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
>> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>> > > > >
>> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>> > > > > >
>> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
>> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>> > > > > > > >
>> > > > > > > >     /* Parameters for control virtqueue, if any */
>> > > > > > > >     if (vi->has_cvq) {
>> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
>> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
>> > > > > > > >             names[total_vqs - 1] = "control";
>> > > > > > > >     }
>> > > > > > > >
>> > > > > > >
>> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
>> > > > > > > this will cause irq sharing between VQs which will degrade
>> > > > > > > performance significantly.
>> > > > > > >
>> > > > >
>> > > > > Why do we need to care about buggy management? I think libvirt has
>> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
>> > > > 
>> > > > And Qemu can calculate it correctly automatically since:
>> > > > 
>> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
>> > > > Author: Jason Wang <jasowang@redhat.com>
>> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
>> > > > 
>> > > >     virtio-net: calculating proper msix vectors on init
>> > > > 
>> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
>> > > >     obvious not suitable for multiqueue guest, so we depends on the user
>> > > >     or management tools to pass a correct vectors parameter. In fact, we
>> > > >     can simplifying this by calculating the number of vectors on realize.
>> > > > 
>> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
>> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
>> > > >     check whether or not host support control vq because it was added
>> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
>> > > > 
>> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
>> > > 
>> > > Yes, devices designed according to the spec need to reserve an interrupt
>> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
>> > > 
>> > > Thanks.
>> > 
>> > These aren't buggy, the spec allows this. So don't fail, but
>> > I'm fine with using polling if not enough vectors.
>> 
>> sharing with config interrupt is easier code-wise though, FWIW -
>> we don't need to maintain two code-paths.
>
>Yes, it works well - config change irq is used less before - and will not fail.

Please note I'm working on such fallback for admin queue. I would Like
to send the patchset by the end of this week. You can then use it easily
for cvq.

Something like:
/* the config->find_vqs() implementation */
int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
                struct virtqueue *vqs[], vq_callback_t *callbacks[],
                const char * const names[], const bool *ctx,
                struct irq_affinity *desc)
{
        int err;

        /* Try MSI-X with one vector per queue. */
        err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
                               VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
        if (!err)
                return 0;
        /* Fallback: MSI-X with one shared vector for config and
         * slow path queues, one vector per queue for the rest. */
        err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
                               VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
        if (!err)
                return 0;
        /* Fallback: MSI-X with one vector for config, one shared for queues. */
        err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
                               VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
        if (!err)
                return 0;
        /* Is there an interrupt? If not give up. */
        if (!(to_vp_device(vdev)->pci_dev->irq))
                return err;
        /* Finally fall back to regular interrupts. */
        return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
}




>
>Thanks.
>
>> 
>> > > > 
>> > > > Thanks
>> > > > 
>> > > > >
>> > > > > > > So no, you can not just do it unconditionally.
>> > > > > > >
>> > > > > > > The correct fix probably requires virtio core/API extensions.
>> > > > > >
>> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
>> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
>> > > > >
>> > > > > Having to path sounds a burden.
>> > > > >
>> > > > > >
>> > > > > > Thanks.
>> > > > > >
>> > > > >
>> > > > >
>> > > > > Thanks
>> > > > >
>> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
>> > > > >
>> > > > > > >
>> > > > > > > --
>> > > > > > > MST
>> > > > > > >
>> > > > > >
>> > > > 
>> 
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >> > > > >
> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >> > > > > >
> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >> > > > > > > >
> >> > > > > > > >     /* Parameters for control virtqueue, if any */
> >> > > > > > > >     if (vi->has_cvq) {
> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> >> > > > > > > >             names[total_vqs - 1] = "control";
> >> > > > > > > >     }
> >> > > > > > > >
> >> > > > > > >
> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> >> > > > > > > this will cause irq sharing between VQs which will degrade
> >> > > > > > > performance significantly.
> >> > > > > > >
> >> > > > >
> >> > > > > Why do we need to care about buggy management? I think libvirt has
> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> >> > > > 
> >> > > > And Qemu can calculate it correctly automatically since:
> >> > > > 
> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> >> > > > Author: Jason Wang <jasowang@redhat.com>
> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> >> > > > 
> >> > > >     virtio-net: calculating proper msix vectors on init
> >> > > > 
> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
> >> > > >     can simplifying this by calculating the number of vectors on realize.
> >> > > > 
> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> >> > > >     check whether or not host support control vq because it was added
> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> >> > > > 
> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> > > 
> >> > > Yes, devices designed according to the spec need to reserve an interrupt
> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> >> > > 
> >> > > Thanks.
> >> > 
> >> > These aren't buggy, the spec allows this. So don't fail, but
> >> > I'm fine with using polling if not enough vectors.
> >> 
> >> sharing with config interrupt is easier code-wise though, FWIW -
> >> we don't need to maintain two code-paths.
> >
> >Yes, it works well - config change irq is used less before - and will not fail.
> 
> Please note I'm working on such fallback for admin queue. I would Like
> to send the patchset by the end of this week. You can then use it easily
> for cvq.
> 
> Something like:
> /* the config->find_vqs() implementation */
> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
>                 const char * const names[], const bool *ctx,
>                 struct irq_affinity *desc)
> {
>         int err;
> 
>         /* Try MSI-X with one vector per queue. */
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
>         if (!err)
>                 return 0;
>         /* Fallback: MSI-X with one shared vector for config and
>          * slow path queues, one vector per queue for the rest. */
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
>         if (!err)
>                 return 0;
>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
>         if (!err)
>                 return 0;
>         /* Is there an interrupt? If not give up. */
>         if (!(to_vp_device(vdev)->pci_dev->irq))
>                 return err;
>         /* Finally fall back to regular interrupts. */
>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> }
> 
> 


Well for cvq, we'll need to adjust the API so core
knows cvq interrupts are be shared with config not
datapath.



> 
> >
> >Thanks.
> >
> >> 
> >> > > > 
> >> > > > Thanks
> >> > > > 
> >> > > > >
> >> > > > > > > So no, you can not just do it unconditionally.
> >> > > > > > >
> >> > > > > > > The correct fix probably requires virtio core/API extensions.
> >> > > > > >
> >> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> >> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> >> > > > >
> >> > > > > Having to path sounds a burden.
> >> > > > >
> >> > > > > >
> >> > > > > > Thanks.
> >> > > > > >
> >> > > > >
> >> > > > >
> >> > > > > Thanks
> >> > > > >
> >> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> >> > > > >
> >> > > > > > >
> >> > > > > > > --
> >> > > > > > > MST
> >> > > > > > >
> >> > > > > >
> >> > > > 
> >> 
> >

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jiri Pirko]

Wed, Jun 26, 2024 at 10:08:14AM CEST, mst@redhat.com wrote:
>On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
>> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
>> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
>> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
>> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>> >> > > > >
>> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>> >> > > > > >
>> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
>> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>> >> > > > > > > >
>> >> > > > > > > >     /* Parameters for control virtqueue, if any */
>> >> > > > > > > >     if (vi->has_cvq) {
>> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
>> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
>> >> > > > > > > >             names[total_vqs - 1] = "control";
>> >> > > > > > > >     }
>> >> > > > > > > >
>> >> > > > > > >
>> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
>> >> > > > > > > this will cause irq sharing between VQs which will degrade
>> >> > > > > > > performance significantly.
>> >> > > > > > >
>> >> > > > >
>> >> > > > > Why do we need to care about buggy management? I think libvirt has
>> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
>> >> > > > 
>> >> > > > And Qemu can calculate it correctly automatically since:
>> >> > > > 
>> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
>> >> > > > Author: Jason Wang <jasowang@redhat.com>
>> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
>> >> > > > 
>> >> > > >     virtio-net: calculating proper msix vectors on init
>> >> > > > 
>> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
>> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
>> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
>> >> > > >     can simplifying this by calculating the number of vectors on realize.
>> >> > > > 
>> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
>> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
>> >> > > >     check whether or not host support control vq because it was added
>> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
>> >> > > > 
>> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
>> >> > > 
>> >> > > Yes, devices designed according to the spec need to reserve an interrupt
>> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
>> >> > > 
>> >> > > Thanks.
>> >> > 
>> >> > These aren't buggy, the spec allows this. So don't fail, but
>> >> > I'm fine with using polling if not enough vectors.
>> >> 
>> >> sharing with config interrupt is easier code-wise though, FWIW -
>> >> we don't need to maintain two code-paths.
>> >
>> >Yes, it works well - config change irq is used less before - and will not fail.
>> 
>> Please note I'm working on such fallback for admin queue. I would Like
>> to send the patchset by the end of this week. You can then use it easily
>> for cvq.
>> 
>> Something like:
>> /* the config->find_vqs() implementation */
>> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
>>                 const char * const names[], const bool *ctx,
>>                 struct irq_affinity *desc)
>> {
>>         int err;
>> 
>>         /* Try MSI-X with one vector per queue. */
>>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
>>         if (!err)
>>                 return 0;
>>         /* Fallback: MSI-X with one shared vector for config and
>>          * slow path queues, one vector per queue for the rest. */
>>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
>>         if (!err)
>>                 return 0;
>>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
>>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
>>         if (!err)
>>                 return 0;
>>         /* Is there an interrupt? If not give up. */
>>         if (!(to_vp_device(vdev)->pci_dev->irq))
>>                 return err;
>>         /* Finally fall back to regular interrupts. */
>>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
>> }
>> 
>> 
>
>
>Well for cvq, we'll need to adjust the API so core
>knows cvq interrupts are be shared with config not
>datapath.

Agreed. I was thinking about introducing some info struct and pass array
of it instead of callbacks[] and names[]. Then the struct can contain
flag indication. Something like:

struct vq_info {
	vq_callback_t *callback;
	const char *name;
	bool slow_path;
};


>
>
>
>> 
>> >
>> >Thanks.
>> >
>> >> 
>> >> > > > 
>> >> > > > Thanks
>> >> > > > 
>> >> > > > >
>> >> > > > > > > So no, you can not just do it unconditionally.
>> >> > > > > > >
>> >> > > > > > > The correct fix probably requires virtio core/API extensions.
>> >> > > > > >
>> >> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
>> >> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
>> >> > > > >
>> >> > > > > Having to path sounds a burden.
>> >> > > > >
>> >> > > > > >
>> >> > > > > > Thanks.
>> >> > > > > >
>> >> > > > >
>> >> > > > >
>> >> > > > > Thanks
>> >> > > > >
>> >> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
>> >> > > > >
>> >> > > > > > >
>> >> > > > > > > --
>> >> > > > > > > MST
>> >> > > > > > >
>> >> > > > > >
>> >> > > > 
>> >> 
>> >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Michael S. Tsirkin]

On Wed, Jun 26, 2024 at 10:43:11AM +0200, Jiri Pirko wrote:
> Wed, Jun 26, 2024 at 10:08:14AM CEST, mst@redhat.com wrote:
> >On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
> >> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
> >> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> >> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> >> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >> >> > > > >
> >> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >> >> > > > > >
> >> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> >> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >> >> > > > > > > >
> >> >> > > > > > > >     /* Parameters for control virtqueue, if any */
> >> >> > > > > > > >     if (vi->has_cvq) {
> >> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> >> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> >> >> > > > > > > >             names[total_vqs - 1] = "control";
> >> >> > > > > > > >     }
> >> >> > > > > > > >
> >> >> > > > > > >
> >> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> >> >> > > > > > > this will cause irq sharing between VQs which will degrade
> >> >> > > > > > > performance significantly.
> >> >> > > > > > >
> >> >> > > > >
> >> >> > > > > Why do we need to care about buggy management? I think libvirt has
> >> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> >> >> > > > 
> >> >> > > > And Qemu can calculate it correctly automatically since:
> >> >> > > > 
> >> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> >> >> > > > Author: Jason Wang <jasowang@redhat.com>
> >> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> >> >> > > > 
> >> >> > > >     virtio-net: calculating proper msix vectors on init
> >> >> > > > 
> >> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> >> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> >> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
> >> >> > > >     can simplifying this by calculating the number of vectors on realize.
> >> >> > > > 
> >> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> >> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> >> >> > > >     check whether or not host support control vq because it was added
> >> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> >> >> > > > 
> >> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> >> >> > > 
> >> >> > > Yes, devices designed according to the spec need to reserve an interrupt
> >> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> >> >> > > 
> >> >> > > Thanks.
> >> >> > 
> >> >> > These aren't buggy, the spec allows this. So don't fail, but
> >> >> > I'm fine with using polling if not enough vectors.
> >> >> 
> >> >> sharing with config interrupt is easier code-wise though, FWIW -
> >> >> we don't need to maintain two code-paths.
> >> >
> >> >Yes, it works well - config change irq is used less before - and will not fail.
> >> 
> >> Please note I'm working on such fallback for admin queue. I would Like
> >> to send the patchset by the end of this week. You can then use it easily
> >> for cvq.
> >> 
> >> Something like:
> >> /* the config->find_vqs() implementation */
> >> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> >>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
> >>                 const char * const names[], const bool *ctx,
> >>                 struct irq_affinity *desc)
> >> {
> >>         int err;
> >> 
> >>         /* Try MSI-X with one vector per queue. */
> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
> >>         if (!err)
> >>                 return 0;
> >>         /* Fallback: MSI-X with one shared vector for config and
> >>          * slow path queues, one vector per queue for the rest. */
> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
> >>         if (!err)
> >>                 return 0;
> >>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
> >>         if (!err)
> >>                 return 0;
> >>         /* Is there an interrupt? If not give up. */
> >>         if (!(to_vp_device(vdev)->pci_dev->irq))
> >>                 return err;
> >>         /* Finally fall back to regular interrupts. */
> >>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> >> }
> >> 
> >> 
> >
> >
> >Well for cvq, we'll need to adjust the API so core
> >knows cvq interrupts are be shared with config not
> >datapath.
> 
> Agreed. I was thinking about introducing some info struct and pass array
> of it instead of callbacks[] and names[]. Then the struct can contain
> flag indication. Something like:
> 
> struct vq_info {
> 	vq_callback_t *callback;
> 	const char *name;
> 	bool slow_path;
> };
> 

Yes. Add ctx too? There were attempts at it already btw.


> >
> >
> >
> >> 
> >> >
> >> >Thanks.
> >> >
> >> >> 
> >> >> > > > 
> >> >> > > > Thanks
> >> >> > > > 
> >> >> > > > >
> >> >> > > > > > > So no, you can not just do it unconditionally.
> >> >> > > > > > >
> >> >> > > > > > > The correct fix probably requires virtio core/API extensions.
> >> >> > > > > >
> >> >> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
> >> >> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
> >> >> > > > >
> >> >> > > > > Having to path sounds a burden.
> >> >> > > > >
> >> >> > > > > >
> >> >> > > > > > Thanks.
> >> >> > > > > >
> >> >> > > > >
> >> >> > > > >
> >> >> > > > > Thanks
> >> >> > > > >
> >> >> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
> >> >> > > > >
> >> >> > > > > > >
> >> >> > > > > > > --
> >> >> > > > > > > MST
> >> >> > > > > > >
> >> >> > > > > >
> >> >> > > > 
> >> >> 
> >> >
> >

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jiri Pirko]

Wed, Jun 26, 2024 at 11:58:08AM CEST, mst@redhat.com wrote:
>On Wed, Jun 26, 2024 at 10:43:11AM +0200, Jiri Pirko wrote:
>> Wed, Jun 26, 2024 at 10:08:14AM CEST, mst@redhat.com wrote:
>> >On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
>> >> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
>> >> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
>> >> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
>> >> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
>> >> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>> >> >> > > > >
>> >> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>> >> >> > > > > >
>> >> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> >> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
>> >> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>> >> >> > > > > > > >
>> >> >> > > > > > > >     /* Parameters for control virtqueue, if any */
>> >> >> > > > > > > >     if (vi->has_cvq) {
>> >> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
>> >> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
>> >> >> > > > > > > >             names[total_vqs - 1] = "control";
>> >> >> > > > > > > >     }
>> >> >> > > > > > > >
>> >> >> > > > > > >
>> >> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
>> >> >> > > > > > > this will cause irq sharing between VQs which will degrade
>> >> >> > > > > > > performance significantly.
>> >> >> > > > > > >
>> >> >> > > > >
>> >> >> > > > > Why do we need to care about buggy management? I think libvirt has
>> >> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
>> >> >> > > > 
>> >> >> > > > And Qemu can calculate it correctly automatically since:
>> >> >> > > > 
>> >> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
>> >> >> > > > Author: Jason Wang <jasowang@redhat.com>
>> >> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
>> >> >> > > > 
>> >> >> > > >     virtio-net: calculating proper msix vectors on init
>> >> >> > > > 
>> >> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
>> >> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
>> >> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
>> >> >> > > >     can simplifying this by calculating the number of vectors on realize.
>> >> >> > > > 
>> >> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
>> >> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
>> >> >> > > >     check whether or not host support control vq because it was added
>> >> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
>> >> >> > > > 
>> >> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>> >> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>> >> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> >> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
>> >> >> > > 
>> >> >> > > Yes, devices designed according to the spec need to reserve an interrupt
>> >> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
>> >> >> > > 
>> >> >> > > Thanks.
>> >> >> > 
>> >> >> > These aren't buggy, the spec allows this. So don't fail, but
>> >> >> > I'm fine with using polling if not enough vectors.
>> >> >> 
>> >> >> sharing with config interrupt is easier code-wise though, FWIW -
>> >> >> we don't need to maintain two code-paths.
>> >> >
>> >> >Yes, it works well - config change irq is used less before - and will not fail.
>> >> 
>> >> Please note I'm working on such fallback for admin queue. I would Like
>> >> to send the patchset by the end of this week. You can then use it easily
>> >> for cvq.
>> >> 
>> >> Something like:
>> >> /* the config->find_vqs() implementation */
>> >> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>> >>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
>> >>                 const char * const names[], const bool *ctx,
>> >>                 struct irq_affinity *desc)
>> >> {
>> >>         int err;
>> >> 
>> >>         /* Try MSI-X with one vector per queue. */
>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>> >>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
>> >>         if (!err)
>> >>                 return 0;
>> >>         /* Fallback: MSI-X with one shared vector for config and
>> >>          * slow path queues, one vector per queue for the rest. */
>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>> >>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
>> >>         if (!err)
>> >>                 return 0;
>> >>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>> >>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
>> >>         if (!err)
>> >>                 return 0;
>> >>         /* Is there an interrupt? If not give up. */
>> >>         if (!(to_vp_device(vdev)->pci_dev->irq))
>> >>                 return err;
>> >>         /* Finally fall back to regular interrupts. */
>> >>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
>> >> }
>> >> 
>> >> 
>> >
>> >
>> >Well for cvq, we'll need to adjust the API so core
>> >knows cvq interrupts are be shared with config not
>> >datapath.
>> 
>> Agreed. I was thinking about introducing some info struct and pass array
>> of it instead of callbacks[] and names[]. Then the struct can contain
>> flag indication. Something like:
>> 
>> struct vq_info {
>> 	vq_callback_t *callback;
>> 	const char *name;
>> 	bool slow_path;
>> };
>> 
>
>Yes. Add ctx too? There were attempts at it already btw.

Yep, ctx too. I can take a stab at it if noone else is interested.


>
>
>> >
>> >
>> >
>> >> 
>> >> >
>> >> >Thanks.
>> >> >
>> >> >> 
>> >> >> > > > 
>> >> >> > > > Thanks
>> >> >> > > > 
>> >> >> > > > >
>> >> >> > > > > > > So no, you can not just do it unconditionally.
>> >> >> > > > > > >
>> >> >> > > > > > > The correct fix probably requires virtio core/API extensions.
>> >> >> > > > > >
>> >> >> > > > > > If the introduction of cvq irq causes interrupts to become shared, then
>> >> >> > > > > > ctrlq need to fall back to polling mode and keep the status quo.
>> >> >> > > > >
>> >> >> > > > > Having to path sounds a burden.
>> >> >> > > > >
>> >> >> > > > > >
>> >> >> > > > > > Thanks.
>> >> >> > > > > >
>> >> >> > > > >
>> >> >> > > > >
>> >> >> > > > > Thanks
>> >> >> > > > >
>> >> >> > > > > [1] https://www.linux-kvm.org/page/Multiqueue
>> >> >> > > > >
>> >> >> > > > > > >
>> >> >> > > > > > > --
>> >> >> > > > > > > MST
>> >> >> > > > > > >
>> >> >> > > > > >
>> >> >> > > > 
>> >> >> 
>> >> >
>> >
>

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Jiri Pirko]

Wed, Jun 26, 2024 at 01:51:00PM CEST, jiri@resnulli.us wrote:
>Wed, Jun 26, 2024 at 11:58:08AM CEST, mst@redhat.com wrote:
>>On Wed, Jun 26, 2024 at 10:43:11AM +0200, Jiri Pirko wrote:
>>> Wed, Jun 26, 2024 at 10:08:14AM CEST, mst@redhat.com wrote:
>>> >On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
>>> >> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
>>> >> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> >> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
>>> >> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
>>> >> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
>>> >> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
>>> >> >> > > > >
>>> >> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
>>> >> >> > > > > >
>>> >> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>>> >> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
>>> >> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>>> >> >> > > > > > > >
>>> >> >> > > > > > > >     /* Parameters for control virtqueue, if any */
>>> >> >> > > > > > > >     if (vi->has_cvq) {
>>> >> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
>>> >> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
>>> >> >> > > > > > > >             names[total_vqs - 1] = "control";
>>> >> >> > > > > > > >     }
>>> >> >> > > > > > > >
>>> >> >> > > > > > >
>>> >> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
>>> >> >> > > > > > > this will cause irq sharing between VQs which will degrade
>>> >> >> > > > > > > performance significantly.
>>> >> >> > > > > > >
>>> >> >> > > > >
>>> >> >> > > > > Why do we need to care about buggy management? I think libvirt has
>>> >> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
>>> >> >> > > > 
>>> >> >> > > > And Qemu can calculate it correctly automatically since:
>>> >> >> > > > 
>>> >> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
>>> >> >> > > > Author: Jason Wang <jasowang@redhat.com>
>>> >> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
>>> >> >> > > > 
>>> >> >> > > >     virtio-net: calculating proper msix vectors on init
>>> >> >> > > > 
>>> >> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
>>> >> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
>>> >> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
>>> >> >> > > >     can simplifying this by calculating the number of vectors on realize.
>>> >> >> > > > 
>>> >> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
>>> >> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
>>> >> >> > > >     check whether or not host support control vq because it was added
>>> >> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
>>> >> >> > > > 
>>> >> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
>>> >> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
>>> >> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>>> >> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
>>> >> >> > > 
>>> >> >> > > Yes, devices designed according to the spec need to reserve an interrupt
>>> >> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
>>> >> >> > > 
>>> >> >> > > Thanks.
>>> >> >> > 
>>> >> >> > These aren't buggy, the spec allows this. So don't fail, but
>>> >> >> > I'm fine with using polling if not enough vectors.
>>> >> >> 
>>> >> >> sharing with config interrupt is easier code-wise though, FWIW -
>>> >> >> we don't need to maintain two code-paths.
>>> >> >
>>> >> >Yes, it works well - config change irq is used less before - and will not fail.
>>> >> 
>>> >> Please note I'm working on such fallback for admin queue. I would Like
>>> >> to send the patchset by the end of this week. You can then use it easily
>>> >> for cvq.
>>> >> 
>>> >> Something like:
>>> >> /* the config->find_vqs() implementation */
>>> >> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
>>> >>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
>>> >>                 const char * const names[], const bool *ctx,
>>> >>                 struct irq_affinity *desc)
>>> >> {
>>> >>         int err;
>>> >> 
>>> >>         /* Try MSI-X with one vector per queue. */
>>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>> >>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
>>> >>         if (!err)
>>> >>                 return 0;
>>> >>         /* Fallback: MSI-X with one shared vector for config and
>>> >>          * slow path queues, one vector per queue for the rest. */
>>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>> >>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
>>> >>         if (!err)
>>> >>                 return 0;
>>> >>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
>>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
>>> >>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
>>> >>         if (!err)
>>> >>                 return 0;
>>> >>         /* Is there an interrupt? If not give up. */
>>> >>         if (!(to_vp_device(vdev)->pci_dev->irq))
>>> >>                 return err;
>>> >>         /* Finally fall back to regular interrupts. */
>>> >>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
>>> >> }
>>> >> 
>>> >> 
>>> >
>>> >
>>> >Well for cvq, we'll need to adjust the API so core
>>> >knows cvq interrupts are be shared with config not
>>> >datapath.
>>> 
>>> Agreed. I was thinking about introducing some info struct and pass array
>>> of it instead of callbacks[] and names[]. Then the struct can contain
>>> flag indication. Something like:
>>> 
>>> struct vq_info {
>>> 	vq_callback_t *callback;
>>> 	const char *name;
>>> 	bool slow_path;
>>> };
>>> 
>>
>>Yes. Add ctx too? There were attempts at it already btw.
>
>Yep, ctx too. I can take a stab at it if noone else is interested.

Since this work is in v4, and I hope it will get merged soon, I plan to
send v2 of admin queue parallelization patchset after that. Here it is:
https://github.com/jpirko/linux_mlxsw/tree/wip_virtio_parallel_aq2

Heng, note the last patch:
virtio_pci: allow to indicate virtqueue being slow path

That is not part of my set, it is ment to be merged in your control
queue patchset. Then you can just indicate cvq to be slow like this:

        /* Parameters for control virtqueue, if any */
        if (vi->has_cvq) {
                vqs_info[total_vqs - 1].callback = virtnet_cvq_done;
                vqs_info[total_vqs - 1].name = "control";
                vqs_info[total_vqs - 1].slow_path = true;
        }

I just wanted to let you know this is in process so you may prepare.
Will keep you informed.

Thanks.

Re: [PATCH net-next v4 2/5] virtio_net: enable irq for the control vq

[Heng Qi]

On Mon, 8 Jul 2024 13:40:42 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> Wed, Jun 26, 2024 at 01:51:00PM CEST, jiri@resnulli.us wrote:
> >Wed, Jun 26, 2024 at 11:58:08AM CEST, mst@redhat.com wrote:
> >>On Wed, Jun 26, 2024 at 10:43:11AM +0200, Jiri Pirko wrote:
> >>> Wed, Jun 26, 2024 at 10:08:14AM CEST, mst@redhat.com wrote:
> >>> >On Wed, Jun 26, 2024 at 09:52:58AM +0200, Jiri Pirko wrote:
> >>> >> Thu, Jun 20, 2024 at 12:31:34PM CEST, hengqi@linux.alibaba.com wrote:
> >>> >> >On Thu, 20 Jun 2024 06:11:40 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>> >> >> On Thu, Jun 20, 2024 at 06:10:51AM -0400, Michael S. Tsirkin wrote:
> >>> >> >> > On Thu, Jun 20, 2024 at 05:53:15PM +0800, Heng Qi wrote:
> >>> >> >> > > On Thu, 20 Jun 2024 16:26:05 +0800, Jason Wang <jasowang@redhat.com> wrote:
> >>> >> >> > > > On Thu, Jun 20, 2024 at 4:21 PM Jason Wang <jasowang@redhat.com> wrote:
> >>> >> >> > > > >
> >>> >> >> > > > > On Thu, Jun 20, 2024 at 3:35 PM Heng Qi <hengqi@linux.alibaba.com> wrote:
> >>> >> >> > > > > >
> >>> >> >> > > > > > On Wed, 19 Jun 2024 17:19:12 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >>> >> >> > > > > > > On Thu, Jun 20, 2024 at 12:19:05AM +0800, Heng Qi wrote:
> >>> >> >> > > > > > > > @@ -5312,7 +5315,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> >>> >> >> > > > > > > >
> >>> >> >> > > > > > > >     /* Parameters for control virtqueue, if any */
> >>> >> >> > > > > > > >     if (vi->has_cvq) {
> >>> >> >> > > > > > > > -           callbacks[total_vqs - 1] = NULL;
> >>> >> >> > > > > > > > +           callbacks[total_vqs - 1] = virtnet_cvq_done;
> >>> >> >> > > > > > > >             names[total_vqs - 1] = "control";
> >>> >> >> > > > > > > >     }
> >>> >> >> > > > > > > >
> >>> >> >> > > > > > >
> >>> >> >> > > > > > > If the # of MSIX vectors is exactly for data path VQs,
> >>> >> >> > > > > > > this will cause irq sharing between VQs which will degrade
> >>> >> >> > > > > > > performance significantly.
> >>> >> >> > > > > > >
> >>> >> >> > > > >
> >>> >> >> > > > > Why do we need to care about buggy management? I think libvirt has
> >>> >> >> > > > > been teached to use 2N+2 since the introduction of the multiqueue[1].
> >>> >> >> > > > 
> >>> >> >> > > > And Qemu can calculate it correctly automatically since:
> >>> >> >> > > > 
> >>> >> >> > > > commit 51a81a2118df0c70988f00d61647da9e298483a4
> >>> >> >> > > > Author: Jason Wang <jasowang@redhat.com>
> >>> >> >> > > > Date:   Mon Mar 8 12:49:19 2021 +0800
> >>> >> >> > > > 
> >>> >> >> > > >     virtio-net: calculating proper msix vectors on init
> >>> >> >> > > > 
> >>> >> >> > > >     Currently, the default msix vectors for virtio-net-pci is 3 which is
> >>> >> >> > > >     obvious not suitable for multiqueue guest, so we depends on the user
> >>> >> >> > > >     or management tools to pass a correct vectors parameter. In fact, we
> >>> >> >> > > >     can simplifying this by calculating the number of vectors on realize.
> >>> >> >> > > > 
> >>> >> >> > > >     Consider we have N queues, the number of vectors needed is 2*N + 2
> >>> >> >> > > >     (#queue pairs + plus one config interrupt and control vq). We didn't
> >>> >> >> > > >     check whether or not host support control vq because it was added
> >>> >> >> > > >     unconditionally by qemu to avoid breaking legacy guests such as Minix.
> >>> >> >> > > > 
> >>> >> >> > > >     Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com
> >>> >> >> > > >     Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
> >>> >> >> > > >     Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> >>> >> >> > > >     Signed-off-by: Jason Wang <jasowang@redhat.com>
> >>> >> >> > > 
> >>> >> >> > > Yes, devices designed according to the spec need to reserve an interrupt
> >>> >> >> > > vector for ctrlq. So, Michael, do we want to be compatible with buggy devices?
> >>> >> >> > > 
> >>> >> >> > > Thanks.
> >>> >> >> > 
> >>> >> >> > These aren't buggy, the spec allows this. So don't fail, but
> >>> >> >> > I'm fine with using polling if not enough vectors.
> >>> >> >> 
> >>> >> >> sharing with config interrupt is easier code-wise though, FWIW -
> >>> >> >> we don't need to maintain two code-paths.
> >>> >> >
> >>> >> >Yes, it works well - config change irq is used less before - and will not fail.
> >>> >> 
> >>> >> Please note I'm working on such fallback for admin queue. I would Like
> >>> >> to send the patchset by the end of this week. You can then use it easily
> >>> >> for cvq.
> >>> >> 
> >>> >> Something like:
> >>> >> /* the config->find_vqs() implementation */
> >>> >> int vp_find_vqs(struct virtio_device *vdev, unsigned int nvqs,
> >>> >>                 struct virtqueue *vqs[], vq_callback_t *callbacks[],
> >>> >>                 const char * const names[], const bool *ctx,
> >>> >>                 struct irq_affinity *desc)
> >>> >> {
> >>> >>         int err;
> >>> >> 
> >>> >>         /* Try MSI-X with one vector per queue. */
> >>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>> >>                                VP_VQ_VECTOR_POLICY_EACH, ctx, desc);
> >>> >>         if (!err)
> >>> >>                 return 0;
> >>> >>         /* Fallback: MSI-X with one shared vector for config and
> >>> >>          * slow path queues, one vector per queue for the rest. */
> >>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>> >>                                VP_VQ_VECTOR_POLICY_SHARED_SLOW, ctx, desc);
> >>> >>         if (!err)
> >>> >>                 return 0;
> >>> >>         /* Fallback: MSI-X with one vector for config, one shared for queues. */
> >>> >>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names,
> >>> >>                                VP_VQ_VECTOR_POLICY_SHARED, ctx, desc);
> >>> >>         if (!err)
> >>> >>                 return 0;
> >>> >>         /* Is there an interrupt? If not give up. */
> >>> >>         if (!(to_vp_device(vdev)->pci_dev->irq))
> >>> >>                 return err;
> >>> >>         /* Finally fall back to regular interrupts. */
> >>> >>         return vp_find_vqs_intx(vdev, nvqs, vqs, callbacks, names, ctx);
> >>> >> }
> >>> >> 
> >>> >> 
> >>> >
> >>> >
> >>> >Well for cvq, we'll need to adjust the API so core
> >>> >knows cvq interrupts are be shared with config not
> >>> >datapath.
> >>> 
> >>> Agreed. I was thinking about introducing some info struct and pass array
> >>> of it instead of callbacks[] and names[]. Then the struct can contain
> >>> flag indication. Something like:
> >>> 
> >>> struct vq_info {
> >>> 	vq_callback_t *callback;
> >>> 	const char *name;
> >>> 	bool slow_path;
> >>> };
> >>> 
> >>
> >>Yes. Add ctx too? There were attempts at it already btw.
> >
> >Yep, ctx too. I can take a stab at it if noone else is interested.
> 
> Since this work is in v4, and I hope it will get merged soon, I plan to

I've seen your set and will help review it tomorrow.

> send v2 of admin queue parallelization patchset after that. Here it is:
> https://github.com/jpirko/linux_mlxsw/tree/wip_virtio_parallel_aq2
> 
> Heng, note the last patch:
> virtio_pci: allow to indicate virtqueue being slow path
> 
> That is not part of my set, it is ment to be merged in your control
> queue patchset. Then you can just indicate cvq to be slow like this:
> 
>         /* Parameters for control virtqueue, if any */
>         if (vi->has_cvq) {
>                 vqs_info[total_vqs - 1].callback = virtnet_cvq_done;
>                 vqs_info[total_vqs - 1].name = "control";
>                 vqs_info[total_vqs - 1].slow_path = true;
>         }
> 
> I just wanted to let you know this is in process so you may prepare.
> Will keep you informed.

Thanks for letting me know!

Regards,
Heng

> 
> Thanks.