On 05/10/2018, 15:45, "Ananyev, Konstantin" <konstantin.anan...@intel.com> 
wrote:

    We all know that 32bit load/store on cpu we support - are atomic.
Well, not necessarily true for unaligned loads and stores. But the "problem" 
here is that we are not directly generating 32-bit load and store instructions 
(that would require inline assembly), we are performing C-level reads and 
writes and trust the compiler to generate the machine instructions.

    If it wouldn't be the case - DPDK would be broken in dozen places.
And maybe it is, if you compile for a new architecture or with a new compiler 
(which are getting more and more aggressive with regards to utilising e.g. 
undefined behavior in order to optimise the generated code).

    So what the point to pretend that "it might be not atomic" if we do know 
for sure that it is?
Any argument that includes the words "for sure" is surely suspect.

    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.
    But argument that we shouldn't assume 32bit load/store ops as atomic sounds 
a bit flaky to me.
    Konstantin
I prefer to declare intent and requirements to the compiler, not to depend on 
assumptions even if I can be reasonably sure my assumptions are correct right 
here right now. Compilers will find new ways to break non-compliant code if 
that means it can improve the execution time of compliant code.

Someone may modify the code or follow the model for some similar thing but 
change that 32-bit variable to a 64-bit variable. Now for a 32-bit target, the 
plain C read from a volatile 64-bit variable will not be atomic but there will 
be no warning from the compiler, it will happily generate a sequence of 
non-atomic loads. If you instead use __atomic_load_n() to read the variable, 
you would get a compiler or linker error (unless you link with -latomic which 
will actually provide an atomic load, e.g. by using a lock to protect the 
access).


Reply via email to