[PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently the admin queue command execution is serialized by a lock.
This patchsets lifts this limitation allowing to execute admin queue
commands in parallel. To do that, admin queue processing needs to be
converted from polling to interrupt based completion.

Patches #1-#6 are preparations, making things a bit smoother as well.
Patch #7 implements interrupt based completion for admin queue.
Patch #8 finally removes the admin queue serialization lock.

Jiri Pirko (8):
  virtio_pci: push out single vq find code to vp_find_one_vq_msix()
  virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
  virtio: push out code to vp_avq_index()
  virtio: create admin queues alongside other virtqueues
  virtio_pci_modern: create admin queue of queried size
  virtio_pci_modern: pass cmd as an identification token
  virtio_pci_modern: use completion instead of busy loop to wait on
    admin cmd result
  virtio_pci_modern: remove admin queue serialization lock

 drivers/virtio/virtio.c            |  28 +----
 drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
 drivers/virtio/virtio_pci_common.h |   9 +-
 drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
 include/linux/virtio.h             |   2 +
 include/linux/virtio_config.h      |   2 -
 6 files changed, 150 insertions(+), 160 deletions(-)

-- 
2.45.1

[PATCH virtio 1/8] virtio_pci: push out single vq find code to vp_find_one_vq_msix()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In order to be reused for admin queue setup, push out common code to
setup and configure irq for one vq into a separate helper.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_common.c | 69 ++++++++++++++++++------------
 1 file changed, 41 insertions(+), 28 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index f6b0b00e4599..1fb2183e409b 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -284,6 +284,44 @@ void vp_del_vqs(struct virtio_device *vdev)
 	vp_dev->vqs = NULL;
 }
 
+static struct virtqueue *vp_find_one_vq_msix(struct virtio_device *vdev,
+					     int queue_idx,
+					     vq_callback_t *callback,
+					     const char *name, bool ctx,
+					     int *allocated_vectors)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtqueue *vq;
+	u16 msix_vec;
+	int err;
+
+	if (!callback)
+		msix_vec = VIRTIO_MSI_NO_VECTOR;
+	else if (vp_dev->per_vq_vectors)
+		msix_vec = (*allocated_vectors)++;
+	else
+		msix_vec = VP_MSIX_VQ_VECTOR;
+	vq = vp_setup_vq(vdev, queue_idx++, callback, name, ctx, msix_vec);
+	if (IS_ERR(vq))
+		return vq;
+
+	if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
+		return vq;
+
+	/* allocate per-vq irq if available and necessary */
+	snprintf(vp_dev->msix_names[msix_vec], sizeof(*vp_dev->msix_names),
+		 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
+	err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
+			  vring_interrupt, 0,
+			  vp_dev->msix_names[msix_vec], vq);
+	if (err) {
+		vp_del_vq(vq);
+		return ERR_PTR(err);
+	}
+
+	return vq;
+}
+
 static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		struct virtqueue *vqs[], vq_callback_t *callbacks[],
 		const char * const names[], bool per_vq_vectors,
@@ -291,7 +329,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		struct irq_affinity *desc)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	u16 msix_vec;
 	int i, err, nvectors, allocated_vectors, queue_idx = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
@@ -321,37 +358,13 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 			vqs[i] = NULL;
 			continue;
 		}
-
-		if (!callbacks[i])
-			msix_vec = VIRTIO_MSI_NO_VECTOR;
-		else if (vp_dev->per_vq_vectors)
-			msix_vec = allocated_vectors++;
-		else
-			msix_vec = VP_MSIX_VQ_VECTOR;
-		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
-				     ctx ? ctx[i] : false,
-				     msix_vec);
+		vqs[i] = vp_find_one_vq_msix(vdev, queue_idx++, callbacks[i],
+					     names[i], ctx ? ctx[i] : false,
+					     &allocated_vectors);
 		if (IS_ERR(vqs[i])) {
 			err = PTR_ERR(vqs[i]);
 			goto error_find;
 		}
-
-		if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
-			continue;
-
-		/* allocate per-vq irq if available and necessary */
-		snprintf(vp_dev->msix_names[msix_vec],
-			 sizeof *vp_dev->msix_names,
-			 "%s-%s",
-			 dev_name(&vp_dev->vdev.dev), names[i]);
-		err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
-				  vring_interrupt, 0,
-				  vp_dev->msix_names[msix_vec],
-				  vqs[i]);
-		if (err) {
-			vp_del_vq(vqs[i]);
-			goto error_find;
-		}
 	}
 	return 0;
 
-- 
2.45.1

[PATCH virtio 2/8] virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

It is guaranteed by the virtio_pci and PCI layers that none of the VFs
is probed before setup_vq() is called for admin queue and after del_vq()
is called for admin queue. Therefore treat vp_dev->admin_vq.info.vq as
static, don't null it and don't take cmd lock during assign.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_common.h |  2 +-
 drivers/virtio/virtio_pci_modern.c | 11 +----------
 2 files changed, 2 insertions(+), 11 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 7fef52bee455..148d1ab01a69 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -45,7 +45,7 @@ struct virtio_pci_vq_info {
 struct virtio_pci_admin_vq {
 	/* Virtqueue info associated with this admin queue. */
 	struct virtio_pci_vq_info info;
-	/* serializing admin commands execution and virtqueue deletion */
+	/* serializing admin commands execution. */
 	struct mutex cmd_lock;
 	u64 supported_cmds;
 	/* Name of the admin queue: avq.$vq_index. */
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index f62b530aa3b5..925166498e2e 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -580,11 +580,8 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 		goto err;
 	}
 
-	if (is_avq) {
-		mutex_lock(&vp_dev->admin_vq.cmd_lock);
+	if (is_avq)
 		vp_dev->admin_vq.info.vq = vq;
-		mutex_unlock(&vp_dev->admin_vq.cmd_lock);
-	}
 
 	return vq;
 
@@ -621,12 +618,6 @@ static void del_vq(struct virtio_pci_vq_info *info)
 	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
 
-	if (vp_is_avq(&vp_dev->vdev, vq->index)) {
-		mutex_lock(&vp_dev->admin_vq.cmd_lock);
-		vp_dev->admin_vq.info.vq = NULL;
-		mutex_unlock(&vp_dev->admin_vq.cmd_lock);
-	}
-
 	if (vp_dev->msix_enabled)
 		vp_modern_queue_vector(mdev, vq->index,
 				       VIRTIO_MSI_NO_VECTOR);
-- 
2.45.1

[PATCH virtio 3/8] virtio: push out code to vp_avq_index()

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

To prepare for the follow-up patch to use the code as an op, push out
the code that gets admin virtqueue base index and count to a separate
helper.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_modern.c | 31 ++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 925166498e2e..ee8fb7a23d55 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -28,6 +28,21 @@ static u64 vp_get_features(struct virtio_device *vdev)
 	return vp_modern_get_features(&vp_dev->mdev);
 }
 
+static int vp_avq_index(struct virtio_device *vdev, u16 *index, u16 *num)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+
+	*num = 0;
+	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
+		return 0;
+
+	*num = vp_modern_avq_num(&vp_dev->mdev);
+	if (!(*num))
+		return -EINVAL;
+	*index = vp_modern_avq_index(&vp_dev->mdev);
+	return 0;
+}
+
 static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -730,19 +745,15 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 static int vp_modern_create_avq(struct virtio_device *vdev)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_admin_vq *avq;
+	struct virtio_pci_admin_vq *avq = &vp_dev->admin_vq;
 	struct virtqueue *vq;
-	u16 admin_q_num;
-
-	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
-		return 0;
+	u16 num;
+	int err;
 
-	admin_q_num = vp_modern_avq_num(&vp_dev->mdev);
-	if (!admin_q_num)
-		return -EINVAL;
+	err = vp_avq_index(vdev, &avq->vq_index, &num);
+	if (err || !num)
+		return err;
 
-	avq = &vp_dev->admin_vq;
-	avq->vq_index = vp_modern_avq_index(&vp_dev->mdev);
 	sprintf(avq->name, "avq.%u", avq->vq_index);
 	vq = vp_dev->setup_vq(vp_dev, &vp_dev->admin_vq.info, avq->vq_index, NULL,
 			      avq->name, NULL, VIRTIO_MSI_NO_VECTOR);
-- 
2.45.1

[PATCH virtio 4/8] virtio: create admin queues alongside other virtqueues

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Admin virtqueue is just another virtqueue nothing that special about it.
The current implementation treats it somehow separate though in terms
of creation and deletion. Unify the admin virtqueue creation and
deletion flows to be aligned with the rest of virtqueues, creating
it from vp_find_vqs_*() helpers. Let the admin virtqueue to be deleted
by vp_del_vqs() as the rest.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio.c            | 28 +-----------
 drivers/virtio/virtio_pci_common.c | 40 ++++++++++++++++--
 drivers/virtio/virtio_pci_common.h |  4 +-
 drivers/virtio/virtio_pci_modern.c | 68 +++---------------------------
 include/linux/virtio_config.h      |  2 -
 5 files changed, 45 insertions(+), 97 deletions(-)

diff --git a/drivers/virtio/virtio.c b/drivers/virtio/virtio.c
index 9510c551dce8..315bc2a42f1e 100644
--- a/drivers/virtio/virtio.c
+++ b/drivers/virtio/virtio.c
@@ -302,15 +302,9 @@ static int virtio_dev_probe(struct device *_d)
 	if (err)
 		goto err;
 
-	if (dev->config->create_avq) {
-		err = dev->config->create_avq(dev);
-		if (err)
-			goto err;
-	}
-
 	err = drv->probe(dev);
 	if (err)
-		goto err_probe;
+		goto err;
 
 	/* If probe didn't do it, mark device DRIVER_OK ourselves. */
 	if (!(dev->config->get_status(dev) & VIRTIO_CONFIG_S_DRIVER_OK))
@@ -323,9 +317,6 @@ static int virtio_dev_probe(struct device *_d)
 
 	return 0;
 
-err_probe:
-	if (dev->config->destroy_avq)
-		dev->config->destroy_avq(dev);
 err:
 	virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
 	return err;
@@ -341,9 +332,6 @@ static void virtio_dev_remove(struct device *_d)
 
 	drv->remove(dev);
 
-	if (dev->config->destroy_avq)
-		dev->config->destroy_avq(dev);
-
 	/* Driver should have reset device. */
 	WARN_ON_ONCE(dev->config->get_status(dev));
 
@@ -518,9 +506,6 @@ int virtio_device_freeze(struct virtio_device *dev)
 		}
 	}
 
-	if (dev->config->destroy_avq)
-		dev->config->destroy_avq(dev);
-
 	return 0;
 }
 EXPORT_SYMBOL_GPL(virtio_device_freeze);
@@ -556,16 +541,10 @@ int virtio_device_restore(struct virtio_device *dev)
 	if (ret)
 		goto err;
 
-	if (dev->config->create_avq) {
-		ret = dev->config->create_avq(dev);
-		if (ret)
-			goto err;
-	}
-
 	if (drv->restore) {
 		ret = drv->restore(dev);
 		if (ret)
-			goto err_restore;
+			goto err;
 	}
 
 	/* If restore didn't do it, mark device DRIVER_OK ourselves. */
@@ -576,9 +555,6 @@ int virtio_device_restore(struct virtio_device *dev)
 
 	return 0;
 
-err_restore:
-	if (dev->config->destroy_avq)
-		dev->config->destroy_avq(dev);
 err:
 	virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
 	return ret;
diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 1fb2183e409b..07c0511f170a 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -236,9 +236,6 @@ void vp_del_vqs(struct virtio_device *vdev)
 	int i;
 
 	list_for_each_entry_safe(vq, n, &vdev->vqs, list) {
-		if (vp_dev->is_avq && vp_dev->is_avq(vdev, vq->index))
-			continue;
-
 		if (vp_dev->per_vq_vectors) {
 			int v = vp_dev->vqs[vq->index]->msix_vector;
 
@@ -329,12 +326,20 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		struct irq_affinity *desc)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtio_pci_admin_vq *avq = &vp_dev->admin_vq;
 	int i, err, nvectors, allocated_vectors, queue_idx = 0;
+	u16 avq_num = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	if (vp_dev->avq_index) {
+		err = vp_dev->avq_index(vdev, &avq->vq_index, &avq_num);
+		if (err)
+			goto error_find;
+	}
+
 	if (per_vq_vectors) {
 		/* Best option: one for change interrupt, one per vq. */
 		nvectors = 1;
@@ -366,6 +371,17 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 			goto error_find;
 		}
 	}
+
+	if (!avq_num)
+		return 0;
+	sprintf(avq->name, "avq.%u", avq->vq_index);
+	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, NULL, avq->name,
+				     false, &allocated_vectors);
+	if (IS_ERR(vqs[i])) {
+		err = PTR_ERR(vqs[i]);
+		goto error_find;
+	}
+
 	return 0;
 
 error_find:
@@ -378,12 +394,20 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 		const char * const names[], const bool *ctx)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
+	struct virtio_pci_admin_vq *avq = &vp_dev->admin_vq;
 	int i, err, queue_idx = 0;
+	u16 avq_num = 0;
 
 	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
 	if (!vp_dev->vqs)
 		return -ENOMEM;
 
+	if (vp_dev->avq_index) {
+		err = vp_dev->avq_index(vdev, &avq->vq_index, &avq_num);
+		if (err)
+			goto out_del_vqs;
+	}
+
 	err = request_irq(vp_dev->pci_dev->irq, vp_interrupt, IRQF_SHARED,
 			dev_name(&vdev->dev), vp_dev);
 	if (err)
@@ -405,6 +429,16 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 		}
 	}
 
