DPDK patches and discussions
 help / color / mirror / Atom feed
From: Jerin Jacob <jerin.jacob@caviumnetworks.com>
To: Ola Liljedahl <Ola.Liljedahl@arm.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
	Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>,
	"Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Gavin Hu (Arm Technology China)" <Gavin.Hu@arm.com>,
	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: Sun, 7 Oct 2018 09:32:46 +0530	[thread overview]
Message-ID: <20181007040243.GA1850@jerin> (raw)
In-Reply-To: <F83A969A-15F5-4D79-85B4-CB435E2C9B51@arm.com>

-----Original Message-----
> Date: Sat, 6 Oct 2018 19:44:35 +0000
> From: Ola Liljedahl <Ola.Liljedahl@arm.com>
> To: Jerin Jacob <jerin.jacob@caviumnetworks.com>, "dev@dpdk.org"
>  <dev@dpdk.org>
> CC: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, "Ananyev,
>  Konstantin" <konstantin.ananyev@intel.com>, "Gavin Hu (Arm Technology
>  China)" <Gavin.Hu@arm.com>, 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
> user-agent: Microsoft-MacOutlook/10.10.0.180812
> 
> 
> On 06/10/2018, 09:42, "Jerin Jacob" <jerin.jacob@caviumnetworks.com> wrote:
> 
>     -----Original Message-----
>     > Date: Fri, 5 Oct 2018 20:34:15 +0000
>     > From: Ola Liljedahl <Ola.Liljedahl@arm.com>
>     > To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>, Jerin Jacob
>     >  <jerin.jacob@caviumnetworks.com>
>     > CC: "Ananyev, Konstantin" <konstantin.ananyev@intel.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: [PATCH v3 1/3] ring: read tail using atomic load
>     > user-agent: Microsoft-MacOutlook/10.10.0.180812
>     >
>     > External Email
>     >
>     > On 05/10/2018, 22:29, "Honnappa Nagarahalli" <Honnappa.Nagarahalli@arm.com> wrote:
>     >
>     >     >
>     >     > I doubt it is possible to benchmark with such a precision so to see the
>     >     > potential difference of one ADD instruction.
>     >     > Just changes in function alignment can affect performance by percents. And
>     >     > the natural variation when not using a 100% deterministic system is going to
>     >     > be a lot larger than one cycle per ring buffer operation.
>     >     >
>     >     > Some of the other patches are also for correctness (e.g. load-acquire of tail)
>     >     The discussion is about this patch alone. Other patches are already Acked.
>     > So the benchmarking then makes zero sense.
> 
>     Why ?
> Because the noise in benchmarking (due to e.g. non-deterministic systems, potential changes in function and loop alignment) will be much larger than 1 cycle additional overhead per ring buffer operation. What will benchmarking tell us about the performance impact of this change that adds one ALU operation?

Yes. There will be noise in bench marking. That the reason why checked
with generated assembly code only of this patch. Found LDR vs LDR + ADD
case.How much overhead it gives, it completely based micro
architecture, like how many instruction issue it has, constraints on executing LD/ST
on specific instruction issue etc?

In any case LDR will be better than LDR + ADD in any micro
architecture.
> 
> 
>     >
>     >
>     >     > so while performance measurements may be interesting, we can't skip a bug
>     >     > fix just because it proves to decrease performance.
>     >     IMO, this patch is not a bug fix - in terms of it fixing any failures with the current code.
>     > It's a fix for correctness. Per the C++11 (and probably C11 as well due to the shared memory model), we have undefined behaviour here. If the compiler detects UB, it is allowed to do anything. Current compilers might not exploit this but future compilers could.
> 
>     All I am saying this, The code is not same and compiler(the very latest
>     gcc 8.2) is not smart enough understand it is a dead code.
> What code is dead? The ADD instruction has a purpose, it is adding an offset (from ring buffer start to the tail field) to a base pointer. It's is merely (most likely) not the optimal code sequence for any ARM processor.
> 
>  I think,
>     The moment any __builtin_gcc comes the compiler add predefined template
>     which has additional "add" instruction.
> I suspect the add instruction is because this atomic_load operates on a struct member at a non-zero offset and GCC's "template(s)" for atomic operations don't support register + immediate offset (because basically on AArch64/A64 ISA, all atomic operations except atomic_load(RELAXED) and atomic_store(RELAXED) only support the addressing mode base register without offset). I would be surprised if this minor instance of non-optimal code generation couldn't be corrected in the compiler.
> 
>     I think this specific case,
>     we ALL know that,
>     a) ht->tail will be 32 bit for life long of DPDK, it will be atomic in
>     all DPDK supported processors
>     b) The rte_pause() down which never make and compiler reordering etc.
> For 32-bit ARM and 64-bit POWER (ppc64), the rte_pause() implementation looks like this (DPDK 18.08-rc0):
> static inline void rte_pause(void)
> {
> }
> 
> How does calling this function prevent compiler optimisations e.g. of the loop or of surrounding memory accesses?
> Is rte_pause() supposed to behave like some kind of compiler barrier? I can't see anything in the DPDK documentation for rte_pause() that claims this.

