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 <nhor...@tuxdriver.com> >>>>> CC: Thomas Monjalon <tho...@monjalon.net> >>>>> CC: "Mcnamara, John" <john.mcnam...@intel.com> >>>>> CC: Bruce Richardson <bruce.richard...@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.yi...@intel.com> > > Neil > >>> >>> Neil >>> >>> >>>>> 14 files changed, 139 insertions(+), 116 deletions(-) >>>> >>>> <...> >>>> >>>> >> >>
