[v4] meter: provide experimental alias of API for old apps

  On v20.02 some meter APIs have been matured and symbols moved from
EXPERIMENTAL to DPDK_20.0.1 block.

This can break the applications that were using these mentioned APIs on
v19.11. Although there is no modification on the APIs and the action is
positive and matures the APIs, the affect can be negative to
applications.

Since experimental APIs can change or go away without notice as part of
contract, to prevent this negative affect that may occur by maturing
experimental API, a process update already suggested, which enables
aliasing without forcing it:
https://patches.dpdk.org/patch/65863/

This patch provides aliasing by duplicating the existing and versioned
symbols as experimental.

Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
aliasing done between EXPERIMENTAL and DPDK_21.

Also following changes done to enabling aliasing:

Created VERSION_SYMBOL_EXPERIMENTAL helper macro.

Updated the 'check-symbols.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)

Enabled function versioning for meson build for the library.

Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API")
Cc: stable@dpdk.org

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>
Cc: Andrzej Ostruszka <amo@semihalf.com>
Cc: Ray Kinsella <mdr@ashroe.eu>

v2:
* Commit log updated

v3:
* added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro

v4:
* update script name in commit log, remove empty line
---
 buildtools/check-symbols.sh                   |  3 +-
 .../include/rte_function_versioning.h         |  9 +++
 lib/librte_meter/meson.build                  |  1 +
 lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
 lib/librte_meter/rte_meter_version.map        |  8 +++
 5 files changed, 76 insertions(+), 4 deletions(-)
  

14/05/2020 18:11, Ferruh Yigit:
> On v20.02 some meter APIs have been matured and symbols moved from
> EXPERIMENTAL to DPDK_20.0.1 block.
> 
> This can break the applications that were using these mentioned APIs on
> v19.11. Although there is no modification on the APIs and the action is
> positive and matures the APIs, the affect can be negative to
> applications.
> 
> Since experimental APIs can change or go away without notice as part of
> contract, to prevent this negative affect that may occur by maturing
> experimental API, a process update already suggested, which enables
> aliasing without forcing it:
> https://patches.dpdk.org/patch/65863/
> 
> This patch provides aliasing by duplicating the existing and versioned
> symbols as experimental.
> 
> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
> aliasing done between EXPERIMENTAL and DPDK_21.
> 
> Also following changes done to enabling aliasing:
> 
> Created VERSION_SYMBOL_EXPERIMENTAL helper macro.
> 
> Updated the 'check-symbols.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)
> 
> Enabled function versioning for meson build for the library.
> 
> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API")
> Cc: stable@dpdk.org
> 
> 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>
> Cc: Andrzej Ostruszka <amo@semihalf.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>
> 
> v2:
> * Commit log updated
> 
> v3:
> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro
> 
> v4:
> * update script name in commit log, remove empty line
> ---
>  buildtools/check-symbols.sh                   |  3 +-
>  .../include/rte_function_versioning.h         |  9 +++
>  lib/librte_meter/meson.build                  |  1 +
>  lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
>  lib/librte_meter/rte_meter_version.map        |  8 +++
>  5 files changed, 76 insertions(+), 4 deletions(-)

This patch is introducing the tooling and use it for the lib meter.
It is based on approval of the process documentation.

Ray sent a patch for the process documentation which refers to this tooling.

As raised by David, there is a circular dependency :)

I think it would be clearer to merge tooling and doc together,
and have meter changes separate.
Would it work for you?
  

On 15/05/2020 14:31, Thomas Monjalon wrote:
> 14/05/2020 18:11, Ferruh Yigit:
>> On v20.02 some meter APIs have been matured and symbols moved from
>> EXPERIMENTAL to DPDK_20.0.1 block.
>>
>> This can break the applications that were using these mentioned APIs on
>> v19.11. Although there is no modification on the APIs and the action is
>> positive and matures the APIs, the affect can be negative to
>> applications.
>>
>> Since experimental APIs can change or go away without notice as part of
>> contract, to prevent this negative affect that may occur by maturing
>> experimental API, a process update already suggested, which enables
>> aliasing without forcing it:
>> https://patches.dpdk.org/patch/65863/
>>
>> This patch provides aliasing by duplicating the existing and versioned
>> symbols as experimental.
>>
>> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
>> aliasing done between EXPERIMENTAL and DPDK_21.
>>
>> Also following changes done to enabling aliasing:
>>
>> Created VERSION_SYMBOL_EXPERIMENTAL helper macro.
>>
>> Updated the 'check-symbols.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)
>>
>> Enabled function versioning for meson build for the library.
>>
>> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API")
>> Cc: stable@dpdk.org
>>
>> 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>
>> Cc: Andrzej Ostruszka <amo@semihalf.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>
>>
>> v2:
>> * Commit log updated
>>
>> v3:
>> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro
>>
>> v4:
>> * update script name in commit log, remove empty line
>> ---
>>  buildtools/check-symbols.sh                   |  3 +-
>>  .../include/rte_function_versioning.h         |  9 +++
>>  lib/librte_meter/meson.build                  |  1 +
>>  lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
>>  lib/librte_meter/rte_meter_version.map        |  8 +++
>>  5 files changed, 76 insertions(+), 4 deletions(-)
> 
> This patch is introducing the tooling and use it for the lib meter.
> It is based on approval of the process documentation.
> 
> Ray sent a patch for the process documentation which refers to this tooling.
> 
> As raised by David, there is a circular dependency :)
> 
> I think it would be clearer to merge tooling and doc together,
> and have meter changes separate.
> Would it work for you?
>

