From: Neil Horman <nhorman@tuxdriver.com>
To: Ferruh Yigit <ferruh.yigit@intel.com>
Cc: dev@dpdk.org, thomas@monjalon.net, john.mcnamara@intel.com,
bruce.richardson@intel.com
Subject: Re: [dpdk-dev] [PATCHv4 4/5] dpdk: add __experimental tag to appropriate api calls
Date: Fri, 12 Jan 2018 09:25:15 -0500 [thread overview]
Message-ID: <20180112142515.GB20015@hmswarspite.think-freely.org> (raw)
In-Reply-To: <5d3ac7b3-b21d-489c-ec06-b235ada2ec52@intel.com>
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?
Neil
> >
> > Neil
> >
> >
> >>> 14 files changed, 139 insertions(+), 116 deletions(-)
> >>
> >> <...>
> >>
> >>
>
>
next prev parent reply other threads:[~2018-01-12 14:25 UTC|newest]
Thread overview: 83+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-12-01 18:56 [dpdk-dev] [PATCH 0/4] dpdk: enhance EXPERIMENTAL api tagging Neil Horman
2017-12-01 18:56 ` [dpdk-dev] [PATCH 1/4] buildtools: Add tool to check EXPERIMENTAL api exports Neil Horman
2017-12-01 18:56 ` [dpdk-dev] [PATCH 2/4] compat: Add __experimental macro Neil Horman
2017-12-01 18:56 ` [dpdk-dev] [PATCH 3/4] dpdk: add __experimental tag to appropriate api calls Neil Horman
2017-12-01 18:56 ` [dpdk-dev] [PATCH 4/4] Makefiles: Add experimental tag check and warnings to trigger on use Neil Horman
2017-12-08 17:14 ` [dpdk-dev] [PATCHv2 0/4] dpdk: enhance EXPERIMENTAL api tagging Neil Horman
2017-12-08 17:14 ` [dpdk-dev] [PATCHv2 1/4] buildtools: Add tool to check EXPERIMENTAL api exports Neil Horman
2017-12-08 17:14 ` [dpdk-dev] [PATCHv2 2/4] compat: Add __experimental macro Neil Horman
2017-12-08 17:14 ` [dpdk-dev] [PATCHv2 3/4] Makefiles: Add experimental tag check and warnings to trigger on use Neil Horman
2017-12-11 11:35 ` Bruce Richardson
2017-12-11 12:40 ` Neil Horman
2017-12-11 12:45 ` Bruce Richardson
2017-12-11 18:44 ` Neil Horman
2017-12-08 17:14 ` [dpdk-dev] [PATCHv2 4/4] dpdk: add __experimental tag to appropriate api calls Neil Horman
2017-12-11 19:36 ` [dpdk-dev] [PATCHv3 0/4] dpdk: enhance EXPERIMENTAL api tagging Neil Horman
2017-12-11 19:36 ` [dpdk-dev] [PATCHv3 1/4] buildtools: Add tool to check EXPERIMENTAL api exports Neil Horman
2017-12-11 19:36 ` [dpdk-dev] [PATCHv3 2/4] compat: Add __experimental macro Neil Horman
2017-12-11 19:36 ` [dpdk-dev] [PATCHv3 3/4] Makefiles: Add experimental tag check and warnings to trigger on use Neil Horman
2017-12-11 19:36 ` [dpdk-dev] [PATCHv3 4/4] dpdk: add __experimental tag to appropriate api calls Neil Horman
2017-12-12 14:07 ` [dpdk-dev] [PATCHv3 0/4] dpdk: enhance EXPERIMENTAL api tagging Bruce Richardson
2017-12-30 17:15 ` Neil Horman
2018-01-04 12:56 ` Neil Horman
2018-01-05 14:08 ` Thomas Monjalon
2018-01-05 16:00 ` Neil Horman
2018-01-09 1:32 ` [dpdk-dev] [dpdk-ci] " Neil Horman
2018-01-09 9:20 ` Thomas Monjalon
2018-01-09 12:36 ` Neil Horman
2018-01-19 15:44 ` Neil Horman
2017-12-12 14:33 ` [dpdk-dev] " Mcnamara, John
2017-12-12 20:18 ` Neil Horman
2017-12-12 15:11 ` Wiles, Keith
2017-12-12 20:14 ` Neil Horman
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 " Neil Horman
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 1/5] buildtools: Add tool to check EXPERIMENTAL api exports Neil Horman
2018-01-21 18:31 ` Thomas Monjalon
2018-01-21 22:07 ` Neil Horman
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 2/5] compat: Add __experimental macro Neil Horman
2018-01-21 18:37 ` Thomas Monjalon
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 3/5] Makefiles: Add experimental tag check and warnings to trigger on use Neil Horman
2018-01-11 20:06 ` Ferruh Yigit
2018-01-11 20:50 ` Neil Horman
2018-01-12 11:49 ` Ferruh Yigit
2018-01-12 12:44 ` Neil Horman
2018-01-21 18:54 ` Thomas Monjalon
2018-01-22 1:34 ` Neil Horman
2018-01-22 1:37 ` Thomas Monjalon
2018-01-21 18:50 ` Thomas Monjalon
2018-01-22 1:19 ` Neil Horman
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 4/5] dpdk: add __experimental tag to appropriate api calls Neil Horman
2018-01-11 20:06 ` Ferruh Yigit
2018-01-11 21:24 ` Neil Horman
2018-01-12 11:50 ` Ferruh Yigit
2018-01-12 14:25 ` Neil Horman [this message]
2018-01-12 15:53 ` Ferruh Yigit
2017-12-13 15:17 ` [dpdk-dev] [PATCHv4 5/5] doc: Add ABI __experimental tag documentation Neil Horman
2017-12-13 15:32 ` Bruce Richardson
2018-01-11 20:06 ` Ferruh Yigit
2018-01-11 21:29 ` Neil Horman
2018-01-12 11:50 ` Ferruh Yigit
2018-01-12 14:37 ` Neil Horman
2018-01-12 15:55 ` Ferruh Yigit
2018-01-13 0:28 ` Neil Horman
2018-01-13 15:56 ` Thomas Monjalon
2018-01-14 14:36 ` Neil Horman
2018-01-14 16:27 ` Thomas Monjalon
2018-01-21 20:14 ` Thomas Monjalon
2017-12-13 15:32 ` [dpdk-dev] [PATCHv4 0/4] dpdk: enhance EXPERIMENTAL api tagging Bruce Richardson
2017-12-21 14:21 ` Neil Horman
2017-12-30 19:20 ` Luca Boccassi
2017-12-31 1:57 ` Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [PATCH 0/5] " Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [[PATCH v5] 1/5] buildtools: Add tool to check EXPERIMENTAL api exports Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [[PATCH v5] 2/5] compat: Add __rte_experimental macro Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [[PATCH v5] 3/5] Makefiles: Add experimental tag check and warnings to trigger on use Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [[PATCH v5] 4/5] dpdk: add __rte_experimental tag to appropriate api calls Neil Horman
2018-01-22 1:48 ` [dpdk-dev] [[PATCH v5] 5/5] doc: Add ABI __experimental tag documentation Neil Horman
2018-01-23 10:35 ` Mcnamara, John
2018-01-29 21:42 ` Thomas Monjalon
2018-01-29 21:46 ` [dpdk-dev] [PATCH 0/5] dpdk: enhance EXPERIMENTAL api tagging Thomas Monjalon
2018-01-30 15:54 ` Neil Horman
2018-01-30 16:15 ` Thomas Monjalon
2018-01-31 12:18 ` Neil Horman
2018-01-31 12:36 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20180112142515.GB20015@hmswarspite.think-freely.org \
--to=nhorman@tuxdriver.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@intel.com \
--cc=john.mcnamara@intel.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).