From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <ferruh.yigit@intel.com> Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 95A2A12001 for <dev@dpdk.org>; Fri, 12 Jan 2018 16:53:26 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Jan 2018 07:53:25 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.46,349,1511856000"; d="scan'208";a="18716965" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.237.220.48]) ([10.237.220.48]) by FMSMGA003.fm.intel.com with ESMTP; 12 Jan 2018 07:53:23 -0800 To: Neil Horman <nhorman@tuxdriver.com> Cc: dev@dpdk.org, thomas@monjalon.net, john.mcnamara@intel.com, bruce.richardson@intel.com References: <20171201185628.16261-1-nhorman@tuxdriver.com> <20171213151728.16747-1-nhorman@tuxdriver.com> <20171213151728.16747-5-nhorman@tuxdriver.com> <bd98e4f5-ee8a-a97e-b788-9c9dde049177@intel.com> <20180111212405.GB6879@hmswarspite.think-freely.org> <5d3ac7b3-b21d-489c-ec06-b235ada2ec52@intel.com> <20180112142515.GB20015@hmswarspite.think-freely.org> From: Ferruh Yigit <ferruh.yigit@intel.com> Message-ID: <168a8d95-0252-a3b1-b96b-461cae44294b@intel.com> Date: Fri, 12 Jan 2018 15:53:23 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.2 MIME-Version: 1.0 In-Reply-To: <20180112142515.GB20015@hmswarspite.think-freely.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCHv4 4/5] dpdk: add __experimental tag to appropriate api calls X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions <dev.dpdk.org> List-Unsubscribe: <https://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <https://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> X-List-Received-Date: Fri, 12 Jan 2018 15:53:27 -0000 On 1/12/2018 2:25 PM, Neil Horman wrote: > On Fri, Jan 12, 2018 at 11:50:01AM +0000, Ferruh Yigit wrote: >> On 1/11/2018 9:24 PM, Neil Horman wrote: >>> On Thu, Jan 11, 2018 at 08:06:33PM +0000, Ferruh Yigit wrote: >>>> On 12/13/2017 3:17 PM, Neil Horman wrote: >>>>> Append the __experimental tag to api calls appearing in the EXPERIMENTAL >>>>> section of their libraries version map >>>>> >>>>> Signed-off-by: Neil Horman <nhorman@tuxdriver.com> >>>>> CC: Thomas Monjalon <thomas@monjalon.net> >>>>> CC: "Mcnamara, John" <john.mcnamara@intel.com> >>>>> CC: Bruce Richardson <bruce.richardson@intel.com> >>>>> --- >>>>> lib/librte_eal/common/eal_common_dev.c | 6 ++- >>>>> lib/librte_eal/common/eal_common_devargs.c | 7 +-- >>>>> lib/librte_eal/common/include/rte_dev.h | 6 ++- >>>>> lib/librte_eal/common/include/rte_devargs.h | 8 ++-- >>>>> lib/librte_eal/common/include/rte_service.h | 47 ++++++++++--------- >>>>> .../common/include/rte_service_component.h | 14 +++--- >>>>> lib/librte_eal/common/rte_service.c | 52 ++++++++++++---------- >>>>> lib/librte_eal/linuxapp/eal/eal.c | 1 + >>>>> lib/librte_ether/rte_mtr.c | 25 ++++++----- >>>>> lib/librte_ether/rte_mtr.h | 26 +++++------ >>>>> lib/librte_flow_classify/rte_flow_classify.c | 13 +++--- >>>>> lib/librte_flow_classify/rte_flow_classify.h | 11 ++--- >>>>> lib/librte_security/rte_security.c | 16 +++---- >>>>> lib/librte_security/rte_security.h | 23 +++++----- >>>> >>>> It may not be the responsibility of this patchset, but there are more >>>> experimental APIs in DPDK. >>>> >>> Thats an interesting statement to make. This patchset creates a build time >>> check that compares symbols located in the EXPERIMENTAL version section of a >>> libraries' version map file to the symbols that are marked with this new tag, >>> throwing an error if they don't match. I believe what you say in that there may >>> be additional APIs that are experimental, but given that, I would have to >>> conclude one of the following: >>> >>> 1) The missing API's are macros or static inline functions that are not exported >>> from libraries directly >>> >>> 2) The documentation for experimental API's are out of sync, in that they have >>> legitimately moved to be supported API's and the documentation needs to be >>> updated >>> >>> 3) There are API's which are experimental that have been incorrectly placed in a >>> versioned tag. >>> >>> I made a pretty good effort to scan comments for the word EXPERIMENTAL so that I >>> could catch item (1). And while I may not have caught them all, I'd like to >>> think I got a good chunk of them. That leaves cleanup of (2) and (3), which I >>> think this patchset can help us idenfity. >>> >>>> Using EXPERIMENTAL tag in linker script is relatively new approach and this was >>>> not a requirement, so many experimental APIs are documented in API documentation >>>> (header file doxygen comment). >>>> Sample: librte_member >>>> >>> That sounds like case (3) above. >>> >>> Thats a bit odd. I understand that the use of the EXPERIMENTAL version tag is >>> new, but once it was introduced it should have been made a requirement. There >>> would have been no penalty for moving the version number (as doing so would not >>> have violated ABI guarantees, given that those API's were appropriately >>> documented as experimental). If they have not been, then the use of the >>> EXPERIMENTAL tag isn't overly useful, as it doesn't provide any segregation of >>> the stable ABI from the unstable ABI. >>> >>>> It is required to scan all header files and update their linker scripts for the >>>> experimental APIs. >>>> >>> Yes and no. If a given library is not marked as experimental in its version >>> map, this change won't flag it as a problem, but if its intended to be >>> experimental (i.e. if its likely to have its ABI fluctuate), then yes, we should >>> take the appropriate steps to flag it as such properly. >>> >>> If a given library is intended to be experimental, I would say yes, >>> the author should make the appropriate chage to the version map, and then the >>> corresponding change to the headers and c files with this new tag. >>> Alternatively, they might choose to simply update the documentation to reflect >>> the fact that the ABI for that library is now stable. >>> >>> The thing that should definately not hapen though, is a half measure. We >>> shouldn't allow libraries to call themselves experimental, and then excuse them >>> from any rules we have regarding their in-code representation. If we have an >>> EXPERIMENTAL version in the map, we should require its use, and by extension >>> require this tag when its merged for the reasons previously outlined >> >> My comment is for your item (3), but it is not fair to say "incorrectly placed" >> because if I don't miss anything this has never been documented as correct way >> to do, and lots of the existing usage is _before_ we start using EXPERIMENTAL >> tag in the linker script, so they were doing right thing for that time. >> > Ok, Apologies, perhaps incorrectly placed isn't a fair term to use. Perhaps it > would be better to say the experimental specification was insufficiently > documented? The point however remains. Defining an API category that conveys a > freedom of modification of ABI needs to follow rules for identification, and > providing a mechanism to tag said ABIs in the code without a requirement to use > it creates an confusing situation to say the least (i.e. some APIS will be > listed as belonging to the EXPERIMENTAL version, others won't, but will be > documented as such, creating an ambiguous status). > >> Question is, is this patchset should fix them, since now this patchset defines >> using EXPERIMENTAL tag in linker script as way to do it, or should we wait >> maintainers to fix it after this has been documented. Waiting for maintainer may >> take time because not all maintainers are following the mail list closely to >> capture all expectations. >> > > In answer, I think waiting for maintainers to correct their experimental > use isn't going to get anything done. As you said, not all maintainers monitor > all conversations closely enough to pick up on the need, and as we move forward > this effort will likely get de-prioitized, as the status quo will just continue > to exist. I would propose that we accept this patch, and then I subsequently > preform an audit on the documentation to find other APIs which are documented as > EXPERIMENTAL, but not tagged as such in their version files. I can submit > subsequent patches to reconcile those APIs that I find, which should get on the > respective maintainers radars. Does that sound reasonable? Sounds good to me, hence Series Acked-by: Ferruh Yigit <ferruh.yigit@intel.com> > > Neil > >>> >>> Neil >>> >>> >>>>> 14 files changed, 139 insertions(+), 116 deletions(-) >>>> >>>> <...> >>>> >>>> >> >>