On 08/10/2018, 12:47, "Jerin Jacob" <jerin.ja...@caviumnetworks.com> wrote:
-----Original Message-----
> Date: Mon, 8 Oct 2018 10:25:45 +0000
> From: Ola Liljedahl <ola.liljed...@arm.com>
> To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> CC: "dev@dpdk.org" <dev@dpdk.org>, Honnappa Nagarahalli
> <honnappa.nagaraha...@arm.com>, "Ananyev, Konstantin"
> <konstantin.anan...@intel.com>, "Gavin Hu (Arm Technology China)"
> <gavin...@arm.com>, Steve Capper <steve.cap...@arm.com>, nd
<n...@arm.com>,
> "sta...@dpdk.org" <sta...@dpdk.org>
> Subject: Re: [PATCH v3 1/3] ring: read tail using atomic load
> user-agent: Microsoft-MacOutlook/10.11.0.180909
>
>
> On 08/10/2018, 12:00, "Jerin Jacob" <jerin.ja...@caviumnetworks.com>
wrote:
>
> -----Original Message-----
> > Date: Mon, 8 Oct 2018 09:22:05 +0000
> > From: Ola Liljedahl <ola.liljed...@arm.com>
> > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > CC: "dev@dpdk.org" <dev@dpdk.org>, Honnappa Nagarahalli
> > <honnappa.nagaraha...@arm.com>, "Ananyev, Konstantin"
> > <konstantin.anan...@intel.com>, "Gavin Hu (Arm Technology China)"
> > <gavin...@arm.com>, Steve Capper <steve.cap...@arm.com>, nd
<n...@arm.com>,
> > "sta...@dpdk.org" <sta...@dpdk.org>
> > Subject: Re: [PATCH v3 1/3] ring: read tail using atomic load
> > user-agent: Microsoft-MacOutlook/10.11.0.180909
> >
> > External Email
> >
> > On 08/10/2018, 08:06, "Jerin Jacob"
<jerin.ja...@caviumnetworks.com> wrote:
> >
> > -----Original Message-----
> > > Date: Sun, 7 Oct 2018 20:44:54 +0000
> > > From: Ola Liljedahl <ola.liljed...@arm.com>
> > > To: Jerin Jacob <jerin.ja...@caviumnetworks.com>
> > > CC: "dev@dpdk.org" <dev@dpdk.org>, Honnappa Nagarahalli
> > > <honnappa.nagaraha...@arm.com>, "Ananyev, Konstantin"
> > > <konstantin.anan...@intel.com>, "Gavin Hu (Arm Technology
China)"
> > > <gavin...@arm.com>, Steve Capper <steve.cap...@arm.com>, nd
<n...@arm.com>,
> > > "sta...@dpdk.org" <sta...@dpdk.org>
> > > Subject: Re: [PATCH v3 1/3] ring: read tail using atomic load
> > > user-agent: Microsoft-MacOutlook/10.11.0.180909
> > >
> >
> >
> > Could you please fix the email client for inline reply.
> > Sorry that doesn't seem to be possible with Outlook for Mac 16 or
Office365. The official Office365/Outlook
> > documentation doesn't match the actual user interface...
> >
> >
> >
> >
https://www.kernel.org/doc/html/v4.19-rc7/process/email-clients.html
> >
> >
> > >
> > > On 07/10/2018, 06:03, "Jerin Jacob"
<jerin.ja...@caviumnetworks.com> wrote:
> > >
> > > 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?
> > > The existing case is actually not interesting (IMO) as it
exposes undefined behaviour which allows the compiler to do anything. But you
seem to be satisfied with "works for me, right here right now". I think the
cost of avoiding undefined behaviour is acceptable (actually I don't think it
even will be noticeable).
> >
> > I am not convinced because of use of volatile in head and tail
indexes.
> > For me that brings the defined behavior.
> > As long as you don't mix in C11 atomic accesses (just use "plain"
accesses to volatile objects),
> > it is AFAIK defined behaviour (but not necessarily using atomic
loads and stores). But I quoted
> > the C11 spec where it explicitly mentions that mixing atomic and
non-atomic accesses to the same
> > object is undefined behaviour. Don't argue with me, argue with the
C11 spec.
> > If you want to disobey the spec, this should at least be called out
for in the code with a comment.
>
> That's boils down only one question, should we follow C11 spec? Why
not only take load
> acquire and store release semantics only just like Linux kernel and
FreeBSD.
> And introduce even more undefined behaviour?
Yes. The all world(Linux and Freebsd) is running with undefined behavior
and still it runs.
Are you saying that Linux kernel is using GCC __atomic or C11
_Atomic/stdatomic.h features?
I can't see any traces of this in e.g. Linux 4.6.
It seems like you don't understand. The undefined behaviour comes from mixing
non-atomic and atomic accesses
in the C11 source code. It doesn't have anything to do about if a read or write
of a volatile object is translated
to an instruction that may or may not provide an atomic load or store for some
specific architecture that
you are compiling for.
Since the Linux kernel (AFAIK) doesn't use C11 _Atomic datatypes or GCC
__atomic builtins, there is no
undefined behaviour per the C11 standard. But it is relying on implementation
specific behaviour (which
I grant is not so likely to change due to backwards compatibility requirements).
>
> Does not look like C11 memory model is super efficient in term of gcc
> implementation.
> You are making a chicken out of a feather.
>
> I think this "problem" with one additional ADD instruction will only
concern __atomic_load_n(__ATOMIC_RELAXED) and
__atomic_store_n(__ATOMIC_RELAXED) because the compiler separates the address
generation (add offset of struct member) from the load or store itself. For
other atomic operations and memory orderings (e.g.
__atomic_load_n(__ATOMIC_ACQUIRE), the extra ADD instruction will be included
anyway (as long as we access a non-first struct member) because e.g. LDAR only
accepts a base register with no offset.
>
> I suggest minimising the imposed memory orderings can have a much larger
(positive) effect on performance compared to avoiding one ADD instruction
(memory accesses are much slower than CPU ALU instructions).
> Using C11 memory model and identifying exactly which objects are used for
synchronisation and whether (any) updates to shared memory are acquired or
released (no updates to shared memory means relaxed order can be used) will
provide maximum freedom to the compiler and hardware to get the best result.
No more comments on this. It is not data driven.
Now this is something that would be interesting to benchmark. I don't know to
what
extent current compilers actually utilise the freedoms to optimise memory
accesses
according to e.g. acquire and release ordering of surrounding atomic operations.
But I would like to know and this could possibly also lead to suggestions to
compiler
developers. I don't think there are too many multithreaded benchmarks and even
fewer
which don't use locks and other synchronisation mechanisms from OS's and
libraries.
But I have some multithreaded applications and benchmarks.
>
> The FreeBSD and DPDK ring buffers show some fundamental misunderstandings
here. Instead excessive orderings and explicit barriers have been used as
band-aids, with unknown effects on performance.
>
>
> >
> >
> > That the reason why I shared
> > the generated assembly code. If you think other way, Pick any
compiler
> > and see generated output.
> > This is what one compiler for one architecture generates today.
These things change. Other things
> > that used to work or worked for some specific architecture has
stopped working in newer versions of
> > the compiler.
> >
> >
> > And
> >
> > Freebsd implementation of ring buffer(Which DPDK derived from),
Don't have
> > such logic, See
https://github.com/freebsd/freebsd/blob/master/sys/sys/buf_ring.h#L108
> > It looks like FreeBSD uses some kind of C11 atomic memory
model-inspired API although I don't see
> > exactly how e.g. atomic_store_rel_int() is implemented. The code
also mixes in explicit barriers
> > so definitively not pure C11 memory model usage. And finally, it
doesn't establish the proper
> > load-acquire/store-release relationships (e.g. store-release
cons_tail requires a load-acquire cons_tail,
> > same for prod_tail).
> >
> > "* multi-producer safe lock-free ring buffer enqueue"
> > The comment is also wrong. This design is not lock-free, how could
it be when there is spinning
> > (waiting) for other threads in the code? If a thread must wait for
other threads, then by definition
> > the design is blocking.
> >
> > So you are saying that because FreeBSD is doing it wrong, DPDK can
also do it wrong?
> >
> >
> > See below too.
> >
> > >
> > > Skipping the compiler memory barrier in rte_pause()
potentially allows for optimisations that provide much more benefit, e.g.
hiding some cache miss latency for later loads. The DPDK ring buffer
implementation is defined so to enable inlining of enqueue/dequeue functions
into the caller, any code could immediately follow these calls.
> > >
> > > From INTERNATIONAL STANDARD ©ISO/IEC ISO/IEC 9899:201x
> > > Programming languages — C
> > >
> > > 5.1.2.4
> > > 4 Two expression evaluations conflict if one of them modifies
a memory location and the other one reads or modifies the same memory location.
> > >
> > > 25 The execution of a program contains a data race if it
contains two conflicting actions in different threads, at least one of which is
not atomic, and neither happens before the other. Any such data race results in
undefined behavior.
> >
> > IMO, Both condition will satisfy if the variable is volatile
and 32bit read will atomic
> > for 32b and 64b machines. If not, the problem persist for
generic case
> > as well(lib/librte_ring/rte_ring_generic.h)
> > The read from a volatile object is not an atomic access per the C11
spec. It just happens to
> > be translated to an instruction (on x86-64 and AArch64/A64) that
implements an atomic load.
> > I don't think any compiler would change this code generation and
suddenly generate some
> > non-atomic load instruction for a program that *only* uses volatile
to do "atomic" accesses.
> > But a future compiler could detect the mix of atomic and non-atomic
accesses and mark this
> > expression as causing undefined behaviour and that would have
consequences for code generation.
> >
> >
> > I agree with you on C11 memory model semantics usage. The
reason why I
> > propose name for the file as rte_ring_c11_mem.h as DPDK it self
did not
> > had definitions for load acquire and store release semantics.
> > I was looking for taking load acquire and store release
semantics
> > from C11 instead of creating new API like Linux kernel for
FreeBSD(APIs
> > like atomic_load_acq_32(), atomic_store_rel_32()). If the file
name is your
> > concern then we could create new abstractions as well. That
would help
> > exiting KNI problem as well.
> > I appreciate your embrace of the C11 memory model. I think it is
better for describing
> > (both to the compiler and to humans) which and how objects are used
for synchronisation.
> >
> > However, I don't think an API as you suggest (and others have
suggested before, e.g. as
> > done in ODP) is a good idea. There is an infinite amount of
possible base types, an
> > increasing number of operations and a bunch of different memory
orderings, a "complete"
> > API would be very large and difficult to test, and most members of
the API would never be used.
> > GCC and Clang both support the __atomic intrinsics. This API avoids
the problems I
> > described above. Or we could use the official C11 syntax
(stdatomic.h). But then we
> > have the problem with using pre-C11 compilers...
>
> I have no objection, if everyone agrees to move C11 memory model
> with __atomic intrinsics. But if we need to keep both have then
> atomic_load_acq_32() kind of API make sense.
>
>
> >
> >
> >
> >
> > I think, currently it mixed usage because, the same variable
declaration
> > used for C11 vs non C11 usage.Ideally we wont need "volatile"
for C11
> > case. Either we need to change only to C11 mode OR have APIs for
> > atomic_load_acq_() and atomic_store_rel_() to allow both models
like
> > Linux kernel and FreeBSD.
> >
> > >
> > > -- Ola
> > >
> > >
> > >
> >
> >
>
>