+	if (!avq_num)
+		return 0;
+	sprintf(avq->name, "avq.%u", avq->vq_index);
+	vqs[i] = vp_setup_vq(vdev, queue_idx++, NULL, avq->name,
+			     false, VIRTIO_MSI_NO_VECTOR);
+	if (IS_ERR(vqs[i])) {
+		err = PTR_ERR(vqs[i]);
+		goto out_del_vqs;
+	}
+
 	return 0;
 out_del_vqs:
 	vp_del_vqs(vdev);
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 148d1ab01a69..b3ef76287b43 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -43,8 +43,6 @@ struct virtio_pci_vq_info {
 };
 
 struct virtio_pci_admin_vq {
-	/* Virtqueue info associated with this admin queue. */
-	struct virtio_pci_vq_info info;
 	/* serializing admin commands execution. */
 	struct mutex cmd_lock;
 	u64 supported_cmds;
@@ -102,7 +100,7 @@ struct virtio_pci_device {
 	void (*del_vq)(struct virtio_pci_vq_info *info);
 
 	u16 (*config_vector)(struct virtio_pci_device *vp_dev, u16 vector);
-	bool (*is_avq)(struct virtio_device *vdev, unsigned int index);
+	int (*avq_index)(struct virtio_device *vdev, u16 *index, u16 *num);
 };
 
 /* Constants for MSI-X */
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index ee8fb7a23d55..adbaf1ac168e 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -53,7 +53,8 @@ static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
 	return index == vp_dev->admin_vq.vq_index;
 }
 
-static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
+static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
+				    struct virtio_pci_admin_vq *admin_vq,
 				    u16 opcode,
 				    struct scatterlist **sgs,
 				    unsigned int out_num,
@@ -63,7 +64,7 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_admin_vq *admin_vq,
 	struct virtqueue *vq;
 	int ret, len;
 
-	vq = admin_vq->info.vq;
+	vq = vp_dev->vqs[admin_vq->vq_index]->vq;
 	if (!vq)
 		return -EIO;
 
@@ -138,7 +139,7 @@ int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
 	}
 
 	mutex_lock(&vp_dev->admin_vq.cmd_lock);
-	ret = virtqueue_exec_admin_cmd(&vp_dev->admin_vq,
+	ret = virtqueue_exec_admin_cmd(vp_dev, &vp_dev->admin_vq,
 				       le16_to_cpu(cmd->opcode),
 				       sgs, out_num, in_num, sgs);
 	mutex_unlock(&vp_dev->admin_vq.cmd_lock);
@@ -203,27 +204,12 @@ static void virtio_pci_admin_cmd_list_init(struct virtio_device *virtio_dev)
 
 static void vp_modern_avq_activate(struct virtio_device *vdev)
 {
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
-
 	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
 		return;
 
-	__virtqueue_unbreak(admin_vq->info.vq);
 	virtio_pci_admin_cmd_list_init(vdev);
 }
 
-static void vp_modern_avq_deactivate(struct virtio_device *vdev)
-{
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
-
-	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
-		return;
-
-	__virtqueue_break(admin_vq->info.vq);
-}
-
 static void vp_transport_features(struct virtio_device *vdev, u64 features)
 {
 	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
@@ -418,8 +404,6 @@ static void vp_reset(struct virtio_device *vdev)
 	while (vp_modern_get_status(mdev))
 		msleep(1);
 
-	vp_modern_avq_deactivate(vdev);
-
 	/* Flush pending VQ/configuration callbacks. */
 	vp_synchronize_vectors(vdev);
 }
@@ -595,9 +579,6 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 		goto err;
 	}
 
-	if (is_avq)
-		vp_dev->admin_vq.info.vq = vq;
-
 	return vq;
 
 err:
@@ -742,41 +723,6 @@ static bool vp_get_shm_region(struct virtio_device *vdev,
 	return true;
 }
 
-static int vp_modern_create_avq(struct virtio_device *vdev)
-{
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-	struct virtio_pci_admin_vq *avq = &vp_dev->admin_vq;
-	struct virtqueue *vq;
-	u16 num;
-	int err;
-
-	err = vp_avq_index(vdev, &avq->vq_index, &num);
-	if (err || !num)
-		return err;
-
-	sprintf(avq->name, "avq.%u", avq->vq_index);
-	vq = vp_dev->setup_vq(vp_dev, &vp_dev->admin_vq.info, avq->vq_index, NULL,
-			      avq->name, NULL, VIRTIO_MSI_NO_VECTOR);
-	if (IS_ERR(vq)) {
-		dev_err(&vdev->dev, "failed to setup admin virtqueue, err=%ld",
-			PTR_ERR(vq));
-		return PTR_ERR(vq);
-	}
-
-	vp_modern_set_queue_enable(&vp_dev->mdev, avq->info.vq->index, true);
-	return 0;
-}
-
-static void vp_modern_destroy_avq(struct virtio_device *vdev)
-{
-	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
-
-	if (!virtio_has_feature(vdev, VIRTIO_F_ADMIN_VQ))
-		return;
-
-	vp_dev->del_vq(&vp_dev->admin_vq.info);
-}
-
 static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.get		= NULL,
 	.set		= NULL,
@@ -795,8 +741,6 @@ static const struct virtio_config_ops virtio_pci_config_nodev_ops = {
 	.get_shm_region  = vp_get_shm_region,
 	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
 	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
-	.create_avq = vp_modern_create_avq,
-	.destroy_avq = vp_modern_destroy_avq,
 };
 
 static const struct virtio_config_ops virtio_pci_config_ops = {
@@ -817,8 +761,6 @@ static const struct virtio_config_ops virtio_pci_config_ops = {
 	.get_shm_region  = vp_get_shm_region,
 	.disable_vq_and_reset = vp_modern_disable_vq_and_reset,
 	.enable_vq_after_reset = vp_modern_enable_vq_after_reset,
-	.create_avq = vp_modern_create_avq,
-	.destroy_avq = vp_modern_destroy_avq,
 };
 
 /* the PCI probing function */
@@ -842,7 +784,7 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->config_vector = vp_config_vector;
 	vp_dev->setup_vq = setup_vq;
 	vp_dev->del_vq = del_vq;
-	vp_dev->is_avq = vp_is_avq;
+	vp_dev->avq_index = vp_avq_index;
 	vp_dev->isr = mdev->isr;
 	vp_dev->vdev.id = mdev->id;
 
diff --git a/include/linux/virtio_config.h b/include/linux/virtio_config.h
index da9b271b54db..dfa05a3cc0a0 100644
--- a/include/linux/virtio_config.h
+++ b/include/linux/virtio_config.h
@@ -122,8 +122,6 @@ struct virtio_config_ops {
 			       struct virtio_shm_region *region, u8 id);
 	int (*disable_vq_and_reset)(struct virtqueue *vq);
 	int (*enable_vq_after_reset)(struct virtqueue *vq);
-	int (*create_avq)(struct virtio_device *vdev);
-	void (*destroy_avq)(struct virtio_device *vdev);
 };
 
 /* If driver didn't advertise the feature, it will never appear. */
-- 
2.45.1

[PATCH virtio 5/8] virtio_pci_modern: create admin queue of queried size

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Don't limit the admin queue size to VIRTIO_AVQ_SGS_MAX and rather rely
on the queried queue size.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_modern.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index adbaf1ac168e..41aa97e338ac 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -551,8 +551,7 @@ static struct virtqueue *setup_vq(struct virtio_pci_device *vp_dev,
 	if (index >= vp_modern_get_num_queues(mdev) && !is_avq)
 		return ERR_PTR(-EINVAL);
 
-	num = is_avq ?
-		VIRTIO_AVQ_SGS_MAX : vp_modern_get_queue_size(mdev, index);
+	num = vp_modern_get_queue_size(mdev, index);
 	/* Check if queue is either not available or already active. */
 	if (!num || vp_modern_get_queue_enable(mdev, index))
 		return ERR_PTR(-ENOENT);
-- 
2.45.1

[PATCH virtio 6/8] virtio_pci_modern: pass cmd as an identification token

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

In preparation to asynchronous admin queue processing, pass cmd pointer
as a data arg to virtqueue_add_sgs().

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_modern.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index 41aa97e338ac..b4041e541fc3 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -59,7 +59,7 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
 				    struct scatterlist **sgs,
 				    unsigned int out_num,
 				    unsigned int in_num,
-				    void *data)
+				    struct virtio_admin_cmd *cmd)
 {
 	struct virtqueue *vq;
 	int ret, len;
@@ -73,7 +73,7 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
 	    !((1ULL << opcode) & admin_vq->supported_cmds))
 		return -EOPNOTSUPP;
 
-	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, data, GFP_KERNEL);
+	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
 	if (ret < 0)
 		return -EIO;
 
@@ -141,7 +141,7 @@ int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
 	mutex_lock(&vp_dev->admin_vq.cmd_lock);
 	ret = virtqueue_exec_admin_cmd(vp_dev, &vp_dev->admin_vq,
 				       le16_to_cpu(cmd->opcode),
-				       sgs, out_num, in_num, sgs);
+				       sgs, out_num, in_num, cmd);
 	mutex_unlock(&vp_dev->admin_vq.cmd_lock);
 
 	if (ret) {
-- 
2.45.1

[PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

Currently, the code waits in a busy loop on every admin virtqueue issued
command to get a reply. That prevents callers from issuing multiple
commands in parallel.

To overcome this limitation, introduce a virtqueue event callback for
admin virtqueue. For every issued command, use completion mechanism
to wait on a reply. In the event callback, trigger the completion
is done for every incoming reply.

Alongside with that, introduce a spin lock to protect the admin
virtqueue operations.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_common.c | 10 +++---
 drivers/virtio/virtio_pci_common.h |  3 ++
 drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++-------
 include/linux/virtio.h             |  2 ++
 4 files changed, 51 insertions(+), 16 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
index 07c0511f170a..5ff7304c7a2a 100644
--- a/drivers/virtio/virtio_pci_common.c
+++ b/drivers/virtio/virtio_pci_common.c
@@ -346,6 +346,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 		for (i = 0; i < nvqs; ++i)
 			if (names[i] && callbacks[i])
 				++nvectors;
+		if (avq_num)
+			++nvectors;
 	} else {
 		/* Second best: one for change, shared for all vqs. */
 		nvectors = 2;
@@ -375,8 +377,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
 	if (!avq_num)
 		return 0;
 	sprintf(avq->name, "avq.%u", avq->vq_index);
-	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, NULL, avq->name,
-				     false, &allocated_vectors);
+	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, vp_modern_avq_done,
+				     avq->name, false, &allocated_vectors);
 	if (IS_ERR(vqs[i])) {
 		err = PTR_ERR(vqs[i]);
 		goto error_find;
@@ -432,8 +434,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
 	if (!avq_num)
 		return 0;
 	sprintf(avq->name, "avq.%u", avq->vq_index);
-	vqs[i] = vp_setup_vq(vdev, queue_idx++, NULL, avq->name,
-			     false, VIRTIO_MSI_NO_VECTOR);
+	vqs[i] = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done,
+			     avq->name, false, VIRTIO_MSI_NO_VECTOR);
 	if (IS_ERR(vqs[i])) {
 		err = PTR_ERR(vqs[i]);
 		goto out_del_vqs;
diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index b3ef76287b43..38a0b6df0844 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -45,6 +45,8 @@ struct virtio_pci_vq_info {
 struct virtio_pci_admin_vq {
 	/* serializing admin commands execution. */
 	struct mutex cmd_lock;
+	/* Protects virtqueue access. */
+	spinlock_t lock;
 	u64 supported_cmds;
 	/* Name of the admin queue: avq.$vq_index. */
 	char name[10];
@@ -174,6 +176,7 @@ struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
 #define VIRTIO_ADMIN_CMD_BITMAP 0
 #endif
 
+void vp_modern_avq_done(struct virtqueue *vq);
 int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
 			     struct virtio_admin_cmd *cmd);
 
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index b4041e541fc3..b9937e4b8a69 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -53,6 +53,23 @@ static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
 	return index == vp_dev->admin_vq.vq_index;
 }
 
+void vp_modern_avq_done(struct virtqueue *vq)
+{
+	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
+	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
+	struct virtio_admin_cmd *cmd;
+	unsigned long flags;
+	unsigned int len;
+
+	spin_lock_irqsave(&admin_vq->lock, flags);
+	do {
+		virtqueue_disable_cb(vq);
+		while ((cmd = virtqueue_get_buf(vq, &len)))
+			complete(&cmd->completion);
+	} while (!virtqueue_enable_cb(vq));
+	spin_unlock_irqrestore(&admin_vq->lock, flags);
+}
+
 static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
 				    struct virtio_pci_admin_vq *admin_vq,
 				    u16 opcode,
@@ -62,7 +79,8 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
 				    struct virtio_admin_cmd *cmd)
 {
 	struct virtqueue *vq;
-	int ret, len;
+	unsigned long flags;
+	int ret;
 
 	vq = vp_dev->vqs[admin_vq->vq_index]->vq;
 	if (!vq)
@@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
 	    !((1ULL << opcode) & admin_vq->supported_cmds))
 		return -EOPNOTSUPP;
 
-	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
-	if (ret < 0)
-		return -EIO;
-
-	if (unlikely(!virtqueue_kick(vq)))
-		return -EIO;
+	init_completion(&cmd->completion);
 
-	while (!virtqueue_get_buf(vq, &len) &&
-	       !virtqueue_is_broken(vq))
-		cpu_relax();
+again:
+	spin_lock_irqsave(&admin_vq->lock, flags);
+	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
+	if (ret < 0) {
+		if (ret == -ENOSPC) {
+			spin_unlock_irqrestore(&admin_vq->lock, flags);
+			cpu_relax();
+			goto again;
+		}
+		goto unlock_err;
+	}
+	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
+		goto unlock_err;
+	spin_unlock_irqrestore(&admin_vq->lock, flags);
 
-	if (virtqueue_is_broken(vq))
-		return -EIO;
+	wait_for_completion(&cmd->completion);
 
 	return 0;
+
+unlock_err:
+	spin_unlock_irqrestore(&admin_vq->lock, flags);
+	return -EIO;
 }
 
 int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
@@ -787,6 +814,7 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->isr = mdev->isr;
 	vp_dev->vdev.id = mdev->id;
 
