From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from sender163-mail.zoho.com (sender163-mail.zoho.com [74.201.84.163]) by dpdk.org (Postfix) with ESMTP id 95F8A2C63 for ; Wed, 6 Apr 2016 06:14:18 +0200 (CEST) Received: from localhost (177.220.172.170 [177.220.172.170]) by mx.zohomail.com with SMTPS id 1459916054440159.95210314947167; Tue, 5 Apr 2016 21:14:14 -0700 (PDT) Date: Wed, 6 Apr 2016 01:14:09 -0300 From: Flavio Leitner To: Yuanhan Liu Cc: Ilya Maximets , dev@dpdk.org, Dyasly Sergey , Thomas Monjalon , "Xie, Huawei" Message-ID: <20160406041409.GA8362@plex.redhat.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> <20160405054733.GO3080@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160405054733.GO3080@yliu-dev.sh.intel.com> User-Agent: Mutt/1.5.24 (2015-08-30) X-Zoho-Virus-Status: 1 Subject: Re: [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: Wed, 06 Apr 2016 04:14:19 -0000 On Tue, Apr 05, 2016 at 01:47:33PM +0800, Yuanhan Liu wrote: > 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. There is a plan to use vHost PMD, so from OVS point of view the virtio stuff would be hidden because vhost PMD would look like just as a regular ethernet, right? I think we are waiting for 16.04 to be released with that so we can start push changes to OVS as well. -- fbl