Re: [dpdk-dev] [PATCH v2 2/2] mempool/nb_stack: add non-blocking stack mempool

[Gavin Hu (Arm Technology China)]

> -----Original Message-----
> From: Eads, Gage <gage.e...@intel.com>
> Sent: Thursday, January 17, 2019 11:16 PM
> To: Richardson, Bruce <bruce.richard...@intel.com>
> Cc: Gavin Hu (Arm Technology China) <gavin...@arm.com>;
> dev@dpdk.org; olivier.m...@6wind.com; arybche...@solarflare.com;
> Ananyev, Konstantin <konstantin.anan...@intel.com>; Honnappa
> Nagarahalli <honnappa.nagaraha...@arm.com>; Ruifeng Wang (Arm
> Technology China) <ruifeng.w...@arm.com>; Phil Yang (Arm Technology
> China) <phil.y...@arm.com>
> Subject: RE: [dpdk-dev] [PATCH v2 2/2] mempool/nb_stack: add non-
> blocking stack mempool
>
>
>
> > -----Original Message-----
> > From: Richardson, Bruce
> > Sent: Thursday, January 17, 2019 8:21 AM
> > To: Eads, Gage <gage.e...@intel.com>
> > Cc: Gavin Hu (Arm Technology China) <gavin...@arm.com>;
> dev@dpdk.org;
> > olivier.m...@6wind.com; arybche...@solarflare.com; Ananyev,
> Konstantin
> > <konstantin.anan...@intel.com>; Honnappa Nagarahalli
> > <honnappa.nagaraha...@arm.com>; Ruifeng Wang (Arm Technology
> China)
> > <ruifeng.w...@arm.com>; Phil Yang (Arm Technology China)
> > <phil.y...@arm.com>
> > Subject: Re: [dpdk-dev] [PATCH v2 2/2] mempool/nb_stack: add non-
> blocking
> > stack mempool
> >
> > On Thu, Jan 17, 2019 at 02:11:22PM +0000, Eads, Gage wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Gavin Hu (Arm Technology China) [mailto:gavin...@arm.com]
> > > > Sent: Thursday, January 17, 2019 2:06 AM
> > > > To: Eads, Gage <gage.e...@intel.com>; dev@dpdk.org
> > > > Cc: olivier.m...@6wind.com; arybche...@solarflare.com; Richardson,
> > > > Bruce <bruce.richard...@intel.com>; Ananyev, Konstantin
> > > > <konstantin.anan...@intel.com>; Honnappa Nagarahalli
> > > > <honnappa.nagaraha...@arm.com>; Ruifeng Wang (Arm Technology
> China)
> > > > <ruifeng.w...@arm.com>; Phil Yang (Arm Technology China)
> > > > <phil.y...@arm.com>
> > > > Subject: RE: [dpdk-dev] [PATCH v2 2/2] mempool/nb_stack: add
> > > > non-blocking stack mempool
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: dev <dev-boun...@dpdk.org> On Behalf Of Gage Eads
> > > > > Sent: Wednesday, January 16, 2019 6:33 AM
> > > > > To: dev@dpdk.org
> > > > > Cc: olivier.m...@6wind.com; arybche...@solarflare.com;
> > > > > bruce.richard...@intel.com; konstantin.anan...@intel.com
> > > > > Subject: [dpdk-dev] [PATCH v2 2/2] mempool/nb_stack: add
> > > > > non-blocking stack mempool
> > > > >
> > > > > This commit adds support for non-blocking (linked list based)
> > > > > stack mempool handler. The stack uses a 128-bit compare-and-
> swap
> > > > > instruction, and thus is limited to x86_64. The 128-bit CAS
> > > > > atomically updates the stack top pointer and a modification
> > > > > counter, which protects against the ABA problem.
> > > > >
> > > > > In mempool_perf_autotest the lock-based stack outperforms the
> non-
> > > > > blocking handler*, however:
> > > > > - For applications with preemptible pthreads, a lock-based stack's
> > > > >   worst-case performance (i.e. one thread being preempted while
> > > > >   holding the spinlock) is much worse than the non-blocking stack's.
> > > > > - Using per-thread mempool caches will largely mitigate the
> performance
> > > > >   difference.
> > > > >
> > > > > *Test setup: x86_64 build with default config, dual-socket Xeon
> > > > > E5-2699 v4, running on isolcpus cores with a tickless scheduler.
> > > > > The lock-based stack's rate_persec was 1x-3.5x the non-blocking
> stack's.
> > > > >
> > > > > Signed-off-by: Gage Eads <gage.e...@intel.com>
> > > > > ---
> > > > >  MAINTAINERS                                        |   4 +
> > > > >  config/common_base                                 |   1 +
> > > > >  doc/guides/prog_guide/env_abstraction_layer.rst    |   5 +
> > > > >  drivers/mempool/Makefile                           |   3 +
> > > > >  drivers/mempool/meson.build                        |   5 +
> > > > >  drivers/mempool/nb_stack/Makefile                  |  23 ++++
> > > > >  drivers/mempool/nb_stack/meson.build               |   4 +
> > > > >  drivers/mempool/nb_stack/nb_lifo.h                 | 147
> > > > > +++++++++++++++++++++
> > > > >  drivers/mempool/nb_stack/rte_mempool_nb_stack.c    | 125
> > > > > ++++++++++++++++++
> > > > >  .../nb_stack/rte_mempool_nb_stack_version.map      |   4 +
> > > > >  mk/rte.app.mk                                      |   7 +-
> > > > >  11 files changed, 326 insertions(+), 2 deletions(-)  create mode
> > > > > 100644 drivers/mempool/nb_stack/Makefile  create mode 100644
> > > > > drivers/mempool/nb_stack/meson.build
> > > > >  create mode 100644 drivers/mempool/nb_stack/nb_lifo.h
> > > > >  create mode 100644
> > > > > drivers/mempool/nb_stack/rte_mempool_nb_stack.c
> > > > >  create mode 100644
> > > > > drivers/mempool/nb_stack/rte_mempool_nb_stack_version.map
> > > > >
> > > > > diff --git a/MAINTAINERS b/MAINTAINERS index
> 470f36b9c..5519d3323
> > > > > 100644
> > > > > --- a/MAINTAINERS
> > > > > +++ b/MAINTAINERS
> > > > > @@ -416,6 +416,10 @@ M: Artem V. Andreev
> > > > > <artem.andr...@oktetlabs.ru>
> > > > >  M: Andrew Rybchenko <arybche...@solarflare.com>
> > > > >  F: drivers/mempool/bucket/
> > > > >
> > > > > +Non-blocking stack memory pool
> > > > > +M: Gage Eads <gage.e...@intel.com>
> > > > > +F: drivers/mempool/nb_stack/
> > > > > +
> > > > >
> > > > >  Bus Drivers
> > > > >  -----------
> > > > > diff --git a/config/common_base b/config/common_base index
> > > > > 964a6956e..8a51f36b1 100644
> > > > > --- a/config/common_base
> > > > > +++ b/config/common_base
> > > > > @@ -726,6 +726,7 @@ CONFIG_RTE_LIBRTE_MEMPOOL_DEBUG=n
> #
> > > > > CONFIG_RTE_DRIVER_MEMPOOL_BUCKET=y
> > > > >  CONFIG_RTE_DRIVER_MEMPOOL_BUCKET_SIZE_KB=64
> > > > > +CONFIG_RTE_DRIVER_MEMPOOL_NB_STACK=y
> > > >
> > > > NAK,  as this applies to x86_64 only, it will break arm/ppc and even
> > > > 32bit i386 configurations.
> > > >
> > >
> > > Hi Gavin,
> > >
> > > This patch resolves that in the make and meson build files, which
> ensure that
> > the library is only built for x86-64 targets:

