From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtp.tuxdriver.com (charlotte.tuxdriver.com [70.61.120.58]) by dpdk.org (Postfix) with ESMTP id AA4047CC4 for ; Thu, 11 Jan 2018 22:24:15 +0100 (CET) Received: from [107.15.66.59] (helo=localhost) by smtp.tuxdriver.com with esmtpsa (TLSv1:AES256-SHA:256) (Exim 4.63) (envelope-from ) id 1eZkKT-0000hw-Sv; Thu, 11 Jan 2018 16:24:12 -0500 Date: Thu, 11 Jan 2018 16:24:05 -0500 From: Neil Horman To: Ferruh Yigit Cc: dev@dpdk.org, thomas@monjalon.net, john.mcnamara@intel.com, bruce.richardson@intel.com Message-ID: <20180111212405.GB6879@hmswarspite.think-freely.org> References: <20171201185628.16261-1-nhorman@tuxdriver.com> <20171213151728.16747-1-nhorman@tuxdriver.com> <20171213151728.16747-5-nhorman@tuxdriver.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) X-Spam-Score: -2.9 (--) X-Spam-Status: No 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 11 Jan 2018 21:24:16 -0000 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 > > CC: Thomas Monjalon > > CC: "Mcnamara, John" > > CC: Bruce Richardson > > --- > > 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 Neil > > 14 files changed, 139 insertions(+), 116 deletions(-) > > <...> > >