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
