| Message ID | 1449027793-30975-2-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 345788DAE; Wed, 2 Dec 2015 04:39:57 +0100 (CET) Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 325598D86 for <dev@dpdk.org>; Wed, 2 Dec 2015 04:39:55 +0100 (CET) Received: from orsmga001.jf.intel.com ([10.7.209.18]) by orsmga102.jf.intel.com with ESMTP; 01 Dec 2015 19:39:54 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.20,371,1444719600"; d="scan'208";a="832354330" Received: from yliu-dev.sh.intel.com ([10.239.66.49]) by orsmga001.jf.intel.com with ESMTP; 01 Dec 2015 19:39:52 -0800 From: Yuanhan Liu <yuanhan.liu@linux.intel.com> To: dev@dpdk.org Date: Wed, 2 Dec 2015 11:43:10 +0800 Message-Id: <1449027793-30975-2-git-send-email-yuanhan.liu@linux.intel.com> X-Mailer: git-send-email 1.9.0 In-Reply-To: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com> References: <1449027793-30975-1-git-send-email-yuanhan.liu@linux.intel.com> Cc: Victor Kaplansky <vkaplans@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com> Subject: [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 <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
Commit Message
Yuanhan Liu
Dec. 2, 2015, 3:43 a.m. UTC
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 <yuanhan.liu@linux.intel.com>
---
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(-)
Comments
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 <yuanhan.liu@linux.intel.com> > --- > 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. 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. - Panu -
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 <yuanhan.liu@linux.intel.com> > >--- > > 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? > > 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? 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
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 <yuanhan.liu@linux.intel.com> >>> --- >>> 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 >
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? > 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? --yliu
2015-12-02 22:31, Yuanhan Liu: > 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). As Panu suggested, it would be better to reserve some room now in 2.2 which already breaks vhost ABI. Maybe we have a chance to keep the same vhost ABI in 2.3. The 2.2 release will probably be closed in less than 2 weeks.
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 >
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 > >
On Wed, Dec 02, 2015 at 05:38:10PM +0100, Thomas Monjalon wrote: > 2015-12-02 22:31, Yuanhan Liu: > > 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). > > As Panu suggested, it would be better to reserve some room now > in 2.2 which already breaks vhost ABI. > Maybe we have a chance to keep the same vhost ABI in 2.3. Got it. I will cook up one now. --yliu > > The 2.2 release will probably be closed in less than 2 weeks.
2015-12-02 15:53, Panu Matilainen: > 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. > > 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. Please could you point where the vhost ABI is broken in 2.2?
On 12/07/2015 01:07 AM, Thomas Monjalon wrote: > 2015-12-02 15:53, Panu Matilainen: >> 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. >> >> 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. > > Please could you point where the vhost ABI is broken in 2.2? > The vhost ABI break was announced for DPDK 2.2 in commit 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: > commit 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d > Author: Ouyang Changchun <changchun.ouyang@intel.com> > Date: Tue Jun 16 09:38:43 2015 +0800 > > doc: announce ABI changes for vhost-user multiple queues > > It announces the planned ABI changes for vhost-user multiple > queues feature on v2.2. > > Signed-off-by: Changchun Ouyang <changchun.ouyang@intel.com> > Acked-by: Neil Horman <nhorman@tuxdriver.com> So the ABI process was properly followed, except for actually bumping LIBABIVER. Bumping LIBABIVER is mentioned in doc/guides/contributing/versioning.rst but it doesn't specify *when* this should be done, eg should the first patch breaking the ABI bump it or should it done be shortly before the next stable release, or something else. As it is, it seems a bit too easy to simply forget. - Panu -
2015-12-07 08:29, Panu Matilainen: > On 12/07/2015 01:07 AM, Thomas Monjalon wrote: > > 2015-12-02 15:53, Panu Matilainen: > >> 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. > >> > >> 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. > > > > Please could you point where the vhost ABI is broken in 2.2? > > > > The vhost ABI break was announced for DPDK 2.2 in commit > 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: [...] > So the ABI process was properly followed, except for actually bumping > LIBABIVER. Bumping LIBABIVER is mentioned in > doc/guides/contributing/versioning.rst but it doesn't specify *when* > this should be done, eg should the first patch breaking the ABI bump it > or should it done be shortly before the next stable release, or > something else. As it is, it seems a bit too easy to simply forget. I thought it was not needed to explicitly say that commits must be atomic and we do not have to wait to do the required changes. In this case, I've missed it when reviewing the vhost patches breaking the ABI.
On 12/07/2015 01:28 PM, Thomas Monjalon wrote: > 2015-12-07 08:29, Panu Matilainen: >> On 12/07/2015 01:07 AM, Thomas Monjalon wrote: >>> 2015-12-02 15:53, Panu Matilainen: >>>> 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. >>>> >>>> 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. >>> >>> Please could you point where the vhost ABI is broken in 2.2? >>> >> >> The vhost ABI break was announced for DPDK 2.2 in commit >> 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: > [...] >> So the ABI process was properly followed, except for actually bumping >> LIBABIVER. Bumping LIBABIVER is mentioned in >> doc/guides/contributing/versioning.rst but it doesn't specify *when* >> this should be done, eg should the first patch breaking the ABI bump it >> or should it done be shortly before the next stable release, or >> something else. As it is, it seems a bit too easy to simply forget. > > I thought it was not needed to explicitly say that commits must be atomic > and we do not have to wait to do the required changes. The "problem" is that during a development cycle, an ABI could be broken several times but LIBABIVER should only be bumped once. So ABI breaking commits will often not be atomic wrt LIBABIVER, no matter which way its done. For example libtool recommendation is that library versions are updated only just before public releases: https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html#Updating-version-info - Panu - > In this case, I've missed it when reviewing the vhost patches breaking the > ABI. >
2015-12-07 13:41, Panu Matilainen: > On 12/07/2015 01:28 PM, Thomas Monjalon wrote: > > 2015-12-07 08:29, Panu Matilainen: > >> On 12/07/2015 01:07 AM, Thomas Monjalon wrote: > >>> 2015-12-02 15:53, Panu Matilainen: > >> The vhost ABI break was announced for DPDK 2.2 in commit > >> 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: > > [...] > >> So the ABI process was properly followed, except for actually bumping > >> LIBABIVER. Bumping LIBABIVER is mentioned in > >> doc/guides/contributing/versioning.rst but it doesn't specify *when* > >> this should be done, eg should the first patch breaking the ABI bump it > >> or should it done be shortly before the next stable release, or > >> something else. As it is, it seems a bit too easy to simply forget. > > > > I thought it was not needed to explicitly say that commits must be atomic > > and we do not have to wait to do the required changes. > > The "problem" is that during a development cycle, an ABI could be broken > several times but LIBABIVER should only be bumped once. So ABI breaking > commits will often not be atomic wrt LIBABIVER, no matter which way its > done. If the ABI version has already been changed, there should be a merge conflict. I think it's better to manage a conflict than forget to update the version. > For example libtool recommendation is that library versions are updated > only just before public releases: > https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html#Updating-version-info Interesting link. It makes me think that we do not manage ABI break when downgrading the library (case of only new API keeping the ABI number). > > In this case, I've missed it when reviewing the vhost patches breaking the > > ABI.
On 12/07/2015 03:55 PM, Thomas Monjalon wrote: > 2015-12-07 13:41, Panu Matilainen: >> On 12/07/2015 01:28 PM, Thomas Monjalon wrote: >>> 2015-12-07 08:29, Panu Matilainen: >>>> On 12/07/2015 01:07 AM, Thomas Monjalon wrote: >>>>> 2015-12-02 15:53, Panu Matilainen: >>>> The vhost ABI break was announced for DPDK 2.2 in commit >>>> 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: >>> [...] >>>> So the ABI process was properly followed, except for actually bumping >>>> LIBABIVER. Bumping LIBABIVER is mentioned in >>>> doc/guides/contributing/versioning.rst but it doesn't specify *when* >>>> this should be done, eg should the first patch breaking the ABI bump it >>>> or should it done be shortly before the next stable release, or >>>> something else. As it is, it seems a bit too easy to simply forget. >>> >>> I thought it was not needed to explicitly say that commits must be atomic >>> and we do not have to wait to do the required changes. Heh, now that I look more carefully... it IS documented, line 38 of contributing/versioning.rst: > ABI versions are set at the time of major release labeling, and the > ABI may change multiple times, without warning, between the last > release label and the HEAD label of the git tree. >> The "problem" is that during a development cycle, an ABI could be broken >> several times but LIBABIVER should only be bumped once. So ABI breaking >> commits will often not be atomic wrt LIBABIVER, no matter which way its >> done. > > If the ABI version has already been changed, there should be a merge conflict. > I think it's better to manage a conflict than forget to update the version. What I'm thinking of is something that would tie LIBABIVER to the deprecation announcement in a way that could be easily checked (programmatically and manually). As it is now, its quite non-trivial to figure what LIBABIVER *should* be for a given library at a given point - you need to dig up deprecation.rst history and Makefile history and whatnot, and its all quite error-prone. >> For example libtool recommendation is that library versions are updated >> only just before public releases: >> https://www.gnu.org/software/libtool/manual/html_node/Updating-version-info.html#Updating-version-info > > Interesting link. It makes me think that we do not manage ABI break when > downgrading the library (case of only new API keeping the ABI number). Hmm, not quite sure what you mean here, but full libtool-style versioning is not really needed with symbol versioning. - Panu - > >>> In this case, I've missed it when reviewing the vhost patches breaking the >>> ABI. >
2015-12-07 18:48, Panu Matilainen: > On 12/07/2015 03:55 PM, Thomas Monjalon wrote: > > 2015-12-07 13:41, Panu Matilainen: > >> On 12/07/2015 01:28 PM, Thomas Monjalon wrote: > >>> 2015-12-07 08:29, Panu Matilainen: > >>>> On 12/07/2015 01:07 AM, Thomas Monjalon wrote: > >>>>> 2015-12-02 15:53, Panu Matilainen: > >>>> The vhost ABI break was announced for DPDK 2.2 in commit > >>>> 3c848bd7b1c6f4f681b833322a748fdefbb5fb2d: > >>> [...] > >>>> So the ABI process was properly followed, except for actually bumping > >>>> LIBABIVER. Bumping LIBABIVER is mentioned in > >>>> doc/guides/contributing/versioning.rst but it doesn't specify *when* > >>>> this should be done, eg should the first patch breaking the ABI bump it > >>>> or should it done be shortly before the next stable release, or > >>>> something else. As it is, it seems a bit too easy to simply forget. > >>> > >>> I thought it was not needed to explicitly say that commits must be atomic > >>> and we do not have to wait to do the required changes. > > Heh, now that I look more carefully... it IS documented, line 38 of > contributing/versioning.rst: > > > ABI versions are set at the time of major release labeling, and the > > ABI may change multiple times, without warning, between the last > > release label and the HEAD label of the git tree. Interesting :) > >> The "problem" is that during a development cycle, an ABI could be broken > >> several times but LIBABIVER should only be bumped once. So ABI breaking > >> commits will often not be atomic wrt LIBABIVER, no matter which way its > >> done. > > > > If the ABI version has already been changed, there should be a merge conflict. > > I think it's better to manage a conflict than forget to update the version. > > What I'm thinking of is something that would tie LIBABIVER to the > deprecation announcement in a way that could be easily checked > (programmatically and manually). As it is now, its quite non-trivial to > figure what LIBABIVER *should* be for a given library at a given point - > you need to dig up deprecation.rst history and Makefile history and > whatnot, and its all quite error-prone. Yes clearly we need a safer process. You are welcome to suggest one. I like the idea of being based on some "parse-able" RST changes.
On 12/2/2015 11:40 AM, Yuanhan Liu wrote: [...] > + > + addr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, off); > + if (addr == MAP_FAILED) { > + RTE_LOG(ERR, VHOST_CONFIG, "mmap log base failed!\n"); > + return -1; > + } Yuanhan: mmap could fail with non-zero offset for huge page based mapping. Check our workaround in user_set_mem_table. I think you have done the validation, but i guess off is zero here. > + > + /* TODO: unmap on stop */ > + dev->log_base = addr; > + dev->log_size = size; > + > + return 0; > +} > diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.h b/lib/librte_vhost/vhost_user/virtio-net-user.h > index b82108d..013cf38 100644 > --- a/lib/librte_vhost/vhost_user/virtio-net-user.h > +++ b/lib/librte_vhost/vhost_user/virtio-net-user.h > @@ -49,6 +49,7 @@ void user_set_vring_kick(struct vhost_device_ctx, struct VhostUserMsg *); > > void user_set_protocol_features(struct vhost_device_ctx ctx, > uint64_t protocol_features); > +int user_set_log_base(struct vhost_device_ctx ctx, struct VhostUserMsg *); > > int user_get_vring_base(struct vhost_device_ctx, struct vhost_vring_state *); >
On Tue, Dec 08, 2015 at 05:57:54AM +0000, Xie, Huawei wrote: > On 12/2/2015 11:40 AM, Yuanhan Liu wrote: > [...] > > + > > + addr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, off); > > + if (addr == MAP_FAILED) { > > + RTE_LOG(ERR, VHOST_CONFIG, "mmap log base failed!\n"); > > + return -1; > > + } > Yuanhan: > mmap could fail with non-zero offset for huge page based mapping. Check > our workaround in user_set_mem_table. > I think you have done the validation, but i guess off is zero here. Yes, off is zero. And thanks for the remind; will fix it in next version. --yliu
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; diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.c b/lib/librte_vhost/vhost_user/vhost-net-user.c index 2dc0547..76bcac2 100644 --- a/lib/librte_vhost/vhost_user/vhost-net-user.c +++ b/lib/librte_vhost/vhost_user/vhost-net-user.c @@ -388,7 +388,12 @@ vserver_message_handler(int connfd, void *dat, int *remove) break; case VHOST_USER_SET_LOG_BASE: - RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n"); + user_set_log_base(ctx, &msg); + + /* it needs a reply */ + msg.size = sizeof(msg.payload.u64); + send_vhost_message(connfd, &msg); + break; case VHOST_USER_SET_LOG_FD: close(msg.fds[0]); RTE_LOG(INFO, VHOST_CONFIG, "not implemented.\n"); diff --git a/lib/librte_vhost/vhost_user/vhost-net-user.h b/lib/librte_vhost/vhost_user/vhost-net-user.h index 38637cc..6d252a3 100644 --- a/lib/librte_vhost/vhost_user/vhost-net-user.h +++ b/lib/librte_vhost/vhost_user/vhost-net-user.h @@ -83,6 +83,11 @@ typedef struct VhostUserMemory { VhostUserMemoryRegion regions[VHOST_MEMORY_MAX_NREGIONS]; } VhostUserMemory; +typedef struct VhostUserLog { + uint64_t mmap_size; + uint64_t mmap_offset; +} VhostUserLog; + typedef struct VhostUserMsg { VhostUserRequest request; @@ -97,6 +102,7 @@ typedef struct VhostUserMsg { struct vhost_vring_state state; struct vhost_vring_addr addr; VhostUserMemory memory; + VhostUserLog log; } payload; int fds[VHOST_MEMORY_MAX_NREGIONS]; } __attribute((packed)) VhostUserMsg; diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.c b/lib/librte_vhost/vhost_user/virtio-net-user.c index 2934d1c..1d705fd 100644 --- a/lib/librte_vhost/vhost_user/virtio-net-user.c +++ b/lib/librte_vhost/vhost_user/virtio-net-user.c @@ -365,3 +365,47 @@ user_set_protocol_features(struct vhost_device_ctx ctx, dev->protocol_features = protocol_features; } + +int +user_set_log_base(struct vhost_device_ctx ctx, + struct VhostUserMsg *msg) +{ + struct virtio_net *dev; + int fd = msg->fds[0]; + uint64_t size, off; + void *addr; + + dev = get_device(ctx); + if (!dev) + return -1; + + if (fd < 0) { + RTE_LOG(ERR, VHOST_CONFIG, "invalid log fd: %d\n", fd); + return -1; + } + + if (msg->size != sizeof(VhostUserLog)) { + RTE_LOG(ERR, VHOST_CONFIG, + "invalid log base msg size: %"PRId32" != %d\n", + msg->size, (int)sizeof(VhostUserLog)); + return -1; + } + + size = msg->payload.log.mmap_size; + off = msg->payload.log.mmap_offset; + RTE_LOG(INFO, VHOST_CONFIG, + "log mmap size: %"PRId64", offset: %"PRId64"\n", + size, off); + + addr = mmap(0, size, PROT_READ | PROT_WRITE, MAP_SHARED, fd, off); + if (addr == MAP_FAILED) { + RTE_LOG(ERR, VHOST_CONFIG, "mmap log base failed!\n"); + return -1; + } + + /* TODO: unmap on stop */ + dev->log_base = addr; + dev->log_size = size; + + return 0; +} diff --git a/lib/librte_vhost/vhost_user/virtio-net-user.h b/lib/librte_vhost/vhost_user/virtio-net-user.h index b82108d..013cf38 100644 --- a/lib/librte_vhost/vhost_user/virtio-net-user.h +++ b/lib/librte_vhost/vhost_user/virtio-net-user.h @@ -49,6 +49,7 @@ void user_set_vring_kick(struct vhost_device_ctx, struct VhostUserMsg *); void user_set_protocol_features(struct vhost_device_ctx ctx, uint64_t protocol_features); +int user_set_log_base(struct vhost_device_ctx ctx, struct VhostUserMsg *); int user_get_vring_base(struct vhost_device_ctx, struct vhost_vring_state *);