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

[Neil Horman <nhorman@tuxdriver.com>]

On Tue, Nov 26, 2019 at 09:25:49AM +0000, Ray Kinsella wrote:
> 
> 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?
> 
Note, just to be clear, its not every variable, only the ones exposed as global
variable meant to be accessed outside of the library that are not part of a
versioned ABI.

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