From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 20216A04DD; Tue, 26 Nov 2019 15:16:30 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id EEF482B96; Tue, 26 Nov 2019 15:16:28 +0100 (CET) Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id 6476B28EE; Tue, 26 Nov 2019 15:16:27 +0100 (CET) Received: from 2606-a000-111b-43ee-0000-0000-0000-115f.inf6.spectrum.com ([2606:a000:111b:43ee::115f] helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1iZbcl-0007tB-Gy; Tue, 26 Nov 2019 09:15:31 -0500 Date: Tue, 26 Nov 2019 09:15:26 -0500 From: Neil Horman To: Ray Kinsella Cc: David Marchand , dev@dpdk.org, thomas@monjalon.net, arybchenko@solarflare.com, stable@dpdk.org, John McNamara , Marko Kovacevic , Qiming Yang , Wenzhuo Lu , Declan Doherty , Adrien Mazarguil , Ferruh Yigit , Cristian Dumitrescu Message-ID: <20191126141526.GB21200@hmswarspite.think-freely.org> References: <20191125161314.18804-1-david.marchand@redhat.com> <23c3c0ff-6cf1-3aca-50b8-a89037afdcac@ashroe.eu> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <23c3c0ff-6cf1-3aca-50b8-a89037afdcac@ashroe.eu> User-Agent: Mutt/1.12.1 (2019-06-15) X-Spam-Score: -2.9 (--) X-Spam-Status: No Subject: Re: [dpdk-dev] [RFC PATCH] mark experimental variables X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 > > --- > > 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() { # > > "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 () { # > > 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 > > >