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 913C0231C for ; Wed, 2 Dec 2015 17:58:06 +0100 (CET) Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id D7C73C0CC626; Wed, 2 Dec 2015 16:58:05 +0000 (UTC) Received: from sopuli.koti.laiskiainen.org (vpn1-6-161.ams2.redhat.com [10.36.6.161]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id tB2Gw4cl017323; Wed, 2 Dec 2015 11:58:04 -0500 To: Yuanhan Liu 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> From: Panu Matilainen Message-ID: <565F231B.5090203@redhat.com> Date: Wed, 2 Dec 2015 18:58:03 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: <20151202150923.GV2325@yliu-dev.sh.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Scanned-By: MIMEDefang 2.68 on 10.5.11.27 Cc: dev@dpdk.org, Victor Kaplansky , "Michael S. Tsirkin" 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 16:58:07 -0000 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 - > > --yliu >