+	spin_lock_init(&vp_dev->admin_vq.lock);
 	mutex_init(&vp_dev->admin_vq.cmd_lock);
 	return 0;
 }
diff --git a/include/linux/virtio.h b/include/linux/virtio.h
index 26c4325aa373..5db8ee175e71 100644
--- a/include/linux/virtio.h
+++ b/include/linux/virtio.h
@@ -10,6 +10,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/gfp.h>
 #include <linux/dma-mapping.h>
+#include <linux/completion.h>
 
 /**
  * struct virtqueue - a queue to register buffers for sending or receiving.
@@ -109,6 +110,7 @@ struct virtio_admin_cmd {
 	__le64 group_member_id;
 	struct scatterlist *data_sg;
 	struct scatterlist *result_sg;
+	struct completion completion;
 };
 
 /**
-- 
2.45.1

[PATCH virtio 8/8] virtio_pci_modern: remove admin queue serialization lock

[Jiri Pirko]

From: Jiri Pirko <jiri@nvidia.com>

The admin queue operations are protected by newly introduced
spin lock. To make it possible to issue parallel commands, remove the
admin queue serialization lock.

Signed-off-by: Jiri Pirko <jiri@nvidia.com>
---
 drivers/virtio/virtio_pci_common.h | 2 --
 drivers/virtio/virtio_pci_modern.c | 5 -----
 2 files changed, 7 deletions(-)

diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
index 38a0b6df0844..b3d2adca4383 100644
--- a/drivers/virtio/virtio_pci_common.h
+++ b/drivers/virtio/virtio_pci_common.h
@@ -43,8 +43,6 @@ struct virtio_pci_vq_info {
 };
 
 struct virtio_pci_admin_vq {
-	/* serializing admin commands execution. */
-	struct mutex cmd_lock;
 	/* Protects virtqueue access. */
 	spinlock_t lock;
 	u64 supported_cmds;
diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
index b9937e4b8a69..d983e37c7d39 100644
--- a/drivers/virtio/virtio_pci_modern.c
+++ b/drivers/virtio/virtio_pci_modern.c
@@ -165,12 +165,9 @@ int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
 		in_num++;
 	}
 
-	mutex_lock(&vp_dev->admin_vq.cmd_lock);
 	ret = virtqueue_exec_admin_cmd(vp_dev, &vp_dev->admin_vq,
 				       le16_to_cpu(cmd->opcode),
 				       sgs, out_num, in_num, cmd);
-	mutex_unlock(&vp_dev->admin_vq.cmd_lock);
-
 	if (ret) {
 		dev_err(&vdev->dev,
 			"Failed to execute command on admin vq: %d\n.", ret);
@@ -815,7 +812,6 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
 	vp_dev->vdev.id = mdev->id;
 
 	spin_lock_init(&vp_dev->admin_vq.lock);
-	mutex_init(&vp_dev->admin_vq.cmd_lock);
 	return 0;
 }
 
@@ -823,6 +819,5 @@ void virtio_pci_modern_remove(struct virtio_pci_device *vp_dev)
 {
 	struct virtio_pci_modern_device *mdev = &vp_dev->mdev;
 
-	mutex_destroy(&vp_dev->admin_vq.cmd_lock);
 	vp_modern_remove(mdev);
 }
-- 
2.45.1

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Heng Qi]

On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently the admin queue command execution is serialized by a lock.
> This patchsets lifts this limitation allowing to execute admin queue
> commands in parallel. To do that, admin queue processing needs to be
> converted from polling to interrupt based completion.
> 
> Patches #1-#6 are preparations, making things a bit smoother as well.
> Patch #7 implements interrupt based completion for admin queue.

Hi, Jiri

Before this set, I pushed the cvq irq set [1], and the discussion focused on the
fact that the newly added irq vector may cause the IO queue to fall back to
shared interrupt mode.
But it is true that devices implemented according to the specification should
not encounter this problem. So what do you think?

[1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/

> Patch #8 finally removes the admin queue serialization lock.
> 
> Jiri Pirko (8):
>   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>   virtio: push out code to vp_avq_index()
>   virtio: create admin queues alongside other virtqueues
>   virtio_pci_modern: create admin queue of queried size
>   virtio_pci_modern: pass cmd as an identification token
>   virtio_pci_modern: use completion instead of busy loop to wait on
>     admin cmd result
>   virtio_pci_modern: remove admin queue serialization lock
> 
>  drivers/virtio/virtio.c            |  28 +----
>  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>  drivers/virtio/virtio_pci_common.h |   9 +-
>  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>  include/linux/virtio.h             |   2 +
>  include/linux/virtio_config.h      |   2 -
>  6 files changed, 150 insertions(+), 160 deletions(-)
> 
> -- 
> 2.45.1
> 
> 

Re: [PATCH virtio 1/8] virtio_pci: push out single vq find code to vp_find_one_vq_msix()

[Heng Qi]

On Mon, 24 Jun 2024 11:04:44 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> In order to be reused for admin queue setup, push out common code to
> setup and configure irq for one vq into a separate helper.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 69 ++++++++++++++++++------------
>  1 file changed, 41 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index f6b0b00e4599..1fb2183e409b 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -284,6 +284,44 @@ void vp_del_vqs(struct virtio_device *vdev)
>  	vp_dev->vqs = NULL;
>  }
>  
> +static struct virtqueue *vp_find_one_vq_msix(struct virtio_device *vdev,
> +					     int queue_idx,
> +					     vq_callback_t *callback,
> +					     const char *name, bool ctx,
> +					     int *allocated_vectors)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> +	struct virtqueue *vq;
> +	u16 msix_vec;
> +	int err;
> +
> +	if (!callback)
> +		msix_vec = VIRTIO_MSI_NO_VECTOR;
> +	else if (vp_dev->per_vq_vectors)
> +		msix_vec = (*allocated_vectors)++;
> +	else
> +		msix_vec = VP_MSIX_VQ_VECTOR;
> +	vq = vp_setup_vq(vdev, queue_idx++, callback, name, ctx, msix_vec);

Looks like we don't need this ++.

Thanks.

> +	if (IS_ERR(vq))
> +		return vq;
> +
> +	if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
> +		return vq;
> +
> +	/* allocate per-vq irq if available and necessary */
> +	snprintf(vp_dev->msix_names[msix_vec], sizeof(*vp_dev->msix_names),
> +		 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
> +	err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> +			  vring_interrupt, 0,
> +			  vp_dev->msix_names[msix_vec], vq);
> +	if (err) {
> +		vp_del_vq(vq);
> +		return ERR_PTR(err);
> +	}
> +
> +	return vq;
> +}
> +
>  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>  		struct virtqueue *vqs[], vq_callback_t *callbacks[],
>  		const char * const names[], bool per_vq_vectors,
> @@ -291,7 +329,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>  		struct irq_affinity *desc)
>  {
>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
> -	u16 msix_vec;
>  	int i, err, nvectors, allocated_vectors, queue_idx = 0;
>  
>  	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
> @@ -321,37 +358,13 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>  			vqs[i] = NULL;
>  			continue;
>  		}
> -
> -		if (!callbacks[i])
> -			msix_vec = VIRTIO_MSI_NO_VECTOR;
> -		else if (vp_dev->per_vq_vectors)
> -			msix_vec = allocated_vectors++;
> -		else
> -			msix_vec = VP_MSIX_VQ_VECTOR;
> -		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
> -				     ctx ? ctx[i] : false,
> -				     msix_vec);
> +		vqs[i] = vp_find_one_vq_msix(vdev, queue_idx++, callbacks[i],
> +					     names[i], ctx ? ctx[i] : false,
> +					     &allocated_vectors);
>  		if (IS_ERR(vqs[i])) {
>  			err = PTR_ERR(vqs[i]);
>  			goto error_find;
>  		}
> -
> -		if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
> -			continue;
> -
> -		/* allocate per-vq irq if available and necessary */
> -		snprintf(vp_dev->msix_names[msix_vec],
> -			 sizeof *vp_dev->msix_names,
> -			 "%s-%s",
> -			 dev_name(&vp_dev->vdev.dev), names[i]);
> -		err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
> -				  vring_interrupt, 0,
> -				  vp_dev->msix_names[msix_vec],
> -				  vqs[i]);
> -		if (err) {
> -			vp_del_vq(vqs[i]);
> -			goto error_find;
> -		}
>  	}
>  	return 0;
>  
> -- 
> 2.45.1
> 
> 

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 11:04:43AM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently the admin queue command execution is serialized by a lock.
> This patchsets lifts this limitation allowing to execute admin queue
> commands in parallel. To do that, admin queue processing needs to be
> converted from polling to interrupt based completion.
> 
> Patches #1-#6 are preparations, making things a bit smoother as well.
> Patch #7 implements interrupt based completion for admin queue.
> Patch #8 finally removes the admin queue serialization lock.

Okay ... I assume this is infrastructure for some other need?
I'll review, but I'd like to see this actually used.


> Jiri Pirko (8):
>   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>   virtio: push out code to vp_avq_index()
>   virtio: create admin queues alongside other virtqueues
>   virtio_pci_modern: create admin queue of queried size
>   virtio_pci_modern: pass cmd as an identification token
>   virtio_pci_modern: use completion instead of busy loop to wait on
>     admin cmd result
>   virtio_pci_modern: remove admin queue serialization lock
> 
>  drivers/virtio/virtio.c            |  28 +----
>  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>  drivers/virtio/virtio_pci_common.h |   9 +-
>  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>  include/linux/virtio.h             |   2 +
>  include/linux/virtio_config.h      |   2 -
>  6 files changed, 150 insertions(+), 160 deletions(-)
> 
> -- 
> 2.45.1

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> > From: Jiri Pirko <jiri@nvidia.com>
> > 
> > Currently the admin queue command execution is serialized by a lock.
> > This patchsets lifts this limitation allowing to execute admin queue
> > commands in parallel. To do that, admin queue processing needs to be
> > converted from polling to interrupt based completion.
> > 
> > Patches #1-#6 are preparations, making things a bit smoother as well.
> > Patch #7 implements interrupt based completion for admin queue.
> 
> Hi, Jiri
> 
> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> fact that the newly added irq vector may cause the IO queue to fall back to
> shared interrupt mode.
> But it is true that devices implemented according to the specification should
> not encounter this problem. So what do you think?
> 
> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/

It's true - this can cause guest to run out of vectors for a variety of
reasons.

First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
I am not sure why we don't allow IRQF_SHARED for the config
interrupt though. So I think addressing this part can be deferred.

Second, we might not have enough msix vectors on the device. Here sharing
with e.g. cvq and further with config interrupt would make sense.

Jiri do you think you can help Heng Qi hammer out a solution for cvq?
I feel this will work will then benefit in a similar way,
and having us poll aggressively for cvq but not admin commands
does not make much sense, right?

> > Patch #8 finally removes the admin queue serialization lock.
> > 
> > Jiri Pirko (8):
> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> >   virtio: push out code to vp_avq_index()
> >   virtio: create admin queues alongside other virtqueues
> >   virtio_pci_modern: create admin queue of queried size
> >   virtio_pci_modern: pass cmd as an identification token
> >   virtio_pci_modern: use completion instead of busy loop to wait on
> >     admin cmd result
> >   virtio_pci_modern: remove admin queue serialization lock
> > 
> >  drivers/virtio/virtio.c            |  28 +----
> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> >  drivers/virtio/virtio_pci_common.h |   9 +-
> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> >  include/linux/virtio.h             |   2 +
> >  include/linux/virtio_config.h      |   2 -
> >  6 files changed, 150 insertions(+), 160 deletions(-)
> > 
> > -- 
> > 2.45.1
> > 
> > 

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>  		return -EOPNOTSUPP;
>  
> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	if (unlikely(!virtqueue_kick(vq)))
> -		return -EIO;
> +	init_completion(&cmd->completion);
>  
> -	while (!virtqueue_get_buf(vq, &len) &&
> -	       !virtqueue_is_broken(vq))
> -		cpu_relax();
> +again:
> +	spin_lock_irqsave(&admin_vq->lock, flags);
> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> +	if (ret < 0) {
> +		if (ret == -ENOSPC) {
> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
> +			cpu_relax();
> +			goto again;
> +		}
> +		goto unlock_err;
> +	}
> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
> +		goto unlock_err;
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>  
> -	if (virtqueue_is_broken(vq))
> -		return -EIO;
> +	wait_for_completion(&cmd->completion);
>  
>  	return 0;
> +
> +unlock_err:
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
> +	return -EIO;
>  }
>  


The reason we had the virtqueue_is_broken check previously,
is because this way surprise removal works: it happens to set
vq broken flag on all vqs.


So if you are changing that to a completion, I think surprise
removal needs to trigger a callback so the completion
can be signalled.

I think the cvq work also needs this, BTW.




