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

Kees Bakker kees at sodaq.com
Tue Jan 8 21:47:19 CET 2019


On 08-01-19 16:10, Kaspar Schleiser wrote:
> Hi Kees,
Hey Kaspar,
> 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.
Well, in the example code, the compiler would just read 
sched_active_thread->waiters
once and keep it in a register. If either sched_active_thread or waiters
can change in between the uses in that function you must declare some
things volatile. What is also interesting here is to ask ourselves,
can sched_active_thread change in between? If so you would need to declare
that variable as
     thread_t * volatile sched_active_thread;
(( This was mentioned in one of the Github issues, but the discussion 
stalled. ))

> This was certainly a case of only (at most) half-understanding volatile,
Which proves my point :-)
> which then turned into "if it is not broken, don't fix it".
I'm not a fan of that. It is like looking away and hoping there
are no problems. No matter how hard it is, we need to fully
understand, especially a thing like a scheduler.

> 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.
>

Of course we need thorough testing. Let's not rush. If I wanted to
experiment with volatile changes, what would be good test programs
for this?
-- 
Kees


More information about the devel mailing list