DPDK patches and discussions
 help / color / Atom feed
* [dpdk-dev] [RFC PATCH] mark experimental variables
@ 2019-11-25 16:13 David Marchand
  2019-11-26  9:25 ` Ray Kinsella
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: David Marchand @ 2019-11-25 16:13 UTC (permalink / raw)
  To: nhorman, dev
  Cc: thomas, arybchenko, stable, Ray Kinsella, John McNamara,
	Marko Kovacevic, Qiming Yang, Wenzhuo Lu, Declan Doherty,
	Adrien Mazarguil, Ferruh Yigit, Cristian Dumitrescu

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.";
+		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``).
 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
-- 
2.23.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-25 16:13 [dpdk-dev] [RFC PATCH] mark experimental variables David Marchand
@ 2019-11-26  9:25 ` Ray Kinsella
  2019-11-26  9:50   ` David Marchand
  2019-11-26 14:15   ` Neil Horman
  2019-11-26 14:22 ` Neil Horman
  2019-12-02 15:20 ` [dpdk-dev] [PATCH] " David Marchand
  2 siblings, 2 replies; 11+ messages in thread
From: Ray Kinsella @ 2019-11-26  9:25 UTC (permalink / raw)
  To: David Marchand, nhorman, dev
  Cc: thomas, arybchenko, stable, John McNamara, Marko Kovacevic,
	Qiming Yang, Wenzhuo Lu, Declan Doherty, Adrien Mazarguil,
	Ferruh Yigit, Cristian Dumitrescu


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
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-26  9:25 ` Ray Kinsella
@ 2019-11-26  9:50   ` David Marchand
  2019-11-26 14:15   ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: David Marchand @ 2019-11-26  9:50 UTC (permalink / raw)
  To: Ray Kinsella
  Cc: Neil Horman, dev, Thomas Monjalon, Andrew Rybchenko, dpdk stable,
	John McNamara, Marko Kovacevic, Qiming Yang, Wenzhuo Lu,
	Declan Doherty, Adrien Mazarguil, Ferruh Yigit,
	Cristian Dumitrescu

On Tue, Nov 26, 2019 at 10:26 AM Ray Kinsella <mdr@ashroe.eu> 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?

We use this framework so that the users are aware they are relying on
experimental API that can change overnight.
It would not happen if we could hide all of this behind accessors.
But here we reference them through macros / inlines with performance in mind.


This performance impact needs to be confirmed as a real requirement.
I caught some oddity like the librte_port stuff (with no in-tree
users, but different topic).


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

Yes.


> > @@ -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``).

"or" sounds better.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-26  9:25 ` Ray Kinsella
  2019-11-26  9:50   ` David Marchand
@ 2019-11-26 14:15   ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Neil Horman @ 2019-11-26 14:15 UTC (permalink / raw)
  To: Ray Kinsella
  Cc: David Marchand, dev, thomas, arybchenko, stable, John McNamara,
	Marko Kovacevic, Qiming Yang, Wenzhuo Lu, Declan Doherty,
	Adrien Mazarguil, Ferruh Yigit, Cristian Dumitrescu

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-25 16:13 [dpdk-dev] [RFC PATCH] mark experimental variables David Marchand
  2019-11-26  9:25 ` Ray Kinsella
@ 2019-11-26 14:22 ` Neil Horman
  2019-11-27 20:45   ` David Marchand
  2019-12-02 15:20 ` [dpdk-dev] [PATCH] " David Marchand
  2 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2019-11-26 14:22 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, thomas, arybchenko, stable, Ray Kinsella, John McNamara,
	Marko Kovacevic, Qiming Yang, Wenzhuo Lu, Declan Doherty,
	Adrien Mazarguil, Ferruh Yigit, Cristian Dumitrescu

On Mon, Nov 25, 2019 at 05:13:14PM +0100, 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.
> 
I don't see any definition of __rte_experimental_var here, won't the
preprocessor choke on this when you try to compile without that?

Neil

> ---
>  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.";
> +		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``).
>  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
> -- 
> 2.23.0
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-26 14:22 ` Neil Horman
@ 2019-11-27 20:45   ` David Marchand
  2019-11-29 11:43     ` Neil Horman
  0 siblings, 1 reply; 11+ messages in thread
From: David Marchand @ 2019-11-27 20:45 UTC (permalink / raw)
  To: Neil Horman
  Cc: dev, Thomas Monjalon, Andrew Rybchenko, dpdk stable,
	Ray Kinsella, John McNamara, Marko Kovacevic, Qiming Yang,
	Wenzhuo Lu, Declan Doherty, Adrien Mazarguil, Ferruh Yigit,
	Cristian Dumitrescu

On Tue, Nov 26, 2019 at 3:22 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> On Mon, Nov 25, 2019 at 05:13:14PM +0100, 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.
> >
> I don't see any definition of __rte_experimental_var here, won't the
> preprocessor choke on this when you try to compile without that?

Sorry, not getting your point.
If there is an issue, then it is the same as __rte_experimental.

[snip]

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


-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-27 20:45   ` David Marchand
@ 2019-11-29 11:43     ` Neil Horman
  2019-11-29 12:03       ` David Marchand
  0 siblings, 1 reply; 11+ messages in thread
