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 40E25A0093; Mon, 18 May 2020 08:30:14 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 8D2271D52B; Mon, 18 May 2020 08:30:12 +0200 (CEST) Received: from relay0119.mxlogin.com (relay0119.mxlogin.com [199.181.239.119]) by dpdk.org (Postfix) with ESMTP id F06001D42A for ; Mon, 18 May 2020 08:30:10 +0200 (CEST) Received: from filter003.mxroute.com ([168.235.111.26] 168-235-111-26.cloud.ramnode.com) (Authenticated sender: mN4UYu2MZsgR) by relay0119.mxlogin.com (ZoneMTA) with ESMTPSA id 1722679b2bb000e6c4.002 for (version=TLSv1/SSLv3 cipher=ECDHE-RSA-AES128-GCM-SHA256); Mon, 18 May 2020 06:30:09 +0000 X-Zone-Loop: bddd8e5e6b701404c144559777118c95991904e5b225 X-Originating-IP: [168.235.111.26] Received: from galaxy.mxroute.com (unknown [23.92.70.113]) by filter003.mxroute.com (Postfix) with ESMTPS id 49FAA6002E; Mon, 18 May 2020 06:30:03 +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 1jaYq5-00069w-5L; Mon, 18 May 2020 02:01:25 -0400 To: "Dumitrescu, Cristian" , "Yigit, Ferruh" , Neil Horman , Eelco Chaudron Cc: "dev@dpdk.org" , Thomas Monjalon , David Marchand , "stable@dpdk.org" , Luca Boccassi , "Richardson, Bruce" , "Stokes, Ian" , Andrzej Ostruszka References: <20200513121149.2283385-1-ferruh.yigit@intel.com> <20200514161104.1546493-1-ferruh.yigit@intel.com> From: Ray Kinsella 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: <476bc533-b967-ec72-fe9c-8286eab4639e@ashroe.eu> Date: Mon, 18 May 2020 07:29:53 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:68.0) Gecko/20100101 Thunderbird/68.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit X-AuthUser: mdr@ashroe.eu Subject: Re: [dpdk-dev] [PATCH v4] meter: provide experimental alias of API for old apps 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 17/05/2020 20:52, Dumitrescu, Cristian wrote: > Hi Ferruh, > >> -----Original Message----- >> From: Yigit, Ferruh >> Sent: Thursday, May 14, 2020 5:11 PM >> To: Ray Kinsella ; Neil Horman >> ; Dumitrescu, Cristian >> ; Eelco Chaudron >> Cc: dev@dpdk.org; Yigit, Ferruh ; Thomas Monjalon >> ; David Marchand ; >> stable@dpdk.org; Luca Boccassi ; Richardson, Bruce >> ; Stokes, Ian ; Andrzej >> Ostruszka >> Subject: [PATCH v4] meter: provide experimental alias of API for old apps >> >> On v20.02 some meter APIs have been matured and symbols moved from >> EXPERIMENTAL to DPDK_20.0.1 block. >> >> This can break the applications that were using these mentioned APIs on >> v19.11. Although there is no modification on the APIs and the action is >> positive and matures the APIs, the affect can be negative to >> applications. >> >> Since experimental APIs can change or go away without notice as part of >> contract, to prevent this negative affect that may occur by maturing >> experimental API, a process update already suggested, which enables >> aliasing without forcing it: >> https://patches.dpdk.org/patch/65863/ >> > > Personally, I am not convinced this is really needed. > > Are there any users asking for this? As it happens it is all breaking our abi regression test suite. One of the things we do is to run the unit tests binary from v19.11 against the latest release. > Is there any other library where this is also applied, or is librte_meter the only library? librte_meter is the only example AFAIK. But then we only have one example of needing symbol versioning also at the moment (Cryptodev). This is going to happen with experimental symbols that have been around a while, that have become used in applications. It is a non-mandatory tool a maintainer can use to preserve abi compatibility. > >> This patch provides aliasing by duplicating the existing and versioned >> symbols as experimental. >> >> Since symbols moved from DPDK_20.0.1 to DPDK_21 block in the v20.05, the >> aliasing done between EXPERIMENTAL and DPDK_21. >> >> Also following changes done to enabling aliasing: >> >> Created VERSION_SYMBOL_EXPERIMENTAL helper macro. >> >> Updated the 'check-symbols.sh' buildtool, which was complaining that the >> symbol is in EXPERIMENTAL tag in .map file but it is not in the >> .experimental section (__rte_experimental tag is missing). >> Updated tool in a way it won't complain if the symbol in the >> EXPERIMENTAL tag duplicated in some other block in .map file (versioned) >> >> Enabled function versioning for meson build for the library. >> >> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM >> API") >> Cc: stable@dpdk.org >> >> Signed-off-by: Ferruh Yigit >> --- >> Cc: Neil Horman >> Cc: Thomas Monjalon >> Cc: Luca Boccassi >> Cc: David Marchand >> Cc: Bruce Richardson >> Cc: Ian Stokes >> Cc: Eelco Chaudron >> Cc: Andrzej Ostruszka >> Cc: Ray Kinsella >> >> v2: >> * Commit log updated >> >> v3: >> * added suggested comment to VERSION_SYMBOL_EXPERIMENTAL macro >> >> v4: >> * update script name in commit log, remove empty line >> --- >> buildtools/check-symbols.sh | 3 +- >> .../include/rte_function_versioning.h | 9 +++ >> lib/librte_meter/meson.build | 1 + >> lib/librte_meter/rte_meter.c | 59 ++++++++++++++++++- >> lib/librte_meter/rte_meter_version.map | 8 +++ >> 5 files changed, 76 insertions(+), 4 deletions(-) >> >> diff --git a/buildtools/check-symbols.sh b/buildtools/check-symbols.sh >> index 3df57c322c..e407553a34 100755 >> --- a/buildtools/check-symbols.sh >> +++ b/buildtools/check-symbols.sh >> @@ -26,7 +26,8 @@ ret=0 >> for SYM in `$LIST_SYMBOL -S EXPERIMENTAL $MAPFILE |cut -d ' ' -f 3` >> do >> if grep -q "\.text.*[[:space:]]$SYM$" $DUMPFILE && >> - ! grep -q "\.text\.experimental.*[[:space:]]$SYM$" >> $DUMPFILE >> + ! grep -q "\.text\.experimental.*[[:space:]]$SYM$" >> $DUMPFILE && >> + $LIST_SYMBOL -s $SYM $MAPFILE | grep -q EXPERIMENTAL >> then >> cat >&2 <<- END_OF_MESSAGE >> $SYM is not flagged as experimental >> diff --git a/lib/librte_eal/include/rte_function_versioning.h >> b/lib/librte_eal/include/rte_function_versioning.h >> index b9f862d295..f588f2643b 100644 >> --- a/lib/librte_eal/include/rte_function_versioning.h >> +++ b/lib/librte_eal/include/rte_function_versioning.h >> @@ -46,6 +46,14 @@ >> */ >> #define VERSION_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b) >> RTE_STR(e) ", " RTE_STR(b) "@DPDK_" RTE_STR(n)) >> >> +/* >> + * VERSION_SYMBOL_EXPERIMENTAL >> + * Creates a symbol version table entry binding the symbol >> @EXPERIMENTAL to the internal >> + * function name . The macro is used when a symbol matures to >> become part of the stable ABI, >> + * to provide an alias to experimental for some time. >> + */ >> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) __asm__(".symver " >> RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@EXPERIMENTAL") >> + >> /* >> * BIND_DEFAULT_SYMBOL >> * Creates a symbol version entry instructing the linker to bind references to >> @@ -79,6 +87,7 @@ >> * No symbol versioning in use >> */ >> #define VERSION_SYMBOL(b, e, n) >> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) >> #define __vsym >> #define BIND_DEFAULT_SYMBOL(b, e, n) >> #define MAP_STATIC_SYMBOL(f, p) f __attribute__((alias(RTE_STR(p)))) >> diff --git a/lib/librte_meter/meson.build b/lib/librte_meter/meson.build >> index 646fd4d43f..fce0368437 100644 >> --- a/lib/librte_meter/meson.build >> +++ b/lib/librte_meter/meson.build >> @@ -3,3 +3,4 @@ >> >> sources = files('rte_meter.c') >> headers = files('rte_meter.h') >> +use_function_versioning = true >> diff --git a/lib/librte_meter/rte_meter.c b/lib/librte_meter/rte_meter.c >> index da01429a8b..c600b05064 100644 >> --- a/lib/librte_meter/rte_meter.c >> +++ b/lib/librte_meter/rte_meter.c >> @@ -9,6 +9,7 @@ >> #include >> #include >> #include >> +#include >> >> #include "rte_meter.h" >> >> @@ -119,8 +120,8 @@ rte_meter_trtcm_config(struct rte_meter_trtcm *m, >> return 0; >> } >> >> -int >> -rte_meter_trtcm_rfc4115_profile_config( >> +static int >> +rte_meter_trtcm_rfc4115_profile_config_( >> struct rte_meter_trtcm_rfc4115_profile *p, >> struct rte_meter_trtcm_rfc4115_params *params) >> { >> @@ -145,7 +146,35 @@ rte_meter_trtcm_rfc4115_profile_config( >> } >> >> int >> -rte_meter_trtcm_rfc4115_config( >> +rte_meter_trtcm_rfc4115_profile_config_s( >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + struct rte_meter_trtcm_rfc4115_params *params); >> +int >> +rte_meter_trtcm_rfc4115_profile_config_s( >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + struct rte_meter_trtcm_rfc4115_params *params) >> +{ >> + return rte_meter_trtcm_rfc4115_profile_config_(p, params); >> +} >> +BIND_DEFAULT_SYMBOL(rte_meter_trtcm_rfc4115_profile_config, _s, 21); >> +MAP_STATIC_SYMBOL(int rte_meter_trtcm_rfc4115_profile_config(struct >> rte_meter_trtcm_rfc4115_profile *p, >> + struct rte_meter_trtcm_rfc4115_params *params), >> rte_meter_trtcm_rfc4115_profile_config_s); >> + >> +int >> +rte_meter_trtcm_rfc4115_profile_config_e( >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + struct rte_meter_trtcm_rfc4115_params *params); >> +int >> +rte_meter_trtcm_rfc4115_profile_config_e( >> + struct rte_meter_trtcm_rfc4115_profile *p, >> + struct rte_meter_trtcm_rfc4115_params *params) >> +{ >> + return rte_meter_trtcm_rfc4115_profile_config_(p, params); >> +} >> +VERSION_SYMBOL_EXPERIMENTAL(rte_meter_trtcm_rfc4115_profile_conf >> ig, _e); >> + >> +static int >> +rte_meter_trtcm_rfc4115_config_( >> struct rte_meter_trtcm_rfc4115 *m, >> struct rte_meter_trtcm_rfc4115_profile *p) >> { >> @@ -160,3 +189,27 @@ rte_meter_trtcm_rfc4115_config( >> >> return 0; >> } >> + >> +int >> +rte_meter_trtcm_rfc4115_config_s(struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p); >> +int >> +rte_meter_trtcm_rfc4115_config_s(struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p) >> +{ >> + return rte_meter_trtcm_rfc4115_config_(m, p); >> +} >> +BIND_DEFAULT_SYMBOL(rte_meter_trtcm_rfc4115_config, _s, 21); >> +MAP_STATIC_SYMBOL(int rte_meter_trtcm_rfc4115_config(struct >> rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p), >> rte_meter_trtcm_rfc4115_config_s); >> + >> +int >> +rte_meter_trtcm_rfc4115_config_e(struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p); >> +int >> +rte_meter_trtcm_rfc4115_config_e(struct rte_meter_trtcm_rfc4115 *m, >> + struct rte_meter_trtcm_rfc4115_profile *p) >> +{ >> + return rte_meter_trtcm_rfc4115_config_(m, p); >> +} >> +VERSION_SYMBOL_EXPERIMENTAL(rte_meter_trtcm_rfc4115_config, _e); > > To me, this is a significant amount of dead code that does not add any functionality and does not bring any added value to the library for any user. I am not a build system expert, but I would definitely prefer avoiding adding any C code to the library for this purpose, and just modify the map file, would this approach be possible? Approach is exactly the same as the rest of symbol versioning. > Also, very important, is this C code to be added permanently or is it added just on a temporary basis? If temporary, when is it going to be removed? It will be removed in the v21 (20.11 lts) release. When we officially rev the abi and start afresh. > >> diff --git a/lib/librte_meter/rte_meter_version.map >> b/lib/librte_meter/rte_meter_version.map >> index 2c7dadbcac..b493bcebe9 100644 >> --- a/lib/librte_meter/rte_meter_version.map >> +++ b/lib/librte_meter/rte_meter_version.map >> @@ -20,4 +20,12 @@ DPDK_21 { >> rte_meter_trtcm_rfc4115_color_blind_check; >> rte_meter_trtcm_rfc4115_config; >> rte_meter_trtcm_rfc4115_profile_config; >> + >> } DPDK_20.0; >> + >> +EXPERIMENTAL { >> + global: >> + >> + rte_meter_trtcm_rfc4115_config; >> + rte_meter_trtcm_rfc4115_profile_config; >> +}; >> -- >> 2.25.4 > > Regards, > Cristian >