Re: [dpdk-stable] [PATCH v5 2/2] ring: move the atomic load of head above the loop

[Thomas Monjalon <thomas@monjalon.net>]

05/11/2018 10:44, Olivier Matz:
> Hi,
> 
> On Sat, Nov 03, 2018 at 01:19:29AM +0000, Gavin Hu (Arm Technology China) wrote:
> > From: Bruce Richardson <bruce.richardson@intel.com>
> > > On Fri, Nov 02, 2018 at 07:21:28PM +0800, Gavin Hu wrote:
> > > > In __rte_ring_move_prod_head, move the __atomic_load_n up and out
> > > of
> > > > the do {} while loop as upon failure the old_head will be updated,
> > > > another load is costly and not necessary.
> > > >
> > > > This helps a little on the latency,about 1~5%.
> > > >
> > > >  Test result with the patch(two cores):
> > > >  SP/SC bulk enq/dequeue (size: 8): 5.64  MP/MC bulk enq/dequeue (size:
> > > > 8): 9.58  SP/SC bulk enq/dequeue (size: 32): 1.98  MP/MC bulk
> > > > enq/dequeue (size: 32): 2.30
> > > >
> > > > Fixes: 39368ebfc606 ("ring: introduce C11 memory model barrier
> > > > option")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > > > Reviewed-by: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> > > > Reviewed-by: Steve Capper <steve.capper@arm.com>
> > > > Reviewed-by: Ola Liljedahl <Ola.Liljedahl@arm.com>
> > > > Reviewed-by: Jia He <justin.he@arm.com>
> > > > Acked-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > Tested-by: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > > > ---
> > > >  doc/guides/rel_notes/release_18_11.rst |  7 +++++++
> > > >  lib/librte_ring/rte_ring_c11_mem.h     | 10 ++++------
> > > >  2 files changed, 11 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/doc/guides/rel_notes/release_18_11.rst
> > > > b/doc/guides/rel_notes/release_18_11.rst
> > > > index 376128f..b68afab 100644
> > > > --- a/doc/guides/rel_notes/release_18_11.rst
> > > > +++ b/doc/guides/rel_notes/release_18_11.rst
> > > > @@ -69,6 +69,13 @@ New Features
> > > >    checked out against that dma mask and rejected if out of range. If more
> > > than
> > > >    one device has addressing limitations, the dma mask is the more
> > > restricted one.
> > > >
> > > > +* **Updated the ring library with C11 memory model.**
> > > > +
> > > > +  Updated the ring library with C11 memory model, in our tests the
> > > > + changes  decreased latency by 27~29% and 3~15% for MPMC and SPSC
> > > cases respectively.
> > > > +  The real improvements may vary with the number of contending lcores
> > > > + and the  size of ring.
> > > > +
> > > Is this a little misleading, and will users expect massive performance
> > > improvements generally? The C11 model seems to be used only on some,
> > > but not all, arm platforms, and then only with "make" builds.
> > >
> > > config/arm/meson.build: ['RTE_USE_C11_MEM_MODEL', false]]
> > > config/common_armv8a_linuxapp:CONFIG_RTE_USE_C11_MEM_MODEL=y
> > > config/common_base:CONFIG_RTE_USE_C11_MEM_MODEL=n
> > > config/defconfig_arm64-thunderx-linuxapp-
> > > gcc:CONFIG_RTE_USE_C11_MEM_MODEL=n
> > >
> > > /Bruce
> > 
> > Thank you Bruce for the review, to limit the scope of improvement, I rewrite the note as follows, could you help review? Feel free to change anything if you like.
> > " Updated the ring library with C11 memory model, running ring_perf_autotest on Cavium ThunderX2 platform, the changes  decreased latency by 27~29% and 3~15% for MPMC and SPSC cases (2 lcores) respectively. Note the changes help the relaxed memory ordering architectures (arm, ppc) only when CONFIG_RTE_USE_C11_MEM_MODEL=y was configured, no impact on strong memory ordering architectures like x86. To what extent they help the real use cases depends on other factors, like the number of contending readers/writers, size of the ring, whether or not it is on the critical path."
> 
> I prefer your initial proposal which is more concise. What about
> something like this?
> 
> 
> * **Updated the C11 memory model version of ring library.**
> 
>   The latency is decreased for architectures using the C11 memory model
>   version of the ring library.
> 
>   On Cavium ThunderX2 platform, the changes decreased latency by 27~29%
>   and 3~15% for MPMC and SPSC cases respectively (with 2 lcores). The
>   real improvements may vary with the number of contending lcores and
>   the size of ring.
> 
> 
> About the patch itself:
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Series applied with the suggested notes.

Thanks