From: Neil Horman @ 2019-11-29 11:43 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, Thomas Monjalon, Andrew Rybchenko, dpdk stable,
	Ray Kinsella, John McNamara, Marko Kovacevic, Qiming Yang,
	Wenzhuo Lu, Declan Doherty, Adrien Mazarguil, Ferruh Yigit,
	Cristian Dumitrescu

On Wed, Nov 27, 2019 at 09:45:46PM +0100, David Marchand wrote:
> On Tue, Nov 26, 2019 at 3:22 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > On Mon, Nov 25, 2019 at 05:13:14PM +0100, 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.
> > >
> > I don't see any definition of __rte_experimental_var here, won't the
> > preprocessor choke on this when you try to compile without that?
> 
> Sorry, not getting your point.
> If there is an issue, then it is the same as __rte_experimental.
> 
No, there is no issue, I'm just blind.  For some reason cscope wasn't finding
the definition of __rte_experimental_var when I applied your patch for me.  Its
clear you have it below.

Acked-by: Neil Horman <nhorman@tuxdriver.com>

> [snip]
> 
> > > 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
> > >
> 
> 
> -- 
> David Marchand
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [RFC PATCH] mark experimental variables
  2019-11-29 11:43     ` Neil Horman
@ 2019-11-29 12:03       ` David Marchand
  0 siblings, 0 replies; 11+ messages in thread
From: David Marchand @ 2019-11-29 12:03 UTC (permalink / raw)
  To: Neil Horman
  Cc: dev, Thomas Monjalon, Andrew Rybchenko, dpdk stable,
	Ray Kinsella, John McNamara, Marko Kovacevic, Qiming Yang,
	Wenzhuo Lu, Declan Doherty, Adrien Mazarguil, Ferruh Yigit,
	Cristian Dumitrescu

On Fri, Nov 29, 2019 at 12:43 PM Neil Horman <nhorman@tuxdriver.com> wrote:
>
> On Wed, Nov 27, 2019 at 09:45:46PM +0100, David Marchand wrote:
> > On Tue, Nov 26, 2019 at 3:22 PM Neil Horman <nhorman@tuxdriver.com> wrote:
> > > On Mon, Nov 25, 2019 at 05:13:14PM +0100, 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.
> > > >
> > > I don't see any definition of __rte_experimental_var here, won't the
> > > preprocessor choke on this when you try to compile without that?
> >
> > Sorry, not getting your point.
> > If there is an issue, then it is the same as __rte_experimental.
> >
> No, there is no issue, I'm just blind.  For some reason cscope wasn't finding
> the definition of __rte_experimental_var when I applied your patch for me.  Its
> clear you have it below.

Ok, thanks for confirming.

> Acked-by: Neil Horman <nhorman@tuxdriver.com>

I will do an extra pass and submit a non RFC patch with your ack if I
have no change.
Thanks Neil.


-- 
David Marchand


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [dpdk-dev] [PATCH] mark experimental variables
  2019-11-25 16:13 [dpdk-dev] [RFC PATCH] mark experimental variables David Marchand
  2019-11-26  9:25 ` Ray Kinsella
  2019-11-26 14:22 ` Neil Horman
