diff mbox series

vhost: fix notification for packed ring

Message ID 20181011130655.5485-1-tiwei.bie@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers
Series vhost: fix notification for packed ring |

Checks

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

Commit Message

Tiwei Bie Oct. 11, 2018, 1:06 p.m. UTC 
  The notification can't be disabled in packed ring when
application tries to disable notification, because the
device event flags field is overwritten by an unexpected
value. This patch fixes this issue.

Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
Cc: stable@dpdk.org

Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
---
 lib/librte_vhost/vhost.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

Comments

Maxime Coquelin Oct. 11, 2018, 1:17 p.m. UTC | #1 
On 10/11/2018 03:06 PM, Tiwei Bie wrote:
> The notification can't be disabled in packed ring when
> application tries to disable notification, because the
> device event flags field is overwritten by an unexpected
> value. This patch fixes this issue.
> 
> Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vhost.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index e62f4c594..c9270bdec 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
>   {
>   	uint16_t flags;
>   
> -	if (!enable)
> -		vq->device_event->flags = VRING_EVENT_F_DISABLE;
> +	if (!enable) {
> +		flags = VRING_EVENT_F_DISABLE;
> +		goto out;
> +	}
>   
>   	flags = VRING_EVENT_F_ENABLE;
>   	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
> @@ -677,6 +679,7 @@ vhost_enable_notify_packed(struct virtio_net *dev,
>   			vq->avail_wrap_counter << 15;
>   	}
>   
> +out:
>   	rte_smp_wmb();
>   
>   	vq->device_event->flags = flags;
> 

Thanks for spotting this:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Maxime
Jason Wang Oct. 11, 2018, 1:34 p.m. UTC | #2 
On 2018年10月11日 21:06, Tiwei Bie wrote:
> The notification can't be disabled in packed ring when
> application tries to disable notification, because the
> device event flags field is overwritten by an unexpected
> value. This patch fixes this issue.
>
> Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
> Cc: stable@dpdk.org
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
>   lib/librte_vhost/vhost.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> index e62f4c594..c9270bdec 100644
> --- a/lib/librte_vhost/vhost.c
> +++ b/lib/librte_vhost/vhost.c
> @@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
>   {
>   	uint16_t flags;
>   
> -	if (!enable)
> -		vq->device_event->flags = VRING_EVENT_F_DISABLE;
> +	if (!enable) {
> +		flags = VRING_EVENT_F_DISABLE;
> +		goto out;
> +	}
>   
>   	flags = VRING_EVENT_F_ENABLE;
>   	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
> @@ -677,6 +679,7 @@ vhost_enable_notify_packed(struct virtio_net *dev,
>   			vq->avail_wrap_counter << 15;
>   	}
>   
> +out:
>   	rte_smp_wmb();
>   

It looks to me the barrier is even not needed for !enable.

Thanks

>   	vq->device_event->flags = flags;
Michael S. Tsirkin Oct. 11, 2018, 1:36 p.m. UTC | #3 
On Thu, Oct 11, 2018 at 09:34:06PM +0800, Jason Wang wrote:
> 
> 
> On 2018年10月11日 21:06, Tiwei Bie wrote:
> > The notification can't be disabled in packed ring when
> > application tries to disable notification, because the
> > device event flags field is overwritten by an unexpected
> > value. This patch fixes this issue.
> > 
> > Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
> > Cc: stable@dpdk.org
> > 
> > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > ---
> >   lib/librte_vhost/vhost.c | 7 +++++--
> >   1 file changed, 5 insertions(+), 2 deletions(-)
> > 
> > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> > index e62f4c594..c9270bdec 100644
> > --- a/lib/librte_vhost/vhost.c
> > +++ b/lib/librte_vhost/vhost.c
> > @@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
> >   {
> >   	uint16_t flags;
> > -	if (!enable)
> > -		vq->device_event->flags = VRING_EVENT_F_DISABLE;
> > +	if (!enable) {
> > +		flags = VRING_EVENT_F_DISABLE;
> > +		goto out;
> > +	}
> >   	flags = VRING_EVENT_F_ENABLE;
> >   	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
> > @@ -677,6 +679,7 @@ vhost_enable_notify_packed(struct virtio_net *dev,
> >   			vq->avail_wrap_counter << 15;
> >   	}
> > +out:
> >   	rte_smp_wmb();
> 
> It looks to me the barrier is even not needed for !enable.
> 
> Thanks

Good point, I agree.
Besides that

Reviewed-by: Michael S. Tsirkin <mst@redhat.com>



> >   	vq->device_event->flags = flags;
Tiwei Bie Oct. 11, 2018, 2:10 p.m. UTC | #4 
On Thu, Oct 11, 2018 at 09:36:48AM -0400, Michael S. Tsirkin wrote:
> On Thu, Oct 11, 2018 at 09:34:06PM +0800, Jason Wang wrote:
> > 
> > 
> > On 2018年10月11日 21:06, Tiwei Bie wrote:
> > > The notification can't be disabled in packed ring when
> > > application tries to disable notification, because the
> > > device event flags field is overwritten by an unexpected
> > > value. This patch fixes this issue.
> > > 
> > > Fixes: b1cce26af1dc ("vhost: add notification for packed ring")
> > > Cc: stable@dpdk.org
> > > 
> > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > ---
> > >   lib/librte_vhost/vhost.c | 7 +++++--
> > >   1 file changed, 5 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> > > index e62f4c594..c9270bdec 100644
> > > --- a/lib/librte_vhost/vhost.c
> > > +++ b/lib/librte_vhost/vhost.c
> > > @@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
> > >   {
> > >   	uint16_t flags;
> > > -	if (!enable)
> > > -		vq->device_event->flags = VRING_EVENT_F_DISABLE;
> > > +	if (!enable) {
> > > +		flags = VRING_EVENT_F_DISABLE;
> > > +		goto out;
> > > +	}
> > >   	flags = VRING_EVENT_F_ENABLE;
> > >   	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
> > > @@ -677,6 +679,7 @@ vhost_enable_notify_packed(struct virtio_net *dev,
> > >   			vq->avail_wrap_counter << 15;
> > >   	}
> > > +out:
> > >   	rte_smp_wmb();
> > 
> > It looks to me the barrier is even not needed for !enable.
> > 
> > Thanks
> 
> Good point, I agree.
> Besides that
> 
> Reviewed-by: Michael S. Tsirkin <mst@redhat.com>
> 

Yeah, it's better to return directly after disabling the
interrupt. I will send a new version to do this change.

Thanks!

> 
> 
> > >   	vq->device_event->flags = flags;
diff mbox series

Patch

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index e62f4c594..c9270bdec 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -667,8 +667,10 @@  vhost_enable_notify_packed(struct virtio_net *dev,
 {
 	uint16_t flags;
 
-	if (!enable)
-		vq->device_event->flags = VRING_EVENT_F_DISABLE;
+	if (!enable) {
+		flags = VRING_EVENT_F_DISABLE;
+		goto out;
+	}
 
 	flags = VRING_EVENT_F_ENABLE;
 	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {
@@ -677,6 +679,7 @@  vhost_enable_notify_packed(struct virtio_net *dev,
 			vq->avail_wrap_counter << 15;
 	}
 
+out:
 	rte_smp_wmb();
 
 	vq->device_event->flags = flags;