DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Ola Liljedahl <Ola.Liljedahl@arm.com>,
	"Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>,
	"dev@dpdk.org" <dev@dpdk.org>,
	Steve Capper <Steve.Capper@arm.com>, nd <nd@arm.com>,
	"stable@dpdk.org" <stable@dpdk.org>
Subject: Re: [dpdk-dev] [PATCH v3 1/3] ring: read tail using atomic load
Date: Fri, 5 Oct 2018 22:37:27 +0530	[thread overview]
Message-ID: <20181005170725.GA18671@jerin> (raw)
In-Reply-To: <AM6PR08MB3672F618AABF380FDCF87B3A98EB0@AM6PR08MB3672.eurprd08.prod.outlook.com>

-----Original Message-----
> Date: Fri, 5 Oct 2018 15:11:44 +0000
> From: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>
> To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>, Ola Liljedahl
>  <Ola.Liljedahl@arm.com>, "Gavin Hu (Arm Technology China)"
>  <Gavin.Hu@arm.com>, Jerin Jacob <jerin.jacob@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, Steve Capper <Steve.Capper@arm.com>, nd
>  <nd@arm.com>, "stable@dpdk.org" <stable@dpdk.org>
> Subject: RE: [PATCH v3 1/3] ring: read tail using atomic load
> 
> > >         > Hi Jerin,
> > >         >
> > >         > Thanks for your review, inline comments from our internal
> > discussions.
> > >         >
> > >         > BR. Gavin
> > >         >
> > >         > > -----Original Message-----
> > >         > > From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
> > >         > > Sent: Saturday, September 29, 2018 6:49 PM
> > >         > > To: Gavin Hu (Arm Technology China) <Gavin.Hu@arm.com>
> > >         > > Cc: dev@dpdk.org; Honnappa Nagarahalli
> > >         > > <Honnappa.Nagarahalli@arm.com>; Steve Capper
> > >         > > <Steve.Capper@arm.com>; Ola Liljedahl <Ola.Liljedahl@arm.com>;
> > nd
> > >         > > <nd@arm.com>; stable@dpdk.org
> > >         > > Subject: Re: [PATCH v3 1/3] ring: read tail using atomic load
> > >         > >
> > >         > > -----Original Message-----
> > >         > > > Date: Mon, 17 Sep 2018 16:17:22 +0800
> > >         > > > From: Gavin Hu <gavin.hu@arm.com>
> > >         > > > To: dev@dpdk.org
> > >         > > > CC: gavin.hu@arm.com, Honnappa.Nagarahalli@arm.com,
> > >         > > > steve.capper@arm.com,  Ola.Liljedahl@arm.com,
> > >         > > > jerin.jacob@caviumnetworks.com, nd@arm.com,
> > stable@dpdk.org
> > >         > > > Subject: [PATCH v3 1/3] ring: read tail using atomic load
> > >         > > > X-Mailer: git-send-email 2.7.4
> > >         > > >
> > >         > > > External Email
> > >         > > >
> > >         > > > In update_tail, read ht->tail using __atomic_load.Although the
> > >         > > > compiler currently seems to be doing the right thing even without
> > >         > > > _atomic_load, we don't want to give the compiler freedom to
> > optimise
> > >         > > > what should be an atomic load, it should not be arbitarily moved
> > >         > > > around.
> > >         > > >
> > >         > > > Fixes: 39368ebfc6 ("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>
> > >         > > > ---
> > >         > > >  lib/librte_ring/rte_ring_c11_mem.h | 3 ++-
> > >         > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > >         > > >
> > >         > The read of ht->tail needs to be atomic, a non-atomic read would not
> > be correct.
> > >
> > >         That's a 32bit value load.
> > >         AFAIK on all CPUs that we support it is an atomic operation.
> > >     [Ola] But that the ordinary C load is translated to an atomic load for the
> > target architecture is incidental.
> > >
> > >     If the design requires an atomic load (which is the case here), we
> > > should use an atomic load on the language level. Then we can be sure it will
> > always be translated to an atomic load for the target in question or
> > compilation will fail. We don't have to depend on assumptions.
> >
> > We all know that 32bit load/store on cpu we support - are atomic.
> > If it wouldn't be the case - DPDK would be broken in dozen places.
> > So what the point to pretend that "it might be not atomic" if we do know for
> > sure that it is?
> > I do understand that you want to use atomic_load(relaxed) here for
> > consistency, and to conform with C11 mem-model and I don't see any harm in
> > that.
> We can continue to discuss the topic, it is a good discussion. But, as far this patch is concerned, can I consider this as us having a consensus? The file rte_ring_c11_mem.h is specifically for C11 memory model and I also do not see any harm in having code that completely conforms to C11 memory model.

Have you guys checked the output assembly with and without atomic load?
There is an extra "add" instruction with at least the code I have checked.
I think, compiler is not smart enough to understand it is a dead code for
arm64.

➜ [~] $ aarch64-linux-gnu-gcc -v
Using built-in specs.
COLLECT_GCC=aarch64-linux-gnu-gcc
COLLECT_LTO_WRAPPER=/usr/lib/gcc/aarch64-linux-gnu/8.2.0/lto-wrapper
Target: aarch64-linux-gnu
Configured with: /build/aarch64-linux-gnu-gcc/src/gcc-8.2.0/configure
--prefix=/usr --program-prefix=aarch64-linux-gnu-
--with-local-prefix=/usr/aarch64-linux-gnu
--with-sysroot=/usr/aarch64-linux-gnu
--with-build-sysroot=/usr/aarch64-linux-gnu --libdir=/usr/lib
--libexecdir=/usr/lib --target=aarch64-linux-gnu
--host=x86_64-pc-linux-gnu --build=x86_64-pc-linux-gnu --disable-nls
--enable-languages=c,c++ --enable-shared --enable-threads=posix
--with-system-zlib --with-isl --enable-__cxa_atexit
--disable-libunwind-exceptions --enable-clocale=gnu
--disable-libstdcxx-pch --disable-libssp --enable-gnu-unique-object
--enable-linker-build-id --enable-lto --enable-plugin
--enable-install-libiberty --with-linker-hash-style=gnu
--enable-gnu-indirect-function --disable-multilib --disable-werror
--enable-checking=release
Thread model: posix
gcc version 8.2.0 (GCC)


# build setup
make -j 8 config T=arm64-armv8a-linuxapp-gcc  CROSS=aarch64-linux-gnu-
make -j 8 test-build CROSS=aarch64-linux-gnu-

# generate asm
aarch64-linux-gnu-gdb -batch -ex 'file build/app/test ' -ex 'disassemble /rs bucket_enqueue_single' 

I have uploaded generated file for your convenience
with_atomic_load.txt(includes patch 1,2,3)
-----------------------
https://pastebin.com/SQ6w1yRu

without_atomic_load.txt(includes patch 2,3)
-----------------------
https://pastebin.com/BpvnD0CA


without_atomic
-------------
23              if (!single)
   0x000000000068d290 <+240>:   85 00 00 35     cbnz    w5, 0x68d2a0 <bucket_enqueue_single+256>
   0x000000000068d294 <+244>:   82 04 40 b9     ldr     w2, [x4, #4]
   0x000000000068d298 <+248>:   5f 00 01 6b     cmp     w2, w1
   0x000000000068d29c <+252>:   21 01 00 54     b.ne    0x68d2c0 <bucket_enqueue_single+288>  // b.any

24                      while (unlikely(ht->tail != old_val))
25                              rte_pause();


with_atomic
-----------
23              if (!single)
   0x000000000068ceb0 <+240>:   00 10 04 91     add     x0, x0, #0x104
   0x000000000068ceb4 <+244>:   84 00 00 35     cbnz    w4, 0x68cec4 <bucket_enqueue_single+260>
   0x000000000068ceb8 <+248>:   02 00 40 b9     ldr     w2, [x0]
   0x000000000068cebc <+252>:   3f 00 02 6b     cmp     w1, w2
   0x000000000068cec0 <+256>:   01 09 00 54     b.ne    0x68cfe0 <bucket_enqueue_single+544>  // b.any

24                      while (unlikely(old_val != __atomic_load_n(&ht->tail, __ATOMIC_RELAXED)))


I don't want to block this series of patches due this patch. Can we make
re spin one series with 2 and 3 patches. And Wait for patch 1 to conclude?

Thoughts?




> 
> > But argument that we shouldn't assume 32bit load/store ops as atomic
> > sounds a bit flaky to me.
> > Konstantin
> >
> >
> > >
> > >
> > >
> > >         > But there are no memory ordering requirements (with
> > >         > regards to other loads and/or stores by this thread) so relaxed
> > memory order is sufficient.
> > >         > Another aspect of using __atomic_load_n() is that the
> > > compiler cannot "optimise" this load (e.g. combine, hoist etc), it has to be
> > done as
> > >         > specified in the source code which is also what we need here.
> > >
> > >         I think Jerin points that rte_pause() acts here as compiler barrier too,
> > >         so no need to worry that compiler would optimize out the loop.
> > >     [Ola] Sorry missed that. But the barrier behaviour of rte_pause()
> > > is not part of C11, is it essentially a hand-made feature to support
> > > the legacy multithreaded memory model (which uses explicit HW and
> > compiler barriers). I'd prefer code using the C11 memory model not to
> > depend on such legacy features.
> > >
> > >
> > >
> > >         Konstantin
> > >
> > >         >
> > >         > One point worth mentioning though is that this change is for
> > > the rte_ring_c11_mem.h file, not the legacy ring. It may be worth persisting
> > >         > with getting the C11 code right when people are less excited about
> > sending a release out?
> > >         >
> > >         > We can explain that for C11 we would prefer to do loads and stores
> > as per the C11 memory model. In the case of rte_ring, the code is
> > >         > separated cleanly into C11 specific files anyway.
> > >         >
> > >         > I think reading ht->tail using __atomic_load_n() is the most
> > appropriate way. We show that ht->tail is used for synchronization, we
> > >         > acknowledge that ht->tail may be written by other threads
> > > without any other kind of synchronization (e.g. no lock involved) and we
> > require
> > >         > an atomic load (any write to ht->tail must also be atomic).
> > >         >
> > >         > Using volatile and explicit compiler (or processor) memory barriers
> > (fences) is the legacy pre-C11 way of accomplishing these things.
> > > There's
> > >         > a reason why C11/C++11 moved away from the old ways.
> > >         > > >
> > >         > > >         __atomic_store_n(&ht->tail, new_val, __ATOMIC_RELEASE);
> > >         > > > --
> > >         > > > 2.7.4
> > >         > > >
> > >
> > >
> > >
> 

  reply	other threads:[~2018-10-05 17:07 UTC|newest]

Thread overview: 131+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-08-06  1:18 [dpdk-dev] [PATCH] ring: fix c11 memory ordering issue Gavin Hu
2018-08-06  9:19 ` Thomas Monjalon
2018-08-08  1:39   ` Gavin Hu
2018-08-07  3:19 ` [dpdk-dev] [PATCH v2] " Gavin Hu
2018-08-07  5:56   ` He, Jia
2018-08-07  7:56     ` Gavin Hu
2018-08-08  3:07       ` Jerin Jacob
2018-08-08  7:23         ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-09-17  7:47   ` [dpdk-dev] [PATCH v3 1/3] app/testpmd: show errno along with flow API errors Gavin Hu
2018-09-17  7:47     ` [dpdk-dev] [PATCH v3 2/3] net/i40e: remove invalid comment Gavin Hu
2018-09-17  8:25       ` Gavin Hu (Arm Technology China)
2018-09-17  7:47     ` [dpdk-dev] [PATCH v3 3/3] doc: add cross compile part for sample applications Gavin Hu
2018-09-17  9:48       ` Jerin Jacob
2018-09-17 10:28         ` Gavin Hu (Arm Technology China)
2018-09-17 10:34           ` Jerin Jacob
2018-09-17 10:55             ` Gavin Hu (Arm Technology China)
2018-09-17 10:49       ` [dpdk-dev] [PATCH v4] " Gavin Hu
2018-09-17 10:53         ` [dpdk-dev] [PATCH v5] " Gavin Hu
2018-09-18 11:00           ` Jerin Jacob
2018-09-19  0:33           ` [dpdk-dev] [PATCH v6] " Gavin Hu
2018-09-17  8:11     ` [dpdk-dev] [PATCH v4 1/4] bus/fslmc: fix undefined reference of memsegs Gavin Hu
2018-09-17  8:11       ` [dpdk-dev] [PATCH v4 2/4] ring: read tail using atomic load Gavin Hu
2018-09-20  6:41         ` Jerin Jacob
2018-09-25  9:26           ` Gavin Hu (Arm Technology China)
2018-09-17  8:11       ` [dpdk-dev] [PATCH v4 3/4] ring: synchronize the load and store of the tail Gavin Hu
2018-09-17  8:11       ` [dpdk-dev] [PATCH v4 4/4] ring: move the atomic load of head above the loop Gavin Hu
2018-10-27 14:21         ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-09-17  8:17   ` [dpdk-dev] [PATCH v3 1/3] ring: read tail using atomic load Gavin Hu
2018-09-17  8:17     ` [dpdk-dev] [PATCH v3 2/3] ring: synchronize the load and store of the tail Gavin Hu
2018-09-26  9:29       ` Gavin Hu (Arm Technology China)
2018-09-26  9:59         ` Justin He
2018-09-29 10:57       ` Jerin Jacob
2018-10-17  6:29       ` [dpdk-dev] [PATCH 1/2] " Gavin Hu
2018-10-17  6:29         ` [dpdk-dev] [PATCH 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-17  6:35         ` [dpdk-dev] [PATCH 1/2] ring: synchronize the load and store of the tail Gavin Hu (Arm Technology China)
2018-10-27 14:39           ` Thomas Monjalon
2018-10-27 15:00             ` Jerin Jacob
2018-10-27 15:13               ` Thomas Monjalon
2018-10-27 15:34                 ` Jerin Jacob
2018-10-27 15:48                   ` Thomas Monjalon
2018-10-29  2:51                   ` Gavin Hu (Arm Technology China)
2018-10-29  2:57                   ` Gavin Hu (Arm Technology China)
2018-10-29 10:16                     ` Jerin Jacob
2018-10-29 10:47                       ` Thomas Monjalon
2018-10-29 11:10                         ` Jerin Jacob
2018-11-03 20:12                 ` Mattias Rönnblom
2018-11-05 21:51                   ` Honnappa Nagarahalli
2018-11-06 11:03                     ` Mattias Rönnblom
2018-10-31  3:35         ` [dpdk-dev] [PATCH v2 0/2] rte ring c11 bug fix and optimization Gavin Hu
2018-10-31 10:26           ` [dpdk-dev] [PATCH v3 0/2] ring library with c11 memory model " Gavin Hu
2018-10-31 16:58             ` Thomas Monjalon
2018-11-01  9:53             ` [dpdk-dev] [PATCH v4 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-11-01  9:53             ` [dpdk-dev] [PATCH v4 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-11-01 17:26               ` Stephen Hemminger
2018-11-02  0:53                 ` Gavin Hu (Arm Technology China)
2018-11-02  4:30                   ` Honnappa Nagarahalli
2018-11-02  7:15                     ` Gavin Hu (Arm Technology China)
2018-11-02  9:36                       ` Thomas Monjalon
2018-11-02 11:23                         ` Gavin Hu (Arm Technology China)
2018-10-31 10:26           ` [dpdk-dev] [PATCH v3 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-10-31 22:07             ` Stephen Hemminger
2018-11-01  9:56               ` Gavin Hu (Arm Technology China)
2018-10-31 10:26           ` [dpdk-dev] [PATCH v3 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-31  3:35         ` [dpdk-dev] [PATCH v2 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-10-31  3:35         ` [dpdk-dev] [PATCH v2 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-10-31  9:36           ` Thomas Monjalon
2018-10-31 10:27             ` Gavin Hu (Arm Technology China)
2018-11-01  9:53         ` [dpdk-dev] [PATCH v4 0/2] ring library with c11 memory model bug fix and optimization Gavin Hu
2018-11-02 11:21           ` [dpdk-dev] [PATCH v5 " Gavin Hu
2018-11-02 11:21           ` [dpdk-dev] [PATCH v5 1/2] ring: synchronize the load and store of the tail Gavin Hu
2018-11-05  9:30             ` Olivier Matz
2018-11-02 11:21           ` [dpdk-dev] [PATCH v5 2/2] ring: move the atomic load of head above the loop Gavin Hu
2018-11-02 11:43             ` Bruce Richardson
2018-11-03  1:19               ` Gavin Hu (Arm Technology China)
2018-11-03  9:34                 ` Honnappa Nagarahalli
2018-11-05 13:17                   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-11-05 13:41                     ` Jerin Jacob
2018-11-05  9:44                 ` [dpdk-dev] " Olivier Matz
2018-11-05 13:36                   ` [dpdk-dev] [dpdk-stable] " Thomas Monjalon
2018-09-17  8:17     ` [dpdk-dev] [PATCH v3 3/3] " Gavin Hu
2018-09-26  9:29       ` Gavin Hu (Arm Technology China)
2018-09-26 10:06         ` Justin He
2018-09-29  7:19           ` Stephen Hemminger
2018-09-29 10:59       ` Jerin Jacob
2018-09-26  9:29     ` [dpdk-dev] [PATCH v3 1/3] ring: read tail using atomic load Gavin Hu (Arm Technology China)
2018-09-26 10:09       ` Justin He
2018-09-29 10:48     ` Jerin Jacob
2018-10-05  0:47       ` Gavin Hu (Arm Technology China)
2018-10-05  8:21         ` Ananyev, Konstantin
2018-10-05 11:15           ` Ola Liljedahl
2018-10-05 11:36             ` Ola Liljedahl
2018-10-05 13:44               ` Ananyev, Konstantin
2018-10-05 14:21                 ` Ola Liljedahl
2018-10-05 15:11                 ` Honnappa Nagarahalli
2018-10-05 17:07                   ` Jerin Jacob [this message]
2018-10-05 18:05                     ` Ola Liljedahl
2018-10-05 20:06                       ` Honnappa Nagarahalli
2018-10-05 20:17                         ` Ola Liljedahl
2018-10-05 20:29                           ` Honnappa Nagarahalli
2018-10-05 20:34                             ` Ola Liljedahl
2018-10-06  7:41                               ` Jerin Jacob
2018-10-06 19:44                                 ` Ola Liljedahl
2018-10-06 19:59                                   ` Ola Liljedahl
2018-10-07  4:02                                   ` Jerin Jacob
2018-10-07 20:11                                     ` Ola Liljedahl
2018-10-07 20:44                                     ` Ola Liljedahl
2018-10-08  6:06                                       ` Jerin Jacob
2018-10-08  9:22                                         ` Ola Liljedahl
2018-10-08 10:00                                           ` Jerin Jacob
2018-10-08 10:25                                             ` Ola Liljedahl
2018-10-08 10:33                                               ` Gavin Hu (Arm Technology China)
2018-10-08 10:39                                                 ` Ola Liljedahl
2018-10-08 10:41                                                   ` Gavin Hu (Arm Technology China)
2018-10-08 10:49                                                 ` Jerin Jacob
2018-10-10  6:28                                                   ` Gavin Hu (Arm Technology China)
2018-10-10 19:26                                                     ` Honnappa Nagarahalli
2018-10-08 10:46                                               ` Jerin Jacob
2018-10-08 11:21                                                 ` Ola Liljedahl
2018-10-08 11:50                                                   ` Jerin Jacob
2018-10-08 11:59                                                     ` Ola Liljedahl
2018-10-08 12:05                                                       ` Jerin Jacob
2018-10-08 12:20                                                         ` Jerin Jacob
2018-10-08 12:30                                                           ` Ola Liljedahl
2018-10-09  8:53                                                             ` Olivier Matz
2018-10-09  3:16                                             ` Honnappa Nagarahalli
2018-10-08 14:43                                           ` Bruce Richardson
2018-10-08 14:46                                             ` Ola Liljedahl
2018-10-08 15:45                                               ` Ola Liljedahl
2018-10-08  5:27                               ` Honnappa Nagarahalli
2018-10-08 10:01                                 ` Ola Liljedahl
2018-10-27 14:17     ` [dpdk-dev] [dpdk-stable] " 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=20181005170725.GA18671@jerin \
    --to=jerin.jacob@caviumnetworks.com \
    --cc=Gavin.Hu@arm.com \
    --cc=Honnappa.Nagarahalli@arm.com \
    --cc=Ola.Liljedahl@arm.com \
    --cc=Steve.Capper@arm.com \
    --cc=dev@dpdk.org \
    --cc=konstantin.ananyev@intel.com \
    --cc=nd@arm.com \
    --cc=stable@dpdk.org \
    /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).