[riot-notifications] [RIOT-OS/RIOT] cpu/atmega_common: RTT and RTC support (#8842)

ZetaR60 notifications at github.com
Sat Apr 20 09:07:21 CEST 2019


> @ZetaR60: If my interpretion of the standard is indeed wrong, you should go for [compiler memory barriers](https://en.m.wikipedia.org/wiki/Memory_ordering#Compiler_memory_barrier) instead of `volatile`, as those have less impact on performance.
> 
> ```c
> static inline void disable_timer_int(void) {
>     asm volatile("" ::: "memory");
>     TIMSK2 &= ~(1 << OCIE2A);
>     asm volatile("" ::: "memory");
> ```
> 
> and the same for reenable. That will do exactly what is intended and not disable any more optimisation than needed.

No, I feel that this is bad advice and it won't do what you intend with respect to optimization.
1) This use of volatile here does exactly what is needed, with very little possible additional optimization.
2) Memory barriers should not be used as a replacement for volatile. Volatile prevents only reordering with other volatile accesses, but memory barriers prevent _all_ forms of optimization from passing the memory barrier.
3) This is a GCC specific idiom. While using something like this is sometimes okay, a standard construction (like volatile) should be strongly preferred.
4) `_safe_cnt_get()` would require half a dozen memory barriers, compared to a single volatile.


> @ZetaR60: I did some more research regarding moving memory access to globals around accesses to volatiles:
> 
> [This write up by Prof John Regehr](https://blog.regehr.org/archives/28a) states that there two fractions:

This is a broken link for me.


> > One school of thought is that compilers may not move accesses to global variables around accesses to volatile variables.  There seems to be a consistent reading of the standard that backs this up.
> 
> But he states most compilers (at least back in 2010) share exactly your view on that, so that they do move access to non-`volatile` globals around accesses to volatiles. So your claim that additional measures need to be taken to prevent that reordering is 100% correct. (And obviously it doesn't even matter whether or not my reading of the standard is correct, as in the end we will have to work with real world compiler and how they do things.)

A bit off topic then, but according to the standard doc, writes to global variables in the translation environment may be reordered, while writes to globals in the execution environment may not. The translation environment is basically everything from the source file that gets compiled (with includes). The execution environment is basically everything outside of that (i.e. the stuff calling the code). So, essentially, if you reference a global with `extern` it gets protection from reordering. This also applies to pointers that point to memory in the execution environment.


> I'd still suggest to use the compiler memory barrier instead of `volatile` to prevent the reordering. This will prevent optimisation just as much as needed, and also the intent of the code will be more readable.
> 
> Memo to my self: A PR adding memory barriers e.g. via `static inline void memory_barrier(void) { asm....}` would be nice.

Please don't go sprinkling things like this around. I do not mean to offend, but I do not feel that you currently have a deep enough understanding of how volatile and memory barriers work to adjudicate their useage. Adding new memory barriers improperly to working code has a high chance of degrading it, and a low chance of making it safer.

-- 
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/8842#issuecomment-485065048
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.riot-os.org/pipermail/notifications/attachments/20190420/5701f23f/attachment-0001.html>


More information about the notifications mailing list