From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 224151B5DF for ; Fri, 29 Jun 2018 18:22:35 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 29 Jun 2018 09:22:35 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.51,285,1526367600"; d="scan'208";a="51514508" Received: from debian.sh.intel.com (HELO debian) ([10.67.104.228]) by fmsmga008.fm.intel.com with ESMTP; 29 Jun 2018 09:22:33 -0700 Date: Sat, 30 Jun 2018 00:22:35 +0800 From: Tiwei Bie To: Maxime Coquelin Cc: zhihong.wang@intel.com, jfreimann@redhat.com, dev@dpdk.org, mst@redhat.com, jasowang@redhat.com, wexu@redhat.com Message-ID: <20180629162235.GA976@debian> References: <20180622134327.18973-1-maxime.coquelin@redhat.com> <20180622134327.18973-15-maxime.coquelin@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20180622134327.18973-15-maxime.coquelin@redhat.com> User-Agent: Mutt/1.9.5 (2018-04-13) Subject: Re: [dpdk-dev] [PATCH v5 14/15] vhost: add notification for packed ring X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 29 Jun 2018 16:22:36 -0000 On Fri, Jun 22, 2018 at 03:43:26PM +0200, Maxime Coquelin wrote: > Signed-off-by: Maxime Coquelin > --- > lib/librte_vhost/vhost.c | 55 +++++++++++++++++++++++++++---- > lib/librte_vhost/vhost.h | 71 ++++++++++++++++++++++++++++++++++++---- > lib/librte_vhost/vhost_user.c | 29 +++++++++++++--- > lib/librte_vhost/virtio-packed.h | 11 +++++++ > lib/librte_vhost/virtio_net.c | 12 +++---- > 5 files changed, 154 insertions(+), 24 deletions(-) > > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c > index 7cbf1eded..c26162542 100644 > --- a/lib/librte_vhost/vhost.c > +++ b/lib/librte_vhost/vhost.c > @@ -595,7 +595,11 @@ rte_vhost_vring_call(int vid, uint16_t vring_idx) > if (!vq) > return -1; > > - vhost_vring_call(dev, vq); > + if (vq_is_packed(dev)) > + vhost_vring_call_packed(dev, vq); > + else > + vhost_vring_call_split(dev, vq); > + > return 0; > } > > @@ -616,20 +620,59 @@ rte_vhost_avail_entries(int vid, uint16_t queue_id) > return *(volatile uint16_t *)&vq->avail->idx - vq->last_used_idx; > } > > +static inline int > +vhost_enable_notify_split(struct vhost_virtqueue *vq, int enable) > +{ > + if (enable) > + vq->used->flags &= ~VRING_USED_F_NO_NOTIFY; > + else > + vq->used->flags |= VRING_USED_F_NO_NOTIFY; > + > + return 0; > +} > + > +static inline int > +vhost_enable_notify_packed(struct virtio_net *dev, > + struct vhost_virtqueue *vq, int enable) > +{ > + uint16_t flags; > + > + if (!enable) { > + vq->device_event->desc_event_flags = RING_EVENT_FLAGS_DISABLE; > + return 0; > + } > + > + flags = RING_EVENT_FLAGS_DISABLE; Should be RING_EVENT_FLAGS_ENABLE. > + if (dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX)) { > + flags = RING_EVENT_FLAGS_DESC; > + vq->device_event->desc_event_off_wrap = vq->last_avail_idx | > + vq->avail_wrap_counter << 15; > + } > + > + rte_smp_wmb(); > + > + vq->device_event->desc_event_flags = flags; > + > + rte_smp_wmb(); > + > + return 0; > +} > + > int > rte_vhost_enable_guest_notification(int vid, uint16_t queue_id, int enable) > { > struct virtio_net *dev = get_device(vid); > + struct vhost_virtqueue *vq; > > if (!dev) > return -1; > > - if (enable) > - dev->virtqueue[queue_id]->used->flags &= > - ~VRING_USED_F_NO_NOTIFY; > + vq = dev->virtqueue[queue_id]; > + > + if (vq_is_packed(dev)) > + return vhost_enable_notify_packed(dev, vq, enable); > else > - dev->virtqueue[queue_id]->used->flags |= VRING_USED_F_NO_NOTIFY; > - return 0; > + return vhost_enable_notify_split(vq, enable); > } > > void > diff --git a/lib/librte_vhost/vhost.h b/lib/librte_vhost/vhost.h > index 5dcb61c71..99db688a8 100644 > --- a/lib/librte_vhost/vhost.h > +++ b/lib/librte_vhost/vhost.h > @@ -21,6 +21,7 @@ > > #include "rte_vhost.h" > #include "rte_vdpa.h" > +#include "virtio-packed.h" The definitions in virtio-packed.h probably will conflit with the definitions in linux/virtio_ring.h which is included by linux/vhost.h (which is also included by this file) when packed ring is upstreamed to linux. It probably will cause build issues in the future. > > /* Used to indicate that the device is running on a data core */ > #define VIRTIO_DEV_RUNNING 1 > @@ -95,8 +96,14 @@ struct vhost_virtqueue { > struct vring_desc *desc; > struct vring_desc_packed *desc_packed; > }; > - struct vring_avail *avail; > - struct vring_used *used; > + union { > + struct vring_avail *avail; > + struct vring_packed_desc_event *driver_event; > + }; > + union { > + struct vring_used *used; > + struct vring_packed_desc_event *device_event; > + }; > uint32_t size; > > uint16_t last_avail_idx; > @@ -614,7 +621,7 @@ vhost_need_event(uint16_t event_idx, uint16_t new_idx, uint16_t old) > } > > static __rte_always_inline void > -vhost_vring_call(struct virtio_net *dev, struct vhost_virtqueue *vq) > +vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq) > { > /* Flush used->idx update before we read avail->flags. */ > rte_mb(); > @@ -625,11 +632,11 @@ vhost_vring_call(struct virtio_net *dev, struct vhost_virtqueue *vq) > uint16_t new = vq->last_used_idx; > > VHOST_LOG_DEBUG(VHOST_DATA, "%s: used_event_idx=%d, old=%d, new=%d\n", > - __func__, > - vhost_used_event(vq), > - old, new); > + __func__, > + vhost_used_event(vq), > + old, new); > if (vhost_need_event(vhost_used_event(vq), new, old) > - && (vq->callfd >= 0)) { > + && (vq->callfd >= 0)) { > vq->signalled_used = vq->last_used_idx; > eventfd_write(vq->callfd, (eventfd_t) 1); > } > @@ -641,4 +648,54 @@ vhost_vring_call(struct virtio_net *dev, struct vhost_virtqueue *vq) > } > } > > +static __rte_always_inline void > +vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq) > +{ > + uint16_t old, new, off, off_wrap, wrap; > + bool kick = false; > + > + There is no need to have two blank lines. > + /* Flush used desc update. */ > + rte_smp_mb(); > + > + if (!(dev->features & (1ULL << VIRTIO_RING_F_EVENT_IDX))) { > + if (vq->driver_event->desc_event_flags != > + RING_EVENT_FLAGS_DISABLE) > + kick = true; > + goto kick; > + } > + > + old = vq->signalled_used; > + new = vq->last_used_idx; > + vq->signalled_used = new; > + > + if (vq->driver_event->desc_event_flags != RING_EVENT_FLAGS_DESC) { > + if (vq->driver_event->desc_event_flags != > + RING_EVENT_FLAGS_DISABLE) > + kick = true; > + goto kick; > + } > + > + rte_smp_rmb(); > + > + off_wrap = vq->driver_event->desc_event_off_wrap; > + off = off_wrap & ~(1 << 15); > + wrap = vq->used_wrap_counter; > + > + if (new < old) { > + new += vq->size; > + wrap ^= 1; > + } > + > + if (wrap != off_wrap >> 15) > + off += vq->size; > + > + if (vhost_need_event(off, new, old)) > + kick = true; > + > +kick: > + if (kick) > + eventfd_write(vq->callfd, (eventfd_t)1); > +} > + > #endif /* _VHOST_NET_CDEV_H_ */ > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c > index a08d99314..c74924564 100644 > --- a/lib/librte_vhost/vhost_user.c > +++ b/lib/librte_vhost/vhost_user.c > @@ -509,9 +509,6 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index) > len = sizeof(struct vring_desc_packed) * vq->size; > vq->desc_packed = (struct vring_desc_packed *) ring_addr_to_vva > (dev, vq, addr->desc_user_addr, &len); > - vq->desc = NULL; > - vq->avail = NULL; > - vq->used = NULL; There is no need to add them from the beginning. > vq->log_guest_addr = 0; > if (vq->desc_packed == 0 || > len != sizeof(struct vring_desc_packed) * > @@ -526,6 +523,30 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index) > vq = dev->virtqueue[vq_index]; > addr = &vq->ring_addrs; > > + len = sizeof(struct vring_packed_desc_event); > + vq->driver_event = (struct vring_packed_desc_event *) > + (uintptr_t)ring_addr_to_vva(dev, > + vq, addr->avail_user_addr, &len); > + if (vq->driver_event == 0 || It's better to compare with NULL. > + len != sizeof(struct vring_packed_desc_event)) { > + RTE_LOG(DEBUG, VHOST_CONFIG, > + "(%d) failed to find driver area address.\n", > + dev->vid); > + return dev; > + } > + > + len = sizeof(struct vring_packed_desc_event); > + vq->device_event = (struct vring_packed_desc_event *) > + (uintptr_t)ring_addr_to_vva(dev, > + vq, addr->used_user_addr, &len); > + if (vq->device_event == 0 || It's better to compare with NULL. > + len != sizeof(struct vring_packed_desc_event)) { > + RTE_LOG(DEBUG, VHOST_CONFIG, > + "(%d) failed to find device area address.\n", > + dev->vid); > + return dev; > + } > + > return dev; > } > > @@ -542,8 +563,6 @@ translate_ring_addresses(struct virtio_net *dev, int vq_index) > dev->vid); > return dev; > } > - vq->desc_packed = NULL; There is no need to add it from the beginning. Best regards, Tiwei Bie