@ 2019-12-02 15:20 ` " David Marchand
  2019-12-03  8:33   ` Andrew Rybchenko
  2019-12-03 15:26   ` Neil Horman
  2 siblings, 2 replies; 11+ messages in thread
From: David Marchand @ 2019-12-02 15:20 UTC (permalink / raw)
  To: nhorman, dev
  Cc: thomas, arybchenko, mdr, stable, John McNamara, Marko Kovacevic,
	Qiming Yang, Wenzhuo Lu, Nicolas Chautru, Declan Doherty,
	Adrien Mazarguil, Ferruh Yigit, Cristian Dumitrescu,
	Honnappa Nagarahalli

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 exported
variables.

Fixes: a4bcd61de82d ("buildtools: add script to check experimental API exports")
Cc: stable@dpdk.org

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
Changelog since RFC:
- s/resp\./or/ in documentation,
- caught more variables by looking at the *COM* section,

---
 buildtools/check-experimental-syms.sh      | 17 +++++++++++++++--
 devtools/checkpatches.sh                   | 16 ++++++++++++----
 doc/guides/contributing/abi_policy.rst     |  7 ++++---
 drivers/net/ice/rte_pmd_ice.h              |  8 ++++++++
 lib/librte_bbdev/rte_bbdev.h               |  1 +
 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 +++++
 lib/librte_rcu/rte_rcu_qsbr.h              |  1 +
 10 files changed, 71 insertions(+), 9 deletions(-)

diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-experimental-syms.sh
index f3603e5ba..f3a2f92fb 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 -qe "\(\.data\|\*COM\*\).*[[: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..92b0ae40a 100755
--- a/devtools/checkpatches.sh
+++ b/devtools/checkpatches.sh
@@ -90,11 +90,19 @@ check_experimental_tags() { # <patch>
 				"headers ("current_file")";
 			ret = 1;
 		}
-		if ($1 != "+__rte_experimental" || $2 != "") {
+
+		if (NF == 1 && ($1 == "+__rte_experimental" ||
+				$1 == "+__rte_experimental_var")) {
+			next;
+		}
+		if ($0 ~ "__rte_experimental_var") {
+			print "__rte_experimental_var must appear alone on the line" \
+				" immediately preceding the type of a variable.";
+		} else {
 			print "__rte_experimental must appear alone on the line" \
-				" immediately preceding the return type of a function."
-			ret = 1;
+				" immediately preceding the return type of a function.";
 		}
+		ret = 1;
 	}
 	END {
 		exit ret;
@@ -178,7 +186,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..f933234d6 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 (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..a5c80c43c 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,13 +83,19 @@ 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 */
+__rte_experimental_var
 extern uint64_t rte_net_ice_dynflag_proto_xtr_vlan_mask;
+__rte_experimental_var
 extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv4_mask;
+__rte_experimental_var
 extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_mask;
+__rte_experimental_var
 extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
+__rte_experimental_var
 extern uint64_t rte_net_ice_dynflag_proto_xtr_tcp_mask;
 
 /**
diff --git a/lib/librte_bbdev/rte_bbdev.h b/lib/librte_bbdev/rte_bbdev.h
index 591fb7914..e044dec88 100644
--- a/lib/librte_bbdev/rte_bbdev.h
+++ b/lib/librte_bbdev/rte_bbdev.h
@@ -466,6 +466,7 @@ struct __rte_cache_aligned rte_bbdev {
 };
 
 /** @internal array of all devices */
+__rte_experimental_var
 extern struct rte_bbdev rte_bbdev_devices[];
 
 /**
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
diff --git a/lib/librte_rcu/rte_rcu_qsbr.h b/lib/librte_rcu/rte_rcu_qsbr.h
index 0b5585925..55c100673 100644
--- a/lib/librte_rcu/rte_rcu_qsbr.h
+++ b/lib/librte_rcu/rte_rcu_qsbr.h
@@ -35,6 +35,7 @@ extern "C" {
 #include <rte_debug.h>
 #include <rte_atomic.h>
 
+__rte_experimental_var
 extern int rte_rcu_log_type;
 
 #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
-- 
2.23.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] mark experimental variables
  2019-12-02 15:20 ` [dpdk-dev] [PATCH] " David Marchand
@ 2019-12-03  8:33   ` Andrew Rybchenko
  2019-12-03 15:26   ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Andrew Rybchenko @ 2019-12-03  8:33 UTC (permalink / raw)
  To: David Marchand, nhorman, dev
  Cc: thomas, mdr, stable, John McNamara, Marko Kovacevic, Qiming Yang,
	Wenzhuo Lu, Nicolas Chautru, Declan Doherty, Adrien Mazarguil,
	Ferruh Yigit, Cristian Dumitrescu, Honnappa Nagarahalli

On 12/2/19 6:20 PM, 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 exported
> variables.
> 
> Fixes: a4bcd61de82d ("buildtools: add script to check experimental API exports")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [dpdk-dev] [PATCH] mark experimental variables
  2019-12-02 15:20 ` [dpdk-dev] [PATCH] " David Marchand
  2019-12-03  8:33   ` Andrew Rybchenko
@ 2019-12-03 15:26   ` Neil Horman
  1 sibling, 0 replies; 11+ messages in thread
From: Neil Horman @ 2019-12-03 15:26 UTC (permalink / raw)
  To: David Marchand
  Cc: dev, thomas, arybchenko, mdr, stable, John McNamara,
	Marko Kovacevic, Qiming Yang, Wenzhuo Lu, Nicolas Chautru,
	Declan Doherty, Adrien Mazarguil, Ferruh Yigit,
	Cristian Dumitrescu, Honnappa Nagarahalli

