[dpdk-dev] [PATCH] vhost: fix wait for valid descriptor

[dpdk-dev] [PATCH] vhost: fix wait for valid descriptor

[Tomasz Kulasek]

For each virt queue's kickfd and callfd, there are 2 invalid
status: VIRTIO_UNINITIALIZED_EVENTFD and VIRTIO_INVALID_EVENTFD.
Don't set the virt queue to ready status until got the valid
descriptor.

This is safe for polling mode drivers in Guest OS, the backend
vhost process will not post notification to interrupt vector for
PMD mode in guest, but the interrupt vector still valid.

Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")
Cc: yuanhan.liu@linux.intel.com
Cc: stable@dpdk.org

Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
---
 lib/librte_vhost/vhost_user.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
index 65ee33919..4508f697b 100644
--- a/lib/librte_vhost/vhost_user.c
+++ b/lib/librte_vhost/vhost_user.c
@@ -766,7 +766,9 @@ vq_is_ready(struct vhost_virtqueue *vq)
 {
 	return vq && vq->desc && vq->avail && vq->used &&
 	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
-	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
+	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
+	       vq->kickfd != VIRTIO_INVALID_EVENTFD &&
+	       vq->callfd != VIRTIO_INVALID_EVENTFD;
 }
 
 static int
-- 
2.14.1

Re: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor

[Tan, Jianfeng]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Kulasek
> Sent: Saturday, February 10, 2018 1:35 AM
> To: yliu@fridaylinux.org
> Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; stable@dpdk.org; Liu,
> Changpeng
> Subject: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor
> 
> For each virt queue's kickfd and callfd, there are 2 invalid
> status: VIRTIO_UNINITIALIZED_EVENTFD and VIRTIO_INVALID_EVENTFD.
> Don't set the virt queue to ready status until got the valid
> descriptor.

Cannot remember why we introduce two invalid status.

If we treat "VIRTIO_INVALID_EVENTFD" as an indicator to judge if device is ready, then defining one status (invalid) is enough.

Thanks,
Jianfeng

> 
> This is safe for polling mode drivers in Guest OS, the backend
> vhost process will not post notification to interrupt vector for
> PMD mode in guest, but the interrupt vector still valid.
> 
> Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")
> Cc: yuanhan.liu@linux.intel.com
> Cc: stable@dpdk.org
> 
> Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> ---
>  lib/librte_vhost/vhost_user.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> index 65ee33919..4508f697b 100644
> --- a/lib/librte_vhost/vhost_user.c
> +++ b/lib/librte_vhost/vhost_user.c
> @@ -766,7 +766,9 @@ vq_is_ready(struct vhost_virtqueue *vq)
>  {
>  	return vq && vq->desc && vq->avail && vq->used &&
>  	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> -	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
> +	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> +	       vq->kickfd != VIRTIO_INVALID_EVENTFD &&
> +	       vq->callfd != VIRTIO_INVALID_EVENTFD;
>  }
> 
>  static int
> --
> 2.14.1

Re: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor

[Thomas Monjalon]

What is the status of this patch?


11/02/2018 05:21, Tan, Jianfeng:
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Kulasek
> > Sent: Saturday, February 10, 2018 1:35 AM
> > To: yliu@fridaylinux.org
> > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; stable@dpdk.org; Liu,
> > Changpeng
> > Subject: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor
> > 
> > For each virt queue's kickfd and callfd, there are 2 invalid
> > status: VIRTIO_UNINITIALIZED_EVENTFD and VIRTIO_INVALID_EVENTFD.
> > Don't set the virt queue to ready status until got the valid
> > descriptor.
> 
> Cannot remember why we introduce two invalid status.
> 
> If we treat "VIRTIO_INVALID_EVENTFD" as an indicator to judge if device is ready, then defining one status (invalid) is enough.
> 
> Thanks,
> Jianfeng
> 
> > 
> > This is safe for polling mode drivers in Guest OS, the backend
> > vhost process will not post notification to interrupt vector for
> > PMD mode in guest, but the interrupt vector still valid.
> > 
> > Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")
> > Cc: yuanhan.liu@linux.intel.com
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > ---
> >  lib/librte_vhost/vhost_user.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 65ee33919..4508f697b 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -766,7 +766,9 @@ vq_is_ready(struct vhost_virtqueue *vq)
> >  {
> >  	return vq && vq->desc && vq->avail && vq->used &&
> >  	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> > -	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
> > +	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> > +	       vq->kickfd != VIRTIO_INVALID_EVENTFD &&
> > +	       vq->callfd != VIRTIO_INVALID_EVENTFD;
> >  }
> > 
> >  static int
> > --
> > 2.14.1
> 