-- 
MST

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
> From: Jiri Pirko <jiri@nvidia.com>
> 
> Currently, the code waits in a busy loop on every admin virtqueue issued
> command to get a reply. That prevents callers from issuing multiple
> commands in parallel.
> 
> To overcome this limitation, introduce a virtqueue event callback for
> admin virtqueue. For every issued command, use completion mechanism
> to wait on a reply. In the event callback, trigger the completion
> is done for every incoming reply.
> 
> Alongside with that, introduce a spin lock to protect the admin
> virtqueue operations.
> 
> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
> ---
>  drivers/virtio/virtio_pci_common.c | 10 +++---
>  drivers/virtio/virtio_pci_common.h |  3 ++
>  drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++-------
>  include/linux/virtio.h             |  2 ++
>  4 files changed, 51 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 07c0511f170a..5ff7304c7a2a 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -346,6 +346,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>  		for (i = 0; i < nvqs; ++i)
>  			if (names[i] && callbacks[i])
>  				++nvectors;
> +		if (avq_num)
> +			++nvectors;
>  	} else {
>  		/* Second best: one for change, shared for all vqs. */
>  		nvectors = 2;
> @@ -375,8 +377,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>  	if (!avq_num)
>  		return 0;
>  	sprintf(avq->name, "avq.%u", avq->vq_index);
> -	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, NULL, avq->name,
> -				     false, &allocated_vectors);
> +	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, vp_modern_avq_done,
> +				     avq->name, false, &allocated_vectors);
>  	if (IS_ERR(vqs[i])) {
>  		err = PTR_ERR(vqs[i]);
>  		goto error_find;
> @@ -432,8 +434,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
>  	if (!avq_num)
>  		return 0;
>  	sprintf(avq->name, "avq.%u", avq->vq_index);
> -	vqs[i] = vp_setup_vq(vdev, queue_idx++, NULL, avq->name,
> -			     false, VIRTIO_MSI_NO_VECTOR);
> +	vqs[i] = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done,
> +			     avq->name, false, VIRTIO_MSI_NO_VECTOR);
>  	if (IS_ERR(vqs[i])) {
>  		err = PTR_ERR(vqs[i]);
>  		goto out_del_vqs;
> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
> index b3ef76287b43..38a0b6df0844 100644
> --- a/drivers/virtio/virtio_pci_common.h
> +++ b/drivers/virtio/virtio_pci_common.h
> @@ -45,6 +45,8 @@ struct virtio_pci_vq_info {
>  struct virtio_pci_admin_vq {
>  	/* serializing admin commands execution. */
>  	struct mutex cmd_lock;
> +	/* Protects virtqueue access. */
> +	spinlock_t lock;
>  	u64 supported_cmds;
>  	/* Name of the admin queue: avq.$vq_index. */
>  	char name[10];
> @@ -174,6 +176,7 @@ struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
>  #define VIRTIO_ADMIN_CMD_BITMAP 0
>  #endif
>  
> +void vp_modern_avq_done(struct virtqueue *vq);
>  int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>  			     struct virtio_admin_cmd *cmd);
>  
> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
> index b4041e541fc3..b9937e4b8a69 100644
> --- a/drivers/virtio/virtio_pci_modern.c
> +++ b/drivers/virtio/virtio_pci_modern.c
> @@ -53,6 +53,23 @@ static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
>  	return index == vp_dev->admin_vq.vq_index;
>  }
>  
> +void vp_modern_avq_done(struct virtqueue *vq)
> +{
> +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
> +	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
> +	struct virtio_admin_cmd *cmd;
> +	unsigned long flags;
> +	unsigned int len;
> +
> +	spin_lock_irqsave(&admin_vq->lock, flags);
> +	do {
> +		virtqueue_disable_cb(vq);
> +		while ((cmd = virtqueue_get_buf(vq, &len)))
> +			complete(&cmd->completion);
> +	} while (!virtqueue_enable_cb(vq));
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
> +}
> +
>  static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>  				    struct virtio_pci_admin_vq *admin_vq,
>  				    u16 opcode,
> @@ -62,7 +79,8 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>  				    struct virtio_admin_cmd *cmd)
>  {
>  	struct virtqueue *vq;
> -	int ret, len;
> +	unsigned long flags;
> +	int ret;
>  
>  	vq = vp_dev->vqs[admin_vq->vq_index]->vq;
>  	if (!vq)
> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>  		return -EOPNOTSUPP;
>  
> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> -	if (ret < 0)
> -		return -EIO;
> -
> -	if (unlikely(!virtqueue_kick(vq)))
> -		return -EIO;
> +	init_completion(&cmd->completion);
>  
> -	while (!virtqueue_get_buf(vq, &len) &&
> -	       !virtqueue_is_broken(vq))
> -		cpu_relax();
> +again:
> +	spin_lock_irqsave(&admin_vq->lock, flags);
> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
> +	if (ret < 0) {
> +		if (ret == -ENOSPC) {
> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
> +			cpu_relax();
> +			goto again;
> +		}
> +		goto unlock_err;
> +	}
> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
> +		goto unlock_err;


This can actually happen with suprise removal.
So WARN_ON_ONCE isn't really appropriate I think.


> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>  
> -	if (virtqueue_is_broken(vq))
> -		return -EIO;
> +	wait_for_completion(&cmd->completion);
>  
>  	return 0;
> +
> +unlock_err:
> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
> +	return -EIO;
>  }
>  
>  int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
> @@ -787,6 +814,7 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>  	vp_dev->isr = mdev->isr;
>  	vp_dev->vdev.id = mdev->id;
>  
> +	spin_lock_init(&vp_dev->admin_vq.lock);
>  	mutex_init(&vp_dev->admin_vq.cmd_lock);
>  	return 0;
>  }
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index 26c4325aa373..5db8ee175e71 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -10,6 +10,7 @@
>  #include <linux/mod_devicetable.h>
>  #include <linux/gfp.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/completion.h>
>  
>  /**
>   * struct virtqueue - a queue to register buffers for sending or receiving.
> @@ -109,6 +110,7 @@ struct virtio_admin_cmd {
>  	__le64 group_member_id;
>  	struct scatterlist *data_sg;
>  	struct scatterlist *result_sg;
> +	struct completion completion;
>  };
>  
>  /**
> -- 
> 2.45.1

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:07:23PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 11:04:43AM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Currently the admin queue command execution is serialized by a lock.
>> This patchsets lifts this limitation allowing to execute admin queue
>> commands in parallel. To do that, admin queue processing needs to be
>> converted from polling to interrupt based completion.
>> 
>> Patches #1-#6 are preparations, making things a bit smoother as well.
>> Patch #7 implements interrupt based completion for admin queue.
>> Patch #8 finally removes the admin queue serialization lock.
>
>Okay ... I assume this is infrastructure for some other need?
>I'll review, but I'd like to see this actually used.

No other need. The currently, admin queue cmds are serialized. Note that
admin queue is currently created for PF. If you have for example
multiple VFs in legacy mode reading and writing pci bar, that virtio
vfio driver translates it into admin queue commands. So for example
probe on multiple VFs gets serialized. This patchset makes it possible
to run in parallel.


>
>
>> Jiri Pirko (8):
>>   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>>   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>>   virtio: push out code to vp_avq_index()
>>   virtio: create admin queues alongside other virtqueues
>>   virtio_pci_modern: create admin queue of queried size
>>   virtio_pci_modern: pass cmd as an identification token
>>   virtio_pci_modern: use completion instead of busy loop to wait on
>>     admin cmd result
>>   virtio_pci_modern: remove admin queue serialization lock
>> 
>>  drivers/virtio/virtio.c            |  28 +----
>>  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>>  drivers/virtio/virtio_pci_common.h |   9 +-
>>  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>>  include/linux/virtio.h             |   2 +
>>  include/linux/virtio_config.h      |   2 -
>>  6 files changed, 150 insertions(+), 160 deletions(-)
>> 
>> -- 
>> 2.45.1
>

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:30:54PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
>> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>>  		return -EOPNOTSUPP;
>>  
>> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> -	if (ret < 0)
>> -		return -EIO;
>> -
>> -	if (unlikely(!virtqueue_kick(vq)))
>> -		return -EIO;
>> +	init_completion(&cmd->completion);
>>  
>> -	while (!virtqueue_get_buf(vq, &len) &&
>> -	       !virtqueue_is_broken(vq))
>> -		cpu_relax();
>> +again:
>> +	spin_lock_irqsave(&admin_vq->lock, flags);
>> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> +	if (ret < 0) {
>> +		if (ret == -ENOSPC) {
>> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +			cpu_relax();
>> +			goto again;
>> +		}
>> +		goto unlock_err;
>> +	}
>> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
>> +		goto unlock_err;
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>>  
>> -	if (virtqueue_is_broken(vq))
>> -		return -EIO;
>> +	wait_for_completion(&cmd->completion);
>>  
>>  	return 0;
>> +
>> +unlock_err:
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +	return -EIO;
>>  }
>>  
>
>
>The reason we had the virtqueue_is_broken check previously,
>is because this way surprise removal works: it happens to set
>vq broken flag on all vqs.
>
>
>So if you are changing that to a completion, I think surprise
>removal needs to trigger a callback so the completion
>can be signalled.

Got it. Will check that out.


>
>I think the cvq work also needs this, BTW.
>
>
>
>
>-- 
>MST
>

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:34:47PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> Currently, the code waits in a busy loop on every admin virtqueue issued
>> command to get a reply. That prevents callers from issuing multiple
>> commands in parallel.
>> 
>> To overcome this limitation, introduce a virtqueue event callback for
>> admin virtqueue. For every issued command, use completion mechanism
>> to wait on a reply. In the event callback, trigger the completion
>> is done for every incoming reply.
>> 
>> Alongside with that, introduce a spin lock to protect the admin
>> virtqueue operations.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  drivers/virtio/virtio_pci_common.c | 10 +++---
>>  drivers/virtio/virtio_pci_common.h |  3 ++
>>  drivers/virtio/virtio_pci_modern.c | 52 +++++++++++++++++++++++-------
>>  include/linux/virtio.h             |  2 ++
>>  4 files changed, 51 insertions(+), 16 deletions(-)
>> 
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index 07c0511f170a..5ff7304c7a2a 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -346,6 +346,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>>  		for (i = 0; i < nvqs; ++i)
>>  			if (names[i] && callbacks[i])
>>  				++nvectors;
>> +		if (avq_num)
>> +			++nvectors;
>>  	} else {
>>  		/* Second best: one for change, shared for all vqs. */
>>  		nvectors = 2;
>> @@ -375,8 +377,8 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>>  	if (!avq_num)
>>  		return 0;
>>  	sprintf(avq->name, "avq.%u", avq->vq_index);
>> -	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, NULL, avq->name,
>> -				     false, &allocated_vectors);
>> +	vqs[i] = vp_find_one_vq_msix(vdev, avq->vq_index, vp_modern_avq_done,
>> +				     avq->name, false, &allocated_vectors);
>>  	if (IS_ERR(vqs[i])) {
>>  		err = PTR_ERR(vqs[i]);
>>  		goto error_find;
>> @@ -432,8 +434,8 @@ static int vp_find_vqs_intx(struct virtio_device *vdev, unsigned int nvqs,
>>  	if (!avq_num)
>>  		return 0;
>>  	sprintf(avq->name, "avq.%u", avq->vq_index);
>> -	vqs[i] = vp_setup_vq(vdev, queue_idx++, NULL, avq->name,
>> -			     false, VIRTIO_MSI_NO_VECTOR);
>> +	vqs[i] = vp_setup_vq(vdev, queue_idx++, vp_modern_avq_done,
>> +			     avq->name, false, VIRTIO_MSI_NO_VECTOR);
>>  	if (IS_ERR(vqs[i])) {
>>  		err = PTR_ERR(vqs[i]);
>>  		goto out_del_vqs;
>> diff --git a/drivers/virtio/virtio_pci_common.h b/drivers/virtio/virtio_pci_common.h
>> index b3ef76287b43..38a0b6df0844 100644
>> --- a/drivers/virtio/virtio_pci_common.h
>> +++ b/drivers/virtio/virtio_pci_common.h
>> @@ -45,6 +45,8 @@ struct virtio_pci_vq_info {
>>  struct virtio_pci_admin_vq {
>>  	/* serializing admin commands execution. */
>>  	struct mutex cmd_lock;
>> +	/* Protects virtqueue access. */
>> +	spinlock_t lock;
>>  	u64 supported_cmds;
>>  	/* Name of the admin queue: avq.$vq_index. */
>>  	char name[10];
>> @@ -174,6 +176,7 @@ struct virtio_device *virtio_pci_vf_get_pf_dev(struct pci_dev *pdev);
>>  #define VIRTIO_ADMIN_CMD_BITMAP 0
>>  #endif
>>  
>> +void vp_modern_avq_done(struct virtqueue *vq);
>>  int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>>  			     struct virtio_admin_cmd *cmd);
>>  
>> diff --git a/drivers/virtio/virtio_pci_modern.c b/drivers/virtio/virtio_pci_modern.c
>> index b4041e541fc3..b9937e4b8a69 100644
>> --- a/drivers/virtio/virtio_pci_modern.c
>> +++ b/drivers/virtio/virtio_pci_modern.c
>> @@ -53,6 +53,23 @@ static bool vp_is_avq(struct virtio_device *vdev, unsigned int index)
>>  	return index == vp_dev->admin_vq.vq_index;
>>  }
>>  
>> +void vp_modern_avq_done(struct virtqueue *vq)
>> +{
>> +	struct virtio_pci_device *vp_dev = to_vp_device(vq->vdev);
>> +	struct virtio_pci_admin_vq *admin_vq = &vp_dev->admin_vq;
>> +	struct virtio_admin_cmd *cmd;
>> +	unsigned long flags;
>> +	unsigned int len;
>> +
>> +	spin_lock_irqsave(&admin_vq->lock, flags);
>> +	do {
>> +		virtqueue_disable_cb(vq);
>> +		while ((cmd = virtqueue_get_buf(vq, &len)))
>> +			complete(&cmd->completion);
>> +	} while (!virtqueue_enable_cb(vq));
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +}
>> +
>>  static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>>  				    struct virtio_pci_admin_vq *admin_vq,
>>  				    u16 opcode,
>> @@ -62,7 +79,8 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>>  				    struct virtio_admin_cmd *cmd)
>>  {
>>  	struct virtqueue *vq;
>> -	int ret, len;
>> +	unsigned long flags;
>> +	int ret;
>>  
>>  	vq = vp_dev->vqs[admin_vq->vq_index]->vq;
>>  	if (!vq)
>> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>>  		return -EOPNOTSUPP;
>>  
>> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> -	if (ret < 0)
>> -		return -EIO;
>> -
>> -	if (unlikely(!virtqueue_kick(vq)))
>> -		return -EIO;
>> +	init_completion(&cmd->completion);
>>  
>> -	while (!virtqueue_get_buf(vq, &len) &&
>> -	       !virtqueue_is_broken(vq))
>> -		cpu_relax();
>> +again:
>> +	spin_lock_irqsave(&admin_vq->lock, flags);
>> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> +	if (ret < 0) {
>> +		if (ret == -ENOSPC) {
>> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +			cpu_relax();
>> +			goto again;
>> +		}
>> +		goto unlock_err;
>> +	}
>> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
>> +		goto unlock_err;
>
>
>This can actually happen with suprise removal.
>So WARN_ON_ONCE isn't really appropriate I think.

Got it. Will remove this.



>
>
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>>  
>> -	if (virtqueue_is_broken(vq))
>> -		return -EIO;
>> +	wait_for_completion(&cmd->completion);
>>  
>>  	return 0;
>> +
>> +unlock_err:
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +	return -EIO;
>>  }
>>  
>>  int vp_modern_admin_cmd_exec(struct virtio_device *vdev,
>> @@ -787,6 +814,7 @@ int virtio_pci_modern_probe(struct virtio_pci_device *vp_dev)
>>  	vp_dev->isr = mdev->isr;
>>  	vp_dev->vdev.id = mdev->id;
>>  
>> +	spin_lock_init(&vp_dev->admin_vq.lock);
>>  	mutex_init(&vp_dev->admin_vq.cmd_lock);
>>  	return 0;
>>  }
>> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
>> index 26c4325aa373..5db8ee175e71 100644
>> --- a/include/linux/virtio.h
>> +++ b/include/linux/virtio.h
>> @@ -10,6 +10,7 @@
>>  #include <linux/mod_devicetable.h>
>>  #include <linux/gfp.h>
>>  #include <linux/dma-mapping.h>
>> +#include <linux/completion.h>
>>  
>>  /**
>>   * struct virtqueue - a queue to register buffers for sending or receiving.
>> @@ -109,6 +110,7 @@ struct virtio_admin_cmd {
>>  	__le64 group_member_id;
>>  	struct scatterlist *data_sg;
>>  	struct scatterlist *result_sg;
>> +	struct completion completion;
>>  };
>>  
>>  /**
>> -- 
>> 2.45.1
>

Re: [PATCH virtio 1/8] virtio_pci: push out single vq find code to vp_find_one_vq_msix()

[Jiri Pirko]

Mon, Jun 24, 2024 at 12:52:47PM CEST, hengqi@linux.alibaba.com wrote:
>On Mon, 24 Jun 2024 11:04:44 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> From: Jiri Pirko <jiri@nvidia.com>
>> 
>> In order to be reused for admin queue setup, push out common code to
>> setup and configure irq for one vq into a separate helper.
>> 
>> Signed-off-by: Jiri Pirko <jiri@nvidia.com>
>> ---
>>  drivers/virtio/virtio_pci_common.c | 69 ++++++++++++++++++------------
>>  1 file changed, 41 insertions(+), 28 deletions(-)
>> 
>> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
>> index f6b0b00e4599..1fb2183e409b 100644
>> --- a/drivers/virtio/virtio_pci_common.c
>> +++ b/drivers/virtio/virtio_pci_common.c
>> @@ -284,6 +284,44 @@ void vp_del_vqs(struct virtio_device *vdev)
>>  	vp_dev->vqs = NULL;
>>  }
>>  
>> +static struct virtqueue *vp_find_one_vq_msix(struct virtio_device *vdev,
>> +					     int queue_idx,
>> +					     vq_callback_t *callback,
>> +					     const char *name, bool ctx,
>> +					     int *allocated_vectors)
>> +{
>> +	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> +	struct virtqueue *vq;
>> +	u16 msix_vec;
>> +	int err;
>> +
>> +	if (!callback)
>> +		msix_vec = VIRTIO_MSI_NO_VECTOR;
>> +	else if (vp_dev->per_vq_vectors)
>> +		msix_vec = (*allocated_vectors)++;
>> +	else
>> +		msix_vec = VP_MSIX_VQ_VECTOR;
>> +	vq = vp_setup_vq(vdev, queue_idx++, callback, name, ctx, msix_vec);
>
>Looks like we don't need this ++.

True. Will remove this leftover.

>
>Thanks.
>
>> +	if (IS_ERR(vq))
>> +		return vq;
>> +
>> +	if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
>> +		return vq;
>> +
>> +	/* allocate per-vq irq if available and necessary */
>> +	snprintf(vp_dev->msix_names[msix_vec], sizeof(*vp_dev->msix_names),
>> +		 "%s-%s", dev_name(&vp_dev->vdev.dev), name);
>> +	err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
>> +			  vring_interrupt, 0,
>> +			  vp_dev->msix_names[msix_vec], vq);
>> +	if (err) {
>> +		vp_del_vq(vq);
>> +		return ERR_PTR(err);
>> +	}
>> +
>> +	return vq;
>> +}
>> +
>>  static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>>  		struct virtqueue *vqs[], vq_callback_t *callbacks[],
>>  		const char * const names[], bool per_vq_vectors,
>> @@ -291,7 +329,6 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>>  		struct irq_affinity *desc)
>>  {
>>  	struct virtio_pci_device *vp_dev = to_vp_device(vdev);
>> -	u16 msix_vec;
>>  	int i, err, nvectors, allocated_vectors, queue_idx = 0;
>>  
>>  	vp_dev->vqs = kcalloc(nvqs, sizeof(*vp_dev->vqs), GFP_KERNEL);
>> @@ -321,37 +358,13 @@ static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned int nvqs,
>>  			vqs[i] = NULL;
>>  			continue;
>>  		}
>> -
>> -		if (!callbacks[i])
>> -			msix_vec = VIRTIO_MSI_NO_VECTOR;
>> -		else if (vp_dev->per_vq_vectors)
>> -			msix_vec = allocated_vectors++;
>> -		else
>> -			msix_vec = VP_MSIX_VQ_VECTOR;
>> -		vqs[i] = vp_setup_vq(vdev, queue_idx++, callbacks[i], names[i],
>> -				     ctx ? ctx[i] : false,
>> -				     msix_vec);
>> +		vqs[i] = vp_find_one_vq_msix(vdev, queue_idx++, callbacks[i],
>> +					     names[i], ctx ? ctx[i] : false,
>> +					     &allocated_vectors);
>>  		if (IS_ERR(vqs[i])) {
>>  			err = PTR_ERR(vqs[i]);
>>  			goto error_find;
>>  		}
>> -
>> -		if (!vp_dev->per_vq_vectors || msix_vec == VIRTIO_MSI_NO_VECTOR)
>> -			continue;
>> -
>> -		/* allocate per-vq irq if available and necessary */
>> -		snprintf(vp_dev->msix_names[msix_vec],
>> -			 sizeof *vp_dev->msix_names,
>> -			 "%s-%s",
>> -			 dev_name(&vp_dev->vdev.dev), names[i]);
>> -		err = request_irq(pci_irq_vector(vp_dev->pci_dev, msix_vec),
>> -				  vring_interrupt, 0,
>> -				  vp_dev->msix_names[msix_vec],
>> -				  vqs[i]);
>> -		if (err) {
>> -			vp_del_vq(vqs[i]);
>> -			goto error_find;
>> -		}
>>  	}
>>  	return 0;
>>  
>> -- 
>> 2.45.1
>> 
>> 

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 03:09:20PM +0200, Jiri Pirko wrote:
> Mon, Jun 24, 2024 at 01:07:23PM CEST, mst@redhat.com wrote:
> >On Mon, Jun 24, 2024 at 11:04:43AM +0200, Jiri Pirko wrote:
> >> From: Jiri Pirko <jiri@nvidia.com>
> >> 
> >> Currently the admin queue command execution is serialized by a lock.
> >> This patchsets lifts this limitation allowing to execute admin queue
> >> commands in parallel. To do that, admin queue processing needs to be
> >> converted from polling to interrupt based completion.
> >> 
> >> Patches #1-#6 are preparations, making things a bit smoother as well.
> >> Patch #7 implements interrupt based completion for admin queue.
> >> Patch #8 finally removes the admin queue serialization lock.
> >
> >Okay ... I assume this is infrastructure for some other need?
> >I'll review, but I'd like to see this actually used.
> 
> No other need. The currently, admin queue cmds are serialized. Note that
> admin queue is currently created for PF. If you have for example
> multiple VFs in legacy mode reading and writing pci bar, that virtio
> vfio driver translates it into admin queue commands. So for example
> probe on multiple VFs gets serialized. This patchset makes it possible
> to run in parallel.


