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(-)
>>>>
>>>> <...>
>>>>
>>>>
>>
>>