Re: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor

[Tiwei Bie]

On Thu, Jul 26, 2018 at 05:48:04PM +0200, Thomas Monjalon wrote:
> What is the status of this patch?
> 
> 
> 11/02/2018 05:21, Tan, Jianfeng:
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Tomasz Kulasek
> > > Sent: Saturday, February 10, 2018 1:35 AM
> > > To: yliu@fridaylinux.org
> > > Cc: dev@dpdk.org; yuanhan.liu@linux.intel.com; stable@dpdk.org; Liu,
> > > Changpeng
> > > Subject: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor
> > > 
> > > For each virt queue's kickfd and callfd, there are 2 invalid
> > > status: VIRTIO_UNINITIALIZED_EVENTFD and VIRTIO_INVALID_EVENTFD.
> > > Don't set the virt queue to ready status until got the valid
> > > descriptor.
> > 
> > Cannot remember why we introduce two invalid status.
> > 
> > If we treat "VIRTIO_INVALID_EVENTFD" as an indicator to judge if device is ready, then defining one status (invalid) is enough.
> > 
> > Thanks,
> > Jianfeng
> > 
> > > 
> > > This is safe for polling mode drivers in Guest OS, the backend
> > > vhost process will not post notification to interrupt vector for
> > > PMD mode in guest, but the interrupt vector still valid.
> > > 
> > > Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")

This fixes line isn't right. It's the below commit [1]
that introduced the code changed by this patch. The code
was written in this way intentionally by that commit.
With a quick glance at that commit, it seems that this
patch isn't valid.

[1] https://git.dpdk.org/dpdk/commit/?id=fb871d0a4dc1c038a381c524cdb86fe83d21d842
commit fb871d0a4dc1c038a381c524cdb86fe83d21d842
Author: Tetsuya Mukawa <mukawa@igel.co.jp>
Date:   Mon Mar 14 17:53:32 2016 +0900

    vhost: fix default value of kickfd and callfd
    
    Currently, default values of kickfd and callfd are -1.
    If the values are -1, current code guesses kickfd and callfd haven't
    been initialized yet. Then vhost library will guess the virtqueue isn't
    ready for processing.
    
    But callfd and kickfd will be set as -1 when "--enable-kvm"
    isn't specified in QEMU command line. It means we cannot treat -1 as
    uninitialized state.
    
    The patch defines -1 and -2 as VIRTIO_INVALID_EVENTFD and
    VIRTIO_UNINITIALIZED_EVENTFD, and uses VIRTIO_UNINITIALIZED_EVENTFD for
    the default values of kickfd and callfd.
    
    Signed-off-by: Tetsuya Mukawa <mukawa@igel.co.jp>
    Acked-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>



> > > Cc: yuanhan.liu@linux.intel.com
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Changpeng Liu <changpeng.liu@intel.com>
> > > Signed-off-by: Tomasz Kulasek <tomaszx.kulasek@intel.com>
> > > ---
> > >  lib/librte_vhost/vhost_user.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > > index 65ee33919..4508f697b 100644
> > > --- a/lib/librte_vhost/vhost_user.c
> > > +++ b/lib/librte_vhost/vhost_user.c
> > > @@ -766,7 +766,9 @@ vq_is_ready(struct vhost_virtqueue *vq)
> > >  {
> > >  	return vq && vq->desc && vq->avail && vq->used &&
> > >  	       vq->kickfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> > > -	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD;
> > > +	       vq->callfd != VIRTIO_UNINITIALIZED_EVENTFD &&
> > > +	       vq->kickfd != VIRTIO_INVALID_EVENTFD &&
> > > +	       vq->callfd != VIRTIO_INVALID_EVENTFD;
> > >  }
> > > 
> > >  static int
> > > --
> > > 2.14.1
> > 
>