From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com (mx1.redhat.com [209.132.183.28]) by dpdk.org (Postfix) with ESMTP id D559C594B for ; Wed, 2 Dec 2015 18:24:57 +0100 (CET) Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (Postfix) with ESMTPS id 42F63385D46; Wed, 2 Dec 2015 17:24:57 +0000 (UTC) Received: from redhat.com (vpn1-4-19.ams2.redhat.com [10.36.4.19]) by int-mx09.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with SMTP id tB2HOshm015209; Wed, 2 Dec 2015 12:24:55 -0500 Date: Wed, 2 Dec 2015 19:24:53 +0200 From: "Michael S. Tsirkin" To: Panu Matilainen Message-ID: <20151202192302-mutt-send-email-mst@redhat.com> References: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com> <1449027793-30975-2-git-send-email-yuanhan.liu@linux.intel.com> <565EF7E9.6080400@redhat.com> <20151202143101.GR2325@yliu-dev.sh.intel.com> <565F04AE.2070404@redhat.com> <20151202150923.GV2325@yliu-dev.sh.intel.com> <565F231B.5090203@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <565F231B.5090203@redhat.com> X-Scanned-By: MIMEDefang 2.68 on 10.5.11.22 Cc: dev@dpdk.org, Victor Kaplansky Subject: Re: [dpdk-dev] [PATCH 1/4] vhost: handle VHOST_USER_SET_LOG_BASE request 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, 02 Dec 2015 17:24:58 -0000 On Wed, Dec 02, 2015 at 06:58:03PM +0200, Panu Matilainen wrote: > On 12/02/2015 05:09 PM, Yuanhan Liu wrote: > >On Wed, Dec 02, 2015 at 04:48:14PM +0200, Panu Matilainen wrote: > >... > >>>>>diff --git a/lib/librte_vhost/rte_virtio_net.h b/lib/librte_vhost/rte_virtio_net.h > >>>>>index 5687452..416dac2 100644 > >>>>>--- a/lib/librte_vhost/rte_virtio_net.h > >>>>>+++ b/lib/librte_vhost/rte_virtio_net.h > >>>>>@@ -127,6 +127,8 @@ struct virtio_net { > >>>>> #define IF_NAME_SZ (PATH_MAX > IFNAMSIZ ? PATH_MAX : IFNAMSIZ) > >>>>> char ifname[IF_NAME_SZ]; /**< Name of the tap device or socket path. */ > >>>>> uint32_t virt_qp_nb; /**< number of queue pair we have allocated */ > >>>>>+ uint64_t log_size; /**< Size of log area */ > >>>>>+ uint8_t *log_base; /**< Where dirty pages are logged */ > >>>>> void *priv; /**< private context */ > >>>>> struct vhost_virtqueue *virtqueue[VHOST_MAX_QUEUE_PAIRS * 2]; /**< Contains all virtqueue information. */ > >>>>> } __rte_cache_aligned; > >>>> > >>>>This (and other changes in patch 2 breaks the librte_vhost ABI > >>>>again, so you'd need to at least add a deprecation note to 2.2 to be > >>>>able to do it in 2.3 at all according to the ABI policy. > >>> > >>>I was thinking that adding a new field (instead of renaming it or > >>>removing it) isn't an ABI break. So, I was wrong? > >> > >>Adding or removing a field in the middle of a public struct is > >>always an ABI break. Adding to the end often is too, but not always. > >>Renaming a field is an API break but not an ABI break - the compiler > >>cares but the cpu does not. > > > >Good to know. Thanks. > > > >> > >>>> > >>>>Perhaps a better option would be adding some padding to the structs > >>>>now for 2.2 since the vhost ABI is broken there anyway. That would > >>>>at least give a chance to keep it compatible from 2.2 to 2.3. > >>> > >>>It will not be compatible, unless we add exact same fields (not > >>>something like uint8_t pad[xx]). Otherwise, the pad field renaming > >>>is also an ABI break, right? > >> > >>There's no ABI (or API) break in changing reserved unused fields to > >>something else, as long as care is taken with sizes and alignment. > > > >as long as we don't reference the reserved unused fields? > > That would be the definition of an unused field I think :) > Call it "reserved" if you want, it doesn't really matter as long as its > clear its something you shouldn't be using. > > > > >>In any case padding is best added to the end of a struct to minimize > >>risks and keep things simple. > > > >The thing is that isn't it a bit aweful to (always) add pads to > >the end of a struct, especially when you don't know how many > >need to be padded? > > Then you pad for what you think you need, plus a bit extra, and maybe some > more for others who might want to extend it. What is a reasonable amount > needs deciding case by case - if a struct is alloced in the millions then be > (very) conservative, but if there are one or 50 such structs within an app > lifetime then who cares if its bit larger? > > And yeah padding may be annoying, but that's pretty much the only option in > a project where most of the structs are out in the open. > > - Panu - Functions versioning is another option. For a sufficiently widely used struct, it's a lot of work, padding is easier. But it might be better than breaking ABI if e.g. you didn't pad enough. > > > > --yliu > >