Re: [dpdk-dev] [PATCH] eal: promote some service core functions to stable

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Jun 21, 2019 at 02:45:45PM +0200, David Marchand wrote:
> On Thu, Jun 20, 2019 at 10:16 PM David Marchand <david.marchand@redhat.com>
> wrote:
> 
> >
> >
> > On Thu, Jun 20, 2019 at 9:45 PM David Marchand <david.marchand@redhat.com>
> > wrote:
> >
> >>
> >>
> >> On Thu, Jun 20, 2019 at 8:26 PM Aaron Conole <aconole@redhat.com> wrote:
> >>
> >>> Gage Eads <gage.eads@intel.com> writes:
> >>>
> >>> > The functions rte_service_may_be_active(),
> >>> rte_service_lcore_attr_get(),
> >>> > and rte_service_attr_reset_all() were introduced nearly a year ago in
> >>> DPDK
> >>> > 18.08. They can be considered non-experimental for the 19.08 release.
> >>> >
> >>> > rte_service_may_be_active() is used by eventdev and the sw PMD, and
> >>> this
> >>> > commit allows them to not need any experimental API.
> >>> >
> >>> > Signed-off-by: Gage Eads <gage.eads@intel.com>
> >>> > ---
> >>> >  drivers/event/sw/Makefile                   |  1 -
> >>> >  drivers/event/sw/meson.build                |  1 -
> >>> >  lib/librte_eal/common/include/rte_service.h | 15 +++------------
> >>> >  lib/librte_eal/common/rte_service.c         |  6 +++---
> >>> >  lib/librte_eal/rte_eal_version.map          |  6 +++---
> >>> >  lib/librte_eventdev/Makefile                |  1 -
> >>> >  lib/librte_eventdev/meson.build             |  1 -
> >>> >  7 files changed, 9 insertions(+), 22 deletions(-)
> >>> >
> >>> > diff --git a/drivers/event/sw/Makefile b/drivers/event/sw/Makefile
> >>> > index 81236a392..c6600e836 100644
> >>> > --- a/drivers/event/sw/Makefile
> >>> > +++ b/drivers/event/sw/Makefile
> >>> > @@ -7,7 +7,6 @@ include $(RTE_SDK)/mk/rte.vars.mk
> >>> >  LIB = librte_pmd_sw_event.a
> >>> >
> >>> >  # build flags
> >>> > -CFLAGS += -DALLOW_EXPERIMENTAL_API
> >>> >  CFLAGS += -O3
> >>> >  CFLAGS += $(WERROR_FLAGS)
> >>> >  # for older GCC versions, allow us to initialize an event using
> >>> > diff --git a/drivers/event/sw/meson.build
> >>> b/drivers/event/sw/meson.build
> >>> > index 30d221647..985012219 100644
> >>> > --- a/drivers/event/sw/meson.build
> >>> > +++ b/drivers/event/sw/meson.build
> >>> > @@ -1,7 +1,6 @@
> >>> >  # SPDX-License-Identifier: BSD-3-Clause
> >>> >  # Copyright(c) 2017 Intel Corporation
> >>> >
> >>> > -allow_experimental_apis = true
> >>>
> >>> I don't think you can remove these.  There are still some experimental
> >>> APIs (f.e. the rename for rte_cryptodev_sym_session_get_private_data
> >>> marked that function as experimental and it will cause build breakage).
> >>>
> >>> Maybe I'm mis understanding it?  It would be good to get verification
> >>> from Bruce whether that API should not be marked as experimental (it was
> >>> just a rename, so not sure...) - maybe that's a follow up for this
> >>> patch?
> >>>
> >>> See: https://travis-ci.com/ovsrobot/dpdk/jobs/209722145 for an example
> >>>
> >>> The odd thing is I only see it on the clang builds - perhaps it's a
> >>> missing definition for the clang compiler.
> >>>
> >>
> >> Erf, it looks like the __rte_experimental tag is affected by the order in
> >> the declaration of the symbol.
> >>
> >> --- a/lib/librte_cryptodev/rte_cryptodev.h
> >> +++ b/lib/librte_cryptodev/rte_cryptodev.h
> >> @@ -1245,7 +1245,7 @@ struct rte_cryptodev_asym_session *
> >> __rte_experimental
> >>   *  - On success return pointer to user data.
> >>   *  - On failure returns NULL.
> >>   */
> >> -void * __rte_experimental
> >> +__rte_experimental void *
> >>  rte_cryptodev_sym_session_get_user_data(
> >>                                         struct rte_cryptodev_sym_session
> >> *sess);
> >>
> >>
> > https://hastebin.com/micomogoqi.cs
> >
> > Does it mean that the "void *" type had been marked as deprecated with the
> > previous syntax?
> > Requesting compiler experts assistance :-)
> >
> 
> 
> Ok, did a new pass on the tree.. found quite some sites where we have
> issues (and other discrepancies... I started a new patchset).
> Looked at gcc documentation [1], and to me the safer approach would be to
> enforce that __rte_experimental is the first thing of a symbol declaration.
> 
> Comments?
> 
Yes, thats the only way it works, in fact I'm suprised gcc didn't throw an error
about expecting an asm statement if you put it anywhere else

Neil

> 
> 1: https://gcc.gnu.org/onlinedocs/gcc/Attribute-Syntax.html
> 
> -- 
> David Marchand