From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5355AA10DA for ; Thu, 1 Aug 2019 08:40:26 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id C12031C0C5; Thu, 1 Aug 2019 08:40:25 +0200 (CEST) Received: from mga17.intel.com (mga17.intel.com [192.55.52.151]) by dpdk.org (Postfix) with ESMTP id 594101BF9D for ; Thu, 1 Aug 2019 08:40:22 +0200 (CEST) X-Amp-Result: UNKNOWN X-Amp-Original-Verdict: FILE UNKNOWN X-Amp-File-Uploaded: False Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga107.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 31 Jul 2019 23:40:21 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.64,333,1559545200"; d="scan'208";a="191547628" Received: from npg-dpdk-virtio-tbie-2.sh.intel.com (HELO ___) ([10.67.104.66]) by fmsmga001.fm.intel.com with ESMTP; 31 Jul 2019 23:40:19 -0700 Date: Thu, 1 Aug 2019 14:38:28 +0800 From: Tiwei Bie To: JinYu Cc: dev@dpdk.org, changpeng.liu@intel.com, maxime.coquelin@redhat.com, zhihong.wang@intel.com, Lin Li , Xun Ni , Yu Zhang Message-ID: <20190801063827.GA22488@___> References: <20190725212335.9675> <20190731204050.40633-1-jin.yu@intel.com> <20190731204050.40633-2-jin.yu@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20190731204050.40633-2-jin.yu@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Subject: Re: [dpdk-dev] [PATCH v4 1/2] vhost: support inflight share memory protocol feature 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: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Aug 01, 2019 at 04:40:49AM +0800, JinYu wrote: > This patch introduces two new messages VHOST_USER_GET_INFLIGHT_FD > and VHOST_USER_SET_INFLIGHT_FD to support transferring a shared > buffer between qemu and backend. > > Firstly, qemu uses VHOST_USER_GET_INFLIGHT_FD to get the > shared buffer from backend. Then qemu should send it back > through VHOST_USER_SET_INFLIGHT_FD each time we start vhost-user. > > This shared buffer is used to process inflight I/O when backend > reconnect. > > Signed-off-by: Lin Li > Signed-off-by: Xun Ni > Signed-off-by: Yu Zhang > Signed-off-by: Jin Yu > --- > v1 - specify the APIs are split-ring only > v2 - fix APIs and judge split or packed > v3 - Add rte_vhost_ prefix and fix one issue. > v4 - add the packed ring support > --- > lib/librte_vhost/rte_vhost.h | 301 +++++++++++++++++- > lib/librte_vhost/rte_vhost_version.map | 12 + > lib/librte_vhost/vhost.c | 399 ++++++++++++++++++++++- > lib/librte_vhost/vhost.h | 54 ++-- > lib/librte_vhost/vhost_user.c | 423 ++++++++++++++++++++++++- > lib/librte_vhost/vhost_user.h | 13 +- > 6 files changed, 1173 insertions(+), 29 deletions(-) There are some coding style issues reported by devtools/checkpatches.sh, please get them fixed: WARNING:LONG_LINE: line over 80 characters #372: FILE: lib/librte_vhost/rte_vhost.h:952: + uint16_t queue_id, bool *avail_wrap_counter, bool *used_wrap_counter); WARNING:LONG_LINE: line over 80 characters #622: FILE: lib/librte_vhost/vhost.c:935: + vq->inflight_packed->desc[free_head].addr = vq->desc_packed[head].addr; WARNING:LONG_LINE: line over 80 characters #623: FILE: lib/librte_vhost/vhost.c:936: + vq->inflight_packed->desc[free_head].len = vq->desc_packed[head].len; WARNING:LONG_LINE: line over 80 characters #624: FILE: lib/librte_vhost/vhost.c:937: + vq->inflight_packed->desc[free_head].flags = vq->desc_packed[head].flags; WARNING:LONG_LINE: line over 80 characters #625: FILE: lib/librte_vhost/vhost.c:938: + vq->inflight_packed->desc[free_head].id = vq->desc_packed[head].id; WARNING:LONG_LINE: line over 80 characters #783: FILE: lib/librte_vhost/vhost.c:1096: + vq->inflight_packed->used_idx &= vq->inflight_packed->desc_num - 1; WARNING:LONG_LINE: line over 80 characters #803: FILE: lib/librte_vhost/vhost.c:1278: + if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL) WARNING:LONG_LINE: line over 80 characters #826: FILE: lib/librte_vhost/vhost.c:1301: + *last_avail_idx = dev->virtqueue[queue_id]->inflight_packed->old_used_idx; WARNING:LONG_LINE: line over 80 characters #833: FILE: lib/librte_vhost/vhost.c:1308: + uint16_t queue_id, bool *avail_wrap_counter, bool *used_wrap_counter) WARNING:LONG_LINE: line over 80 characters #837: FILE: lib/librte_vhost/vhost.c:1312: + if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL) WARNING:LONG_LINE: line over 80 characters #1116: FILE: lib/librte_vhost/vhost_user.c:1248: + sizeof(uint64_t) * 1 + sizeof(uint16_t) * 4, INFLIGHT_ALIGNMENT); WARNING:LONG_LINE: line over 80 characters #1122: FILE: lib/librte_vhost/vhost_user.c:1254: + sizeof(uint64_t) * 1 + sizeof(uint16_t) * 6 + sizeof(uint8_t) * 9, WARNING:LONG_LINE: line over 80 characters #1258: FILE: lib/librte_vhost/vhost_user.c:1390: + vq->inflight_packed = (struct inflight_info_packed *)addr; WARNING:LONG_LINE: line over 80 characters #1293: FILE: lib/librte_vhost/vhost_user.c:1454: +vhost_check_queue_inflights_split(struct virtio_net *dev, struct vhost_virtqueue *vq) WARNING:LONG_LINE: line over 80 characters #1318: FILE: lib/librte_vhost/vhost_user.c:1479: + inflight_split->desc[inflight_split->last_inflight_io].inflight = 0; WARNING:LONG_LINE: line over 80 characters #1349: FILE: lib/librte_vhost/vhost_user.c:1510: + resubmit->resubmit_list[resubmit->resubmit_num].index = i; WARNING:LONG_LINE: line over 80 characters #1350: FILE: lib/librte_vhost/vhost_user.c:1511: + resubmit->resubmit_list[resubmit->resubmit_num].counter = WARNING:LONG_LINE: line over 80 characters #1394: FILE: lib/librte_vhost/vhost_user.c:1555: + if (inflight_packed->desc[inflight_packed->old_used_idx].inflight == 0) { WARNING:LONG_LINE: line over 80 characters #1395: FILE: lib/librte_vhost/vhost_user.c:1556: + inflight_packed->old_used_idx = inflight_packed->used_idx; ERROR:TRAILING_WHITESPACE: trailing whitespace #1396: FILE: lib/librte_vhost/vhost_user.c:1557: +^I^I^Iinflight_packed->old_used_wrap_counter = $ WARNING:LONG_LINE: line over 80 characters #1398: FILE: lib/librte_vhost/vhost_user.c:1559: + inflight_packed->old_free_head = inflight_packed->free_head; WARNING:LONG_LINE: line over 80 characters #1400: FILE: lib/librte_vhost/vhost_user.c:1561: + inflight_packed->used_idx = inflight_packed->old_used_idx; WARNING:LONG_LINE: line over 80 characters #1403: FILE: lib/librte_vhost/vhost_user.c:1564: + inflight_packed->free_head = inflight_packed->old_free_head; WARNING:LONG_LINE: line over 80 characters #1420: FILE: lib/librte_vhost/vhost_user.c:1581: + sizeof(struct rte_vhost_resubmit_desc)); WARNING:LONG_LINE: line over 80 characters #1428: FILE: lib/librte_vhost/vhost_user.c:1589: + resubmit->resubmit_list[resubmit->resubmit_num].index = i; WARNING:LONG_LINE: line over 80 characters #1429: FILE: lib/librte_vhost/vhost_user.c:1590: + resubmit->resubmit_list[resubmit->resubmit_num].counter = WARNING:NO_AUTHOR_SIGN_OFF: Missing Signed-off-by: line by nominal patch author 'JinYu ' total: 1 errors, 26 warnings, 1425 lines checked __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. __rte_experimental must appear alone on the line immediately preceding the return type of a function. > + > +struct vring_packed_desc { > + uint64_t addr; > + uint32_t len; > + uint16_t id; > + uint16_t flags; > +}; You should keep the guard, otherwise there will be build issues when linux/virtio_ring.h defines above type. > + > +struct vring_split_desc { > + uint64_t addr; > + uint32_t len; > + uint16_t flags; > + uint16_t next; > +}; You don't need to redefine the descriptor for split ring, you can use the one provided by linux/virtio_ring.h > +int > +rte_vhost_set_inflight_desc_packed(int vid, uint16_t vring_idx, > + uint16_t head, uint16_t last, uint16_t *inflight_entry) > +{ > + struct virtio_net *dev; > + struct vhost_virtqueue *vq; > + uint16_t old_free_head, free_head; > + > + dev = get_device(vid); > + if (unlikely(!dev)) > + return -1; > + > + if (unlikely(!(dev->protocol_features & > + (1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)))) > + return 0; > + > + if (unlikely(!vq_is_packed(dev))) > + return -1; > + > + if (unlikely(vring_idx >= VHOST_MAX_VRING)) > + return -1; > + > + vq = dev->virtqueue[vring_idx]; > + if (unlikely(!vq)) > + return -1; > + > + if (unlikely(!vq->inflight_split)) > + return -1; > + > + free_head = vq->inflight_packed->free_head; > + old_free_head = vq->inflight_packed->old_free_head; > + > + /* init header descriptor */ > + vq->inflight_packed->desc[old_free_head].num = 0; > + vq->inflight_packed->desc[old_free_head].counter = vq->global_counter++; > + vq->inflight_packed->desc[old_free_head].inflight = 1; > + > + while (head != ((last + 1) & (vq->size - 1))) { > + vq->inflight_packed->desc[old_free_head].num++; > + vq->inflight_packed->desc[free_head].addr = vq->desc_packed[head].addr; > + vq->inflight_packed->desc[free_head].len = vq->desc_packed[head].len; > + vq->inflight_packed->desc[free_head].flags = vq->desc_packed[head].flags; > + vq->inflight_packed->desc[free_head].id = vq->desc_packed[head].id; > + vq->inflight_packed->desc[old_free_head].last = free_head; > + free_head = vq->inflight_packed->desc[free_head].next; > + vq->inflight_packed->free_head = free_head; > + head = (head + 1) & (vq->size - 1); You can't assume the ring size will be a power of two in packed ring. > +int > +rte_vhost_clr_inflight_desc_packed(int vid, uint16_t vring_idx, > + uint16_t head) > +{ > + struct virtio_net *dev; > + struct vhost_virtqueue *vq; > + > + dev = get_device(vid); > + if (unlikely(!dev)) > + return -1; > + > + if (unlikely(!(dev->protocol_features & > + (1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)))) > + return 0; > + > + if (unlikely(!vq_is_packed(dev))) > + return -1; > + > + if (unlikely(vring_idx >= VHOST_MAX_VRING)) > + return -1; > + > + vq = dev->virtqueue[vring_idx]; > + if (unlikely(!vq)) > + return -1; > + > + if (unlikely(!vq->inflight_packed)) > + return -1; > + > + rte_compiler_barrier(); > + > + vq->inflight_packed->desc[head].inflight = 0; > + > + rte_compiler_barrier(); > + > + vq->inflight_packed->old_free_head = vq->inflight_packed->free_head; > + vq->inflight_packed->old_used_idx = vq->inflight_packed->used_idx; > + vq->inflight_packed->old_used_wrap_counter = > + vq->inflight_packed->used_wrap_counter; The indent of the last line needs to be fixed. > +int > +rte_vhost_set_last_inflight_io_packed(int vid, uint16_t vring_idx, > + uint16_t head) > +{ > + struct virtio_net *dev; > + struct vhost_virtqueue *vq; > + > + dev = get_device(vid); > + if (unlikely(!dev)) > + return -1; > + > + if (unlikely(!(dev->protocol_features & > + (1ULL << VHOST_USER_PROTOCOL_F_INFLIGHT_SHMFD)))) > + return 0; > + > + if (unlikely(!vq_is_packed(dev))) > + return -1; > + > + if (unlikely(vring_idx >= VHOST_MAX_VRING)) > + return -1; > + > + vq = dev->virtqueue[vring_idx]; > + if (!vq) > + return -1; > + > + if (unlikely(!vq->inflight_packed)) > + return -1; > + > + vq->inflight_packed->desc[vq->inflight_packed->desc[head].last].next = > + vq->inflight_packed->free_head; Ditto. > + vq->inflight_packed->free_head = head; > + vq->inflight_packed->used_idx += vq->inflight_packed->desc[head].num; > + if (vq->inflight_packed->used_idx >= vq->inflight_packed->desc_num) { > + vq->inflight_packed->used_idx &= vq->inflight_packed->desc_num - 1; Can't assume the ring size will be a power of two. > + vq->inflight_packed->used_wrap_counter = > + !vq->inflight_packed->used_wrap_counter; The indent needs to be fixed. > + } > + > + return 0; > +} > + > uint16_t > rte_vhost_avail_entries(int vid, uint16_t queue_id) > { > @@ -950,6 +1270,61 @@ int rte_vhost_get_vring_base(int vid, uint16_t queue_id, > return 0; > } > > +int rte_vhost_get_vring_base_counter(int vid, uint16_t queue_id, > + bool *avail_wrap_counter, bool *used_wrap_counter) > +{ > + struct virtio_net *dev = get_device(vid); > + > + if (dev == NULL || avail_wrap_counter == NULL || used_wrap_counter == NULL) > + return -1; > + > + *avail_wrap_counter = dev->virtqueue[queue_id]->avail_wrap_counter; > + *used_wrap_counter = dev->virtqueue[queue_id]->used_wrap_counter; I think we should report wrap counters in the most significant bits for packed ring in rte_vhost_get_vring_base(). > +int rte_vhost_set_vring_base_counter(int vid, uint16_t queue_id, > + bool avail_wrap_counter, bool used_wrap_counter) > +{ > + struct virtio_net *dev = get_device(vid); > + > + if (!dev) > + return -1; > + > + dev->virtqueue[queue_id]->avail_wrap_counter = avail_wrap_counter; > + dev->virtqueue[queue_id]->used_wrap_counter = used_wrap_counter; Ditto. > + > + return 0; > +} > + Can you provide some steps in your cover letter for us to give it a try with your example? Thanks, Tiwei