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 4BF3B58DB for ; Wed, 2 Dec 2015 15:48:17 +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 7456D8F28B; Wed, 2 Dec 2015 14:48:16 +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 tB2EmE6D007212; Wed, 2 Dec 2015 09:48:15 -0500 To: Yuanhan Liu , Thomas Monjalon 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> From: Panu Matilainen Message-ID: <565F04AE.2070404@redhat.com> Date: Wed, 2 Dec 2015 16:48:14 +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: <20151202143101.GR2325@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 14:48:17 -0000 On 12/02/2015 04:31 PM, Yuanhan Liu wrote: > On Wed, Dec 02, 2015 at 03:53:45PM +0200, Panu Matilainen wrote: >> On 12/02/2015 05:43 AM, Yuanhan Liu wrote: >>> VHOST_USER_SET_LOG_BASE request is used to tell the backend (dpdk >>> vhost-user) where we should log dirty pages, and how big the log >>> buffer is. >>> >>> This request introduces a new payload: >>> >>> typedef struct VhostUserLog { >>> uint64_t mmap_size; >>> uint64_t mmap_offset; >>> } VhostUserLog; >>> >>> Also, a fd is delivered from QEMU by ancillary data. >>> >>> With those info given, an area of memory is mmaped, assigned >>> to dev->log_base, for logging dirty pages. >>> >>> Signed-off-by: Yuanhan Liu >>> --- >>> lib/librte_vhost/rte_virtio_net.h | 2 ++ >>> lib/librte_vhost/vhost_user/vhost-net-user.c | 7 ++++- >>> lib/librte_vhost/vhost_user/vhost-net-user.h | 6 ++++ >>> lib/librte_vhost/vhost_user/virtio-net-user.c | 44 +++++++++++++++++++++++++++ >>> lib/librte_vhost/vhost_user/virtio-net-user.h | 1 + >>> 5 files changed, 59 insertions(+), 1 deletion(-) >>> >>> 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. >> >> 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. In any case padding is best added to the end of a struct to minimize risks and keep things simple. - Panu - > > Thomas, should I write an ABI deprecation note? Can I make it for > v2.2 release If I make one tomorrow? (Sorry that I'm not awared > of that it would be an ABI break). > > --yliu >