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

2019-01-08 Thread Kees Bakker

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
___
devel mailing list
devel@riot-os.org
https://lists.riot-os.org/mailman/listinfo/devel


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

2019-01-08 Thread Kaspar Schleiser
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