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

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

Hi,

On 9/30/20 4:48 AM, Xia, Chenbo wrote:
> Hi Fan & Maxime,
> 
> I am thinking that should we move set_features outside of new_device callback
> for crypto device? I see that net and blk devices both set features between register
> and start, and personally I think this makes sense that device features are set
> before device start and ready. How do you think 😊?

Indeed, we cannot consider the device to be ready (and so call
.new_device callback) if features haven't been negotiated.

I agree, rte_vhost_driver_set_features() has to be called before
.new_device(), and that's the reason why it takes socket's path and not
vid as input.

Thanks,
Maxime


> Thanks,
> Chenbo
> 
>> -----Original Message-----
>> From: Zhang, Roy Fan <roy.fan.zhang@intel.com>
>> Sent: Wednesday, September 30, 2020 2:15 AM
>> To: Maxime Coquelin <maxime.coquelin@redhat.com>; 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>; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: RE: [dpdk-dev] [PATCH] vhost: return ready when at least 1 vring
>> is configured
>>
>> Hi Maxime,
>>
>> Thanks for telling me that but vhost_crypto is still breaking.
>>
>> Virtio-crypto (both LKCF virtio-crypt driver and DPDK virtio-crypto PMD)
>> won't create device queue until the crypto session is required to be
>> created. Thus vq_is_ready() will never return true during initialization -
>> hence new_device() in vhost_crypto sample app will never be triggered.
>> Also since rte_vhost_driver_set_features() is called inside
>> rte_vhost_crypto_create(), which is triggered by new_device() handler, we
>> cannot update the dev->flags to bypass the "if (dev->flags &
>> VIRTIO_DEV_BUILTIN_VIRTIO_NET)" check before new_device() is called.
>> what the new logic virtio_is_ready() requirement will never meet for
>> vhost_crypto.
>>
>> A way to fix it is with the following change -
>>
>> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
>> index b00e1f91d..e5263a360 100644
>> --- a/lib/librte_vhost/vhost_user.c
>> +++ b/lib/librte_vhost/vhost_user.c
>> @@ -1937,6 +1937,14 @@ vhost_user_set_vring_kick(struct virtio_net **pdev,
>> struct VhostUserMsg *msg,
>>                 }
>>         }
>>
>> +       /* virtio-crypto vq is not ready until session is created. Check
>> +        * here if we need to initialize device again
>> +        */
>> +       if (!(dev->flags & VIRTIO_DEV_RUNNING)) {
>> +               if (dev->notify_ops->new_device(dev->vid) == 0)
>> +                       dev->flags |= VIRTIO_DEV_RUNNING;
>> +       }
>> +
>>         return RTE_VHOST_MSG_RESULT_OK;
>>  }
>>
>> but I cannot add virtio_is_ready() inside the " if (!(dev->flags &
>> VIRTIO_DEV_RUNNING))" check, since we need new_device() is called to set
>> the feature flags - if to set the feature flags in the example instead,
>> the logic will not right, since virtio-net does not require the user to
>> set the flags themselves either.
>>
>> Regards,
>> Fan
>>
>>
>>> -----Original Message-----
>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Sent: Tuesday, September 29, 2020 3:06 PM
>>> To: Zhang, Roy Fan <roy.fan.zhang@intel.com>; 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: [dpdk-dev] [PATCH] vhost: return ready when at least 1
>> vring is
>>> configured
>>>
>>> Hi Fan,
>>>
>>> The patch is already merged in main branch:
>>>
>>> commit 09424c3f74311555c33d3d4cdc2ca3654ce13b1c
>>> Author: Maxime Coquelin <maxime.coquelin@redhat.com>
>>> Date:   Wed Sep 23 11:49:02 2020 +0200
>>>
>>>     vhost: fix external backends readiness
>>>
>>>     Commit d0fcc38f5fa4 ("vhost: improve device readiness notifications")
>>>     makes the assumption that every Virtio devices are considered
>>>     ready for preocessing as soon as first queue pair is configured
>>>     and enabled.
>>>
>>>     While this is true for Virtio-net, it isn't for Virtio-scsi
>>>     and Virtio-blk.
>>>
>>>     This patch fixes this by only making this assumption for
>>>     the builtin Virtio-net backend, and restores back to previous
>>>     behaviour for other backends.
>>>
>>>     Fixes: d0fcc38f5fa4 ("vhost: improve device readiness notifications")
>>>
>>>     Reported-by: Changpeng Liu <changpeng.liu@intel.com>
>>>     Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>     Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
>>>     Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>>
>>>
>>> Regards,
>>> Maxime
>>>
>>> On 9/29/20 3:54 PM, Zhang, Roy Fan wrote:
>>>> Hi Maxime,
>>>>
>>>> Vhost-crypto has exactly the same issue. Changpeng's patch fixed it.
>>>> Could you give me a shout when your patch is out, so I can have a test?
>>>>
>>>> Regards,
>>>> Fan
>>>>
>>>>> -----Original Message-----
>>>>> From: dev <dev-bounces@dpdk.org> On Behalf Of Maxime Coquelin
>>>>> Sent: Wednesday, September 23, 2020 9:05 AM
>>>>> 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: [dpdk-dev] [PATCH] vhost: return ready when at least 1
>> vring
>>> is
>>>>> configured
>>>>>
>>>>> Hi Changpeng,
>>>>>
>>>>> On 9/22/20 9:22 AM, Liu, Changpeng wrote:
>>>>>> Hi Maxime,
>>>>>>
>>>>>> The code you wrote still need to check nr_vring is 0 or not, see the
>> extra
>>> 2
>>>>> lines added below, then it can work with my tests for now, could you
>>> submit
>>>>> a patch to DPDK to apply the patch? Thanks.
>>>>>
>>>>> Thanks! You are right.
>>>>>
>>>>> I'll send the patch now including your fix.
>>>>>
>>>>>> BTW, dpdk vhost library still has an issue, it's not related with
>> commit
>>>>> d0fcc38f, the Guest driver may only kick 1 vring even it sends
>>> NUM_QUEUES
>>>>> with a bigger value,
>>>>>> this is quite common in seabios, e.g: virtio_blk will only use 1
>> vring in
>>>>> seabios, this means the backend will never get started in BIOS.
>>>>>>
>>>>>
>>>>> If I understand correctly, this is not a regression but has always
>> been
>>>>> here?
>>>>>
>>>>> We should work on fixing it anyway, but I'm not sure to have the time
>>>>> for v20.11.0. It would be great if you could provide steps to
>> reproduce
>>>>> it. Maybe file a bug in DPDK tracker?
>>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>>
>>>>>>> -----Original Message-----
>>>>>>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>>>>> Sent: Monday, September 21, 2020 6:20 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
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> 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;
>>>>>>> +       }
>>>>>>
>>>>>>    +         if(!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))
>>>>>>
>>>>
>