Re: [dpdk-dev] [PATCH] vhost: return ready when at least 1 vring is configured

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 9/21/20 7:03 AM, Liu, Changpeng wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Friday, September 18, 2020 5:54 PM
>> To: Liu, Changpeng <changpeng.liu@intel.com>; dev@dpdk.org
>> Cc: matan@mellanox.com; Xia, Chenbo <chenbo.xia@intel.com>; Zawadzki,
>> Tomasz <tomasz.zawadzki@intel.com>
>> Subject: Re: [PATCH] vhost: return ready when at least 1 vring is configured
>>
>> Hi Changpeng,
>>
>> On 9/1/20 9:07 AM, Changpeng Liu wrote:
>>> Commit d0fcc38f "vhost: improve device readiness notifications"
>>> needs at least 2 vrings before changing the device state to
>>> ready, this is fine for NET device but not correct for BLK
>>> device.
>>>
>>> The number of vring required should be based on the device
>>> type, e.g. virtio_scsi device needs at least 3 vrings, and
>>> virtio_net needs at least 2 vrings, virtio_blk needs at least
>>> 1 vring. So instead of doing it in vhost library it's better
>>> that the application who uses this library do this check.
>>>
>>> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>> ---
>>>  lib/librte_vhost/vhost_user.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>>> index c3c924f..4d1883c 100644
>>> --- a/lib/librte_vhost/vhost_user.c
>>> +++ b/lib/librte_vhost/vhost_user.c
>>> @@ -1343,7 +1343,7 @@
>>>  	       vq->enabled;
>>>  }
>>>
>>> -#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 2u
>>> +#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 1u
>>
>> I think it would be better to rely on VIRTIO_DEV_BUILTIN_VIRTIO_NET to
>> know whether it should wait for 1 or 2 queues to determine if ready.
> virtio_scsi needs at least 3 vrings, so both 1 and 2 can't work for virtio_scsi device.
> Can we expose an API to let the caller to set the minimum number of vrings required by
> virtio device?

OK, thanks for pointing this out, I missed it.

I'm not in favor of introducing an new API for this.
I propose to restrict change introduced in commit d0fcc38f to the
builtin net backend. Can you have a try with below patch?

Thanks in advance,
Maxime

>>
>>
>>>  static int
>>>  virtio_is_ready(struct virtio_net *dev)
>>>
> 

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 501218e192..f571ef93fc 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -1343,21 +1343,25 @@ vq_is_ready(struct virtio_net *dev, struct
vhost_virtqueue *vq)
               vq->enabled;
 }

-#define VIRTIO_DEV_NUM_VQS_TO_BE_READY 2u
+#define VIRTIO_BUILTIN_NUM_VQS_TO_BE_READY 2u

 static int
 virtio_is_ready(struct virtio_net *dev)
 {
        struct vhost_virtqueue *vq;
-       uint32_t i;
+       uint32_t i, nr_vring = dev->nr_vring;

        if (dev->flags & VIRTIO_DEV_READY)
                return 1;

-       if (dev->nr_vring < VIRTIO_DEV_NUM_VQS_TO_BE_READY)
-               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;
+       }

-       for (i = 0; i < VIRTIO_DEV_NUM_VQS_TO_BE_READY; i++) {
+       for (i = 0; i < nr_vring; i++) {
                vq = dev->virtqueue[i];

                if (!vq_is_ready(dev, vq))