Well probe is mostly serialized but I think I get what you mean -
I am guessing you mean something else like legacy access commands.
> 
> >
> >
> >> Jiri Pirko (8):
> >>   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> >>   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> >>   virtio: push out code to vp_avq_index()
> >>   virtio: create admin queues alongside other virtqueues
> >>   virtio_pci_modern: create admin queue of queried size
> >>   virtio_pci_modern: pass cmd as an identification token
> >>   virtio_pci_modern: use completion instead of busy loop to wait on
> >>     admin cmd result
> >>   virtio_pci_modern: remove admin queue serialization lock
> >> 
> >>  drivers/virtio/virtio.c            |  28 +----
> >>  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> >>  drivers/virtio/virtio_pci_common.h |   9 +-
> >>  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> >>  include/linux/virtio.h             |   2 +
> >>  include/linux/virtio_config.h      |   2 -
> >>  6 files changed, 150 insertions(+), 160 deletions(-)
> >> 
> >> -- 
> >> 2.45.1
> >

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 03:16:06PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 03:09:20PM +0200, Jiri Pirko wrote:
>> Mon, Jun 24, 2024 at 01:07:23PM CEST, mst@redhat.com wrote:
>> >On Mon, Jun 24, 2024 at 11:04:43AM +0200, Jiri Pirko wrote:
>> >> From: Jiri Pirko <jiri@nvidia.com>
>> >> 
>> >> Currently the admin queue command execution is serialized by a lock.
>> >> This patchsets lifts this limitation allowing to execute admin queue
>> >> commands in parallel. To do that, admin queue processing needs to be
>> >> converted from polling to interrupt based completion.
>> >> 
>> >> Patches #1-#6 are preparations, making things a bit smoother as well.
>> >> Patch #7 implements interrupt based completion for admin queue.
>> >> Patch #8 finally removes the admin queue serialization lock.
>> >
>> >Okay ... I assume this is infrastructure for some other need?
>> >I'll review, but I'd like to see this actually used.
>> 
>> No other need. The currently, admin queue cmds are serialized. Note that
>> admin queue is currently created for PF. If you have for example
>> multiple VFs in legacy mode reading and writing pci bar, that virtio
>> vfio driver translates it into admin queue commands. So for example
>> probe on multiple VFs gets serialized. This patchset makes it possible
>> to run in parallel.
>
>
>Well probe is mostly serialized but I think I get what you mean -

I mean probe of multiple VFs that belong under one PF.


>I am guessing you mean something else like legacy access commands.

Yes. For transitional VF, the legacy bar0 access is translated by
virtio vfio driver to admin commands. See virtiovf_pci_bar0_rw().


>> 
>> >
>> >
>> >> Jiri Pirko (8):
>> >>   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> >>   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> >>   virtio: push out code to vp_avq_index()
>> >>   virtio: create admin queues alongside other virtqueues
>> >>   virtio_pci_modern: create admin queue of queried size
>> >>   virtio_pci_modern: pass cmd as an identification token
>> >>   virtio_pci_modern: use completion instead of busy loop to wait on
>> >>     admin cmd result
>> >>   virtio_pci_modern: remove admin queue serialization lock
>> >> 
>> >>  drivers/virtio/virtio.c            |  28 +----
>> >>  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> >>  drivers/virtio/virtio_pci_common.h |   9 +-
>> >>  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> >>  include/linux/virtio.h             |   2 +
>> >>  include/linux/virtio_config.h      |   2 -
>> >>  6 files changed, 150 insertions(+), 160 deletions(-)
>> >> 
>> >> -- 
>> >> 2.45.1
>> >
>

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
>> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> > From: Jiri Pirko <jiri@nvidia.com>
>> > 
>> > Currently the admin queue command execution is serialized by a lock.
>> > This patchsets lifts this limitation allowing to execute admin queue
>> > commands in parallel. To do that, admin queue processing needs to be
>> > converted from polling to interrupt based completion.
>> > 
>> > Patches #1-#6 are preparations, making things a bit smoother as well.
>> > Patch #7 implements interrupt based completion for admin queue.
>> 
>> Hi, Jiri
>> 
>> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
>> fact that the newly added irq vector may cause the IO queue to fall back to
>> shared interrupt mode.
>> But it is true that devices implemented according to the specification should
>> not encounter this problem. So what do you think?

Wait. Please note that admin queue is only created and used by PF virtio
device. And most probably, this is on hypervisor managing the VFs that
are passed to guest VMs. These VFs does not have admin queue.

Therefore, this is hardly comparable to control vq.


>> 
>> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
>
>It's true - this can cause guest to run out of vectors for a variety of
>reasons.
>
>First we have guest irqs - I am guessing avq could use IRQF_SHARED ?

There is no avq in quest, under normal circumstances. Unless for some
reason somebody passes trough virtio PF into guest.


>I am not sure why we don't allow IRQF_SHARED for the config
>interrupt though. So I think addressing this part can be deferred.
>
>Second, we might not have enough msix vectors on the device. Here sharing
>with e.g. cvq and further with config interrupt would make sense.

For cvq irq vector, I believe that sharing with config irq makes sense.
Even for admin queue maybe. But again, admin queue is on PF. I don't
think this is a real concern.


