| Message ID | 20181011130655.5485-1-tiwei.bie@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Maxime Coquelin |
| Headers | show |
| Series | vhost: fix notification for packed ring | expand |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
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
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;
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;
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 --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;
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(-)