How about fixing rte_pause() then?
Meaning issuing power saving instructions on missing archs.

> 
> 
>     so why to loose one cycle at worst case? It is easy loose one cycle and it very
>     difficult to get one back in fastpath.
> I suggest you read up on why undefined behaviour is bad in C. Have a chat with Andrew Pinski.
> 
> Not depending on the compiler memory barrier in rte_pause() would allow the compiler to make optimisations (e.g. schedule loads earlier) that actually increase performance. Since the atomic load of ht->tail here has relaxed MO, the compiler is allowed hoist later loads (and stores) ahead of it (and also push down and/or merge with stores after the load of ht->tail). But the compiler (memory) barrier in (that supposedly is part of) rte_pause() prevents such optimisations (well some accesses could be pushed down between the atomic load and the compiler barrier but not further than that).
> 
> A C compiler that supports C11 and beyond implements the C11 memory model. The compiler understands the memory model and can optimise memory accesses according to the semantics of the model and the ordering directives in the code. Atomic operations using ATOMIC_SEQ_CST, ATOMIC_ACQUIRE, ATOMIC_RELEASE (and ATOMIC_ACQ_REL, ignoring ATOMIC_CONSUME here) each allow and disallow certain kinds of movements and other optimisations of memory accesses (loads and stores, I assume prefetches are also included). Atomic operations with ATOMIC_RELAXED don't impose any ordering constraints so give maximum flexibility to the compiler. Using a compiler memory barrier (e.g. asm volatile ("":::"memory")) is a much more brutal way of constraining the compiler.

In arm64 case, it will have ATOMIC_RELAXED followed by asm volatile ("":::"memory") of rte_pause().
I would n't have any issue, if the generated code code is same or better than the exiting case. but it not the case, Right?


> 
> 
>     >
>     >
>     >
>     >     >
>     >     > -- Ola
>     >     >
>     >     > On 05/10/2018, 22:06, "Honnappa Nagarahalli"
>     >     > <Honnappa.Nagarahalli@arm.com> wrote:
>     >     >
>     >     >     Hi Jerin,
>     >     >           Thank you for generating the disassembly, that is really helpful. I
>     >     > agree with you that we have the option of moving parts 2 and 3 forward. I
>     >     > will let Gavin take a decision.
>     >     >
>     >     >     I suggest that we run benchmarks on this patch alone and in combination
>     >     > with other patches in the series. We have few Arm machines and we will run
>     >     > on all of them along with x86. We take a decision based on that.
>     >     >
>     >     >     Would that be a way to move forward? I think this should address both
>     >     > your and Ola's concerns.
>     >     >
>     >     >     I am open for other suggestions as well.
>     >     >
>     >     >     Thank you,
>     >     >     Honnappa
>     >     >
>     >     >     >
>     >     >     > So you don't want to write the proper C11 code because the compiler
>     >     >     > generates one extra instruction that way?
>     >     >     > You don't even know if that one extra instruction has any measurable
>     >     >     > impact on performance. E.g. it could be issued the cycle before together
>     >     >     > with other instructions.
>     >     >     >
>     >     >     > We can complain to the compiler writers that the code generation for
>     >     >     > __atomic_load_n(, __ATOMIC_RELAXED) is not optimal (at least on
>     >     >     > ARM/A64). I think the problem is that the __atomic builtins only accept
>     >     > a
>     >     >     > base address without any offset and this is possibly because e.g.
>     >     > load/store
>     >     >     > exclusive (LDX/STX) and load-acquire (LDAR) and store-release (STLR)
>     >     > only
>     >     >     > accept a base register with no offset. So any offset has to be added
>     >     > before
>     >     >     > the actual "atomic" instruction, LDR in this case.
>     >     >     >
>     >     >     >
>     >     >     > -- Ola
>     >     >     >
>     >     >     >
>     >     >     > On 05/10/2018, 19:07, "Jerin Jacob" <jerin.jacob@caviumnetworks.com>
>     >     >     > wrote:
>     >     >     >
>     >     >     >     -----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
>     >     >     >     > > >         > > >
>     >     >     >     > > >
>     >     >     >     > > >
>     >     >     >     > > >
>     >     >     >     >
>     >     >     >
>     >     >
>     >     >
>     >
>     >
>     >
> 
> 

  parent reply	other threads:[~2018-10-07  4:03 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
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 [this message]
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=20181007040243.GA1850@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).