On Mon, Dec 02, 2019 at 04:20:29PM +0100, 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 exported
> variables.
> 
> Fixes: a4bcd61de82d ("buildtools: add script to check experimental API exports")
> Cc: stable@dpdk.org
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> Changelog since RFC:
> - s/resp\./or/ in documentation,
> - caught more variables by looking at the *COM* section,
> 
> ---
>  buildtools/check-experimental-syms.sh      | 17 +++++++++++++++--
>  devtools/checkpatches.sh                   | 16 ++++++++++++----
>  doc/guides/contributing/abi_policy.rst     |  7 ++++---
>  drivers/net/ice/rte_pmd_ice.h              |  8 ++++++++
>  lib/librte_bbdev/rte_bbdev.h               |  1 +
>  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 +++++
>  lib/librte_rcu/rte_rcu_qsbr.h              |  1 +
>  10 files changed, 71 insertions(+), 9 deletions(-)
> 
> diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-experimental-syms.sh
> index f3603e5ba..f3a2f92fb 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 -qe "\(\.data\|\*COM\*\).*[[: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..92b0ae40a 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -90,11 +90,19 @@ check_experimental_tags() { # <patch>
>  				"headers ("current_file")";
>  			ret = 1;
>  		}
> -		if ($1 != "+__rte_experimental" || $2 != "") {
> +
> +		if (NF == 1 && ($1 == "+__rte_experimental" ||
> +				$1 == "+__rte_experimental_var")) {
> +			next;
> +		}
> +		if ($0 ~ "__rte_experimental_var") {
> +			print "__rte_experimental_var must appear alone on the line" \
> +				" immediately preceding the type of a variable.";
> +		} else {
>  			print "__rte_experimental must appear alone on the line" \
> -				" immediately preceding the return type of a function."
> -			ret = 1;
> +				" immediately preceding the return type of a function.";
>  		}
> +		ret = 1;
>  	}
>  	END {
>  		exit ret;
> @@ -178,7 +186,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..f933234d6 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 (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..a5c80c43c 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,13 +83,19 @@ 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 */
> +__rte_experimental_var
>  extern uint64_t rte_net_ice_dynflag_proto_xtr_vlan_mask;
> +__rte_experimental_var
>  extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv4_mask;
> +__rte_experimental_var
>  extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_mask;
> +__rte_experimental_var
>  extern uint64_t rte_net_ice_dynflag_proto_xtr_ipv6_flow_mask;
> +__rte_experimental_var
>  extern uint64_t rte_net_ice_dynflag_proto_xtr_tcp_mask;
>  
>  /**
> diff --git a/lib/librte_bbdev/rte_bbdev.h b/lib/librte_bbdev/rte_bbdev.h
> index 591fb7914..e044dec88 100644
> --- a/lib/librte_bbdev/rte_bbdev.h
> +++ b/lib/librte_bbdev/rte_bbdev.h
> @@ -466,6 +466,7 @@ struct __rte_cache_aligned rte_bbdev {
>  };
>  
>  /** @internal array of all devices */
> +__rte_experimental_var
>  extern struct rte_bbdev rte_bbdev_devices[];
>  
>  /**
> 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
> diff --git a/lib/librte_rcu/rte_rcu_qsbr.h b/lib/librte_rcu/rte_rcu_qsbr.h
> index 0b5585925..55c100673 100644
> --- a/lib/librte_rcu/rte_rcu_qsbr.h
> +++ b/lib/librte_rcu/rte_rcu_qsbr.h
> @@ -35,6 +35,7 @@ extern "C" {
>  #include <rte_debug.h>
>  #include <rte_atomic.h>
>  
> +__rte_experimental_var
>  extern int rte_rcu_log_type;
>  
>  #if RTE_LOG_DP_LEVEL >= RTE_LOG_DEBUG
> -- 
> 2.23.0
> 
> 
Acked-by: Neil Horman <nhorman@tuxdriver.com>

^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, back to index

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-25 16:13 [dpdk-dev] [RFC PATCH] mark experimental variables David Marchand
2019-11-26  9:25 ` Ray Kinsella
2019-11-26  9:50   ` David Marchand
2019-11-26 14:15   ` Neil Horman
2019-11-26 14:22 ` Neil Horman
2019-11-27 20:45   ` David Marchand
2019-11-29 11:43     ` Neil Horman
2019-11-29 12:03       ` David Marchand
2019-12-02 15:20 ` [dpdk-dev] [PATCH] " David Marchand
2019-12-03  8:33   ` Andrew Rybchenko
2019-12-03 15:26   ` Neil Horman

DPDK patches and discussions

Archives are clonable:
	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev


Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/ public-inbox