Hi everyone, > I seem to remember that we tried making sched_active_thread non-volatile > at some point, breaking things, but that has also been a long time ago.
I think the reason might be that the compiler reordered access to the variables. See C99 standard section 5.1.3 §5: > The least requirements on a conforming implementation are: > — At sequence points, volatile objects are stable in the sense that previous > accesses are complete and subsequent accesses have not yet occurred. > [...] See also section 6.8 §4: > [...] The end of a full expression is a sequence point. So e.g. a call to `thread_yield_higher()` would be a sequence point, so all memory accesses to variable declared `volatile` written before that call would be actually executed before, and all accesses after would have to be executed afterwards. So regarding to the `volatile` variable the call to `thread_yield_higher()` would be a memory barrier (but not regarding to every other variable). And this is exactly why `volatile` solves a problem here. I think getting rid of `volatile` and adding "proper" memory barriers to places where context switches are supposed to happen will make the code more robust and might allow the compiler to optimize the code better: More robust, because some memory accesses near context switches might have just by luck not yet been moved by the compiler to the wrong side of a context switch - there might be non-volatile variables that are vulnerable to being moved. And better optimized because `volatile` will blindly turn off all optimization while accessing the affected variables, but memory barriers are more fine-grained. `void __sync_synchronize(void)` placed just before and just after a context change seems to be the reasonable thing to me. (Also `core/atomic_c11.c` conveniently provides a reasonable fallback for compilers not having it - at least as long as the CPU does not reorder commands. But I bet this is true for most/all IoT class CPUs as of now.) @Kees: Still interested in this conversion? Mind to take the lead? I'm happy to help in any way I can. Kind regards, Marian On Tue, 8 Jan 2019 16:10:27 +0100 Kaspar Schleiser <kas...@schleiser.de> wrote: > Hi Kees, > > On 1/7/19 9:19 PM, Kees Bakker wrote: > > My main concern is: who made it volatile in the first place? > > I did, almost a decade ago. > > > And what was the reasoning behind it? Volatile is one of the least > > understood properties of the C language (my personal opinion). I'm > > hoping that the volatile was not just thrown in because it feels good > > when doing threads. And in other places the volatile is ignored, > > hopefully for a good reason (optimisation is _not_ a good reason). > IIRC the intention was so the IPC code would create read / write > accesses whenever accessing fields of thread_t. > > E.g.: > > void msg_recv(...) > { > if (!sched_active_thread->waiters) { > // platform specific macro to suspend thread > } > > thread_t *waiter = sched_active_thread->waiters; > > // ... > } > > (or similar) > > My understanding of volatile back then was that the compiler could, > without volatile, assume that sched_active_thread->waiters equals NULL. > > This was certainly a case of only (at most) half-understanding volatile, > which then turned into "if it is not broken, don't fix it". > > Nowadays such code is always guarded with disabled IRQs. > > I seem to remember that we tried making sched_active_thread non-volatile > at some point, breaking things, but that has also been a long time ago. > > I'm all for removing the qualifier. But we do have to make sure to > thoroughly test core/ on all platforms. > > Kaspar > _______________________________________________ > devel mailing list > devel@riot-os.org > https://lists.riot-os.org/mailman/listinfo/devel
pgp9Ip5Xc1mdx.pgp
Description: OpenPGP digital signature
_______________________________________________ devel mailing list devel@riot-os.org https://lists.riot-os.org/mailman/listinfo/devel