Re: [dpdk-dev] [PATCH v5 1/4] lib/rcu: add resource reclamation APIs

[David Marchand <david.marchand@redhat.com>]

On Wed, Apr 22, 2020 at 10:42 AM David Marchand
<david.marchand@redhat.com> wrote:
>
> On Wed, Apr 22, 2020 at 10:37 AM Ananyev, Konstantin
> <konstantin.ananyev@intel.com> wrote:
> >
> > > Add resource reclamation APIs to make it simple for applications
> > > and libraries to integrate rte_rcu library.
> > >
> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > Reviewed-by: Ola Liljedhal <ola.liljedhal@arm.com>
> > > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > > ---
> > >  lib/librte_rcu/Makefile            |   2 +-
> > >  lib/librte_rcu/meson.build         |   7 +
> > >  lib/librte_rcu/rcu_qsbr_pvt.h      |  66 +++++++++
> > >  lib/librte_rcu/rte_rcu_qsbr.c      | 227 ++++++++++++++++++++++++++++-
> > >  lib/librte_rcu/rte_rcu_qsbr.h      | 194 +++++++++++++++++++++++-
> > >  lib/librte_rcu/rte_rcu_version.map |   4 +
> > >  lib/meson.build                    |   6 +-
> > >  7 files changed, 501 insertions(+), 5 deletions(-)
> > >  create mode 100644 lib/librte_rcu/rcu_qsbr_pvt.h
> > >
> > > diff --git a/lib/librte_rcu/Makefile b/lib/librte_rcu/Makefile
> > > index 728669975..553bca2ef 100644
> > > --- a/lib/librte_rcu/Makefile
> > > +++ b/lib/librte_rcu/Makefile
> > > @@ -7,7 +7,7 @@ include $(RTE_SDK)/mk/rte.vars.mk
> > >  LIB = librte_rcu.a
> > >
> > >  CFLAGS += $(WERROR_FLAGS) -I$(SRCDIR) -O3
> > > -LDLIBS += -lrte_eal
> > > +LDLIBS += -lrte_eal -lrte_ring
> > >
> > >  EXPORT_MAP := rte_rcu_version.map
> > >
> > > diff --git a/lib/librte_rcu/meson.build b/lib/librte_rcu/meson.build
> > > index c009ae4b7..3eb2ace17 100644
> > > --- a/lib/librte_rcu/meson.build
> > > +++ b/lib/librte_rcu/meson.build
> > > @@ -3,3 +3,10 @@
> > >
> > >  sources = files('rte_rcu_qsbr.c')
> > >  headers = files('rte_rcu_qsbr.h')
> > > +
> > > +# for clang 32-bit compiles we need libatomic for 64-bit atomic ops
> > > +if cc.get_id() == 'clang' and dpdk_conf.get('RTE_ARCH_64') == false
> > > +     ext_deps += cc.find_library('atomic')
> > > +endif
> > > +
> >
> > As a nit - as Pavan patch is already integrated into mainline,
> > this is not necessary any more, I think.
>
> I can handle this.
>
>
> > Also noticed that most of make builds failed due to dependency problem:
> > http://mails.dpdk.org/archives/test-report/2020-April/127765.html
> > I can't reproduce it locally, but my guess that we need to move rcu above
> > ring in this mk file: mk/rte.app.mk
> > Probably something like that:
> > diff --git a/mk/rte.app.mk b/mk/rte.app.mk
> > index da12b9eec..8e5d023de 100644
> > --- a/mk/rte.app.mk
> > +++ b/mk/rte.app.mk
> > @@ -91,13 +91,13 @@ _LDLIBS-$(CONFIG_RTE_LIBRTE_MEMPOOL)        += -lrte_mempool
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_STACK)          += -lrte_stack
> >  _LDLIBS-$(CONFIG_RTE_DRIVER_MEMPOOL_RING)   += -lrte_mempool_ring
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_OCTEONTX2_MEMPOOL) += -lrte_mempool_octeontx2
> > +_LDLIBS-$(CONFIG_RTE_LIBRTE_RCU)            += -lrte_rcu
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_RING)           += -lrte_ring
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_PCI)            += -lrte_pci
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_EAL)            += -lrte_eal
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_CMDLINE)        += -lrte_cmdline
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_REORDER)        += -lrte_reorder
> >  _LDLIBS-$(CONFIG_RTE_LIBRTE_SCHED)          += -lrte_sched
> > -_LDLIBS-$(CONFIG_RTE_LIBRTE_RCU)            += -lrte_rcu
>
> No, just moving will not express a dependency.

Fixed with:

diff --git a/lib/Makefile b/lib/Makefile
index 2cbb096f1..8bc0c2e4a 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -118,6 +118,7 @@ DIRS-$(CONFIG_RTE_LIBRTE_TELEMETRY) += librte_telemetry
 DEPDIRS-librte_telemetry := librte_eal librte_metrics librte_ethdev
 DIRS-$(CONFIG_RTE_LIBRTE_RCU) += librte_rcu
-DEPDIRS-librte_rcu := librte_eal
+DEPDIRS-librte_rcu := librte_eal librte_ring

 ifeq ($(CONFIG_RTE_EXEC_ENV_LINUX),y)
 DIRS-$(CONFIG_RTE_LIBRTE_KNI) += librte_kni
diff --git a/lib/librte_rcu/meson.build b/lib/librte_rcu/meson.build
index 3eb2ace17..09abc5204 100644
--- a/lib/librte_rcu/meson.build
+++ b/lib/librte_rcu/meson.build
@@ -4,9 +4,4 @@
 sources = files('rte_rcu_qsbr.c')
 headers = files('rte_rcu_qsbr.h')

-# for clang 32-bit compiles we need libatomic for 64-bit atomic ops
-if cc.get_id() == 'clang' and dpdk_conf.get('RTE_ARCH_64') == false
-       ext_deps += cc.find_library('atomic')
-endif
-
 deps += ['ring']


-- 
David Marchand