[dpdk-dev] [RFC] meter: fix ABI break due to experimental tag removal

Neil Horman nhorman at tuxdriver.com
Tue Feb 4 13:02:30 CET 2020


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 <ferruh.yigit at intel.com>
> >> ---
> >> Cc: Neil Horman <nhorman at tuxdriver.com>
> >> Cc: Thomas Monjalon <thomas at monjalon.net>
> >> Cc: Luca Boccassi <bluca at debian.org>
> >> Cc: David Marchand <david.marchand at redhat.com>
> >> Cc: Bruce Richardson <bruce.richardson at intel.com>
> >> Cc: Ian Stokes <ian.stokes at intel.com>
> >> Cc: Eelco Chaudron <echaudro at redhat.com>
> >>
> >> 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 <rte_common.h>
> >>  #include <rte_log.h>
> >>  #include <rte_cycles.h>
> >> +#include <rte_function_versioning.h>
> >>  
> >>  #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 = '<unknown 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
> >>
> >>
> 
> 


More information about the dev mailing list