[riot-notifications] [RIOT-OS/RIOT] sys: Added simple memory barrier API (#11438)

Juan I Carrano notifications at github.com
Wed Apr 24 16:57:46 CEST 2019


I agree with the general idea of this PR (having both compiler and cpu barriers). A couple of issues:

Implementation of barrier() is OK, except that a function marked inline may not be inlined by the compiler, so I believe it is better to have a macro. That's what linux does:

https://github.com/torvalds/linux/blob/6f0d349d922ba44e4348a17a78ea51b7135965b1/include/linux/compiler-gcc.h#L16

As for memory_barrier, it is just another name for `__sync_synchronize`, so I don't know which should be the "real" one and which one the "alias". The biggest problem, however, is that RIOT's `__sync_synchronize` does not do what it is supposed to:

https://github.com/RIOT-OS/RIOT/blob/11da5e8e671c12e5182c1f7aa651489edbe5390a/core/atomic_c11.c#L354-L361

See, it is essentially a compiler barrier.

I think we should fix this before introducing a function that claims to do something that is not true. Of course, if the target does not have barrier instructions it is because memory access reordering by the CPU is not possible so that is not a problem.

Another issue with that implementation is that it is a function defined in a C file. It is pointless to put a (compiler) barrier in a function since a call to code in another translation unit is already a barrier: since the compiler does not know what the external function does, it must assume anything may have changed (this may not be true with LTO).

Now, as far as I understand, __sync_synchronize is a builtin, so it is probably already defined by the compiler for many platforms.


-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/RIOT-OS/RIOT/pull/11438#issuecomment-486278028
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190424/93a236b4/attachment.html>


More information about the notifications mailing list