>
>Jiri do you think you can help Heng Qi hammer out a solution for cvq?
>I feel this will work will then benefit in a similar way,
>and having us poll aggressively for cvq but not admin commands
>does not make much sense, right?
>
>> > Patch #8 finally removes the admin queue serialization lock.
>> > 
>> > Jiri Pirko (8):
>> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> >   virtio: push out code to vp_avq_index()
>> >   virtio: create admin queues alongside other virtqueues
>> >   virtio_pci_modern: create admin queue of queried size
>> >   virtio_pci_modern: pass cmd as an identification token
>> >   virtio_pci_modern: use completion instead of busy loop to wait on
>> >     admin cmd result
>> >   virtio_pci_modern: remove admin queue serialization lock
>> > 
>> >  drivers/virtio/virtio.c            |  28 +----
>> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> >  drivers/virtio/virtio_pci_common.h |   9 +-
>> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> >  include/linux/virtio.h             |   2 +
>> >  include/linux/virtio_config.h      |   2 -
>> >  6 files changed, 150 insertions(+), 160 deletions(-)
>> > 
>> > -- 
>> > 2.45.1
>> > 
>> > 
>

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> >> > From: Jiri Pirko <jiri@nvidia.com>
> >> > 
> >> > Currently the admin queue command execution is serialized by a lock.
> >> > This patchsets lifts this limitation allowing to execute admin queue
> >> > commands in parallel. To do that, admin queue processing needs to be
> >> > converted from polling to interrupt based completion.
> >> > 
> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
> >> > Patch #7 implements interrupt based completion for admin queue.
> >> 
> >> Hi, Jiri
> >> 
> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> >> fact that the newly added irq vector may cause the IO queue to fall back to
> >> shared interrupt mode.
> >> But it is true that devices implemented according to the specification should
> >> not encounter this problem. So what do you think?
> 
> Wait. Please note that admin queue is only created and used by PF virtio
> device. And most probably, this is on hypervisor managing the VFs that
> are passed to guest VMs. These VFs does not have admin queue.
> 
> Therefore, this is hardly comparable to control vq.


Well Parav recently posted patches adding admin queue
to VFs, with new "self" group type.


> 
> >> 
> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
> >
> >It's true - this can cause guest to run out of vectors for a variety of
> >reasons.
> >
> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
> 
> There is no avq in quest, under normal circumstances. Unless for some
> reason somebody passes trough virtio PF into guest.


At the moment, but this will change soon.


> 
> >I am not sure why we don't allow IRQF_SHARED for the config
> >interrupt though. So I think addressing this part can be deferred.
> >
> >Second, we might not have enough msix vectors on the device. Here sharing
> >with e.g. cvq and further with config interrupt would make sense.
> 
> For cvq irq vector, I believe that sharing with config irq makes sense.
> Even for admin queue maybe. But again, admin queue is on PF. I don't
> think this is a real concern.
> 
> 
> >
> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
> >I feel this will work will then benefit in a similar way,
> >and having us poll aggressively for cvq but not admin commands
> >does not make much sense, right?
> >
> >> > Patch #8 finally removes the admin queue serialization lock.
> >> > 
> >> > Jiri Pirko (8):
> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> >> >   virtio: push out code to vp_avq_index()
> >> >   virtio: create admin queues alongside other virtqueues
> >> >   virtio_pci_modern: create admin queue of queried size
> >> >   virtio_pci_modern: pass cmd as an identification token
> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
> >> >     admin cmd result
> >> >   virtio_pci_modern: remove admin queue serialization lock
> >> > 
> >> >  drivers/virtio/virtio.c            |  28 +----
> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> >> >  include/linux/virtio.h             |   2 +
> >> >  include/linux/virtio_config.h      |   2 -
> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
> >> > 
> >> > -- 
> >> > 2.45.1
> >> > 
> >> > 
> >

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
>> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
>> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
>> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > From: Jiri Pirko <jiri@nvidia.com>
>> >> > 
>> >> > Currently the admin queue command execution is serialized by a lock.
>> >> > This patchsets lifts this limitation allowing to execute admin queue
>> >> > commands in parallel. To do that, admin queue processing needs to be
>> >> > converted from polling to interrupt based completion.
>> >> > 
>> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
>> >> > Patch #7 implements interrupt based completion for admin queue.
>> >> 
>> >> Hi, Jiri
>> >> 
>> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
>> >> fact that the newly added irq vector may cause the IO queue to fall back to
>> >> shared interrupt mode.
>> >> But it is true that devices implemented according to the specification should
>> >> not encounter this problem. So what do you think?
>> 
>> Wait. Please note that admin queue is only created and used by PF virtio
>> device. And most probably, this is on hypervisor managing the VFs that
>> are passed to guest VMs. These VFs does not have admin queue.
>> 
>> Therefore, this is hardly comparable to control vq.
>
>
>Well Parav recently posted patches adding admin queue
>to VFs, with new "self" group type.

Right, but even so, when device implementation decides to implement and
enable admin queue, it should also make sure to provide correct amount
of vectors. My point is, there should not be any breakage in user
expectation, or am I missing something?


>
>
>> 
>> >> 
>> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
>> >
>> >It's true - this can cause guest to run out of vectors for a variety of
>> >reasons.
>> >
>> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
>> 
>> There is no avq in quest, under normal circumstances. Unless for some
>> reason somebody passes trough virtio PF into guest.
>
>
>At the moment, but this will change soon.
>
>
>> 
>> >I am not sure why we don't allow IRQF_SHARED for the config
>> >interrupt though. So I think addressing this part can be deferred.
>> >
>> >Second, we might not have enough msix vectors on the device. Here sharing
>> >with e.g. cvq and further with config interrupt would make sense.
>> 
>> For cvq irq vector, I believe that sharing with config irq makes sense.
>> Even for admin queue maybe. But again, admin queue is on PF. I don't
>> think this is a real concern.
>> 
>> 
>> >
>> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
>> >I feel this will work will then benefit in a similar way,
>> >and having us poll aggressively for cvq but not admin commands
>> >does not make much sense, right?
>> >
>> >> > Patch #8 finally removes the admin queue serialization lock.
>> >> > 
>> >> > Jiri Pirko (8):
>> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> >> >   virtio: push out code to vp_avq_index()
>> >> >   virtio: create admin queues alongside other virtqueues
>> >> >   virtio_pci_modern: create admin queue of queried size
>> >> >   virtio_pci_modern: pass cmd as an identification token
>> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
>> >> >     admin cmd result
>> >> >   virtio_pci_modern: remove admin queue serialization lock
>> >> > 
>> >> >  drivers/virtio/virtio.c            |  28 +----
>> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
>> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> >> >  include/linux/virtio.h             |   2 +
>> >> >  include/linux/virtio_config.h      |   2 -
>> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
>> >> > 
>> >> > -- 
>> >> > 2.45.1
>> >> > 
>> >> > 
>> >
>

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
>> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> > From: Jiri Pirko <jiri@nvidia.com>
>> > 
>> > Currently the admin queue command execution is serialized by a lock.
>> > This patchsets lifts this limitation allowing to execute admin queue
>> > commands in parallel. To do that, admin queue processing needs to be
>> > converted from polling to interrupt based completion.
>> > 
>> > Patches #1-#6 are preparations, making things a bit smoother as well.
>> > Patch #7 implements interrupt based completion for admin queue.
>> 
>> Hi, Jiri
>> 
>> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
>> fact that the newly added irq vector may cause the IO queue to fall back to
>> shared interrupt mode.
>> But it is true that devices implemented according to the specification should
>> not encounter this problem. So what do you think?
>> 
>> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
>
>It's true - this can cause guest to run out of vectors for a variety of
>reasons.
>
>First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
>I am not sure why we don't allow IRQF_SHARED for the config
>interrupt though. So I think addressing this part can be deferred.
>
>Second, we might not have enough msix vectors on the device. Here sharing
>with e.g. cvq and further with config interrupt would make sense.
>
>Jiri do you think you can help Heng Qi hammer out a solution for cvq?
>I feel this will work will then benefit in a similar way,
>and having us poll aggressively for cvq but not admin commands
>does not make much sense, right?


How about to enhance existing fallback in vp_find_vqs() to something
like this:

        /* Try MSI-X with one vector per queue. */
        err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_VQ, ctx, desc);
        if (!err)
                return 0;
        /* Try MSI-X with one vector per data queue, share one for the
         * rest of the queues and config. */
        err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_DATA_VQ_ONE_FOR_REST, 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, ONE_DATA_VQS_ONE_FOR_REST, ctx, desc);
        if (!err)
                return 0;

I can implement this for admin queue, control queue can benefit from
this eventually.



>
>> > Patch #8 finally removes the admin queue serialization lock.
>> > 
>> > Jiri Pirko (8):
>> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> >   virtio: push out code to vp_avq_index()
>> >   virtio: create admin queues alongside other virtqueues
>> >   virtio_pci_modern: create admin queue of queried size
>> >   virtio_pci_modern: pass cmd as an identification token
>> >   virtio_pci_modern: use completion instead of busy loop to wait on
>> >     admin cmd result
>> >   virtio_pci_modern: remove admin queue serialization lock
>> > 
>> >  drivers/virtio/virtio.c            |  28 +----
>> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> >  drivers/virtio/virtio_pci_common.h |   9 +-
>> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> >  include/linux/virtio.h             |   2 +
>> >  include/linux/virtio_config.h      |   2 -
>> >  6 files changed, 150 insertions(+), 160 deletions(-)
>> > 
>> > -- 
>> > 2.45.1
>> > 
>> > 
>

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 04:51:37PM +0200, Jiri Pirko wrote:
> Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
> >On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
> >> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> >> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> >> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> >> >> > From: Jiri Pirko <jiri@nvidia.com>
> >> >> > 
> >> >> > Currently the admin queue command execution is serialized by a lock.
> >> >> > This patchsets lifts this limitation allowing to execute admin queue
> >> >> > commands in parallel. To do that, admin queue processing needs to be
> >> >> > converted from polling to interrupt based completion.
> >> >> > 
> >> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
> >> >> > Patch #7 implements interrupt based completion for admin queue.
> >> >> 
> >> >> Hi, Jiri
> >> >> 
> >> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> >> >> fact that the newly added irq vector may cause the IO queue to fall back to
> >> >> shared interrupt mode.
> >> >> But it is true that devices implemented according to the specification should
> >> >> not encounter this problem. So what do you think?
> >> 
> >> Wait. Please note that admin queue is only created and used by PF virtio
> >> device. And most probably, this is on hypervisor managing the VFs that
> >> are passed to guest VMs. These VFs does not have admin queue.
> >> 
> >> Therefore, this is hardly comparable to control vq.
> >
> >
> >Well Parav recently posted patches adding admin queue
> >to VFs, with new "self" group type.
> 
> Right, but even so, when device implementation decides to implement and
> enable admin queue, it should also make sure to provide correct amount
> of vectors. My point is, there should not be any breakage in user
> expectation, or am I missing something?


Hmm, I think you are right that cvq is an existing capability
and adminq is newer.

Gimme a couple of days to think all this over, hopefully we'll also see
a new version of the cvq patch, making it easier to see whether they
interact and if so, how.



> 
> >
> >
> >> 
> >> >> 
> >> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
> >> >
> >> >It's true - this can cause guest to run out of vectors for a variety of
> >> >reasons.
> >> >
> >> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
> >> 
> >> There is no avq in quest, under normal circumstances. Unless for some
> >> reason somebody passes trough virtio PF into guest.
> >
> >
> >At the moment, but this will change soon.
> >
> >
> >> 
> >> >I am not sure why we don't allow IRQF_SHARED for the config
> >> >interrupt though. So I think addressing this part can be deferred.
> >> >
> >> >Second, we might not have enough msix vectors on the device. Here sharing
> >> >with e.g. cvq and further with config interrupt would make sense.
> >> 
> >> For cvq irq vector, I believe that sharing with config irq makes sense.
> >> Even for admin queue maybe. But again, admin queue is on PF. I don't
> >> think this is a real concern.
> >> 
> >> 
> >> >
> >> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
> >> >I feel this will work will then benefit in a similar way,
> >> >and having us poll aggressively for cvq but not admin commands
> >> >does not make much sense, right?
> >> >
> >> >> > Patch #8 finally removes the admin queue serialization lock.
> >> >> > 
> >> >> > Jiri Pirko (8):
> >> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> >> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> >> >> >   virtio: push out code to vp_avq_index()
> >> >> >   virtio: create admin queues alongside other virtqueues
> >> >> >   virtio_pci_modern: create admin queue of queried size
> >> >> >   virtio_pci_modern: pass cmd as an identification token
> >> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
> >> >> >     admin cmd result
> >> >> >   virtio_pci_modern: remove admin queue serialization lock
> >> >> > 
> >> >> >  drivers/virtio/virtio.c            |  28 +----
> >> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> >> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
> >> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> >> >> >  include/linux/virtio.h             |   2 +
> >> >> >  include/linux/virtio_config.h      |   2 -
> >> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
> >> >> > 
> >> >> > -- 
> >> >> > 2.45.1
> >> >> > 
> >> >> > 
> >> >
> >

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Mon, Jun 24, 2024 at 05:10:36PM +0200, Jiri Pirko wrote:
> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> >> > From: Jiri Pirko <jiri@nvidia.com>
> >> > 
> >> > Currently the admin queue command execution is serialized by a lock.
> >> > This patchsets lifts this limitation allowing to execute admin queue
> >> > commands in parallel. To do that, admin queue processing needs to be
> >> > converted from polling to interrupt based completion.
> >> > 
> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
> >> > Patch #7 implements interrupt based completion for admin queue.
> >> 
> >> Hi, Jiri
> >> 
> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> >> fact that the newly added irq vector may cause the IO queue to fall back to
> >> shared interrupt mode.
> >> But it is true that devices implemented according to the specification should
> >> not encounter this problem. So what do you think?
> >> 
> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
> >
> >It's true - this can cause guest to run out of vectors for a variety of
> >reasons.
> >
> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
> >I am not sure why we don't allow IRQF_SHARED for the config
> >interrupt though. So I think addressing this part can be deferred.
> >
> >Second, we might not have enough msix vectors on the device. Here sharing
> >with e.g. cvq and further with config interrupt would make sense.
> >
> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
> >I feel this will work will then benefit in a similar way,
> >and having us poll aggressively for cvq but not admin commands
> >does not make much sense, right?
> 
> 
> How about to enhance existing fallback in vp_find_vqs() to something
> like this:
> 
>         /* Try MSI-X with one vector per queue. */
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_VQ, ctx, desc);
>         if (!err)
>                 return 0;
>         /* Try MSI-X with one vector per data queue, share one for the
>          * rest of the queues and config. */
>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_DATA_VQ_ONE_FOR_REST, 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, ONE_DATA_VQS_ONE_FOR_REST, ctx, desc);
>         if (!err)
>                 return 0;
> 
> I can implement this for admin queue, control queue can benefit from
> this eventually.
> 

