| Message ID | 1514310190-140916-1-git-send-email-junjie.j.chen@intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Yuanhan Liu |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| ci/checkpatch | success | coding style OK |
| ci/Intel-compilation | success | Compilation OK |
Hi, > -----Original Message----- > From: Chen, Junjie J > Sent: Wednesday, December 27, 2017 1:43 AM > To: Wang, Xiao W <xiao.w.wang@intel.com>; yliu@fridaylinux.org; > maxime.coquelin@redhat.com; Bie, Tiwei <tiwei.bie@intel.com> > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com> > Subject: [PATCH v6] vhost: support virtqueue interrupt/notification > suppression > > The driver can suppress interrupt when VIRTIO_F_EVENT_IDX feature bit is > negotiated. The driver set vring flags to 0, and MAY use used_event in > available ring to advise device interrupt util reach an index specified > by used_event. The device ignore the lower bit of vring flags, and send > an interrupt when index reach used_event. > > The device can suppress notification in a manner analogous to the ways > driver suppress interrupt. The device manipulates flags or avail_event in > the used ring in the same way the driver manipulates flags or used_event in > available ring. > > This patch is to enable this feature in vhost. > > Signed-off-by: Junjie Chen <junjie.j.chen@intel.com> > > v6: > Use volatile qualifier to access avail event idx. > > v5: > Remove updating avail event index in backend. > > v2-v4: > Use definition of VIRTIO_F_EVENT_IDX from kernel. > > --- > lib/librte_vhost/vhost.h | 3 +++ > lib/librte_vhost/virtio_net.c | 46 +++++++++++++++++++++++++++++------------- > - > 2 files changed, 34 insertions(+), 15 deletions(-) > > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 1cc81c1..fc41c20 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -103,6 +103,8 @@ struct vhost_virtqueue { > > uint16_t last_avail_idx; > uint16_t last_used_idx; > + /* Last used index we notify to front end. */ > + uint16_t signalled_used; > #define VIRTIO_INVALID_EVENTFD (-1) > #define VIRTIO_UNINITIALIZED_EVENTFD (-2) > > @@ -211,6 +213,7 @@ struct vhost_msg { > (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \ > (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \ > (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | \ > + (1ULL << VIRTIO_RING_F_EVENT_IDX) | \ > (1ULL << VIRTIO_NET_F_MTU) | \ > (1ULL << VIRTIO_F_IOMMU_PLATFORM)) > > diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c > index 6fee16e..f5777fc 100644 > --- a/lib/librte_vhost/virtio_net.c > +++ b/lib/librte_vhost/virtio_net.c > @@ -52,6 +52,34 @@ > > #define MAX_BATCH_LEN 256 > > +#define vhost_used_event(vr) \ > + (*(volatile uint16_t*)&(vr)->avail->ring[(vr)->size]) > + > +static __rte_always_inline void > +vhost_notify(struct virtio_net *dev, struct vhost_virtqueue *vq) > +{ > + /* Don't notify guest if we don't reach index specified by guest. */ > + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { > + uint16_t old = vq->signalled_used; > + uint16_t new = vq->last_used_idx; > + > + LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, > new=%d\n", > + __func__, > + vhost_used_event(vq), > + old, new); > + if (vring_need_event(vhost_used_event(vq), new, old) > + && (vq->callfd >= 0)) { > + vq->signalled_used = vq->last_used_idx; > + eventfd_write(vq->callfd, (eventfd_t) 1); > + } > + } else { > + /* Kick the guest if necessary. */ > + if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > + && (vq->callfd >= 0)) > + eventfd_write(vq->callfd, (eventfd_t)1); > + } > +} > + > static bool > is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t nr_vring) > { > @@ -410,11 +438,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t > queue_id, > > /* flush used->idx update before we read avail->flags. */ > rte_mb(); > - > - /* Kick the guest if necessary. */ > - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > - && (vq->callfd >= 0)) > - eventfd_write(vq->callfd, (eventfd_t)1); > + vhost_notify(dev, vq); > out: > if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) > vhost_user_iotlb_rd_unlock(vq); > @@ -704,11 +728,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t > queue_id, > > /* flush used->idx update before we read avail->flags. */ > rte_mb(); > - > - /* Kick the guest if necessary. */ > - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > - && (vq->callfd >= 0)) > - eventfd_write(vq->callfd, (eventfd_t)1); > + vhost_notify(dev, vq); > } > > out: > @@ -1106,11 +1126,7 @@ update_used_idx(struct virtio_net *dev, struct > vhost_virtqueue *vq, > vq->used->idx += count; > vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx), > sizeof(vq->used->idx)); > - > - /* Kick guest if required. */ > - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) > - && (vq->callfd >= 0)) > - eventfd_write(vq->callfd, (eventfd_t)1); > + vhost_notify(dev, vq); > } > > static __rte_always_inline struct zcopy_mbuf * > -- > 2.0.1 Reviewed-by: Xiao Wang <xiao.w.wang@intel.com>
On Tue, Dec 26, 2017 at 12:43:10PM -0500, Junjie Chen wrote: > The driver can suppress interrupt when VIRTIO_F_EVENT_IDX feature bit is > negotiated. The driver set vring flags to 0, and MAY use used_event in > available ring to advise device interrupt util reach an index specified > by used_event. The device ignore the lower bit of vring flags, and send > an interrupt when index reach used_event. > > The device can suppress notification in a manner analogous to the ways > driver suppress interrupt. The device manipulates flags or avail_event in > the used ring in the same way the driver manipulates flags or used_event in > available ring. > > This patch is to enable this feature in vhost. > > Signed-off-by: Junjie Chen <junjie.j.chen@intel.com> You need put "---" before the change log. Otherwise, it will be tracked in the commit log. > +#define vhost_used_event(vr) \ > + (*(volatile uint16_t*)&(vr)->avail->ring[(vr)->size]) > + > +static __rte_always_inline void > +vhost_notify(struct virtio_net *dev, struct vhost_virtqueue *vq) > +{ > + /* Don't notify guest if we don't reach index specified by guest. */ > + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { > + uint16_t old = vq->signalled_used; > + uint16_t new = vq->last_used_idx; > + > + LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, new=%d\n", > + __func__, > + vhost_used_event(vq), > + old, new); > + if (vring_need_event(vhost_used_event(vq), new, old) It's a bit weird that you use one from the standard linux header file (vring_need_event), while you define you own one (vhost_used_event). Note that the system header file also has "vring_used_event()" defined. Besides that, I have few more comments (and some requirements): - It'd be much better if there is a Tested-by tag. Expeclitly, I'm asking a test with Linux kernel virtio-net driver in guest. - I also hope you could have done a build test on some old distributions. AFAIK, the two macros (vring_need_event and vring_used_event) come from kernel 3.0 (or above). Any kernel older than that would fail the build. - I'd be great if you could make a new one based on top of my latest tree: I have just applied a patchset that should conflict with this one. --yliu
Hi > -----Original Message----- > From: Yuanhan Liu [mailto:yliu@fridaylinux.org] > Sent: Monday, January 8, 2018 10:07 PM > To: Chen, Junjie J <junjie.j.chen@intel.com> > Cc: Wang, Xiao W <xiao.w.wang@intel.com>; maxime.coquelin@redhat.com; > Bie, Tiwei <tiwei.bie@intel.com>; dev@dpdk.org; Yao, Lei A > <lei.a.yao@intel.com> > Subject: Re: [PATCH v6] vhost: support virtqueue interrupt/notification > suppression > > On Tue, Dec 26, 2017 at 12:43:10PM -0500, Junjie Chen wrote: > > The driver can suppress interrupt when VIRTIO_F_EVENT_IDX feature bit > > is negotiated. The driver set vring flags to 0, and MAY use used_event > > in available ring to advise device interrupt util reach an index > > specified by used_event. The device ignore the lower bit of vring > > flags, and send an interrupt when index reach used_event. > > > > The device can suppress notification in a manner analogous to the ways > > driver suppress interrupt. The device manipulates flags or avail_event > > in the used ring in the same way the driver manipulates flags or > > used_event in available ring. > > > > This patch is to enable this feature in vhost. > > > > Signed-off-by: Junjie Chen <junjie.j.chen@intel.com> > > You need put "---" before the change log. Otherwise, it will be tracked in the > commit log. Will update this. > > > +#define vhost_used_event(vr) \ > > + (*(volatile uint16_t*)&(vr)->avail->ring[(vr)->size]) > > + > > +static __rte_always_inline void > > +vhost_notify(struct virtio_net *dev, struct vhost_virtqueue *vq) { > > + /* Don't notify guest if we don't reach index specified by guest. */ > > + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { > > + uint16_t old = vq->signalled_used; > > + uint16_t new = vq->last_used_idx; > > + > > + LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, > new=%d\n", > > + __func__, > > + vhost_used_event(vq), > > + old, new); > > + if (vring_need_event(vhost_used_event(vq), new, old) > > It's a bit weird that you use one from the standard linux header file > (vring_need_event), while you define you own one (vhost_used_event). > Note that the system header file also has "vring_used_event()" defined. The vring_used_event is defined and used for virtio in kernel, kernel defines a vhost_used_event in vhost.c for vhost, so I just use a separated macro for vhost end. I'd like to define both vhost_need_event and vhost_used_event in vhost.h to remove potential build issue in old linux distribution and also to keep consistent. Is that OK for you? > > Besides that, I have few more comments (and some requirements): > > - It'd be much better if there is a Tested-by tag. Expeclitly, > I'm asking a test with Linux kernel virtio-net driver in guest. Sure. > > - I also hope you could have done a build test on some old distributions. > AFAIK, the two macros (vring_need_event and vring_used_event) come > from kernel 3.0 (or above). Any kernel older than that would fail > the build. > > - I'd be great if you could make a new one based on top of my latest > tree: I have just applied a patchset that should conflict with this > one. Sure, will do this. > > --yliu
On Tue, Jan 09, 2018 at 02:12:02AM +0000, Chen, Junjie J wrote: > > > + if (vring_need_event(vhost_used_event(vq), new, old) > > > > It's a bit weird that you use one from the standard linux header file > > (vring_need_event), while you define you own one (vhost_used_event). > > Note that the system header file also has "vring_used_event()" defined. > The vring_used_event is defined and used for virtio in kernel, kernel defines a vhost_used_event in vhost.c for vhost, so I just use a separated macro for vhost end. > > I'd like to define both vhost_need_event and vhost_used_event in vhost.h to remove potential build issue in old linux distribution and also to keep consistent. Is that OK for you? Yes. --yliu
diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h index 1cc81c1..fc41c20 100644 --- a/lib/librte_vhost/vhost.h +++ b/lib/librte_vhost/vhost.h @@ -103,6 +103,8 @@ struct vhost_virtqueue { uint16_t last_avail_idx; uint16_t last_used_idx; + /* Last used index we notify to front end. */ + uint16_t signalled_used; #define VIRTIO_INVALID_EVENTFD (-1) #define VIRTIO_UNINITIALIZED_EVENTFD (-2) @@ -211,6 +213,7 @@ struct vhost_msg { (1ULL << VIRTIO_NET_F_GUEST_TSO4) | \ (1ULL << VIRTIO_NET_F_GUEST_TSO6) | \ (1ULL << VIRTIO_RING_F_INDIRECT_DESC) | \ + (1ULL << VIRTIO_RING_F_EVENT_IDX) | \ (1ULL << VIRTIO_NET_F_MTU) | \ (1ULL << VIRTIO_F_IOMMU_PLATFORM)) diff --git a/lib/librte_vhost/virtio_net.c b/lib/librte_vhost/virtio_net.c index 6fee16e..f5777fc 100644 --- a/lib/librte_vhost/virtio_net.c +++ b/lib/librte_vhost/virtio_net.c @@ -52,6 +52,34 @@ #define MAX_BATCH_LEN 256 +#define vhost_used_event(vr) \ + (*(volatile uint16_t*)&(vr)->avail->ring[(vr)->size]) + +static __rte_always_inline void +vhost_notify(struct virtio_net *dev, struct vhost_virtqueue *vq) +{ + /* Don't notify guest if we don't reach index specified by guest. */ + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { + uint16_t old = vq->signalled_used; + uint16_t new = vq->last_used_idx; + + LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, new=%d\n", + __func__, + vhost_used_event(vq), + old, new); + if (vring_need_event(vhost_used_event(vq), new, old) + && (vq->callfd >= 0)) { + vq->signalled_used = vq->last_used_idx; + eventfd_write(vq->callfd, (eventfd_t) 1); + } + } else { + /* Kick the guest if necessary. */ + if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) + && (vq->callfd >= 0)) + eventfd_write(vq->callfd, (eventfd_t)1); + } +} + static bool is_valid_virt_queue_idx(uint32_t idx, int is_tx, uint32_t nr_vring) { @@ -410,11 +438,7 @@ virtio_dev_rx(struct virtio_net *dev, uint16_t queue_id, /* flush used->idx update before we read avail->flags. */ rte_mb(); - - /* Kick the guest if necessary. */ - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) - && (vq->callfd >= 0)) - eventfd_write(vq->callfd, (eventfd_t)1); + vhost_notify(dev, vq); out: if (dev->features & (1ULL << VIRTIO_F_IOMMU_PLATFORM)) vhost_user_iotlb_rd_unlock(vq); @@ -704,11 +728,7 @@ virtio_dev_merge_rx(struct virtio_net *dev, uint16_t queue_id, /* flush used->idx update before we read avail->flags. */ rte_mb(); - - /* Kick the guest if necessary. */ - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) - && (vq->callfd >= 0)) - eventfd_write(vq->callfd, (eventfd_t)1); + vhost_notify(dev, vq); } out: @@ -1106,11 +1126,7 @@ update_used_idx(struct virtio_net *dev, struct vhost_virtqueue *vq, vq->used->idx += count; vhost_log_used_vring(dev, vq, offsetof(struct vring_used, idx), sizeof(vq->used->idx)); - - /* Kick guest if required. */ - if (!(vq->avail->flags & VRING_AVAIL_F_NO_INTERRUPT) - && (vq->callfd >= 0)) - eventfd_write(vq->callfd, (eventfd_t)1); + vhost_notify(dev, vq); } static __rte_always_inline struct zcopy_mbuf *
The driver can suppress interrupt when VIRTIO_F_EVENT_IDX feature bit is negotiated. The driver set vring flags to 0, and MAY use used_event in available ring to advise device interrupt util reach an index specified by used_event. The device ignore the lower bit of vring flags, and send an interrupt when index reach used_event. The device can suppress notification in a manner analogous to the ways driver suppress interrupt. The device manipulates flags or avail_event in the used ring in the same way the driver manipulates flags or used_event in available ring. This patch is to enable this feature in vhost. Signed-off-by: Junjie Chen <junjie.j.chen@intel.com> v6: Use volatile qualifier to access avail event idx. v5: Remove updating avail event index in backend. v2-v4: Use definition of VIRTIO_F_EVENT_IDX from kernel. --- lib/librte_vhost/vhost.h | 3 +++ lib/librte_vhost/virtio_net.c | 46 +++++++++++++++++++++++++++++-------------- 2 files changed, 34 insertions(+), 15 deletions(-)