Re: [dpdk-dev] [PATCH v3 1/3] ring: read tail using atomic load

[Olivier Matz <olivier.matz@6wind.com>]

Hi,

On Mon, Oct 08, 2018 at 12:30:15PM +0000, Ola Liljedahl wrote:
> 
> 
> On 08/10/2018, 14:21, "Jerin Jacob" <jerin.jacob@caviumnetworks.com> wrote:
> 
>     -----Original Message-----
>     > Date: Mon, 8 Oct 2018 17:35:25 +0530
>     > 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
>     > User-Agent: Mutt/1.10.1 (2018-07-13)
>     > 
>     > External Email
>     > 
>     > -----Original Message-----
>     > > Date: Mon, 8 Oct 2018 11:59:16 +0000
>     > > From: Ola Liljedahl <Ola.Liljedahl@arm.com>
>     > > To: Jerin Jacob <jerin.jacob@caviumnetworks.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: [PATCH v3 1/3] ring: read tail using atomic load
>     > > user-agent: Microsoft-MacOutlook/10.11.0.180909
>     > >
>     > >
>     > > On 08/10/2018, 13:50, "Jerin Jacob" <jerin.jacob@caviumnetworks.com> wrote:
>     > >
>     > >
>     > >     I don't know how that creates more undefined behavior. So replied in the
>     > >     context of your reply that, according to your view even Linux is running
>     > >     with undefined behavior.
>     > >
>     > > As I explained, Linux does not use C11 atomics (nor GCC __atomic builtins) so
>     > > cannot express the kind of undefined behaviour caused by mixing conflicting atomic
>     > > (as defined by the C11 standard) and non-atomic accesses to the same object.
>     > >
>     > > Checked the latest version from https://github.com/torvalds/linux
>     > 
>     > Yet another top post. So you removed the complete earlier context. Never
>     > mind.
> Top post? My reply is under your text. As is this.
> 
> Don't blame my stupid mail agent on your misunderstanding of C11.

Sorry, but honnestly, your mail agent configuration makes the thread harder
to read for many people.


>     > I am not saying Linux is using C11 atomic. I asked, Can't we follow
>     > like Linux to use the HW feature of load acquire and store release
>     > semantics with introducing C11 memory model.
>     
>     correction:
>     
>     s/with introducing C11 memory model/with out introducing C11 memory model
> You can generate e.g. AArch64/A64 LDAR and STLR instructions using inline assembler.
> But you won't be able to specify acquire and release ordering to the compiler, so you
> must specify a full memory barrier instead.
>
> But why create a C11-like but custom DPDK specific memory model when the compiler
> already supports a standardised, well defined and tested memory model? You would just be
> creating a mountain of technical debt.

Let's try to sumarize my understanding of the discussion:

- mixing standard access and C11-like access is undefined according to the
  C11 specification. However it works today with current compilers and hw.
- the patch does not fix any identified issue, but it can make the code
  more consistent by using C11 access.
- with the patch, one "add" instruction is added. The impact is difficult
  to measure, but it is expected to be smaller than the noise generated
  by a code alignment change.

I'm unfortunately not an expert about C11 atomic memory model. But, if
it is more consistent with the rest of the code and if we cannot detect
any performance impact, well, I think it could go in.


Regards,
Olivier