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

["Zhang, Roy Fan" <roy.fan.zhang@intel.com>]

Hi Chenbo and Maxime,

Thanks for replying the email. 


> -----Original Message-----
> From: Maxime Coquelin <maxime.coquelin@redhat.com>
> Sent: Wednesday, September 30, 2020 4:37 PM
> To: Xia, Chenbo <chenbo.xia@intel.com>; Zhang, Roy Fan
> <roy.fan.zhang@intel.com>; Liu, Changpeng <changpeng.liu@intel.com>;
> dev@dpdk.org
> Cc: matan@mellanox.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,
> 
> 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 😊?

The reason it is set inside rte_vhost_crypto_create() is logically speaking
the user shouldn't expect to have to set the feature flags even after the create
function is called - and what I know in the application the only way to know the
vid for the first time is when new_device() is invoked. So if there is away to know
the vid before new_device() is invoked I am happy to change the sample app.

> 
> 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.

Different than vhost_blk, vhost_crypto lies in the library and needs to be
able to be treated evenly as virtio_net. Without the new_device() calling
rte_vhost_crypto_create() the feature flag cannot be set. Without setting
the feature flag the device is always treated as virtio_net device, hence it
cannot pass the virtio_is_ready() check as the number of queues virtio
crypt uses is only 1 instead of 2 (required by virtio_net).

Regards,
Fan
> 
> 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))
> >>>>>>
> >>>>
> >