diff mbox

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

Message ID	20180209173455.14116-1-tomaszx.kulasek@intel.com (mailing list archive)
State	Rejected, archived
Delegated to:	Maxime Coquelin
Headers	From: Tomasz Kulasek <tomaszx.kulasek@intel.com> To: yliu@fridaylinux.org Cc: dev@dpdk.org, yuanhan.liu@linux.intel.com, stable@dpdk.org, Changpeng Liu <changpeng.liu@intel.com> Date: Fri, 9 Feb 2018 18:34:55 +0100 Message-Id: <20180209173455.14116-1-tomaszx.kulasek@intel.com> Subject: [dpdk-dev] [PATCH] vhost: fix wait for valid descriptor Precedence: list Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org>

Checks

Context	Check	Description
ci/checkpatch	success	coding style OK
ci/Intel-compilation	success	Compilation OK

Commit Message

Tomasz Kulasek Feb. 9, 2018, 5:34 p.m. UTC

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

Comments

Jianfeng Tan Feb. 11, 2018, 4:21 a.m. UTC | #1

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

Thomas Monjalon July 26, 2018, 3:48 p.m. UTC | #2

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
>

Tiwei Bie July 27, 2018, 6:11 a.m. UTC | #3

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

diff mbox

Patch

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