So I sent a v5 just of the policy and tooling changes.

Ray K
  

On 15/05/2020 14:31, Thomas Monjalon wrote:
> 14/05/2020 18:11, Ferruh Yigit:
>> On v20.02 some meter APIs have been matured and symbols moved from
>> EXPERIMENTAL to DPDK_20.0.1 block.
>>
>> This can break the applications that were using these mentioned APIs on
>> v19.11. Although there is no modification on the APIs and the action is
>> positive and matures the APIs, the affect can be negative to
>> applications.
>>
>> Since experimental APIs can change or go away without notice as part of
>> contract, to prevent this negative affect that may occur by maturing
>> experimental API, a process update already suggested, which enables
>> aliasing without forcing it:
>> https://patches.dpdk.org/patch/65863/
>>
>> This patch provides aliasing by duplicating the existing and versioned
>> symbols as experimental.
>>
>> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
>> aliasing done between EXPERIMENTAL and DPDK_21.
>>
>> Also following changes done to enabling aliasing:
>>
>> Created VERSION_SYMBOL_EXPERIMENTAL helper macro.
>>
>> Updated the 'check-symbols.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)
>>
>> Enabled function versioning for meson build for the library.
>>
>> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API")
>> Cc: stable@dpdk.org
>>
>> 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>
>> Cc: Andrzej Ostruszka <amo@semihalf.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>
>>
>> v2:
>> * Commit log updated
>>
>> v3:
>> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro
>>
>> v4:
>> * update script name in commit log, remove empty line
>> ---
>>  buildtools/check-symbols.sh                   |  3 +-
>>  .../include/rte_function_versioning.h         |  9 +++
>>  lib/librte_meter/meson.build                  |  1 +
>>  lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
>>  lib/librte_meter/rte_meter_version.map        |  8 +++
>>  5 files changed, 76 insertions(+), 4 deletions(-)
> 
> This patch is introducing the tooling and use it for the lib meter.
> It is based on approval of the process documentation.
> 
> Ray sent a patch for the process documentation which refers to this tooling.
> 
> As raised by David, there is a circular dependency :)
> 
> I think it would be clearer to merge tooling and doc together,
> and have meter changes separate.
> Would it work for you?
> 

BTW - send a v6 to make a abundantly clear that 

"doc: fix references to bind_default_symbol".
https://patches.dpdk.org/patch/69850/

is a dependency.
  

Hi Ferruh,

> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, May 14, 2020 5:11 PM
> To: Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>; Dumitrescu, Cristian
> <cristian.dumitrescu@intel.com>; Eelco Chaudron <echaudro@redhat.com>
> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
> <thomas@monjalon.net>; David Marchand <david.marchand@redhat.com>;
> stable@dpdk.org; Luca Boccassi <bluca@debian.org>; Richardson, Bruce
> <bruce.richardson@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Andrzej
> Ostruszka <amo@semihalf.com>
> Subject: [PATCH v4] meter: provide experimental alias of API for old apps
> 
> On v20.02 some meter APIs have been matured and symbols moved from
> EXPERIMENTAL to DPDK_20.0.1 block.
> 
> This can break the applications that were using these mentioned APIs on
> v19.11. Although there is no modification on the APIs and the action is
> positive and matures the APIs, the affect can be negative to
> applications.
> 
> Since experimental APIs can change or go away without notice as part of
> contract, to prevent this negative affect that may occur by maturing
> experimental API, a process update already suggested, which enables
> aliasing without forcing it:
> https://patches.dpdk.org/patch/65863/
> 

Personally, I am not convinced this is really needed.

Are there any users asking for this?

Is there any other library where this is also applied, or is librte_meter the only library?

