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

From: Xia, Chenbo <hidden>
Date: 2020-09-30 02:48:25

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 😊?

Thanks,
Chenbo

quoted hunk ↗ jump to hunk

-----Original Message-----
From: Zhang, Roy Fan <redacted>
Sent: Wednesday, September 30, 2020 2:15 AM
To: Maxime Coquelin <redacted>; Liu, Changpeng
[off-list ref]; dev@dpdk.org
Cc: matan@mellanox.com; Xia, Chenbo <redacted>; Zawadzki,
Tomasz [off-list ref]; Yigit, Ferruh [off-list ref]
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

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Tuesday, September 29, 2020 3:06 PM
To: Zhang, Roy Fan <redacted>; Liu, Changpeng
[off-list ref]; dev@dpdk.org
Cc: matan@mellanox.com; Xia, Chenbo <redacted>; Zawadzki,
Tomasz [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] vhost: return ready when at least 1

vring is

quoted

configured

Hi Fan,

The patch is already merged in main branch:

commit 09424c3f74311555c33d3d4cdc2ca3654ce13b1c
Author: Maxime Coquelin [off-list ref]
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 [off-list ref]
    Signed-off-by: Maxime Coquelin [off-list ref]
    Signed-off-by: Changpeng Liu [off-list ref]
    Reviewed-by: Chenbo Xia [off-list ref]


Regards,
Maxime

On 9/29/20 3:54 PM, Zhang, Roy Fan wrote:

quoted

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

quoted

-----Original Message-----
From: dev <redacted> On Behalf Of Maxime Coquelin
Sent: Wednesday, September 23, 2020 9:05 AM
To: Liu, Changpeng <redacted>; dev@dpdk.org
Cc: matan@mellanox.com; Xia, Chenbo <redacted>;

Zawadzki,

quoted

Tomasz [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] vhost: return ready when at least 1

vring

quoted

is

quoted

configured

Hi Changpeng,

On 9/22/20 9:22 AM, Liu, Changpeng wrote:

quoted

Hi Maxime,

The code you wrote still need to check nr_vring is 0 or not, see the

extra

quoted

lines added below, then it can work with my tests for now, could you

submit

quoted

a patch to DPDK to apply the patch? Thanks.

Thanks! You are right.

I'll send the patch now including your fix.

quoted

BTW, dpdk vhost library still has an issue, it's not related with

commit

quoted

d0fcc38f, the Guest driver may only kick 1 vring even it sends

NUM_QUEUES

quoted

with a bigger value,

quoted

this is quite common in seabios, e.g: virtio_blk will only use 1

vring in

quoted

seabios, this means the backend will never get started in BIOS.

quoted

If I understand correctly, this is not a regression but has always

been

quoted

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

quoted

it. Maybe file a bug in DPDK tracker?

Thanks,
Maxime

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Monday, September 21, 2020 6:20 PM
To: Liu, Changpeng <redacted>; dev@dpdk.org
Cc: matan@mellanox.com; Xia, Chenbo <redacted>;

Zawadzki,

quoted

Tomasz [off-list ref]
Subject: Re: [PATCH] vhost: return ready when at least 1 vring is

configured

quoted



On 9/21/20 7:03 AM, Liu, Changpeng wrote:

quoted

Hi Maxime,

quoted

-----Original Message-----
From: Maxime Coquelin <redacted>
Sent: Friday, September 18, 2020 5:54 PM
To: Liu, Changpeng <redacted>; dev@dpdk.org
Cc: matan@mellanox.com; Xia, Chenbo <redacted>;

Zawadzki,

quoted

Tomasz [off-list ref]
Subject: Re: [PATCH] vhost: return ready when at least 1 vring is

configured

quoted

Hi Changpeng,

On 9/1/20 9:07 AM, Changpeng Liu wrote:

quoted

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

quoted

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

quoted

quoted
to
quoted
quoted
quoted
quoted
know whether it should wait for 1 or 2 queues to determine if

ready.

quoted

quoted
quoted
quoted
quoted
quoted
virtio_scsi needs at least 3 vrings, so both 1 and 2 can't work

for

quoted

virtio_scsi

quoted

device.

quoted

Can we expose an API to let the caller to set the minimum number

of

quoted

vrings

quoted

required by

quoted

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

quoted

 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

quoted

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;

quoted

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

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help