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

[Neil Horman <nhorman@tuxdriver.com>]

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?

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.

A few more nits in line

> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Neil Horman <nhorman@tuxdriver.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Luca Boccassi <bluca@debian.org>
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Bruce Richardson <bruce.richardson@intel.com>
> Cc: Ian Stokes <ian.stokes@intel.com>
> Cc: Eelco Chaudron <echaudro@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)


>  /*
>   * 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
> 
>