Hi Chenbo and Maxime,
Thanks for replying the email.
> -----Original Message-----
> From: Maxime Coquelin <maxime.coque...@redhat.com>
> Sent: Wednesday, September 30, 2020 4:37 PM
> To: Xia, Chenbo <chenbo....@intel.com>; Zhang, Roy Fan
> <roy.fan.zh...@intel.com>; Liu, Changpeng <changpeng....@intel.com>;
> dev@dpdk.org
> Cc: ma...@mellanox.com; Zawadzki, Tomasz <tomasz.zawad...@intel.com>;
> Yigit, Ferruh <ferruh.yi...@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.zh...@intel.com>
> >> Sent: Wednesday, September 30, 2020 2:15 AM
> >> To: Maxime Coquelin <maxime.coque...@redhat.com>; Liu, Changpeng
> >> <changpeng....@intel.com>; dev@dpdk.org
> >> Cc: ma...@mellanox.com; Xia, Chenbo <chenbo....@intel.com>;
> Zawadzki,
> >> Tomasz <tomasz.zawad...@intel.com>; Yigit, Ferruh
> <ferruh.yi...@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.coque...@redhat.com>
> >>> Sent: Tuesday, September 29, 2020 3:06 PM
> >>> To: Zhang, Roy Fan <roy.fan.zh...@intel.com>; Liu, Changpeng
> >>> <changpeng....@intel.com>; dev@dpdk.org
> >>> Cc: ma...@mellanox.com; Xia, Chenbo <chenbo....@intel.com>;
> Zawadzki,
> >>> Tomasz <tomasz.zawad...@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.coque...@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....@intel.com>
> >>> Signed-off-by: Maxime Coquelin <maxime.coque...@redhat.com>
> >>> Signed-off-by: Changpeng Liu <changpeng....@intel.com>
> >>> Reviewed-by: Chenbo Xia <chenbo....@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-boun...@dpdk.org> On Behalf Of Maxime Coquelin
> >>>>> Sent: Wednesday, September 23, 2020 9:05 AM
> >>>>> To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
> >>>>> Cc: ma...@mellanox.com; Xia, Chenbo <chenbo....@intel.com>;
> >>> Zawadzki,
> >>>>> Tomasz <tomasz.zawad...@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.coque...@redhat.com>
> >>>>>>> Sent: Monday, September 21, 2020 6:20 PM
> >>>>>>> To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
> >>>>>>> Cc: ma...@mellanox.com; Xia, Chenbo <chenbo....@intel.com>;
> >>>>> Zawadzki,
> >>>>>>> Tomasz <tomasz.zawad...@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.coque...@redhat.com>
> >>>>>>>>> Sent: Friday, September 18, 2020 5:54 PM
> >>>>>>>>> To: Liu, Changpeng <changpeng....@intel.com>; dev@dpdk.org
> >>>>>>>>> Cc: ma...@mellanox.com; Xia, Chenbo <chenbo....@intel.com>;
> >>>>> Zawadzki,
> >>>>>>>>> Tomasz <tomasz.zawad...@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....@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))
> >>>>>>
> >>>>
> >