Looking down to the changes with Makefile and meson.build, it will be compiled 
out for arm/ppc/i386. That works at least.
But having this entry in the arm/ppc/i386 configurations is very strange, since 
they have no such implementations.
Why not put it into defconfig_x86_64-native-linuxapp-icc/gcc/clang to limit the 
scope?

> > >
> > > diff --git a/drivers/mempool/Makefile b/drivers/mempool/Makefile
> index
> > > 28c2e8360..895cf8a34 100644
> > > --- a/drivers/mempool/Makefile
> > > +++ b/drivers/mempool/Makefile
> > > @@ -10,6 +10,9 @@ endif
> > >  ifeq ($(CONFIG_RTE_EAL_VFIO)$(CONFIG_RTE_LIBRTE_FSLMC_BUS),yy)
> > >  DIRS-$(CONFIG_RTE_LIBRTE_DPAA2_MEMPOOL) += dpaa2  endif
> > > +ifeq ($(CONFIG_RTE_ARCH_X86_64),y)
> > > +DIRS-$(CONFIG_RTE_DRIVER_MEMPOOL_NB_STACK) += nb_stack endif
> > >
> > > diff --git a/drivers/mempool/nb_stack/meson.build
> > > b/drivers/mempool/nb_stack/meson.build
> > > new file mode 100644
> > > index 000000000..4a699511d
> > > --- /dev/null
> > > +++ b/drivers/mempool/nb_stack/meson.build
> > > @@ -0,0 +1,8 @@
> > > +# SPDX-License-Identifier: BSD-3-Clause # Copyright(c) 2019 Intel
> > > +Corporation
> > > +
> > > +if arch_subdir != 'x86' or cc.sizeof('void *') == 4
> > > +build = false
> > > +endif
> > > +
> >
> > Minor suggestion:
> > Can be simplified to "build = dpdk_conf.has('RTE_ARCH_X86_64')", I
> believe.
> >
> > /Bruce
>
> Sure, I'll switch to that check in v4.
>
> Thanks,
> Gage
IMPORTANT NOTICE: The contents of this email and any attachments are 
confidential and may also be privileged. If you are not the intended recipient, 
please notify the sender immediately and do not disclose the contents to any 
other person, use it for any purpose, or store or copy the information in any 
medium. Thank you.