vhost: fix notification for packed ring

PATCH Superseded Tiwei Bie 4 participants 9 messages Oct 11, 2018 – Oct 16, 2018

Share & tools

Tiwei Bie

author

2018-10-11 13:06 UTC #0 lore raw

PATCH 1/1 vhost: fix notification for packed ring

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 [email protected]

Signed-off-by Tiwei Bie <[email protected]>

---
 lib/librte_vhost/vhost.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

lib/librte_vhost/vhost.c +5 -2

index e62f4c594..c9270bdec 100644
@@ -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;

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v1 1/1

2018-10-11 13:17 UTC #1 lore raw

Quoted 32 lines · Tiwei Bie

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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  lib/librte_vhost/vhost.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
lib/librte_vhost/vhost.c +4 -2
index e62f4c594..c9270bdec 100644
@@ -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)) {

lib/librte_vhost/vhost.c vhost_enable_notify_packed(struct virtio_net *dev, · line 685

Quoted 7 lines

lib/librte_vhost/vhost.c +1 -0

@@ -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 <[email protected]>

Maxime

Jason Wang

2018-10-11 13:34 UTC #2 lore raw

In reply to Tiwei Bie

Quoted 31 lines · Tiwei Bie

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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  lib/librte_vhost/vhost.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
lib/librte_vhost/vhost.c +4 -2
index e62f4c594..c9270bdec 100644
@@ -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)) {

lib/librte_vhost/vhost.c vhost_enable_notify_packed(struct virtio_net *dev, · line 684

Quoted 6 lines

lib/librte_vhost/vhost.c +1 -0

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

Quoted 1 line

lib/librte_vhost/vhost.c +0 -0
  	vq->device_event->flags = flags;

Michael S. Tsirkin

Reviewed-by

2018-10-11 13:36 UTC #3 lore raw

Quoted 39 lines · Jason Wang

Quoted 29 lines · Tiwei Bie
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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  lib/librte_vhost/vhost.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
lib/librte_vhost/vhost.c +4 -2
index e62f4c594..c9270bdec 100644
@@ -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)) {
lib/librte_vhost/vhost.c vhost_enable_notify_packed(struct virtio_net *dev, · line 682
Quoted 4 lines
lib/librte_vhost/vhost.c +1 -0
@@ -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.

Quoted 1 line

Thanks

Good point, I agree.
Besides that

Reviewed-by Michael S. Tsirkin <[email protected]>

Quoted 1 line

Quoted 1 line
lib/librte_vhost/vhost.c +0 -0
  	vq->device_event->flags = flags;

Tiwei Bie

author

2018-10-11 14:10 UTC #4 lore raw

Quoted 40 lines · Michael S. Tsirkin

Quoted 39 lines · Jason Wang
Quoted 29 lines · Tiwei Bie
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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
---
  lib/librte_vhost/vhost.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)
lib/librte_vhost/vhost.c +4 -2
index e62f4c594..c9270bdec 100644
@@ -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)) {
lib/librte_vhost/vhost.c vhost_enable_notify_packed(struct virtio_net *dev, · line 682
Quoted 4 lines
lib/librte_vhost/vhost.c +1 -0
@@ -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.

Quoted 7 lines

Quoted 1 line
Thanks
Good point, I agree.
Besides that
Reviewed-by Michael S. Tsirkin <[email protected]>

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

Thanks!

Quoted 3 lines

Quoted 3 lines
Quoted 3 lines
lib/librte_vhost/vhost.c +0 -0
  	vq->device_event->flags = flags;

Tiwei Bie

author

2018-10-11 14:22 UTC #5 lore raw

PATCH v2 1/1 vhost: fix notification for packed ring

In reply to Tiwei Bie

Fixes b1cce26af1dc ("vhost: add notification for packed ring")

Cc [email protected]

Signed-off-by Tiwei Bie <[email protected]>

Reviewed-by Michael S. Tsirkin <[email protected]>

---
v2: return directly after disabling the interrupt (Jason/MST)

 lib/librte_vhost/vhost.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

lib/librte_vhost/vhost.c +3 -1

index e62f4c594..047ee535c 100644
@@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
 {
 	uint16_t flags;
 
-	if (!enable)
+	if (!enable) {
 		vq->device_event->flags = VRING_EVENT_F_DISABLE;
+		return;
+	}
 
 	flags = VRING_EVENT_F_ENABLE;
 	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {

Signature · 1 line

2.19.1

Maxime Coquelin

Reviewed-by on v2 1/1

2018-10-11 15:21 UTC #6 lore raw

Quoted 10 lines · Tiwei Bie

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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>

Reviewed-by Maxime Coquelin <[email protected]>

Quoted 23 lines

---
v2: return directly after disabling the interrupt (Jason/MST)

  lib/librte_vhost/vhost.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

lib/librte_vhost/vhost.c +3 -1

index e62f4c594..047ee535c 100644
@@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
  {
  	uint16_t flags;
 
-	if (!enable)
+	if (!enable) {
  		vq->device_event->flags = VRING_EVENT_F_DISABLE;
+		return;
+	}
 
  	flags = VRING_EVENT_F_ENABLE;
  	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {

Jason Wang

Acked-by on v2 1/1

2018-10-11 23:47 UTC #7 lore raw

In reply to Tiwei Bie

Quoted 22 lines · Tiwei Bie

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 [email protected]
Signed-off-by Tiwei Bie <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
---
v2: return directly after disabling the interrupt (Jason/MST)
  lib/librte_vhost/vhost.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
lib/librte_vhost/vhost.c +0 -0
index e62f4c594..047ee535c 100644
@@ -667,8 +667,10 @@ vhost_enable_notify_packed(struct virtio_net *dev,
  {

lib/librte_vhost/vhost.c vhost_enable_notify_packed(struct virtio_net *dev, · line 676

Quoted 10 lines

lib/librte_vhost/vhost.c +3 -1

  	uint16_t flags;
 
-	if (!enable)
+	if (!enable) {
  		vq->device_event->flags = VRING_EVENT_F_DISABLE;
+		return;
+	}
 
  	flags = VRING_EVENT_F_ENABLE;
  	if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) {

Acked-by Jason Wang <[email protected]>

Maxime Coquelin

Applied

2018-10-16 08:36 UTC #8 lore raw

In reply to Tiwei Bie

Quoted 8 lines · Tiwei Bie

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 [email protected]

Quoted 7 lines

Signed-off-by Tiwei Bie <[email protected]>
Reviewed-by Michael S. Tsirkin <[email protected]>
---
v2: return directly after disabling the interrupt (Jason/MST)
  lib/librte_vhost/vhost.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

Applied to dpdk-next-virtio/master.

Thanks,
Maxime