DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Liu, Changpeng" <changpeng.liu@intel.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: "matan@mellanox.com" <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
Date: Mon, 21 Sep 2020 12:19:47 +0200	[thread overview]
Message-ID: <06542643-cc20-5e3e-26ae-7100005dc97d@redhat.com> (raw)
In-Reply-To: <BN7PR11MB27398204C237F4E4D5737F22EE3A0@BN7PR11MB2739.namprd11.prod.outlook.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))


  reply	other threads:[~2020-09-21 10:19 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-01  7:07 Changpeng Liu
2020-09-18  9:53 ` Maxime Coquelin
2020-09-21  5:03   ` Liu, Changpeng
2020-09-21 10:19     ` Maxime Coquelin [this message]
2020-09-22  7:22       ` Liu, Changpeng
2020-09-23  8:05         ` Maxime Coquelin
2020-09-23  8:14           ` Liu, Changpeng
2020-09-29 13:54           ` Zhang, Roy Fan
2020-09-29 14:05             ` Maxime Coquelin
2020-09-29 18:15               ` Zhang, Roy Fan
2020-09-30  2:48                 ` Xia, Chenbo
2020-09-30 15:36                   ` Maxime Coquelin
2020-09-30 16:37                     ` Zhang, Roy Fan
2020-10-01  7:55                       ` Maxime Coquelin
2020-10-01  8:07                         ` Zhang, Roy Fan
2020-10-01  8:26                           ` Maxime Coquelin
2020-10-01  8:42                             ` Zhang, Roy Fan

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=06542643-cc20-5e3e-26ae-7100005dc97d@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=changpeng.liu@intel.com \
    --cc=chenbo.xia@intel.com \
    --cc=dev@dpdk.org \
    --cc=matan@mellanox.com \
    --cc=tomasz.zawadzki@intel.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).