> This patch provides aliasing by duplicating the existing and versioned
> symbols as experimental.
> 
> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
> aliasing done between EXPERIMENTAL and DPDK_21.
> 
> Also following changes done to enabling aliasing:
> 
> Created VERSION_SYMBOL_EXPERIMENTAL helper macro.
> 
> Updated the 'check-symbols.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)
> 
> Enabled function versioning for meson build for the library.
> 
> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM
> API")
> Cc: stable@dpdk.org
> 
> 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>
> Cc: Andrzej Ostruszka <amo@semihalf.com>
> Cc: Ray Kinsella <mdr@ashroe.eu>
> 
> v2:
> * Commit log updated
> 
> v3:
> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro
> 
> v4:
> * update script name in commit log, remove empty line
> ---
>  buildtools/check-symbols.sh                   |  3 +-
>  .../include/rte_function_versioning.h         |  9 +++
>  lib/librte_meter/meson.build                  |  1 +
>  lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
>  lib/librte_meter/rte_meter_version.map        |  8 +++
>  5 files changed, 76 insertions(+), 4 deletions(-)
> 
> diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh
> index 3df57c322c..e407553a34 100755
> --- a/buildtools/check-symbols.sh
> +++ b/buildtools/check-symbols.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/include/rte_function_versioning.h
> b/lib/librte_eal/include/rte_function_versioning.h
> index b9f862d295..f588f2643b 100644
> --- a/lib/librte_eal/include/rte_function_versioning.h
> +++ b/lib/librte_eal/include/rte_function_versioning.h
> @@ -46,6 +46,14 @@
>   */
>  #define VERSION_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b)
> RTE_STR(e) ", " RTE_STR(b) "@DPDK_" RTE_STR(n))
> 
> +/*
> + * VERSION_SYMBOL_EXPERIMENTAL
> + * Creates a symbol version table entry binding the symbol
> <b>@EXPERIMENTAL to the internal
> + * function name <b><e>. The macro is used when a symbol matures to
> become part of the stable ABI,
> + * to provide an alias to experimental for some time.
> + */
> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) __asm__(".symver "
> RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@EXPERIMENTAL")
> +
>  /*
>   * BIND_DEFAULT_SYMBOL
>   * Creates a symbol version entry instructing the linker to bind references to
> @@ -79,6 +87,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/meson.build b/lib/librte_meter/meson.build
> index 646fd4d43f..fce0368437 100644
> --- a/lib/librte_meter/meson.build
> +++ b/lib/librte_meter/meson.build
> @@ -3,3 +3,4 @@
> 
>  sources = files('rte_meter.c')
>  headers = files('rte_meter.h')
> +use_function_versioning = true
> diff --git a/lib/librte_meter/rte_meter.c b/lib/librte_meter/rte_meter.c
> index da01429a8b..c600b05064 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, 21);
> +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_conf
> ig, _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, 21);
> +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);

To me, this is a significant amount of dead code that does not add any functionality and does not bring any added value to the library for any user. I am not a build system expert, but I would definitely prefer avoiding adding any C code to the library for this purpose, and just modify the map file, would this approach be possible?

Also, very important, is this C code to be added permanently or is it added just on a temporary basis? If temporary, when is it going to be removed?

> diff --git a/lib/librte_meter/rte_meter_version.map
> b/lib/librte_meter/rte_meter_version.map
> index 2c7dadbcac..b493bcebe9 100644
> --- a/lib/librte_meter/rte_meter_version.map
> +++ b/lib/librte_meter/rte_meter_version.map
> @@ -20,4 +20,12 @@ DPDK_21 {
>  	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;
> +};
> --
> 2.25.4

Regards,
Cristian
  

On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
> Hi Ferruh,
> 
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Thursday, May 14, 2020 5:11 PM
>> To: Ray Kinsella <mdr@ashroe.eu>; Neil Horman
>> <nhorman@tuxdriver.com>; Dumitrescu, Cristian
>> <cristian.dumitrescu@intel.com>; Eelco Chaudron <echaudro@redhat.com>
>> Cc: dev@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>; Thomas Monjalon
>> <thomas@monjalon.net>; David Marchand <david.marchand@redhat.com>;
>> stable@dpdk.org; Luca Boccassi <bluca@debian.org>; Richardson, Bruce
>> <bruce.richardson@intel.com>; Stokes, Ian <ian.stokes@intel.com>; Andrzej
>> Ostruszka <amo@semihalf.com>
>> Subject: [PATCH v4] meter: provide experimental alias of API for old apps
>>
>> On v20.02 some meter APIs have been matured and symbols moved from
>> EXPERIMENTAL to DPDK_20.0.1 block.
>>
>> This can break the applications that were using these mentioned APIs on
>> v19.11. Although there is no modification on the APIs and the action is
>> positive and matures the APIs, the affect can be negative to
>> applications.
>>
>> Since experimental APIs can change or go away without notice as part of
>> contract, to prevent this negative affect that may occur by maturing
>> experimental API, a process update already suggested, which enables
>> aliasing without forcing it:
>> https://patches.dpdk.org/patch/65863/
>>
> 
> Personally, I am not convinced this is really needed.
> 
> Are there any users asking for this?

As it happens it is all breaking our abi regression test suite.
One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
 
> Is there any other library where this is also applied, or is librte_meter the only library?

librte_meter is the only example AFAIK. 
But then we only have one example of needing symbol versioning also at the moment (Cryptodev).

This is going to happen with experimental symbols that have been around a while, 
that have become used in applications. It is a non-mandatory tool a maintainer can use
to preserve abi compatibility. 

> 
>> This patch provides aliasing by duplicating the existing and versioned
>> symbols as experimental.
>>
>> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the
>> aliasing done between EXPERIMENTAL and DPDK_21.
>>
>> Also following changes done to enabling aliasing:
>>
>> Created VERSION_SYMBOL_EXPERIMENTAL helper macro.
>>
>> Updated the 'check-symbols.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)
>>
>> Enabled function versioning for meson build for the library.
>>
>> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM
>> API")
>> Cc: stable@dpdk.org
>>
>> 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>
>> Cc: Andrzej Ostruszka <amo@semihalf.com>
>> Cc: Ray Kinsella <mdr@ashroe.eu>
>>
>> v2:
>> * Commit log updated
>>
>> v3:
>> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro
>>
>> v4:
>> * update script name in commit log, remove empty line
>> ---
>>  buildtools/check-symbols.sh                   |  3 +-
>>  .../include/rte_function_versioning.h         |  9 +++
>>  lib/librte_meter/meson.build                  |  1 +
>>  lib/librte_meter/rte_meter.c                  | 59 ++++++++++++++++++-
>>  lib/librte_meter/rte_meter_version.map        |  8 +++
>>  5 files changed, 76 insertions(+), 4 deletions(-)
>>
>> diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh
>> index 3df57c322c..e407553a34 100755
>> --- a/buildtools/check-symbols.sh
>> +++ b/buildtools/check-symbols.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/include/rte_function_versioning.h
>> b/lib/librte_eal/include/rte_function_versioning.h
>> index b9f862d295..f588f2643b 100644
>> --- a/lib/librte_eal/include/rte_function_versioning.h
>> +++ b/lib/librte_eal/include/rte_function_versioning.h
>> @@ -46,6 +46,14 @@
>>   */
>>  #define VERSION_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b)
>> RTE_STR(e) ", " RTE_STR(b) "@DPDK_" RTE_STR(n))
>>
>> +/*
>> + * VERSION_SYMBOL_EXPERIMENTAL
>> + * Creates a symbol version table entry binding the symbol
>> <b>@EXPERIMENTAL to the internal
>> + * function name <b><e>. The macro is used when a symbol matures to
>> become part of the stable ABI,
>> + * to provide an alias to experimental for some time.
>> + */
>> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) __asm__(".symver "
>> RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@EXPERIMENTAL")
>> +
>>  /*
>>   * BIND_DEFAULT_SYMBOL
>>   * Creates a symbol version entry instructing the linker to bind references to
>> @@ -79,6 +87,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/meson.build b/lib/librte_meter/meson.build
>> index 646fd4d43f..fce0368437 100644
>> --- a/lib/librte_meter/meson.build
>> +++ b/lib/librte_meter/meson.build
>> @@ -3,3 +3,4 @@
>>
>>  sources = files('rte_meter.c')
>>  headers = files('rte_meter.h')
>> +use_function_versioning = true
>> diff --git a/lib/librte_meter/rte_meter.c b/lib/librte_meter/rte_meter.c
>> index da01429a8b..c600b05064 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, 21);
>> +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_conf
>> ig, _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, 21);
>> +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);
> 
> To me, this is a significant amount of dead code that does not add any functionality and does not bring any added value to the library for any user. I am not a build system expert, but I would definitely prefer avoiding adding any C code to the library for this purpose, and just modify the map file, would this approach be possible?

Approach is exactly the same as the rest of symbol versioning. 
 
> Also, very important, is this C code to be added permanently or is it added just on a temporary basis? If temporary, when is it going to be removed?

It will be removed in the v21 (20.11 lts) release. 
When we officially rev the abi and start afresh. 

> 
>> diff --git a/lib/librte_meter/rte_meter_version.map
>> b/lib/librte_meter/rte_meter_version.map
>> index 2c7dadbcac..b493bcebe9 100644
>> --- a/lib/librte_meter/rte_meter_version.map
>> +++ b/lib/librte_meter/rte_meter_version.map
>> @@ -20,4 +20,12 @@ DPDK_21 {
>>  	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;
>> +};
>> --
>> 2.25.4
> 
> Regards,
> Cristian
>
  

18/05/2020 08:29, Ray Kinsella:
> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
> > From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>
> >> On v20.02 some meter APIs have been matured and symbols moved from
> >> EXPERIMENTAL to DPDK_20.0.1 block.
> >>
> >> This can break the applications that were using these mentioned APIs on
> >> v19.11. Although there is no modification on the APIs and the action is
> >> positive and matures the APIs, the affect can be negative to
> >> applications.
> >>
> >> Since experimental APIs can change or go away without notice as part of
> >> contract, to prevent this negative affect that may occur by maturing
> >> experimental API, a process update already suggested, which enables
> >> aliasing without forcing it:
> >> https://patches.dpdk.org/patch/65863/
> >>
> > 
> > Personally, I am not convinced this is really needed.
> > 
> > Are there any users asking for this?
> 
> As it happens it is all breaking our abi regression test suite.
> One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
>  
> > Is there any other library where this is also applied, or is librte_meter the only library?
> 
> librte_meter is the only example AFAIK. 
> But then we only have one example of needing symbol versioning also at the moment (Cryptodev).
> 
> This is going to happen with experimental symbols that have been around a while, 
> that have become used in applications. It is a non-mandatory tool a maintainer can use
> to preserve abi compatibility.

If you want to maintain ABI compatibility of experimental symbols,
it IS a mandatory tool.
You cannot enforce your "ABI regression test suite" and at the same time
say it is "non-mandatory".

The real question here is to know whether we want to maintain compatibility
of experimental symbols. We said no. Then we said we can.
The main concern is the message clarity in my opinion.
  

On 18/05/2020 10:22, Thomas Monjalon wrote:
> 18/05/2020 08:29, Ray Kinsella:
>> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>
>>>> On v20.02 some meter APIs have been matured and symbols moved from
>>>> EXPERIMENTAL to DPDK_20.0.1 block.
>>>>
>>>> This can break the applications that were using these mentioned APIs on
>>>> v19.11. Although there is no modification on the APIs and the action is
>>>> positive and matures the APIs, the affect can be negative to
>>>> applications.
>>>>
>>>> Since experimental APIs can change or go away without notice as part of
>>>> contract, to prevent this negative affect that may occur by maturing
>>>> experimental API, a process update already suggested, which enables
>>>> aliasing without forcing it:
>>>> https://patches.dpdk.org/patch/65863/
>>>>
>>>
>>> Personally, I am not convinced this is really needed.
>>>
>>> Are there any users asking for this?
>>
>> As it happens it is all breaking our abi regression test suite.
>> One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
>>  
>>> Is there any other library where this is also applied, or is librte_meter the only library?
>>
>> librte_meter is the only example AFAIK. 
>> But then we only have one example of needing symbol versioning also at the moment (Cryptodev).
>>
>> This is going to happen with experimental symbols that have been around a while, 
>> that have become used in applications. It is a non-mandatory tool a maintainer can use
>> to preserve abi compatibility.
> 
> If you want to maintain ABI compatibility of experimental symbols,
> it IS a mandatory tool.
> You cannot enforce your "ABI regression test suite" and at the same time
> say it is "non-mandatory".> 
> The real question here is to know whether we want to maintain compatibility
> of experimental symbols. We said no. Then we said we can.
> The main concern is the message clarity in my opinion.
> 

There is complete clarity, there is no obligation. 
Our lack of obligation around experimental, is upfront in the policy is upfront in the policy.

"Libraries or APIs marked as experimental may change without constraint, as they are not considered part of an ABI version. Experimental libraries have the major ABI version 0."

Later we give the _option_ without obligation to add an alias to experimental.pls see the v6.

+   - In situations in which an ``experimental`` symbol has been stable for some
+     time. When promoting the symbol to become part of the next ABI version, the
+     maintainer may choose to provide an alias to the ``experimental`` tag, so
+     as not to break consuming applications.

So it is something a Maintainer, _may_ choose to do.
I use the word, "may" not "will" as there is no obligation's associated with experimental.
  

18/05/2020 11:30, Ray Kinsella:
> On 18/05/2020 10:22, Thomas Monjalon wrote:
> > 18/05/2020 08:29, Ray Kinsella:
> >> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
> >>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>
> >>>> On v20.02 some meter APIs have been matured and symbols moved from
> >>>> EXPERIMENTAL to DPDK_20.0.1 block.
> >>>>
> >>>> This can break the applications that were using these mentioned APIs on
> >>>> v19.11. Although there is no modification on the APIs and the action is
> >>>> positive and matures the APIs, the affect can be negative to
> >>>> applications.
> >>>>
> >>>> Since experimental APIs can change or go away without notice as part of
> >>>> contract, to prevent this negative affect that may occur by maturing
> >>>> experimental API, a process update already suggested, which enables
> >>>> aliasing without forcing it:
> >>>> https://patches.dpdk.org/patch/65863/
> >>>>
> >>>
> >>> Personally, I am not convinced this is really needed.
> >>>
> >>> Are there any users asking for this?
> >>
> >> As it happens it is all breaking our abi regression test suite.
> >> One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
> >>  
> >>> Is there any other library where this is also applied, or is librte_meter the only library?
> >>
> >> librte_meter is the only example AFAIK. 
> >> But then we only have one example of needing symbol versioning also at the moment (Cryptodev).
> >>
> >> This is going to happen with experimental symbols that have been around a while, 
> >> that have become used in applications. It is a non-mandatory tool a maintainer can use
> >> to preserve abi compatibility.
> > 
> > If you want to maintain ABI compatibility of experimental symbols,
> > it IS a mandatory tool.
> > You cannot enforce your "ABI regression test suite" and at the same time
> > say it is "non-mandatory".
> > 
> > The real question here is to know whether we want to maintain compatibility
> > of experimental symbols. We said no. Then we said we can.
> > The main concern is the message clarity in my opinion.
> > 
> 
> There is complete clarity, there is no obligation. 
> Our lack of obligation around experimental, is upfront in the policy is upfront in the policy.
> 
> "Libraries or APIs marked as experimental may change without constraint, as they are not considered part of an ABI version. Experimental libraries have the major ABI version 0."
> 
> Later we give the _option_ without obligation to add an alias to experimental.pls see the v6.
> 
> +   - In situations in which an ``experimental`` symbol has been stable for some
> +     time. When promoting the symbol to become part of the next ABI version, the
> +     maintainer may choose to provide an alias to the ``experimental`` tag, so
> +     as not to break consuming applications.
> 
> So it is something a Maintainer, _may_ choose to do.
> I use the word, "may" not "will" as there is no obligation's associated with experimental.


OK Ray, this is my understanding as well.

The only difficult part to understand is when claiming
"it is all breaking our abi regression test suite"
to justify the choice.
As the maintainer (Cristian) says he does not like this change,
it means the regression test suite should skip this case, right?
  

> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Monday, May 18, 2020 11:46 AM
> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>;
> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
> Cc: Neil Horman <nhorman@tuxdriver.com>; Eelco Chaudron
> <echaudro@redhat.com>; dev@dpdk.org; David Marchand
> <david.marchand@redhat.com>; stable@dpdk.org; Luca Boccassi
> <bluca@debian.org>; Richardson, Bruce <bruce.richardson@intel.com>;
> Stokes, Ian <ian.stokes@intel.com>; Andrzej Ostruszka
> <amo@semihalf.com>
> Subject: Re: [PATCH v4] meter: provide experimental alias of API for old apps
> 
> 18/05/2020 11:30, Ray Kinsella:
> > On 18/05/2020 10:22, Thomas Monjalon wrote:
> > > 18/05/2020 08:29, Ray Kinsella:
> > >> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
> > >>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> > >>>>
> > >>>> On v20.02 some meter APIs have been matured and symbols moved
> from
> > >>>> EXPERIMENTAL to DPDK_20.0.1 block.
> > >>>>
> > >>>> This can break the applications that were using these mentioned APIs
> on
> > >>>> v19.11. Although there is no modification on the APIs and the action is
> > >>>> positive and matures the APIs, the affect can be negative to
> > >>>> applications.
> > >>>>
> > >>>> Since experimental APIs can change or go away without notice as part
> of
> > >>>> contract, to prevent this negative affect that may occur by maturing
> > >>>> experimental API, a process update already suggested, which
> enables
> > >>>> aliasing without forcing it:
> > >>>> https://patches.dpdk.org/patch/65863/
> > >>>>
> > >>>
> > >>> Personally, I am not convinced this is really needed.
> > >>>
> > >>> Are there any users asking for this?
> > >>
> > >> As it happens it is all breaking our abi regression test suite.
> > >> One of the things we do is to run the unit tests binary from v19.11
> against the latest release.
> > >>
> > >>> Is there any other library where this is also applied, or is librte_meter
> the only library?
> > >>
> > >> librte_meter is the only example AFAIK.
> > >> But then we only have one example of needing symbol versioning also
> at the moment (Cryptodev).
> > >>
> > >> This is going to happen with experimental symbols that have been
> around a while,
> > >> that have become used in applications. It is a non-mandatory tool a
> maintainer can use
> > >> to preserve abi compatibility.
> > >
> > > If you want to maintain ABI compatibility of experimental symbols,
> > > it IS a mandatory tool.
> > > You cannot enforce your "ABI regression test suite" and at the same time
> > > say it is "non-mandatory".
> > >
> > > The real question here is to know whether we want to maintain
> compatibility
> > > of experimental symbols. We said no. Then we said we can.
> > > The main concern is the message clarity in my opinion.
> > >
> >
> > There is complete clarity, there is no obligation.
> > Our lack of obligation around experimental, is upfront in the policy is
> upfront in the policy.
> >
> > "Libraries or APIs marked as experimental may change without constraint,
> as they are not considered part of an ABI version. Experimental libraries have
> the major ABI version 0."
> >
> > Later we give the _option_ without obligation to add an alias to
> experimental.pls see the v6.
> >
> > +   - In situations in which an ``experimental`` symbol has been stable for
> some
> > +     time. When promoting the symbol to become part of the next ABI
> version, the
> > +     maintainer may choose to provide an alias to the ``experimental`` tag,
> so
> > +     as not to break consuming applications.
> >
> > So it is something a Maintainer, _may_ choose to do.
> > I use the word, "may" not "will" as there is no obligation's associated with
> experimental.
> 
> 
> OK Ray, this is my understanding as well.
> 
> The only difficult part to understand is when claiming
> "it is all breaking our abi regression test suite"
> to justify the choice.
> As the maintainer (Cristian) says he does not like this change,
> it means the regression test suite should skip this case, right?
> 

I am yet to be convinced of the value of this, but if some people think it is useful, I am willing to compromise. This is subject to this code being temporary code to be removed for 20.11 release, which Ray already confirmed.

Ray, a few more suggestions, are you OK with them?
1. Move this code to a separate file in the library (suggest rte_meter_abi_compat.c as the file name)
2. Clearly state in the patch description this is temporary code to be removed for 20.11 release.
3. Agree that you or Ferruh take the AR to send a patch prior to the 20.11 release to remove this code.

Thanks,
Cristian
  

On 18/05/2020 11:46, Thomas Monjalon wrote:
> 18/05/2020 11:30, Ray Kinsella:
>> On 18/05/2020 10:22, Thomas Monjalon wrote:
>>> 18/05/2020 08:29, Ray Kinsella:
>>>> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>
>>>>>> On v20.02 some meter APIs have been matured and symbols moved from
>>>>>> EXPERIMENTAL to DPDK_20.0.1 block.
>>>>>>
>>>>>> This can break the applications that were using these mentioned APIs on
>>>>>> v19.11. Although there is no modification on the APIs and the action is
>>>>>> positive and matures the APIs, the affect can be negative to
>>>>>> applications.
>>>>>>
>>>>>> Since experimental APIs can change or go away without notice as part of
>>>>>> contract, to prevent this negative affect that may occur by maturing
>>>>>> experimental API, a process update already suggested, which enables
>>>>>> aliasing without forcing it:
>>>>>> https://patches.dpdk.org/patch/65863/
>>>>>>
>>>>>
>>>>> Personally, I am not convinced this is really needed.
>>>>>
>>>>> Are there any users asking for this?
>>>>
>>>> As it happens it is all breaking our abi regression test suite.
>>>> One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
>>>>  
>>>>> Is there any other library where this is also applied, or is librte_meter the only library?
>>>>
>>>> librte_meter is the only example AFAIK. 
>>>> But then we only have one example of needing symbol versioning also at the moment (Cryptodev).
>>>>
>>>> This is going to happen with experimental symbols that have been around a while, 
>>>> that have become used in applications. It is a non-mandatory tool a maintainer can use
>>>> to preserve abi compatibility.
>>>
>>> If you want to maintain ABI compatibility of experimental symbols,
>>> it IS a mandatory tool.
>>> You cannot enforce your "ABI regression test suite" and at the same time
>>> say it is "non-mandatory".
>>>
>>> The real question here is to know whether we want to maintain compatibility
>>> of experimental symbols. We said no. Then we said we can.
>>> The main concern is the message clarity in my opinion.
>>>
>>
>> There is complete clarity, there is no obligation. 
>> Our lack of obligation around experimental, is upfront in the policy is upfront in the policy.
>>
>> "Libraries or APIs marked as experimental may change without constraint, as they are not considered part of an ABI version. Experimental libraries have the major ABI version 0."
>>
>> Later we give the _option_ without obligation to add an alias to experimental.pls see the v6.
>>
>> +   - In situations in which an ``experimental`` symbol has been stable for some
>> +     time. When promoting the symbol to become part of the next ABI version, the
>> +     maintainer may choose to provide an alias to the ``experimental`` tag, so
>> +     as not to break consuming applications.
>>
>> So it is something a Maintainer, _may_ choose to do.
>> I use the word, "may" not "will" as there is no obligation's associated with experimental.
> 
> 
> OK Ray, this is my understanding as well.
> 
> The only difficult part to understand is when claiming
> "it is all breaking our abi regression test suite"
> to justify the choice.

Justification, is the same as any other consumer of DPDK saying you broke my APP.

> As the maintainer (Cristian) says he does not like this change,
> it means the regression test suite should skip this case, right?

So the regression test run the v19.11 Unit Test's against the v20.05 rc.
My thought was that would provide reasonably good coverage of the ABI to catch more subtly regression.
Those regressions that affect the behavior of the ABI (the contract), instead of ABI itself.
  

On 18/05/2020 12:18, Dumitrescu, Cristian wrote:
> 
> 
>> -----Original Message-----
>> From: Thomas Monjalon <thomas@monjalon.net>
>> Sent: Monday, May 18, 2020 11:46 AM
>> To: Yigit, Ferruh <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>;
>> Dumitrescu, Cristian <cristian.dumitrescu@intel.com>
>> Cc: Neil Horman <nhorman@tuxdriver.com>; Eelco Chaudron
>> <echaudro@redhat.com>; dev@dpdk.org; David Marchand
>> <david.marchand@redhat.com>; stable@dpdk.org; Luca Boccassi
>> <bluca@debian.org>; Richardson, Bruce <bruce.richardson@intel.com>;
>> Stokes, Ian <ian.stokes@intel.com>; Andrzej Ostruszka
>> <amo@semihalf.com>
>> Subject: Re: [PATCH v4] meter: provide experimental alias of API for old apps
>>
>> 18/05/2020 11:30, Ray Kinsella:
>>> On 18/05/2020 10:22, Thomas Monjalon wrote:
>>>> 18/05/2020 08:29, Ray Kinsella:
>>>>> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
>>>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>>>>>
>>>>>>> On v20.02 some meter APIs have been matured and symbols moved
>> from
>>>>>>> EXPERIMENTAL to DPDK_20.0.1 block.
>>>>>>>
>>>>>>> This can break the applications that were using these mentioned APIs
>> on
>>>>>>> v19.11. Although there is no modification on the APIs and the action is
>>>>>>> positive and matures the APIs, the affect can be negative to
>>>>>>> applications.
>>>>>>>
>>>>>>> Since experimental APIs can change or go away without notice as part
>> of
>>>>>>> contract, to prevent this negative affect that may occur by maturing
>>>>>>> experimental API, a process update already suggested, which
>> enables
>>>>>>> aliasing without forcing it:
>>>>>>> https://patches.dpdk.org/patch/65863/
>>>>>>>
>>>>>>
>>>>>> Personally, I am not convinced this is really needed.
>>>>>>
>>>>>> Are there any users asking for this?
>>>>>
>>>>> As it happens it is all breaking our abi regression test suite.
>>>>> One of the things we do is to run the unit tests binary from v19.11
>> against the latest release.
>>>>>
>>>>>> Is there any other library where this is also applied, or is librte_meter
>> the only library?
>>>>>
>>>>> librte_meter is the only example AFAIK.
>>>>> But then we only have one example of needing symbol versioning also
>> at the moment (Cryptodev).
>>>>>
>>>>> This is going to happen with experimental symbols that have been
>> around a while,
>>>>> that have become used in applications. It is a non-mandatory tool a
>> maintainer can use
>>>>> to preserve abi compatibility.
>>>>
>>>> If you want to maintain ABI compatibility of experimental symbols,
>>>> it IS a mandatory tool.
>>>> You cannot enforce your "ABI regression test suite" and at the same time
>>>> say it is "non-mandatory".
>>>>
>>>> The real question here is to know whether we want to maintain
>> compatibility
>>>> of experimental symbols. We said no. Then we said we can.
>>>> The main concern is the message clarity in my opinion.
>>>>
>>>
>>> There is complete clarity, there is no obligation.
>>> Our lack of obligation around experimental, is upfront in the policy is
>> upfront in the policy.
>>>
>>> "Libraries or APIs marked as experimental may change without constraint,
>> as they are not considered part of an ABI version. Experimental libraries have
>> the major ABI version 0."
>>>
>>> Later we give the _option_ without obligation to add an alias to
>> experimental.pls see the v6.
>>>
>>> +   - In situations in which an ``experimental`` symbol has been stable for
>> some
>>> +     time. When promoting the symbol to become part of the next ABI
>> version, the
>>> +     maintainer may choose to provide an alias to the ``experimental`` tag,
>> so
>>> +     as not to break consuming applications.
>>>
>>> So it is something a Maintainer, _may_ choose to do.
>>> I use the word, "may" not "will" as there is no obligation's associated with
>> experimental.
>>
>>
>> OK Ray, this is my understanding as well.
>>
>> The only difficult part to understand is when claiming
>> "it is all breaking our abi regression test suite"
>> to justify the choice.
>> As the maintainer (Cristian) says he does not like this change,
>> it means the regression test suite should skip this case, right?
>>
> 
> I am yet to be convinced of the value of this, but if some people think it is useful, I am willing to compromise. This is subject to this code being temporary code to be removed for 20.11 release, which Ray already confirmed.
> 
> Ray, a few more suggestions, are you OK with them?
> 1. Move this code to a separate file in the library (suggest rte_meter_abi_compat.c as the file name)
> 2. Clearly state in the patch description this is temporary code to be removed for 20.11 release.
> 3. Agree that you or Ferruh take the AR to send a patch prior to the 20.11 release to remove this code.
> 
> Thanks,
> Cristian

Hi Cristain - I am good with all of the above.

Ray K
  

18/05/2020 13:48, Ray Kinsella:
> On 18/05/2020 11:46, Thomas Monjalon wrote:
> > 18/05/2020 11:30, Ray Kinsella:
> >> On 18/05/2020 10:22, Thomas Monjalon wrote:
> >>> 18/05/2020 08:29, Ray Kinsella:
> >>>> On 17/05/2020 20:52, Dumitrescu, Cristian wrote:
> >>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >>>>>>
> >>>>>> On v20.02 some meter APIs have been matured and symbols moved from
> >>>>>> EXPERIMENTAL to DPDK_20.0.1 block.
> >>>>>>
> >>>>>> This can break the applications that were using these mentioned APIs on
> >>>>>> v19.11. Although there is no modification on the APIs and the action is
> >>>>>> positive and matures the APIs, the affect can be negative to
> >>>>>> applications.
> >>>>>>
> >>>>>> Since experimental APIs can change or go away without notice as part of
> >>>>>> contract, to prevent this negative affect that may occur by maturing
> >>>>>> experimental API, a process update already suggested, which enables
> >>>>>> aliasing without forcing it:
> >>>>>> https://patches.dpdk.org/patch/65863/
> >>>>>>
> >>>>>
> >>>>> Personally, I am not convinced this is really needed.
> >>>>>
> >>>>> Are there any users asking for this?
> >>>>
> >>>> As it happens it is all breaking our abi regression test suite.
> >>>> One of the things we do is to run the unit tests binary from v19.11 against the latest release. 
> >>>>  
> >>>>> Is there any other library where this is also applied, or is librte_meter the only library?
> >>>>
> >>>> librte_meter is the only example AFAIK. 
> >>>> But then we only have one example of needing symbol versioning also at the moment (Cryptodev).
> >>>>
> >>>> This is going to happen with experimental symbols that have been around a while, 
> >>>> that have become used in applications. It is a non-mandatory tool a maintainer can use
> >>>> to preserve abi compatibility.
> >>>
> >>> If you want to maintain ABI compatibility of experimental symbols,
> >>> it IS a mandatory tool.
> >>> You cannot enforce your "ABI regression test suite" and at the same time
> >>> say it is "non-mandatory".
> >>>
> >>> The real question here is to know whether we want to maintain compatibility
> >>> of experimental symbols. We said no. Then we said we can.
> >>> The main concern is the message clarity in my opinion.
> >>>
> >>
> >> There is complete clarity, there is no obligation. 
> >> Our lack of obligation around experimental, is upfront in the policy is upfront in the policy.
> >>
> >> "Libraries or APIs marked as experimental may change without constraint, as they are not considered part of an ABI version. Experimental libraries have the major ABI version 0."
> >>
> >> Later we give the _option_ without obligation to add an alias to experimental.pls see the v6.
> >>
> >> +   - In situations in which an ``experimental`` symbol has been stable for some
> >> +     time. When promoting the symbol to become part of the next ABI version, the
> >> +     maintainer may choose to provide an alias to the ``experimental`` tag, so
> >> +     as not to break consuming applications.
> >>
> >> So it is something a Maintainer, _may_ choose to do.
> >> I use the word, "may" not "will" as there is no obligation's associated with experimental.
> > 
> > 
> > OK Ray, this is my understanding as well.
> > 
> > The only difficult part to understand is when claiming
> > "it is all breaking our abi regression test suite"
> > to justify the choice.
> 
> Justification, is the same as any other consumer of DPDK saying you broke my APP.
> 
> > As the maintainer (Cristian) says he does not like this change,
> > it means the regression test suite should skip this case, right?
> 
> So the regression test run the v19.11 Unit Test's against the v20.05 rc.
> My thought was that would provide reasonably good coverage of the ABI to catch more subtly regression.
> Those regressions that affect the behavior of the ABI (the contract), instead of ABI itself. 

I understand the goal.
And I think, because of this goal, you will try to maintain ABI compat
of *ALL* experimental symbols maturing as stable symbol.
  

On 18/05/2020 13:13, Thomas Monjalon wrote:
> And I think, because of this goal, you will try to maintain ABI compat
> of *ALL* experimental symbols maturing as stable symbol.

I think that is a fair point, what we will ultimately need is a way to filter 
TCs that touch experimental from the Unit Test framework. 

That doesn't exist for v19.11, nor can we respectively invent it.
We should look at that for v20.11 to avoid supporting all "experimental symbols". 

As a better useful solution.
That said - I think having an alias-to-experimental tool in our box, 
stands on its own merit.

Ray K