Re: [PATCH] lightnvm: pblk: Add read memory barrier when reading from rb

[Javier Gonzalez <javier@cnexlabs.com>]

[-- Attachment #1: Type: text/plain, Size: 1818 bytes --]

> On 28 Jun 2018, at 09.59, Matias Bjørling <mb@lightnvm.io> wrote:
> 
> On 06/28/2018 01:31 AM, Heiner Litz wrote:
>> There is a control dependency between two disjoint variables (only read data if flags == WRITTEN). Because x86-TSO allows re-ordering of loads the control dependency can be violated.
> 
> I'm sorry, I do not see it :)
> 
> Here is my understanding:
> 
> entry->w_ctx.flags is used as a flagging mechanism to wait for data to become available when "flags" has PBLK_WRITTEN_DATA.
> 
> The later access to entry->data is independent of this. It is assumed that entry->w_ctx.flags is the guarding barrier.

This is correct. The motivation is to allow several producers to fill the
buffer simultaneously.

> 
> Here is the titbit from the control dependency section that makes me
> say that the entry->data loads is not possible to be done before
> entry->w_ctx.flags:
> 
> " (*) Control dependencies apply only to the then-clause and else-clause
>      of the if-statement containing the control dependency, including
>      any functions that these two clauses call.  Control dependencies
>      do -not- apply to code following the if-statement containing the
>      control dependency."
> 
> The read of entry->data must come after the READ_ONCE of entry->w_ctx.flags.
> 

I also understood it this way when implementing the barrier, but after
an offline discussion with Heiner, he convinced me that reordering could
occur.

>> BTW: ARM has an even more relaxed memory model and also allows to
>> re-order writes. If we want to support ARM correctly, I think we also
>> need to insert a memory barrier between writing data and writing
>> flags (and there might be a couple other places where we need to
>> check).

Heiner: Can you develop on this?

Javier

[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]