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 18DB0A052E; Mon, 3 Feb 2020 13:53:36 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 6E5701BF9A; Mon, 3 Feb 2020 13:53:35 +0100 (CET) Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 3CFAF1BEB2 for ; Mon, 3 Feb 2020 13:53:33 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by fmsmga102.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 03 Feb 2020 04:53:32 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.70,397,1574150400"; d="scan'208";a="248795391" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.61]) ([10.237.221.61]) by orsmga002.jf.intel.com with ESMTP; 03 Feb 2020 04:53:29 -0800 To: Neil Horman Cc: Cristian Dumitrescu , Eelco Chaudron , dev@dpdk.org, Thomas Monjalon , Luca Boccassi , David Marchand , Bruce Richardson , Ian Stokes References: <20200129122953.2016199-1-ferruh.yigit@intel.com> <20200202185334.GA6783@localhost.localdomain> From: Ferruh Yigit Autocrypt: addr=ferruh.yigit@intel.com; prefer-encrypt=mutual; keydata= mQINBFXZCFABEADCujshBOAaqPZpwShdkzkyGpJ15lmxiSr3jVMqOtQS/sB3FYLT0/d3+bvy qbL9YnlbPyRvZfnP3pXiKwkRoR1RJwEo2BOf6hxdzTmLRtGtwWzI9MwrUPj6n/ldiD58VAGQ +iR1I/z9UBUN/ZMksElA2D7Jgg7vZ78iKwNnd+vLBD6I61kVrZ45Vjo3r+pPOByUBXOUlxp9 GWEKKIrJ4eogqkVNSixN16VYK7xR+5OUkBYUO+sE6etSxCr7BahMPKxH+XPlZZjKrxciaWQb +dElz3Ab4Opl+ZT/bK2huX+W+NJBEBVzjTkhjSTjcyRdxvS1gwWRuXqAml/sh+KQjPV1PPHF YK5LcqLkle+OKTCa82OvUb7cr+ALxATIZXQkgmn+zFT8UzSS3aiBBohg3BtbTIWy51jNlYdy ezUZ4UxKSsFuUTPt+JjHQBvF7WKbmNGS3fCid5Iag4tWOfZoqiCNzxApkVugltxoc6rG2TyX CmI2rP0mQ0GOsGXA3+3c1MCdQFzdIn/5tLBZyKy4F54UFo35eOX8/g7OaE+xrgY/4bZjpxC1 1pd66AAtKb3aNXpHvIfkVV6NYloo52H+FUE5ZDPNCGD0/btFGPWmWRmkPybzColTy7fmPaGz cBcEEqHK4T0aY4UJmE7Ylvg255Kz7s6wGZe6IR3N0cKNv++O7QARAQABtCVGZXJydWggWWln aXQgPGZlcnJ1aC55aWdpdEBpbnRlbC5jb20+iQJUBBMBCgA+AhsDAh4BAheABQsJCAcDBRUK CQgLBRYCAwEAFiEE0jZTh0IuwoTjmYHH+TPrQ98TYR8FAl1meboFCQlupOoACgkQ+TPrQ98T YR9ACBAAv2tomhyxY0Tp9Up7mNGLfEdBu/7joB/vIdqMRv63ojkwr9orQq5V16V/25+JEAD0 60cKodBDM6HdUvqLHatS8fooWRueSXHKYwJ3vxyB2tWDyZrLzLI1jxEvunGodoIzUOtum0Ce gPynnfQCelXBja0BwLXJMplM6TY1wXX22ap0ZViC0m714U5U4LQpzjabtFtjT8qOUR6L7hfy YQ72PBuktGb00UR/N5UrR6GqB0x4W41aZBHXfUQnvWIMmmCrRUJX36hOTYBzh+x86ULgg7H2 1499tA4o6rvE13FiGccplBNWCAIroAe/G11rdoN5NBgYVXu++38gTa/MBmIt6zRi6ch15oLA Ln2vHOdqhrgDuxjhMpG2bpNE36DG/V9WWyWdIRlz3NYPCDM/S3anbHlhjStXHOz1uHOnerXM 1jEjcsvmj1vSyYoQMyRcRJmBZLrekvgZeh7nJzbPHxtth8M7AoqiZ/o/BpYU+0xZ+J5/szWZ aYxxmIRu5ejFf+Wn9s5eXNHmyqxBidpCWvcbKYDBnkw2+Y9E5YTpL0mS0dCCOlrO7gca27ux ybtbj84aaW1g0CfIlUnOtHgMCmz6zPXThb+A8H8j3O6qmPoVqT3qnq3Uhy6GOoH8Fdu2Vchh TWiF5yo+pvUagQP6LpslffufSnu+RKAagkj7/RSuZV25Ag0EV9ZMvgEQAKc0Db17xNqtSwEv mfp4tkddwW9XA0tWWKtY4KUdd/jijYqc3fDD54ESYpV8QWj0xK4YM0dLxnDU2IYxjEshSB1T qAatVWz9WtBYvzalsyTqMKP3w34FciuL7orXP4AibPtrHuIXWQOBECcVZTTOdZYGAzaYzxiA ONzF9eTiwIqe9/oaOjTwTLnOarHt16QApTYQSnxDUQljeNvKYt1lZE/gAUUxNLWsYyTT+22/ vU0GDUahsJxs1+f1yEr+OGrFiEAmqrzpF0lCS3f/3HVTU6rS9cK3glVUeaTF4+1SK5ZNO35p iVQCwphmxa+dwTG/DvvHYCtgOZorTJ+OHfvCnSVjsM4kcXGjJPy3JZmUtyL9UxEbYlrffGPQ I3gLXIGD5AN5XdAXFCjjaID/KR1c9RHd7Oaw0Pdcq9UtMLgM1vdX8RlDuMGPrj5sQrRVbgYH fVU/TQCk1C9KhzOwg4Ap2T3tE1umY/DqrXQgsgH71PXFucVjOyHMYXXugLT8YQ0gcBPHy9mZ qw5mgOI5lCl6d4uCcUT0l/OEtPG/rA1lxz8ctdFBVOQOxCvwRG2QCgcJ/UTn5vlivul+cThi 6ERPvjqjblLncQtRg8izj2qgmwQkvfj+h7Ex88bI8iWtu5+I3K3LmNz/UxHBSWEmUnkg4fJl Rr7oItHsZ0ia6wWQ8lQnABEBAAGJAjwEGAEKACYCGwwWIQTSNlOHQi7ChOOZgcf5M+tD3xNh HwUCXWZ5wAUJB3FgggAKCRD5M+tD3xNhH2O+D/9OEz62YuJQLuIuOfL67eFTIB5/1+0j8Tsu o2psca1PUQ61SZJZOMl6VwNxpdvEaolVdrpnSxUF31kPEvR0Igy8HysQ11pj8AcgH0a9FrvU /8k2Roccd2ZIdpNLkirGFZR7LtRw41Kt1Jg+lafI0efkiHKMT/6D/P1EUp1RxOBNtWGV2hrd 0Yg9ds+VMphHHU69fDH02SwgpvXwG8Qm14Zi5WQ66R4CtTkHuYtA63sS17vMl8fDuTCtvfPF HzvdJLIhDYN3Mm1oMjKLlq4PUdYh68Fiwm+boJoBUFGuregJFlO3hM7uHBDhSEnXQr5mqpPM 6R/7Q5BjAxrwVBisH0yQGjsWlnysRWNfExAE2sRePSl0or9q19ddkRYltl6X4FDUXy2DTXa9 a+Fw4e1EvmcF3PjmTYs9IE3Vc64CRQXkhujcN4ZZh5lvOpU8WgyDxFq7bavFnSS6kx7Tk29/ wNJBp+cf9qsQxLbqhW5kfORuZGecus0TLcmpZEFKKjTJBK9gELRBB/zoN3j41hlEl7uTUXTI JQFLhpsFlEdKLujyvT/aCwP3XWT+B2uZDKrMAElF6ltpTxI53JYi22WO7NH7MR16Fhi4R6vh FHNBOkiAhUpoXRZXaCR6+X4qwA8CwHGqHRBfYFSU/Ulq1ZLR+S3hNj2mbnSx0lBs1eEqe2vh cA== Message-ID: <7beadbbd-bf0d-e545-d9bb-0e3ae8228b71@intel.com> Date: Mon, 3 Feb 2020 12:53:28 +0000 MIME-Version: 1.0 In-Reply-To: <20200202185334.GA6783@localhost.localdomain> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC] meter: fix ABI break due to experimental tag removal 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 2/2/2020 6:53 PM, Neil Horman wrote: > On Wed, Jan 29, 2020 at 12:29:53PM +0000, Ferruh Yigit wrote: >> Duplicated the existing symbol and versioned one as experimental and >> other as stable. >> >> Created VERSION_SYMBOL_EXPERIMENTAL helper macro. >> >> Updated the 'check-experimental-syms.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) >> >> Updated meson build system to allow the versioning, >> 'use_function_versioning = true', not sure why it was disabled by >> default. >> >> Fixes: 30512af820fe ("meter: remove experimental flag from RFC4115 trTCM API") >> > I'm not sure I understand the purpose here. > > I think I understand what you are trying to do. I believe that you are > trying to move the experimental symbols in librte_meter to be part of > the official ABI, while still maintaining backward compatibility with > applications built against the experimental symbol versions, correct? Yes, exactly. > > I can understand the desire, and I'm not exactly opposed to providing a > mechanism for that, but it seems somewhat complex, and to be honest, > part of the drawback to using experimental symbols as an application > developer is understanding that you may need to rebuild when those > symbols change (even if the change is the symbol removal and replacement > with an identical one that has a versioned tag). > > I think the right answer for people encountering this condition is to > just rebuild. Otherwise, we are creating an environment in which we are > implicitly communicating to users that we are maintaining a modicum of > stability in experimental symobls. This will mean, even the the API was perfect it will force its users to recompile (and re-package, re-deploy etc..) at least once, this is feeling like we are still breaking user applications easily. And this may create a stronger motivation by applications not to use experimental APIs, I can't decide if this is a good thing or bad thing. The issue is amplified in the LTS, If we remove experimental tag in LTS, will be breaking the apps using that experimental API, just to mature the API. If we keep it as experimental, the API will be mature in main repo, but the LTS has to keep exact same API as experimental up to two years. But if we can do the versioning in the master, LTS can backport it and can have mature version of that API in LTS without breaking the existing users. > > A few more nits in line > >> 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 >> >> I didn't like the idea of duplicating the symbol but not able to find to >> alias it without duplicating, if there is a way please share. >> >> Also not tested with old binaries, only verified the symbols in new >> binaries. >> --- >> buildtools/check-experimental-syms.sh | 3 +- >> .../common/include/rte_function_versioning.h | 4 ++ >> lib/librte_meter/rte_meter.c | 59 ++++++++++++++++++- >> lib/librte_meter/rte_meter_version.map | 8 +++ >> lib/meson.build | 2 +- >> 5 files changed, 71 insertions(+), 5 deletions(-) >> >> diff --git a/buildtools/check-experimental-syms.sh b/buildtools/check-experimental-syms.sh >> index f3603e5ba..35c4bf006 100755 >> --- a/buildtools/check-experimental-syms.sh >> +++ b/buildtools/check-experimental-syms.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/common/include/rte_function_versioning.h b/lib/librte_eal/common/include/rte_function_versioning.h >> index c924351d5..ab102b06e 100644 >> --- a/lib/librte_eal/common/include/rte_function_versioning.h >> +++ b/lib/librte_eal/common/include/rte_function_versioning.h >> @@ -46,6 +46,9 @@ >> */ >> #define VERSION_SYMBOL(b, e, n) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@DPDK_" RTE_STR(n)) >> >> + >> +#define VERSION_SYMBOL_EXPERIMENTAL(b, e) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@EXPERIMENTAL") >> + > I don't know that you want to make a explicit clone of this macro. > instead what you might want to do is something like: > > #define __VERSION_SYMBOL(b, e, n, t) __asm__(".symver " RTE_STR(b) RTE_STR(e) ", " RTE_STR(b) "@" RTE_STR(t) "_" RTE_STR(n)) > > #define VERSION_SYMBOL(b, e, n) __VERSION_SYMBOL(b, e, n, DPDK) > > Thats not exactly right, but you get the idea, then you can emulate > VERSION_SYMBOL_EXPERIMENTAL with __VERSION_SYMBOL(b, e, n, EXPERIMENTAL) +1 > > >> /* >> * BIND_DEFAULT_SYMBOL >> * Creates a symbol version entry instructing the linker to bind references to >> @@ -79,6 +82,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/rte_meter.c b/lib/librte_meter/rte_meter.c >> index da01429a8..5244537fa 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, 20.0.1); >> +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_config, _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, 20.0.1); >> +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); >> diff --git a/lib/librte_meter/rte_meter_version.map b/lib/librte_meter/rte_meter_version.map >> index fc12cc0bf..b4b043174 100644 >> --- a/lib/librte_meter/rte_meter_version.map >> +++ b/lib/librte_meter/rte_meter_version.map >> @@ -20,4 +20,12 @@ DPDK_20.0.1 { >> 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; >> +}; >> diff --git a/lib/meson.build b/lib/meson.build >> index 0af3efab2..553964831 100644 >> --- a/lib/meson.build >> +++ b/lib/meson.build >> @@ -48,7 +48,7 @@ foreach l:libraries >> reason = '' # set if build == false to explain why >> name = l >> allow_experimental_apis = false >> - use_function_versioning = false >> + use_function_versioning = true >> sources = [] >> headers = [] >> includes = [] >> -- >> 2.24.1 >> >>