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 49F5D2952 for ; Tue, 5 Apr 2016 07:46:08 +0200 (CEST) Received: from orsmga003.jf.intel.com ([10.7.209.27]) by fmsmga101.fm.intel.com with ESMTP; 04 Apr 2016 22:46:07 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,442,1455004800"; d="scan'208";a="778213293" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.191]) by orsmga003.jf.intel.com with ESMTP; 04 Apr 2016 22:46:06 -0700 Date: Tue, 5 Apr 2016 13:47:33 +0800 From: Yuanhan Liu To: Ilya Maximets Cc: dev@dpdk.org, Dyasly Sergey , Thomas Monjalon , Flavio Leitner , "Xie, Huawei" Message-ID: <20160405054733.GO3080@yliu-dev.sh.intel.com> References: <1455863563-15751-1-git-send-email-i.maximets@samsung.com> <1455863563-15751-3-git-send-email-i.maximets@samsung.com> <20160219070650.GS21426@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160219070650.GS21426@yliu-dev.sh.intel.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: [dpdk-dev] [RFC] vhost-user public struct refactor (was Re: [PATCH RFC 2/4] vhost: make buf vector for scatter RX) local. X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 05 Apr 2016 05:46:08 -0000 On Fri, Feb 19, 2016 at 03:06:50PM +0800, Yuanhan Liu wrote: > On Fri, Feb 19, 2016 at 09:32:41AM +0300, Ilya Maximets wrote: > > Array of buf_vector's is just an array for temporary storing information > > about available descriptors. It used only locally in virtio_dev_merge_rx() > > and there is no reason for that array to be shared. > > > > Fix that by allocating local buf_vec inside virtio_dev_merge_rx(). > > > > Signed-off-by: Ilya Maximets > > --- > > lib/librte_vhost/rte_virtio_net.h | 1 - > > lib/librte_vhost/vhost_rxtx.c | 45 ++++++++++++++++++++------------------- > > 2 files changed, 23 insertions(+), 23 deletions(-) > > > > diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h > > index 10dcb90..ae1e4fb 100644 > > --- a/lib/librte_vhost/rte_virtio_net.h > > +++ b/lib/librte_vhost/rte_virtio_net.h > > @@ -91,7 +91,6 @@ struct vhost_virtqueue { > > int kickfd; /**< Currently unused as polling mode is enabled. */ > > int enabled; > > uint64_t reserved[16]; /**< Reserve some spaces for future extension. */ > > - struct buf_vector buf_vec[BUF_VECTOR_MAX]; /**< for scatter RX. */ > > } __rte_cache_aligned; > > I like this kind of cleanup, however, it breaks ABI. So, I was considering to add vhost-user Tx delayed-copy (or zero copy) support recently, which comes to yet another ABI violation, as we need add a new field to virtio_memory_regions struct to do guest phys addr to host phys addr translation. You may ask, however, that why do we need expose virtio_memory_regions struct to users at all? You are right, we don't have to. And here is the thing: we exposed way too many fields (or even structures) than necessary. Say, vhost_virtqueue struct should NOT be exposed to user at all: application just need to tell the right queue id to locate a specific queue, and that's all. The structure should be defined in an internal header file. With that, we could do any changes to it we want, without worrying about that we may offense the painful ABI rules. Similar changes could be done to virtio_net struct as well, just exposing very few fields that are necessary and moving all others to an internal structure. Huawei then suggested a more radical yet much cleaner one: just exposing a virtio_net handle to application, just like the way kernel exposes an fd to user for locating a specific file. However, it's more than an ABI change; it's also an API change: some fields are referenced by applications, such as flags, virt_qp_nb. We could expose some new functions to access them though. I'd vote for this one, as it sounds very clean to me. This would also solve the block issue of this patch. Though it would break OVS, I'm thinking that'd be okay, as OVS has dependence on DPDK version: what we need to do is just to send few patches to OVS, and let it points to next release, say DPDK v16.07. Flavio, please correct me if I'm wrong. Thoughts/comments? --yliu