[dpdk-dev] [PATCHv4 4/5] dpdk: add __experimental tag to appropriate api calls

[Ferruh Yigit]

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 at tuxdriver.com>
>>>>> CC: Thomas Monjalon <thomas at monjalon.net>
>>>>> CC: "Mcnamara, John" <john.mcnamara at intel.com>
>>>>> CC: Bruce Richardson <bruce.richardson at 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 at intel.com>

> 
> Neil
> 
>>>
>>> Neil
>>>
>>>
>>>>>  14 files changed, 139 insertions(+), 116 deletions(-)
>>>>
>>>> <...>
>>>>
>>>>
>>
>>