OKay superficially except in attempt 3 I would still combine
adminq with config, for consistency.

> 
> >
> >> > Patch #8 finally removes the admin queue serialization lock.
> >> > 
> >> > Jiri Pirko (8):
> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> >> >   virtio: push out code to vp_avq_index()
> >> >   virtio: create admin queues alongside other virtqueues
> >> >   virtio_pci_modern: create admin queue of queried size
> >> >   virtio_pci_modern: pass cmd as an identification token
> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
> >> >     admin cmd result
> >> >   virtio_pci_modern: remove admin queue serialization lock
> >> > 
> >> >  drivers/virtio/virtio.c            |  28 +----
> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> >> >  include/linux/virtio.h             |   2 +
> >> >  include/linux/virtio_config.h      |   2 -
> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
> >> > 
> >> > -- 
> >> > 2.45.1
> >> > 
> >> > 
> >

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Mon, Jun 24, 2024 at 05:23:56PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 05:10:36PM +0200, Jiri Pirko wrote:
>> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
>> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
>> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> >> > From: Jiri Pirko <jiri@nvidia.com>
>> >> > 
>> >> > Currently the admin queue command execution is serialized by a lock.
>> >> > This patchsets lifts this limitation allowing to execute admin queue
>> >> > commands in parallel. To do that, admin queue processing needs to be
>> >> > converted from polling to interrupt based completion.
>> >> > 
>> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
>> >> > Patch #7 implements interrupt based completion for admin queue.
>> >> 
>> >> Hi, Jiri
>> >> 
>> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
>> >> fact that the newly added irq vector may cause the IO queue to fall back to
>> >> shared interrupt mode.
>> >> But it is true that devices implemented according to the specification should
>> >> not encounter this problem. So what do you think?
>> >> 
>> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
>> >
>> >It's true - this can cause guest to run out of vectors for a variety of
>> >reasons.
>> >
>> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
>> >I am not sure why we don't allow IRQF_SHARED for the config
>> >interrupt though. So I think addressing this part can be deferred.
>> >
>> >Second, we might not have enough msix vectors on the device. Here sharing
>> >with e.g. cvq and further with config interrupt would make sense.
>> >
>> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
>> >I feel this will work will then benefit in a similar way,
>> >and having us poll aggressively for cvq but not admin commands
>> >does not make much sense, right?
>> 
>> 
>> How about to enhance existing fallback in vp_find_vqs() to something
>> like this:
>> 
>>         /* Try MSI-X with one vector per queue. */
>>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_VQ, ctx, desc);
>>         if (!err)
>>                 return 0;
>>         /* Try MSI-X with one vector per data queue, share one for the
>>          * rest of the queues and config. */
>>         err = vp_find_vqs_msix(vdev, nvqs, vqs, callbacks, names, ONE_FOR_EACH_DATA_VQ_ONE_FOR_REST, 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, ONE_DATA_VQS_ONE_FOR_REST, ctx, desc);
>>         if (!err)
>>                 return 0;
>> 
>> I can implement this for admin queue, control queue can benefit from
>> this eventually.
>> 
>
>OKay superficially except in attempt 3 I would still combine
>adminq with config, for consistency.

Yeah, I thought about that when I wrote that. Makes sense.
I'm on it.


>
>> 
>> >
>> >> > Patch #8 finally removes the admin queue serialization lock.
>> >> > 
>> >> > Jiri Pirko (8):
>> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> >> >   virtio: push out code to vp_avq_index()
>> >> >   virtio: create admin queues alongside other virtqueues
>> >> >   virtio_pci_modern: create admin queue of queried size
>> >> >   virtio_pci_modern: pass cmd as an identification token
>> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
>> >> >     admin cmd result
>> >> >   virtio_pci_modern: remove admin queue serialization lock
>> >> > 
>> >> >  drivers/virtio/virtio.c            |  28 +----
>> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
>> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> >> >  include/linux/virtio.h             |   2 +
>> >> >  include/linux/virtio_config.h      |   2 -
>> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
>> >> > 
>> >> > -- 
>> >> > 2.45.1
>> >> > 
>> >> > 
>> >
>

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Heng Qi]

On Mon, 24 Jun 2024 11:16:45 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Jun 24, 2024 at 04:51:37PM +0200, Jiri Pirko wrote:
> > Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
> > >On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
> > >> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> > >> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> > >> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> > >> >> > From: Jiri Pirko <jiri@nvidia.com>
> > >> >> > 
> > >> >> > Currently the admin queue command execution is serialized by a lock.
> > >> >> > This patchsets lifts this limitation allowing to execute admin queue
> > >> >> > commands in parallel. To do that, admin queue processing needs to be
> > >> >> > converted from polling to interrupt based completion.
> > >> >> > 
> > >> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
> > >> >> > Patch #7 implements interrupt based completion for admin queue.
> > >> >> 
> > >> >> Hi, Jiri
> > >> >> 
> > >> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> > >> >> fact that the newly added irq vector may cause the IO queue to fall back to
> > >> >> shared interrupt mode.
> > >> >> But it is true that devices implemented according to the specification should
> > >> >> not encounter this problem. So what do you think?
> > >> 
> > >> Wait. Please note that admin queue is only created and used by PF virtio
> > >> device. And most probably, this is on hypervisor managing the VFs that
> > >> are passed to guest VMs. These VFs does not have admin queue.
> > >> 
> > >> Therefore, this is hardly comparable to control vq.
> > >
> > >
> > >Well Parav recently posted patches adding admin queue
> > >to VFs, with new "self" group type.
> > 
> > Right, but even so, when device implementation decides to implement and
> > enable admin queue, it should also make sure to provide correct amount
> > of vectors. My point is, there should not be any breakage in user
> > expectation, or am I missing something?
> 
> 
> Hmm, I think you are right that cvq is an existing capability
> and adminq is newer.

admin vq has been supported in the kernel for more than half a year, and if at
this point you think that the device must provide interrupt vectors for it, then
I think this is also true for cvq.

> 
> Gimme a couple of days to think all this over, hopefully we'll also see
> a new version of the cvq patch, making it easier to see whether they
> interact and if so, how.
> 
> 
> 
> > 
> > >
> > >
> > >> 
> > >> >> 
> > >> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
> > >> >
> > >> >It's true - this can cause guest to run out of vectors for a variety of
> > >> >reasons.
> > >> >
> > >> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
> > >> 
> > >> There is no avq in quest, under normal circumstances. Unless for some
> > >> reason somebody passes trough virtio PF into guest.
> > >
> > >
> > >At the moment, but this will change soon.
> > >
> > >
> > >> 
> > >> >I am not sure why we don't allow IRQF_SHARED for the config
> > >> >interrupt though. So I think addressing this part can be deferred.
> > >> >
> > >> >Second, we might not have enough msix vectors on the device. Here sharing
> > >> >with e.g. cvq and further with config interrupt would make sense.
> > >> 
> > >> For cvq irq vector, I believe that sharing with config irq makes sense.
> > >> Even for admin queue maybe. But again, admin queue is on PF. I don't
> > >> think this is a real concern.
> > >> 
> > >> 
> > >> >
> > >> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
> > >> >I feel this will work will then benefit in a similar way,
> > >> >and having us poll aggressively for cvq but not admin commands
> > >> >does not make much sense, right?
> > >> >
> > >> >> > Patch #8 finally removes the admin queue serialization lock.
> > >> >> > 
> > >> >> > Jiri Pirko (8):
> > >> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> > >> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> > >> >> >   virtio: push out code to vp_avq_index()
> > >> >> >   virtio: create admin queues alongside other virtqueues
> > >> >> >   virtio_pci_modern: create admin queue of queried size
> > >> >> >   virtio_pci_modern: pass cmd as an identification token
> > >> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
> > >> >> >     admin cmd result
> > >> >> >   virtio_pci_modern: remove admin queue serialization lock
> > >> >> > 
> > >> >> >  drivers/virtio/virtio.c            |  28 +----
> > >> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> > >> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
> > >> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> > >> >> >  include/linux/virtio.h             |   2 +
> > >> >> >  include/linux/virtio_config.h      |   2 -
> > >> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
> > >> >> > 
> > >> >> > -- 
> > >> >> > 2.45.1
> > >> >> > 
> > >> >> > 
> > >> >
> > >
> 

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Tue, Jun 25, 2024 at 04:11:26AM CEST, hengqi@linux.alibaba.com wrote:
>On Mon, 24 Jun 2024 11:16:45 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
>> On Mon, Jun 24, 2024 at 04:51:37PM +0200, Jiri Pirko wrote:
>> > Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
>> > >On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
>> > >> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
>> > >> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
>> > >> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
>> > >> >> > From: Jiri Pirko <jiri@nvidia.com>
>> > >> >> > 
>> > >> >> > Currently the admin queue command execution is serialized by a lock.
>> > >> >> > This patchsets lifts this limitation allowing to execute admin queue
>> > >> >> > commands in parallel. To do that, admin queue processing needs to be
>> > >> >> > converted from polling to interrupt based completion.
>> > >> >> > 
>> > >> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
>> > >> >> > Patch #7 implements interrupt based completion for admin queue.
>> > >> >> 
>> > >> >> Hi, Jiri
>> > >> >> 
>> > >> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
>> > >> >> fact that the newly added irq vector may cause the IO queue to fall back to
>> > >> >> shared interrupt mode.
>> > >> >> But it is true that devices implemented according to the specification should
>> > >> >> not encounter this problem. So what do you think?
>> > >> 
>> > >> Wait. Please note that admin queue is only created and used by PF virtio
>> > >> device. And most probably, this is on hypervisor managing the VFs that
>> > >> are passed to guest VMs. These VFs does not have admin queue.
>> > >> 
>> > >> Therefore, this is hardly comparable to control vq.
>> > >
>> > >
>> > >Well Parav recently posted patches adding admin queue
>> > >to VFs, with new "self" group type.
>> > 
>> > Right, but even so, when device implementation decides to implement and
>> > enable admin queue, it should also make sure to provide correct amount
>> > of vectors. My point is, there should not be any breakage in user
>> > expectation, or am I missing something?
>> 
>> 
>> Hmm, I think you are right that cvq is an existing capability
>> and adminq is newer.
>
>admin vq has been supported in the kernel for more than half a year, and if at

On PF only, so far.

>this point you think that the device must provide interrupt vectors for it, then
>I think this is also true for cvq.

I'm working on a fallback where admin queue and cvq would share config
vector. Lets see.

>
>> 
>> Gimme a couple of days to think all this over, hopefully we'll also see
>> a new version of the cvq patch, making it easier to see whether they
>> interact and if so, how.
>> 
>> 
>> 
>> > 
>> > >
>> > >
>> > >> 
>> > >> >> 
>> > >> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
>> > >> >
>> > >> >It's true - this can cause guest to run out of vectors for a variety of
>> > >> >reasons.
>> > >> >
>> > >> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
>> > >> 
>> > >> There is no avq in quest, under normal circumstances. Unless for some
>> > >> reason somebody passes trough virtio PF into guest.
>> > >
>> > >
>> > >At the moment, but this will change soon.
>> > >
>> > >
>> > >> 
>> > >> >I am not sure why we don't allow IRQF_SHARED for the config
>> > >> >interrupt though. So I think addressing this part can be deferred.
>> > >> >
>> > >> >Second, we might not have enough msix vectors on the device. Here sharing
>> > >> >with e.g. cvq and further with config interrupt would make sense.
>> > >> 
>> > >> For cvq irq vector, I believe that sharing with config irq makes sense.
>> > >> Even for admin queue maybe. But again, admin queue is on PF. I don't
>> > >> think this is a real concern.
>> > >> 
>> > >> 
>> > >> >
>> > >> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
>> > >> >I feel this will work will then benefit in a similar way,
>> > >> >and having us poll aggressively for cvq but not admin commands
>> > >> >does not make much sense, right?
>> > >> >
>> > >> >> > Patch #8 finally removes the admin queue serialization lock.
>> > >> >> > 
>> > >> >> > Jiri Pirko (8):
>> > >> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
>> > >> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
>> > >> >> >   virtio: push out code to vp_avq_index()
>> > >> >> >   virtio: create admin queues alongside other virtqueues
>> > >> >> >   virtio_pci_modern: create admin queue of queried size
>> > >> >> >   virtio_pci_modern: pass cmd as an identification token
>> > >> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
>> > >> >> >     admin cmd result
>> > >> >> >   virtio_pci_modern: remove admin queue serialization lock
>> > >> >> > 
>> > >> >> >  drivers/virtio/virtio.c            |  28 +----
>> > >> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
>> > >> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
>> > >> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
>> > >> >> >  include/linux/virtio.h             |   2 +
>> > >> >> >  include/linux/virtio_config.h      |   2 -
>> > >> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
>> > >> >> > 
>> > >> >> > -- 
>> > >> >> > 2.45.1
>> > >> >> > 
>> > >> >> > 
>> > >> >
>> > >
>> 

RE: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Parav Pandit]

> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Tuesday, June 25, 2024 12:08 PM

> I'm working on a fallback where admin queue and cvq would share config
> vector. Lets see.

