[riot-devel] sched_active_thread is a volatile pointer, but volatile is ignored

Marian Buschsieweke marian.buschsieweke at ovgu.de
Tue Feb 26 22:42:20 CET 2019


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 <kaspar at 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 at riot-os.org
> https://lists.riot-os.org/mailman/listinfo/devel

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 228 bytes
Desc: OpenPGP digital signature
URL: <http://lists.riot-os.org/pipermail/devel/attachments/20190226/56c900d0/attachment.sig>


More information about the devel mailing list