On 10/13/22 03:00, Xia, Chenbo wrote:
-----Original Message-----
From: Pei, Andy <andy....@intel.com>
Sent: Wednesday, October 12, 2022 8:13 PM
To: Xia, Chenbo <chenbo....@intel.com>; dev@dpdk.org
Cc: Xu, Rosen <rosen...@intel.com>; Huang, Wei <wei.hu...@intel.com>; Cao,
Gang <gang....@intel.com>; maxime.coque...@redhat.com
Subject: RE: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any
queue is ready
Hi Chenbo,
Thanks for your reply.
My reply is inline.
-----Original Message-----
From: Xia, Chenbo <chenbo....@intel.com>
Sent: Wednesday, October 12, 2022 5:09 PM
To: Pei, Andy <andy....@intel.com>; dev@dpdk.org
Cc: Xu, Rosen <rosen...@intel.com>; Huang, Wei <wei.hu...@intel.com>;
Cao, Gang <gang....@intel.com>; maxime.coque...@redhat.com
Subject: RE: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any
queue is ready
-----Original Message-----
From: Pei, Andy <andy....@intel.com>
Sent: Friday, September 16, 2022 2:16 PM
To: dev@dpdk.org
Cc: Xia, Chenbo <chenbo....@intel.com>; Xu, Rosen
<rosen...@intel.com>; Huang, Wei <wei.hu...@intel.com>; Cao, Gang
<gang....@intel.com>; maxime.coque...@redhat.com
Subject: [PATCH v3 7/8] vhost: vDPA blk device gets ready when any
queue is ready
When boot from virtio blk device, seabios in QEMU only enables one
queue.
To work in this scenario, vDPA BLK device back-end conf_dev when any
What is conf_dev?
I refer to
/** Driver configure the device (Mandatory) */
int (*dev_conf)(int vid);
So do you think I should use "configure device"?
Yes. It will be better
queue is ready.
Signed-off-by: Andy Pei <andy....@intel.com>
Signed-off-by: Huang Wei <wei.hu...@intel.com>
---
lib/vhost/vhost_user.c | 51
++++++++++++++++++++++++++++++++-------------
-----
1 file changed, 33 insertions(+), 18 deletions(-)
diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index
4ad28ba..9169cf5 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -1449,9 +1449,10 @@
}
#define VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY 2u
+#define VIRTIO_BLK_NUM_VQS_TO_BE_READY 1u
static int
-virtio_is_ready(struct virtio_net *dev)
+virtio_is_ready(struct virtio_net *dev, uint32_t vdpa_type)
{
struct vhost_virtqueue *vq;
uint32_t i, nr_vring = dev->nr_vring; @@ -1462,13 +1463,20 @@
if (!dev->nr_vring)
return 0;
- if (dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET) {
- nr_vring = VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY;
-
- if (dev->nr_vring < nr_vring)
- return 0;
+ if (vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_NET) {
+ if (dev->flags & VIRTIO_DEV_BUILTIN_VIRTIO_NET)
+ nr_vring =
VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY;
+ } else {
+ /*
+ * vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_BLK
+ * is the only case currently
+ */
+ nr_vring = VIRTIO_BLK_NUM_VQS_TO_BE_READY;
You should consider the case when vdpa device is not there. Maybe you
can
use int for vdpa_type, -1 for non-vdpa.
I init vdpa_type to 0;
#define RTE_VHOST_VDPA_DEVICE_TYPE_NET 0
#define RTE_VHOST_VDPA_DEVICE_TYPE_BLK 1
And get_dev_type only return RTE_VHOST_VDPA_DEVICE_TYPE_BLK or
RTE_VHOST_VDPA_DEVICE_TYPE_NET.
I think if when vdpa device is not there, this code runs in the original
way.
Do you think use init vdpa_type to -1 is better?
I was talking about readability, current way will be confusing. So adding
-1 will be better. The check could be if (type == blk) ... else ... as
Type 0/-1 has the same handling.
Also, the vdpa_type can be obtained from dev, so instead of passing the
vdpa_type as argument for the function, it should get fetched directly
in virtio_is_ready().
Maxime
Thanks,
Chenbo
Also note that below check is only needed for some cases.
Yes, I got it. I will fix it in next version.
Thanks,
Chenbo
}
+ if (dev->nr_vring < nr_vring)
+ return 0;
+
for (i = 0; i < nr_vring; i++) {
vq = dev->virtqueue[i];
@@ -3167,7 +3175,25 @@ static int is_vring_iotlb(struct virtio_net
*dev,
if (unlock_required)
vhost_user_unlock_all_queue_pairs(dev);
- if (ret != 0 || !virtio_is_ready(dev))
+ if (ret != 0)
+ goto out;
+
+ vdpa_dev = dev->vdpa_dev;
+ if (vdpa_dev) {
+ if (vdpa_dev->ops->get_dev_type) {
+ ret = vdpa_dev->ops->get_dev_type(vdpa_dev,
&vdpa_type);
+ if (ret) {
+ VHOST_LOG_CONFIG(dev->ifname, ERR,
+ "failed to get vdpa dev type.\n");
+ ret = -1;
+ goto out;
+ }
+ } else {
+ vdpa_type = RTE_VHOST_VDPA_DEVICE_TYPE_NET;
+ }
+ }
+
+ if (!virtio_is_ready(dev, vdpa_type))
goto out;
/*
@@ -3181,20 +3207,9 @@ static int is_vring_iotlb(struct virtio_net
*dev,
dev->flags |= VIRTIO_DEV_RUNNING;
}
- vdpa_dev = dev->vdpa_dev;
if (!vdpa_dev)
goto out;
- if (vdpa_dev->ops->get_dev_type) {
- ret = vdpa_dev->ops->get_dev_type(vdpa_dev, &vdpa_type);
- if (ret) {
- VHOST_LOG_CONFIG(dev->ifname, ERR, "failed to
get vdpa
dev type.\n");
- ret = -1;
- goto out;
- }
- } else {
- vdpa_type = RTE_VHOST_VDPA_DEVICE_TYPE_NET;
- }
if (vdpa_type == RTE_VHOST_VDPA_DEVICE_TYPE_BLK
&& request != VHOST_USER_SET_VRING_CALL)
goto out;
--
1.8.3.1
