Re: [dpdk-dev] [RFC PATCH] mark experimental variables

[Ray Kinsella <mdr@ashroe.eu>]

My 2c is that it feels a little unweildy to have to annotate, every variable declaration.
and also extern reference with __rte_experimental_var.

Is there any easier way?

Other minor comments below. 

On 25/11/2019 16:13, David Marchand wrote:
> So far, we did not pay attention to direct access to variables but they
> are part of the API/ABI too and should be clearly identified.
> 
> Introduce a __rte_experimental_var tag and mark existing variables.
> 
> Fixes: a4bcd61de82d ("buildtools: add script to check experimental API exports")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Quick patch to try to catch experimental variables.
> Not sure if we could use a single section, so please advise if there is
> better to do about this.
> 
> ---
>  buildtools/check-experimental-syms.sh      | 17 +++++++++++++++--
>  devtools/checkpatches.sh                   | 14 +++++++++-----
>  doc/guides/contributing/abi_policy.rst     |  7 ++++---
>  drivers/net/ice/rte_pmd_ice.h              |  3 +++
>  lib/librte_cryptodev/rte_crypto_asym.h     |  3 +++
>  lib/librte_eal/common/include/rte_compat.h |  5 +++++
>  lib/librte_ethdev/rte_flow.h               | 17 +++++++++++++++++
>  lib/librte_port/rte_port_eventdev.h        |  5 +++++
>  8 files changed, 61 insertions(+), 10 deletions(-)
> 
> diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-experimental-syms.sh
> index f3603e5ba..27f7b39f6 100755
> --- a/buildtools/check-experimental-syms.sh
> +++ b/buildtools/check-experimental-syms.sh
> @@ -34,13 +34,26 @@ do
>  		Please add __rte_experimental to the definition of $SYM
>  		END_OF_MESSAGE
>  		ret=1
> +	elif grep -q "\.data.*[[:space:]]$SYM$" $DUMPFILE &&
> +		! grep -q "\.data\.experimental.*[[:space:]]$SYM$" $DUMPFILE
> +	then
> +		cat >&2 <<- END_OF_MESSAGE
> +		$SYM is not flagged as experimental
> +		but is listed in version map
> +		Please add __rte_experimental_var to the definition of $SYM
> +		END_OF_MESSAGE
> +		ret=1
>  	fi
>  done
>  
>  # Filter out symbols suffixed with a . for icc
>  for SYM in `awk '{
> -	if ($2 != "l" && $4 == ".text.experimental" && !($NF ~ /\.$/)) {
> -		print $NF
> +	if ($2 == "l" || $NF ~ /\.$/) {
> +		next;
> +	}
> +	if ($4 == ".text.experimental" ||
> +	    $4 == ".data.experimental") {
> +		print $NF;
>  	}
>  }' $DUMPFILE`
>  do
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index b16bace92..d3d02b8ce 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -90,11 +90,15 @@ check_experimental_tags() { # <patch>
>  				"headers ("current_file")";
>  			ret = 1;
>  		}
> -		if ($1 != "+__rte_experimental" || $2 != "") {
> -			print "__rte_experimental must appear alone on the line" \
> -				" immediately preceding the return type of a function."
> -			ret = 1;
> +
> +		if (NF == 1 && ($1 == "+__rte_experimental" ||
> +				$1 == "+__rte_experimental_var")) {
> +			next;
>  		}
> +		print "__rte_experimental or __rte_experimental_var must " \
> +			"appear alone on the line immediately preceding the " \
> +			"return type of a function.";

or variable?

> +		ret = 1;
>  	}
>  	END {
>  		exit ret;
> @@ -178,7 +182,7 @@ check () { # <patch> <commit> <title>
>  		ret=1
>  	fi
>  
> -	! $verbose || printf '\nChecking __rte_experimental tags:\n'
> +	! $verbose || printf '\nChecking __rte_experimental* tags:\n'
>  	report=$(check_experimental_tags "$tmpinput")
>  	if [ $? -ne 0 ] ; then
>  		$headline_printed || print_headline "$3"
> diff --git a/doc/guides/contributing/abi_policy.rst b/doc/guides/contributing/abi_policy.rst
> index 05ca95980..189ef6491 100644
> --- a/doc/guides/contributing/abi_policy.rst
> +++ b/doc/guides/contributing/abi_policy.rst
> @@ -300,9 +300,10 @@ Note that marking an API as experimental is a multi step process.
>  To mark an API as experimental, the symbols which are desired to be exported
>  must be placed in an EXPERIMENTAL version block in the corresponding libraries'
>  version map script.
> -Secondly, the corresponding prototypes of those exported functions (in the
> -development header files), must be marked with the ``__rte_experimental`` tag
> -(see ``rte_compat.h``).
> +Secondly, the corresponding prototypes of those exported functions (resp.
> +variables) must be marked with the ``__rte_experimental`` (resp.
> +``__rte_experimental_var``) tag in the development header files (see
> +``rte_compat.h``).

Suggest simplifying as follows (remove the resp.). 

Secondly, the corresponding prototypes of those exported functions (or
variables) must be marked with the ``__rte_experimental`` (or
``__rte_experimental_var``) tag in the development header files (see
``rte_compat.h``).

>  The DPDK build makefiles perform a check to ensure that the map file and the
>  C code reflect the same list of symbols.
>  This check can be circumvented by defining ``ALLOW_EXPERIMENTAL_API``
> diff --git a/drivers/net/ice/rte_pmd_ice.h b/drivers/net/ice/rte_pmd_ice.h
> index e254db053..dbd5586d4 100644
> --- a/drivers/net/ice/rte_pmd_ice.h
> +++ b/drivers/net/ice/rte_pmd_ice.h
> @@ -15,6 +15,8 @@
>   */
>  
>  #include <stdio.h>
> +
> +#include <rte_compat.h>
>  #include <rte_mbuf.h>
>  #include <rte_mbuf_dyn.h>
>  
> @@ -81,6 +83,7 @@ union rte_net_ice_proto_xtr_metadata {
>  };
>  
>  /* Offset of mbuf dynamic field for protocol extraction data */
> +__rte_experimental_var
>  extern int rte_net_ice_dynfield_proto_xtr_metadata_offs;
>  
>  /* Mask of mbuf dynamic flags for protocol extraction type */
> diff --git a/lib/librte_cryptodev/rte_crypto_asym.h b/lib/librte_cryptodev/rte_crypto_asym.h
> index 0d34ce8df..d4e84910f 100644
> --- a/lib/librte_cryptodev/rte_crypto_asym.h
> +++ b/lib/librte_cryptodev/rte_crypto_asym.h
> @@ -24,6 +24,7 @@ extern "C" {
>  #include <rte_memory.h>
>  #include <rte_mempool.h>
>  #include <rte_common.h>
> +#include <rte_compat.h>
>  
>  #include "rte_crypto_sym.h"
>  
> @@ -37,10 +38,12 @@ typedef struct rte_crypto_param_t {
>  } rte_crypto_param;
>  
>  /** asym xform type name strings */
> +__rte_experimental_var
>  extern const char *
>  rte_crypto_asym_xform_strings[];
>  
>  /** asym operations type name strings */
> +__rte_experimental_var
>  extern const char *
>  rte_crypto_asym_op_strings[];
>  
> diff --git a/lib/librte_eal/common/include/rte_compat.h b/lib/librte_eal/common/include/rte_compat.h
> index 3eb33784b..3fd05179f 100644
> --- a/lib/librte_eal/common/include/rte_compat.h
> +++ b/lib/librte_eal/common/include/rte_compat.h
> @@ -11,11 +11,16 @@
>  #define __rte_experimental \
>  __attribute__((deprecated("Symbol is not yet part of stable ABI"), \
>  section(".text.experimental")))
> +#define __rte_experimental_var \
> +__attribute__((deprecated("Symbol is not yet part of stable ABI"), \
> +section(".data.experimental")))
>  
>  #else
>  
>  #define __rte_experimental \
>  __attribute__((section(".text.experimental")))
> +#define __rte_experimental_var \
> +__attribute__((section(".data.experimental")))
>  
>  #endif
>  
> diff --git a/lib/librte_ethdev/rte_flow.h b/lib/librte_ethdev/rte_flow.h
> index 452d359a1..c8ea71acc 100644
> --- a/lib/librte_ethdev/rte_flow.h
> +++ b/lib/librte_ethdev/rte_flow.h
> @@ -19,6 +19,7 @@
>  
>  #include <rte_arp.h>
>  #include <rte_common.h>
> +#include <rte_compat.h>
>  #include <rte_ether.h>
>  #include <rte_icmp.h>
>  #include <rte_ip.h>
> @@ -2531,9 +2532,11 @@ struct rte_flow_action_set_meta {
>  };
>  
>  /* Mbuf dynamic field offset for metadata. */
> +__rte_experimental_var
>  extern int rte_flow_dynf_metadata_offs;
>  
>  /* Mbuf dynamic field flag mask for metadata. */
> +__rte_experimental_var
>  extern uint64_t rte_flow_dynf_metadata_mask;
>  
>  /* Mbuf dynamic field pointer for metadata. */
> @@ -2548,14 +2551,24 @@ __rte_experimental
>  static inline uint32_t
>  rte_flow_dynf_metadata_get(struct rte_mbuf *m)
>  {
> +#ifdef ALLOW_EXPERIMENTAL_API
>  	return *RTE_FLOW_DYNF_METADATA(m);
> +#else
> +	RTE_SET_USED(m);
> +	return 0;
> +#endif
>  }
>  
>  __rte_experimental
>  static inline void
>  rte_flow_dynf_metadata_set(struct rte_mbuf *m, uint32_t v)
>  {
> +#ifdef ALLOW_EXPERIMENTAL_API
>  	*RTE_FLOW_DYNF_METADATA(m) = v;
> +#else
> +	RTE_SET_USED(m);
> +	RTE_SET_USED(v);
> +#endif
>  }
>  
>  /*
> @@ -2800,7 +2813,11 @@ __rte_experimental
>  static inline int
>  rte_flow_dynf_metadata_avail(void)
>  {
> +#ifdef ALLOW_EXPERIMENTAL_API
>  	return !!rte_flow_dynf_metadata_mask;
> +#else
> +	return 0;
> +#endif
>  }
>  
>  /**
> diff --git a/lib/librte_port/rte_port_eventdev.h b/lib/librte_port/rte_port_eventdev.h
> index acf88f4e9..59246e204 100644
> --- a/lib/librte_port/rte_port_eventdev.h
> +++ b/lib/librte_port/rte_port_eventdev.h
> @@ -25,6 +25,8 @@ extern "C" {
>  **/
>  
>  #include <stdint.h>
> +
> +#include <rte_compat.h>
>  #include <rte_eventdev.h>
>  
>  #include "rte_port.h"
> @@ -39,6 +41,7 @@ struct rte_port_eventdev_reader_params {
>  };
>  
>  /** Eventdev_reader port operations. */
> +__rte_experimental_var
>  extern struct rte_port_in_ops rte_port_eventdev_reader_ops;
>  
>  /** Eventdev_writer port parameters. */
> @@ -63,6 +66,7 @@ struct rte_port_eventdev_writer_params {
>  };
>  
>  /** Eventdev_writer port operations. */
> +__rte_experimental_var
>  extern struct rte_port_out_ops rte_port_eventdev_writer_ops;
>  
>  /** Event_writer_nodrop port parameters. */
> @@ -90,6 +94,7 @@ struct rte_port_eventdev_writer_nodrop_params {
>  };
>  
>  /** Eventdev_writer_nodrop port operations. */
> +__rte_experimental_var
>  extern struct rte_port_out_ops rte_port_eventdev_writer_nodrop_ops;
>  
>  #ifdef __cplusplus
>