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 471C5A04C1; Tue, 26 Nov 2019 10:26:09 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id A9CEC2A5D; Tue, 26 Nov 2019 10:26:08 +0100 (CET) Received: from relay0104.mxlogin.com (relay0104.mxlogin.com [199.181.239.104]) by dpdk.org (Postfix) with ESMTP id AD61528EE; Tue, 26 Nov 2019 10:26:07 +0100 (CET) Received: from filter004.mxroute.com (unknown [116.203.155.46]) (using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by relay0104.mxlogin.com (Postfix) with ESMTPS id 69648CBC00D0; Tue, 26 Nov 2019 03:26:06 -0600 (CST) Received: from galaxy.mxroute.com (unknown [23.92.70.113]) by filter004.mxroute.com (Postfix) with ESMTPS id DD0373E9EB; Tue, 26 Nov 2019 09:25:58 +0000 (UTC) Received: from [192.198.151.43] by galaxy.mxroute.com with esmtpsa (TLSv1.2:ECDHE-RSA-AES128-GCM-SHA256:128) (Exim 4.91) (envelope-from ) id 1iZWss-0001bl-DW; Tue, 26 Nov 2019 04:11:46 -0500 To: David Marchand , nhorman@tuxdriver.com, dev@dpdk.org Cc: 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 References: <20191125161314.18804-1-david.marchand@redhat.com> From: Ray Kinsella Openpgp: preference=signencrypt Autocrypt: addr=mdr@ashroe.eu; keydata= mQINBFv8B3wBEAC+5ImcgbIvadt3axrTnt7Sxch3FsmWTTomXfB8YiuHT8KL8L/bFRQSL1f6 ASCHu3M89EjYazlY+vJUWLr0BhK5t/YI7bQzrOuYrl9K94vlLwzD19s/zB/g5YGGR5plJr0s JtJsFGEvF9LL3e+FKMRXveQxBB8A51nAHfwG0WSyx53d61DYz7lp4/Y4RagxaJoHp9lakn8j HV2N6rrnF+qt5ukj5SbbKWSzGg5HQF2t0QQ5tzWhCAKTfcPlnP0GymTBfNMGOReWivi3Qqzr S51Xo7hoGujUgNAM41sxpxmhx8xSwcQ5WzmxgAhJ/StNV9cb3HWIoE5StCwQ4uXOLplZNGnS uxNdegvKB95NHZjRVRChg/uMTGpg9PqYbTIFoPXjuk27sxZLRJRrueg4tLbb3HM39CJwSB++ YICcqf2N+GVD48STfcIlpp12/HI+EcDSThzfWFhaHDC0hyirHxJyHXjnZ8bUexI/5zATn/ux TpMbc/vicJxeN+qfaVqPkCbkS71cHKuPluM3jE8aNCIBNQY1/j87k5ELzg3qaesLo2n1krBH bKvFfAmQuUuJT84/IqfdVtrSCTabvDuNBDpYBV0dGbTwaRfE7i+LiJJclUr8lOvHUpJ4Y6a5 0cxEPxm498G12Z3NoY/mP5soItPIPtLR0rA0fage44zSPwp6cQARAQABtBxSYXkgS2luc2Vs bGEgPG1kckBhc2hyb2UuZXU+iQJUBBMBCAA+FiEEcDUDlKDJaDuJlfZfdJdaH/sCCpsFAlv8 B3wCGyMFCQlmAYAFCwkIBwIGFQoJCAsCBBYCAwECHgECF4AACgkQdJdaH/sCCptdtRAAl0oE msa+djBVYLIsax+0f8acidtWg2l9f7kc2hEjp9h9aZCpPchQvhhemtew/nKavik3RSnLTAyn B3C/0GNlmvI1l5PFROOgPZwz4xhJKGN7jOsRrbkJa23a8ly5UXwF3Vqnlny7D3z+7cu1qq/f VRK8qFyWkAb+xgqeZ/hTcbJUWtW+l5Zb+68WGEp8hB7TuJLEWb4+VKgHTpQ4vElYj8H3Z94a 04s2PJMbLIZSgmKDASnyrKY0CzTpPXx5rSJ1q+B1FCsfepHLqt3vKSALa3ld6bJ8fSJtDUJ7 JLiU8dFZrywgDIVme01jPbjJtUScW6jONLvhI8Z2sheR71UoKqGomMHNQpZ03ViVWBEALzEt TcjWgJFn8yAmxqM4nBnZ+hE3LbMo34KCHJD4eg18ojDt3s9VrDLa+V9fNxUHPSib9FD9UX/1 +nGfU/ZABmiTuUDM7WZdXri7HaMpzDRJUKI6b+/uunF8xH/h/MHW16VuMzgI5dkOKKv1LejD dT5mA4R+2zBS+GsM0oa2hUeX9E5WwjaDzXtVDg6kYq8YvEd+m0z3M4e6diFeLS77/sAOgaYL 92UcoKD+Beym/fVuC6/55a0e12ksTmgk5/ZoEdoNQLlVgd2INtvnO+0k5BJcn66ZjKn3GbEC VqFbrnv1GnA58nEInRCTzR1k26h9nmS5Ag0EW/wHfAEQAMth1vHr3fOZkVOPfod3M6DkQir5 xJvUW5EHgYUjYCPIa2qzgIVVuLDqZgSCCinyooG5dUJONVHj3nCbITCpJp4eB3PI84RPfDcC hf/V34N/Gx5mTeoymSZDBmXT8YtvV/uJvn+LvHLO4ZJdvq5ZxmDyxfXFmkm3/lLw0+rrNdK5 pt6OnVlCqEU9tcDBezjUwDtOahyV20XqxtUttN4kQWbDRkhT+HrA9WN9l2HX91yEYC+zmF1S OhBqRoTPLrR6g4sCWgFywqztpvZWhyIicJipnjac7qL/wRS+wrWfsYy6qWLIV80beN7yoa6v ccnuy4pu2uiuhk9/edtlmFE4dNdoRf7843CV9k1yRASTlmPkU59n0TJbw+okTa9fbbQgbIb1 pWsAuicRHyLUIUz4f6kPgdgty2FgTKuPuIzJd1s8s6p2aC1qo+Obm2gnBTduB+/n1Jw+vKpt 07d+CKEKu4CWwvZZ8ktJJLeofi4hMupTYiq+oMzqH+V1k6QgNm0Da489gXllU+3EFC6W1qKj tkvQzg2rYoWeYD1Qn8iXcO4Fpk6wzylclvatBMddVlQ6qrYeTmSbCsk+m2KVrz5vIyja0o5Y yfeN29s9emXnikmNfv/dA5fpi8XCANNnz3zOfA93DOB9DBf0TQ2/OrSPGjB3op7RCfoPBZ7u AjJ9dM7VABEBAAGJAjwEGAEIACYWIQRwNQOUoMloO4mV9l90l1of+wIKmwUCW/wHfAIbDAUJ CWYBgAAKCRB0l1of+wIKm3KlD/9w/LOG5rtgtCUWPl4B3pZvGpNym6XdK8cop9saOnE85zWf u+sKWCrxNgYkYP7aZrYMPwqDvilxhbTsIJl5HhPgpTO1b0i+c0n1Tij3EElj5UCg3q8mEc17 c+5jRrY3oz77g7E3oPftAjaq1ybbXjY4K32o3JHFR6I8wX3m9wJZJe1+Y+UVrrjY65gZFxcA thNVnWKErarVQGjeNgHV4N1uF3pIx3kT1N4GSnxhoz4Bki91kvkbBhUgYfNflGURfZT3wIKK +d50jd7kqRouXUCzTdzmDh7jnYrcEFM4nvyaYu0JjSS5R672d9SK5LVIfWmoUGzqD4AVmUW8 pcv461+PXchuS8+zpltR9zajl72Q3ymlT4BTAQOlCWkD0snBoKNUB5d2EXPNV13nA0qlm4U2 GpROfJMQXjV6fyYRvttKYfM5xYKgRgtP0z5lTAbsjg9WFKq0Fndh7kUlmHjuAIwKIV4Tzo75 QO2zC0/NTaTjmrtiXhP+vkC4pcrOGNsbHuaqvsc/ZZ0siXyYsqbctj/sCd8ka2r94u+c7o4l BGaAm+FtwAfEAkXHu4y5Phuv2IRR+x1wTey1U1RaEPgN8xq0LQ1OitX4t2mQwjdPihZQBCnZ wzOrkbzlJMNrMKJpEgulmxAHmYJKgvZHXZXtLJSejFjR0GdHJcL5rwVOMWB8cg== Message-ID: <23c3c0ff-6cf1-3aca-50b8-a89037afdcac@ashroe.eu> Date: Tue, 26 Nov 2019 09:25:49 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101 Thunderbird/60.9.1 MIME-Version: 1.0 In-Reply-To: <20191125161314.18804-1-david.marchand@redhat.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AuthUser: mdr@ashroe.eu 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" 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 > --- > 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 >