From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 261E9A0532; Tue, 4 Feb 2020 13:02:57 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 645751C12F; Tue, 4 Feb 2020 13:02:56 +0100 (CET) Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 0D2A0F72 for ; Tue, 4 Feb 2020 13:02:55 +0100 (CET) Received: from [107.15.85.130] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1iywua-0002JD-2O; Tue, 04 Feb 2020 07:02:48 -0500 Date: Tue, 4 Feb 2020 07:02:30 -0500 From: Neil Horman To: Ferruh Yigit Cc: Cristian Dumitrescu , Eelco Chaudron , dev@dpdk.org, Thomas Monjalon , Luca Boccassi , David Marchand , Bruce Richardson , Ian Stokes Message-ID: <20200204120230.GA13754@hmswarspite.think-freely.org> References: <20200129122953.2016199-1-ferruh.yigit@intel.com> <20200202185334.GA6783@localhost.localdomain> <7beadbbd-bf0d-e545-d9bb-0e3ae8228b71@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <7beadbbd-bf0d-e545-d9bb-0e3ae8228b71@intel.com> X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [RFC] meter: fix ABI break due to experimental tag removal X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Mon, Feb 03, 2020 at 12:53:28PM +0000, Ferruh Yigit wrote: > On 2/2/2020 6:53 PM, Neil Horman wrote: > > On Wed, Jan 29, 2020 at 12:29:53PM +0000, Ferruh Yigit wrote: > >> Duplicated the existing symbol and versioned one as experimental and > >> other as stable. > >> > >> Created VERSION_SYMBOL_EXPERIMENTAL helper macro. > >> > >> Updated the 'check-experimental-syms.sh' buildtool, which was > >> complaining that the symbol is in EXPERIMENTAL tag in .map file but it > >> is not in the .experimental section (__rte_experimental tag is missing). > >> Updated tool in a way it won't complain if the symbol in the > >> EXPERIMENTAL tag duplicated in some other block in .map file (versioned) > >> > >> Updated meson build system to allow the versioning, > >> 'use_function_versioning = true', not sure why it was disabled by > >> default. > >> > >> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API") > >> > > I'm not sure I understand the purpose here. > > > > I think I understand what you are trying to do. I believe that you are > > trying to move the experimental symbols in librte_meter to be part of > > the official ABI, while still maintaining backward compatibility with > > applications built against the experimental symbol versions, correct? > > Yes, exactly. > Ok, so we're on the same page, good. > > > > I can understand the desire, and I'm not exactly opposed to providing a > > mechanism for that, but it seems somewhat complex, and to be honest, > > part of the drawback to using experimental symbols as an application > > developer is understanding that you may need to rebuild when those > > symbols change (even if the change is the symbol removal and replacement > > with an identical one that has a versioned tag). > > > > I think the right answer for people encountering this condition is to > > just rebuild. Otherwise, we are creating an environment in which we are > > implicitly communicating to users that we are maintaining a modicum of > > stability in experimental symobls. > > This will mean, even the the API was perfect it will force its users to > recompile (and re-package, re-deploy etc..) at least once, this is feeling like Yes, thats correct. > we are still breaking user applications easily. we reserve that right, based on the fact that we marked these symbols as experimental. Thats the deal we provide. Stable (non experimental) API calls are guaranteed not to change for the ABI lifetime, and its our responsibility to maintain that. However, experimental API's are marked as such because we can change them (even if that change is to remove it and replace it with an identical stable version), and application developers assume that risk by using them. I understand the removal of an experimental symbol, replacing it with a stable identical version is a trivial case of that change, and if we can ease the burden, thats fine, but I don't think we need to go out of our way, or incur any additional burden to ease that transition. We created the experimental symbol marking to ease the burden of this sort of development on us, we should be willing to use it. > And this may create a stronger motivation by applications not to use > experimental APIs, I can't decide if this is a good thing or bad thing. > Yeah, for stable, long term support applications, that makes sense, they shouldn't be using experimental API's when they need the guarantee of support, or they should be understanding of their responsibility to rebuild, when we change the API's in both complex and trivial cases like this. > > The issue is amplified in the LTS, > If we remove experimental tag in LTS, will be breaking the apps using that > experimental API, just to mature the API. You shouldn't be making this sort of transition in LTS. Just leave the experimental symbol as is there, and commit the transition for the next LTS release. > If we keep it as experimental, the API will be mature in main repo, but the LTS > has to keep exact same API as experimental up to two years. Yes, thats the meaning of LTS. Things stay stable for its lifetime. And if the only thing you are doing is replacing the experimental ABI version with a stable version (leaving the details of the ABI unchanged), then its a moot point. The only difference will be that the LTS release will have symbols marked as experimental. > > But if we can do the versioning in the master, LTS can backport it and can have > mature version of that API in LTS without breaking the existing users. > But why bother? The only thing you've changed is the version tagging. Its ok to leave that alone in LTS, you just cant change it. Thats part of the burden of an LTS release, it will have some drift from the upstream head, because you have to keep things stable. So you stabilize the upstream ABI version for this API and just never backport it to the current LTS release. > > > > A few more nits in line > > > >> Signed-off-by: Ferruh Yigit > >> --- > >> Cc: Neil Horman > >> Cc: Thomas Monjalon > >> Cc: Luca Boccassi > >> Cc: David Marchand > >> Cc: Bruce Richardson > >> Cc: Ian Stokes > >> Cc: Eelco Chaudron > >> > >> I didn't like the idea of duplicating the symbol but not able to find to > >> alias it without duplicating, if there is a way please share. > >> > >> Also not tested with old binaries, only verified the symbols in new > >> binaries. > >> --- > >> buildtools/check-experimental-syms.sh | 3 +- > >> .../common/include/rte_function_versioning.h | 4 ++ > >> lib/librte_meter/rte_meter.c | 59 ++++++++++++++++++- > >> lib/librte_meter/rte_meter_version.map | 8 +++ > >> lib/meson.build | 2 +- > >> 5 files changed, 71 insertions(+), 5 deletions(-) > >> > >> diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-experimental-syms.sh > >> index f3603e5ba..35c4bf006 100755 > >> --- a/buildtools/check-experimental-syms.sh > >> +++ b/buildtools/check-experimental-syms.sh > >> @@ -26,7 +26,8 @@ ret=0 > >> for SYM in `$LIST_SYMBOL -S EXPERIMENTAL $MAPFILE |cut -d ' ' -f 3` > >> do > >> if grep -q "\.text.*[[:space:]]$SYM$" $DUMPFILE && > >> - ! grep -q "\.text\.experimental.*[[:space:]]$SYM$" $DUMPFILE > >> + ! grep -q "\.text\.experimental.*[[:space:]]$SYM$" $DUMPFILE && > >> + $LIST_SYMBOL -s $SYM $MAPFILE | grep -q EXPERIMENTAL > >> then > >> cat >&2 <<- END_OF_MESSAGE > >> $SYM is not flagged as experimental > >> diff --git a/lib/librte_eal/common/include/rte_function_versioning.h b/lib/librte_eal/common/include/rte_function_versioning.h > >> index c924351d5..ab102b06e 100644 > >> --- a/lib/librte_eal/common/include/rte_function_versioning.h > >> +++ b/lib/librte_eal/common/include/rte_function_versioning.h > >> @@ -46,6 +46,9 @@ > >> */ > >> #define VERSION_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@DPDK_" RTE_STR(n)) > >> > >> + > >> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@EXPERIMENTAL") > >> + > > I don't know that you want to make a explicit clone of this macro. > > instead what you might want to do is something like: > > > > #define __VERSION_SYMBOL(b, e, n, t) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@" RTE_STR(t) "_" RTE_STR(n)) > > > > #define VERSION_SYMBOL(b, e, n) __VERSION_SYMBOL(b, e, n, DPDK) > > > > Thats not exactly right, but you get the idea, then you can emulate > > VERSION_SYMBOL_EXPERIMENTAL with __VERSION_SYMBOL(b, e, n, EXPERIMENTAL) > > +1 > > > > > > >> /* > >> * BIND_DEFAULT_SYMBOL > >> * Creates a symbol version entry instructing the linker to bind references to > >> @@ -79,6 +82,7 @@ > >> * No symbol versioning in use > >> */ > >> #define VERSION_SYMBOL(b, e, n) > >> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) > >> #define __vsym > >> #define BIND_DEFAULT_SYMBOL(b, e, n) > >> #define MAP_STATIC_SYMBOL(f, p) f __attribute__((alias(RTE_STR(p)))) > >> diff --git a/lib/librte_meter/rte_meter.c b/lib/librte_meter/rte_meter.c > >> index da01429a8..5244537fa 100644 > >> --- a/lib/librte_meter/rte_meter.c > >> +++ b/lib/librte_meter/rte_meter.c > >> @@ -9,6 +9,7 @@ > >> #include > >> #include > >> #include > >> +#include > >> > >> #include "rte_meter.h" > >> > >> @@ -119,8 +120,8 @@ rte_meter_trtcm_config(struct rte_meter_trtcm *m, > >> return 0; > >> } > >> > >> -int > >> -rte_meter_trtcm_rfc4115_profile_config( > >> +static int > >> +rte_meter_trtcm_rfc4115_profile_config_( > >> struct rte_meter_trtcm_rfc4115_profile *p, > >> struct rte_meter_trtcm_rfc4115_params *params) > >> { > >> @@ -145,7 +146,35 @@ rte_meter_trtcm_rfc4115_profile_config( > >> } > >> > >> int > >> -rte_meter_trtcm_rfc4115_config( > >> +rte_meter_trtcm_rfc4115_profile_config_s( > >> + struct rte_meter_trtcm_rfc4115_profile *p, > >> + struct rte_meter_trtcm_rfc4115_params *params); > >> +int > >> +rte_meter_trtcm_rfc4115_profile_config_s( > >> + struct rte_meter_trtcm_rfc4115_profile *p, > >> + struct rte_meter_trtcm_rfc4115_params *params) > >> +{ > >> + return rte_meter_trtcm_rfc4115_profile_config_(p, params); > >> +} > >> +BIND_DEFAULT_SYMBOL(rte_meter_trtcm_rfc4115_profile_config, _s, 20.0.1); > >> +MAP_STATIC_SYMBOL(int rte_meter_trtcm_rfc4115_profile_config(struct rte_meter_trtcm_rfc4115_profile *p, > >> + struct rte_meter_trtcm_rfc4115_params *params), rte_meter_trtcm_rfc4115_profile_config_s); > >> + > >> +int > >> +rte_meter_trtcm_rfc4115_profile_config_e( > >> + struct rte_meter_trtcm_rfc4115_profile *p, > >> + struct rte_meter_trtcm_rfc4115_params *params); > >> +int > >> +rte_meter_trtcm_rfc4115_profile_config_e( > >> + struct rte_meter_trtcm_rfc4115_profile *p, > >> + struct rte_meter_trtcm_rfc4115_params *params) > >> +{ > >> + return rte_meter_trtcm_rfc4115_profile_config_(p, params); > >> +} > >> +VERSION_SYMBOL_EXPERIMENTAL(rte_meter_trtcm_rfc4115_profile_config, _e); > >> + > >> +static int > >> +rte_meter_trtcm_rfc4115_config_( > >> struct rte_meter_trtcm_rfc4115 *m, > >> struct rte_meter_trtcm_rfc4115_profile *p) > >> { > >> @@ -160,3 +189,27 @@ rte_meter_trtcm_rfc4115_config( > >> > >> return 0; > >> } > >> + > >> +int > >> +rte_meter_trtcm_rfc4115_config_s(struct rte_meter_trtcm_rfc4115 *m, > >> + struct rte_meter_trtcm_rfc4115_profile *p); > >> +int > >> +rte_meter_trtcm_rfc4115_config_s(struct rte_meter_trtcm_rfc4115 *m, > >> + struct rte_meter_trtcm_rfc4115_profile *p) > >> +{ > >> + return rte_meter_trtcm_rfc4115_config_(m, p); > >> +} > >> +BIND_DEFAULT_SYMBOL(rte_meter_trtcm_rfc4115_config, _s, 20.0.1); > >> +MAP_STATIC_SYMBOL(int rte_meter_trtcm_rfc4115_config(struct rte_meter_trtcm_rfc4115 *m, > >> + struct rte_meter_trtcm_rfc4115_profile *p), rte_meter_trtcm_rfc4115_config_s); > >> + > >> +int > >> +rte_meter_trtcm_rfc4115_config_e(struct rte_meter_trtcm_rfc4115 *m, > >> + struct rte_meter_trtcm_rfc4115_profile *p); > >> +int > >> +rte_meter_trtcm_rfc4115_config_e(struct rte_meter_trtcm_rfc4115 *m, > >> + struct rte_meter_trtcm_rfc4115_profile *p) > >> +{ > >> + return rte_meter_trtcm_rfc4115_config_(m, p); > >> +} > >> +VERSION_SYMBOL_EXPERIMENTAL(rte_meter_trtcm_rfc4115_config, _e); > >> diff --git a/lib/librte_meter/rte_meter_version.map b/lib/librte_meter/rte_meter_version.map > >> index fc12cc0bf..b4b043174 100644 > >> --- a/lib/librte_meter/rte_meter_version.map > >> +++ b/lib/librte_meter/rte_meter_version.map > >> @@ -20,4 +20,12 @@ DPDK_20.0.1 { > >> rte_meter_trtcm_rfc4115_color_blind_check; > >> rte_meter_trtcm_rfc4115_config; > >> rte_meter_trtcm_rfc4115_profile_config; > >> + > >> } DPDK_20.0; > >> + > >> +EXPERIMENTAL { > >> + global: > >> + > >> + rte_meter_trtcm_rfc4115_config; > >> + rte_meter_trtcm_rfc4115_profile_config; > >> +}; > >> diff --git a/lib/meson.build b/lib/meson.build > >> index 0af3efab2..553964831 100644 > >> --- a/lib/meson.build > >> +++ b/lib/meson.build > >> @@ -48,7 +48,7 @@ foreach l:libraries > >> reason = '' # set if build == false to explain why > >> name = l > >> allow_experimental_apis = false > >> - use_function_versioning = false > >> + use_function_versioning = true > >> sources = [] > >> headers = [] > >> includes = [] > >> -- > >> 2.24.1 > >> > >> > >