Cvq or avq resulting in performing more MMIO reads of the config space is not good.
CVQ and AVQ sharing irq is good, but it should not share with config change interrupt.

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 10:11:26AM +0800, Heng Qi wrote:
> On Mon, 24 Jun 2024 11:16:45 -0400, "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > On Mon, Jun 24, 2024 at 04:51:37PM +0200, Jiri Pirko wrote:
> > > Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
> > > >On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
> > > >> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> > > >> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> > > >> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us> wrote:
> > > >> >> > From: Jiri Pirko <jiri@nvidia.com>
> > > >> >> > 
> > > >> >> > Currently the admin queue command execution is serialized by a lock.
> > > >> >> > This patchsets lifts this limitation allowing to execute admin queue
> > > >> >> > commands in parallel. To do that, admin queue processing needs to be
> > > >> >> > converted from polling to interrupt based completion.
> > > >> >> > 
> > > >> >> > Patches #1-#6 are preparations, making things a bit smoother as well.
> > > >> >> > Patch #7 implements interrupt based completion for admin queue.
> > > >> >> 
> > > >> >> Hi, Jiri
> > > >> >> 
> > > >> >> Before this set, I pushed the cvq irq set [1], and the discussion focused on the
> > > >> >> fact that the newly added irq vector may cause the IO queue to fall back to
> > > >> >> shared interrupt mode.
> > > >> >> But it is true that devices implemented according to the specification should
> > > >> >> not encounter this problem. So what do you think?
> > > >> 
> > > >> Wait. Please note that admin queue is only created and used by PF virtio
> > > >> device. And most probably, this is on hypervisor managing the VFs that
> > > >> are passed to guest VMs. These VFs does not have admin queue.
> > > >> 
> > > >> Therefore, this is hardly comparable to control vq.
> > > >
> > > >
> > > >Well Parav recently posted patches adding admin queue
> > > >to VFs, with new "self" group type.
> > > 
> > > Right, but even so, when device implementation decides to implement and
> > > enable admin queue, it should also make sure to provide correct amount
> > > of vectors. My point is, there should not be any breakage in user
> > > expectation, or am I missing something?
> > 
> > 
> > Hmm, I think you are right that cvq is an existing capability
> > and adminq is newer.
> 
> admin vq has been supported in the kernel for more than half a year, and if at
> this point you think that the device must provide interrupt vectors for it, then
> I think this is also true for cvq.

There's a big difference between 15 years and half a year.  I don't
think any hardware devices have been released with the capability just
yet, and yes allocating a vector for each queue is a good idea for
hardware devices.


> > 
> > Gimme a couple of days to think all this over, hopefully we'll also see
> > a new version of the cvq patch, making it easier to see whether they
> > interact and if so, how.
> > 
> > 
> > 
> > > 
> > > >
> > > >
> > > >> 
> > > >> >> 
> > > >> >> [1] https://lore.kernel.org/all/20240619171708-mutt-send-email-mst@kernel.org/
> > > >> >
> > > >> >It's true - this can cause guest to run out of vectors for a variety of
> > > >> >reasons.
> > > >> >
> > > >> >First we have guest irqs - I am guessing avq could use IRQF_SHARED ?
> > > >> 
> > > >> There is no avq in quest, under normal circumstances. Unless for some
> > > >> reason somebody passes trough virtio PF into guest.
> > > >
> > > >
> > > >At the moment, but this will change soon.
> > > >
> > > >
> > > >> 
> > > >> >I am not sure why we don't allow IRQF_SHARED for the config
> > > >> >interrupt though. So I think addressing this part can be deferred.
> > > >> >
> > > >> >Second, we might not have enough msix vectors on the device. Here sharing
> > > >> >with e.g. cvq and further with config interrupt would make sense.
> > > >> 
> > > >> For cvq irq vector, I believe that sharing with config irq makes sense.
> > > >> Even for admin queue maybe. But again, admin queue is on PF. I don't
> > > >> think this is a real concern.
> > > >> 
> > > >> 
> > > >> >
> > > >> >Jiri do you think you can help Heng Qi hammer out a solution for cvq?
> > > >> >I feel this will work will then benefit in a similar way,
> > > >> >and having us poll aggressively for cvq but not admin commands
> > > >> >does not make much sense, right?
> > > >> >
> > > >> >> > Patch #8 finally removes the admin queue serialization lock.
> > > >> >> > 
> > > >> >> > Jiri Pirko (8):
> > > >> >> >   virtio_pci: push out single vq find code to vp_find_one_vq_msix()
> > > >> >> >   virtio_pci_modern: treat vp_dev->admin_vq.info.vq pointer as static
> > > >> >> >   virtio: push out code to vp_avq_index()
> > > >> >> >   virtio: create admin queues alongside other virtqueues
> > > >> >> >   virtio_pci_modern: create admin queue of queried size
> > > >> >> >   virtio_pci_modern: pass cmd as an identification token
> > > >> >> >   virtio_pci_modern: use completion instead of busy loop to wait on
> > > >> >> >     admin cmd result
> > > >> >> >   virtio_pci_modern: remove admin queue serialization lock
> > > >> >> > 
> > > >> >> >  drivers/virtio/virtio.c            |  28 +----
> > > >> >> >  drivers/virtio/virtio_pci_common.c | 109 ++++++++++++++------
> > > >> >> >  drivers/virtio/virtio_pci_common.h |   9 +-
> > > >> >> >  drivers/virtio/virtio_pci_modern.c | 160 ++++++++++++-----------------
> > > >> >> >  include/linux/virtio.h             |   2 +
> > > >> >> >  include/linux/virtio_config.h      |   2 -
> > > >> >> >  6 files changed, 150 insertions(+), 160 deletions(-)
> > > >> >> > 
> > > >> >> > -- 
> > > >> >> > 2.45.1
> > > >> >> > 
> > > >> >> > 
> > > >> >
> > > >
> > 

RE: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Parav Pandit]

> From: Michael S. Tsirkin <mst@redhat.com>
> Sent: Tuesday, June 25, 2024 12:51 PM
> 
> On Tue, Jun 25, 2024 at 10:11:26AM +0800, Heng Qi wrote:
> > On Mon, 24 Jun 2024 11:16:45 -0400, "Michael S. Tsirkin"
> <mst@redhat.com> wrote:
> > > On Mon, Jun 24, 2024 at 04:51:37PM +0200, Jiri Pirko wrote:
> > > > Mon, Jun 24, 2024 at 03:55:53PM CEST, mst@redhat.com wrote:
> > > > >On Mon, Jun 24, 2024 at 03:46:19PM +0200, Jiri Pirko wrote:
> > > > >> Mon, Jun 24, 2024 at 01:23:01PM CEST, mst@redhat.com wrote:
> > > > >> >On Mon, Jun 24, 2024 at 05:53:52PM +0800, Heng Qi wrote:
> > > > >> >> On Mon, 24 Jun 2024 11:04:43 +0200, Jiri Pirko <jiri@resnulli.us>
> wrote:
> > > > >> >> > From: Jiri Pirko <jiri@nvidia.com>
> > > > >> >> >
> > > > >> >> > Currently the admin queue command execution is serialized by a
> lock.
> > > > >> >> > This patchsets lifts this limitation allowing to execute
> > > > >> >> > admin queue commands in parallel. To do that, admin queue
> > > > >> >> > processing needs to be converted from polling to interrupt based
> completion.
> > > > >> >> >
> > > > >> >> > Patches #1-#6 are preparations, making things a bit smoother as
> well.
> > > > >> >> > Patch #7 implements interrupt based completion for admin
> queue.
> > > > >> >>
> > > > >> >> Hi, Jiri
> > > > >> >>
> > > > >> >> Before this set, I pushed the cvq irq set [1], and the
> > > > >> >> discussion focused on the fact that the newly added irq
> > > > >> >> vector may cause the IO queue to fall back to shared interrupt
> mode.
> > > > >> >> But it is true that devices implemented according to the
> > > > >> >> specification should not encounter this problem. So what do you
> think?
> > > > >>
> > > > >> Wait. Please note that admin queue is only created and used by
> > > > >> PF virtio device. And most probably, this is on hypervisor
> > > > >> managing the VFs that are passed to guest VMs. These VFs does not
> have admin queue.
> > > > >>
> > > > >> Therefore, this is hardly comparable to control vq.
> > > > >
> > > > >
> > > > >Well Parav recently posted patches adding admin queue to VFs,
> > > > >with new "self" group type.
> > > >
> > > > Right, but even so, when device implementation decides to
> > > > implement and enable admin queue, it should also make sure to
> > > > provide correct amount of vectors. My point is, there should not
> > > > be any breakage in user expectation, or am I missing something?
> > >
> > >
> > > Hmm, I think you are right that cvq is an existing capability and
> > > adminq is newer.
> >
> > admin vq has been supported in the kernel for more than half a year,
> > and if at this point you think that the device must provide interrupt
> > vectors for it, then I think this is also true for cvq.
> 
> There's a big difference between 15 years and half a year.  I don't think any
> hardware devices have been released with the capability just yet, and yes
> allocating a vector for each queue is a good idea for hardware devices.

We have released the hw with admin capability set.
And device not supporting the irq for AQ is just inefficient device.
So it should be treated as device bug to be fixed by device fw upgrade per say.
I agree with Michael's point.

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Michael S. Tsirkin]

On Tue, Jun 25, 2024 at 06:41:52AM +0000, Parav Pandit wrote:
> 
> > From: Jiri Pirko <jiri@resnulli.us>
> > Sent: Tuesday, June 25, 2024 12:08 PM
> 
> > I'm working on a fallback where admin queue and cvq would share config
> > vector. Lets see.
> 
> Cvq or avq resulting in performing more MMIO reads of the config space is not good.
> CVQ and AVQ sharing irq is good, but it should not share with config change interrupt.

Well if a hardware vendor doesn't want that, allocate enough vectors
and all is well. Your requirement is harder to implement because
ATM cvq isn't special from virtio core POV.

-- 
MST

Re: [PATCH virtio 0/8] virtio_pci_modern: allow parallel admin queue commands execution

[Jiri Pirko]

Tue, Jun 25, 2024 at 09:29:25AM CEST, mst@redhat.com wrote:
>On Tue, Jun 25, 2024 at 06:41:52AM +0000, Parav Pandit wrote:
>> 
>> > From: Jiri Pirko <jiri@resnulli.us>
>> > Sent: Tuesday, June 25, 2024 12:08 PM
>> 
>> > I'm working on a fallback where admin queue and cvq would share config
>> > vector. Lets see.
>> 
>> Cvq or avq resulting in performing more MMIO reads of the config space is not good.
>> CVQ and AVQ sharing irq is good, but it should not share with config change interrupt.
>
>Well if a hardware vendor doesn't want that, allocate enough vectors
>and all is well. Your requirement is harder to implement because
>ATM cvq isn't special from virtio core POV.

Yeah, it's just a fallback.

>
>-- 
>MST
>

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Jiri Pirko]

Mon, Jun 24, 2024 at 01:30:54PM CEST, mst@redhat.com wrote:
>On Mon, Jun 24, 2024 at 11:04:50AM +0200, Jiri Pirko wrote:
>> @@ -73,21 +91,30 @@ static int virtqueue_exec_admin_cmd(struct virtio_pci_device *vp_dev,
>>  	    !((1ULL << opcode) & admin_vq->supported_cmds))
>>  		return -EOPNOTSUPP;
>>  
>> -	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> -	if (ret < 0)
>> -		return -EIO;
>> -
>> -	if (unlikely(!virtqueue_kick(vq)))
>> -		return -EIO;
>> +	init_completion(&cmd->completion);
>>  
>> -	while (!virtqueue_get_buf(vq, &len) &&
>> -	       !virtqueue_is_broken(vq))
>> -		cpu_relax();
>> +again:
>> +	spin_lock_irqsave(&admin_vq->lock, flags);
>> +	ret = virtqueue_add_sgs(vq, sgs, out_num, in_num, cmd, GFP_KERNEL);
>> +	if (ret < 0) {
>> +		if (ret == -ENOSPC) {
>> +			spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +			cpu_relax();
>> +			goto again;
>> +		}
>> +		goto unlock_err;
>> +	}
>> +	if (WARN_ON_ONCE(!virtqueue_kick(vq)))
>> +		goto unlock_err;
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>>  
>> -	if (virtqueue_is_broken(vq))
>> -		return -EIO;
>> +	wait_for_completion(&cmd->completion);
>>  
>>  	return 0;
>> +
>> +unlock_err:
>> +	spin_unlock_irqrestore(&admin_vq->lock, flags);
>> +	return -EIO;
>>  }
>>  
>
>
>The reason we had the virtqueue_is_broken check previously,
>is because this way surprise removal works: it happens to set
>vq broken flag on all vqs.
>
>
>So if you are changing that to a completion, I think surprise
>removal needs to trigger a callback so the completion
>can be signalled.

What exacly do you mean by "surprise removal"?


>
>I think the cvq work also needs this, BTW.
>
>
>
>
>-- 
>MST
>

RE: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Parav Pandit]

> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Tuesday, June 25, 2024 4:37 PM
[..]

> >So if you are changing that to a completion, I think surprise removal
> >needs to trigger a callback so the completion can be signalled.
> 
> What exacly do you mean by "surprise removal"?

PCI devices can be removed from the PCI bus while the driver is running and with outstanding admin commands.
Pci bus's remove() is called and this needs to notify and complete the outstanding commands with the error ENODEV or similar.

Re: [PATCH virtio 7/8] virtio_pci_modern: use completion instead of busy loop to wait on admin cmd result

[Jiri Pirko]

Tue, Jun 25, 2024 at 02:53:57PM CEST, parav@nvidia.com wrote:
>
>
>> From: Jiri Pirko <jiri@resnulli.us>
>> Sent: Tuesday, June 25, 2024 4:37 PM
>[..]
>
>> >So if you are changing that to a completion, I think surprise removal
>> >needs to trigger a callback so the completion can be signalled.
>> 
>> What exacly do you mean by "surprise removal"?
>
>PCI devices can be removed from the PCI bus while the driver is running and with outstanding admin commands.
>Pci bus's remove() is called and this needs to notify and complete the outstanding commands with the error ENODEV or similar.

Ah, I will need to maintain a list of running cmds and use this in in
this case. Okay. Thanks!