"Producer from ISRs" section in http://www.chibios.org/dokuwiki/doku.ph ... el_mutexes
Shouldn't qsize be declared as volatile, since it is being modified in the ISR? Couldn't the compiler generate code to read qsize into a register before the chSysLock()? I have a feeling I may be missing something basic here, or having a senior moment .
Missing volatile keyword in documentation? Topic is solved
- Giovanni
- Site Admin
- Posts: 14457
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: Missing volatile keyword in documentation?
Hi,
I think it should be fine, lock and unlock include barriers in their implementation so the compiler is prevented from moving load or stores outside the critical zone and making assumptions.
It is done inside CMSIS headers, this is an example:
This function is the inner part of lock/unlock primitives.
Note that volatile and "memory":
- Volatile prevents the compiler to move code across the ASM statement
- "memory" is a memory barrier, it enforces the compiler to assume the memory potentially modified by the statement and prevents load/stores outside critical zones.
Giovanni
I think it should be fine, lock and unlock include barriers in their implementation so the compiler is prevented from moving load or stores outside the critical zone and making assumptions.
It is done inside CMSIS headers, this is an example:
Code: Select all
__STATIC_FORCEINLINE void __set_BASEPRI(uint32_t basePri)
{
__ASM volatile ("MSR basepri, %0" : : "r" (basePri) : "memory");
}
This function is the inner part of lock/unlock primitives.
Note that volatile and "memory":
- Volatile prevents the compiler to move code across the ASM statement
- "memory" is a memory barrier, it enforces the compiler to assume the memory potentially modified by the statement and prevents load/stores outside critical zones.
Giovanni
Re: Missing volatile keyword in documentation?
Phew, thanks for that. I thought there must be some memory barrier mechanism at play here, otherwise a lot of assumptions of program correctness were shaken for a moment .
If there is a scenario where a variable like that is accessed multiple times between lock()/unlock() - then surely it should be volatile, correct? In the example code, let's say the condition variable was being signaled repeatedly without qsize necessarily being set to a non-zero value. In that case, wouldn't qsize have to be volatile - since it is being read multiple times in the critical zone?
A more general question is if we do multiple API calls that rely on semaphores, mutex etc .. within a critical zone - does the synchronization object itself need to be volatile? What about other platforms that don't have this memory barrier instruction? In that case, shouldn't any and all synchronization objects (semaphore, mutext, condvar, etc ..) be declared volatile?
If there is a scenario where a variable like that is accessed multiple times between lock()/unlock() - then surely it should be volatile, correct? In the example code, let's say the condition variable was being signaled repeatedly without qsize necessarily being set to a non-zero value. In that case, wouldn't qsize have to be volatile - since it is being read multiple times in the critical zone?
A more general question is if we do multiple API calls that rely on semaphores, mutex etc .. within a critical zone - does the synchronization object itself need to be volatile? What about other platforms that don't have this memory barrier instruction? In that case, shouldn't any and all synchronization objects (semaphore, mutext, condvar, etc ..) be declared volatile?
- Giovanni
- Site Admin
- Posts: 14457
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: Missing volatile keyword in documentation?
Reading multiple time from the same critical zone is not an issue, there is nothing that can change the variable except the current program flow. It does not even matter if the variable is cached into a register, load and stores cannot escape outside the critical zone.
Volatile is required for "shared" variables accessed by ISRs or threads.
Examples:
Another note about that condvar example, S-class functions perform context switch and it includes a memory barrier too, the compiler cannot cache qsize (or global variables in general) across a context switch.
Edit: Changed the 2nd example, it was wrong.
Giovanni
Volatile is required for "shared" variables accessed by ISRs or threads.
Examples:
Code: Select all
/* No need for n to be volatile.*/
chSysLock();
n++;
...
n--;
chSysUnlock()
Code: Select all
/* No need for n to be volatile.*/
chSysLock();
n++;
chSysUnlock()
...
chSysLock();
n--;
chSysUnlock()
Another note about that condvar example, S-class functions perform context switch and it includes a memory barrier too, the compiler cannot cache qsize (or global variables in general) across a context switch.
Edit: Changed the 2nd example, it was wrong.
Giovanni
- Giovanni
- Site Admin
- Posts: 14457
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: Missing volatile keyword in documentation?
I made a little test:
The following code:
Results in:
Note that the compiler optimizes out the loop (for some strange reason it still compare is to zero instead of just clearing "n". If we add a critical section we get:
The compiler does not more optimize access to "n".
Giovanni
The following code:
Code: Select all
while (n > 0) {
n--;
}
Results in:
Code: Select all
80030d0: 4bc2 ldr r3, [pc, #776] ; (80033dc <main+0x30c>)
80030d2: 681a ldr r2, [r3, #0]
80030d4: 2a00 cmp r2, #0
80030d6: bfc8 it gt
80030d8: 2200 movgt r2, #0
80030de: 601a strgt r2, [r3, #0]
Note that the compiler optimizes out the loop (for some strange reason it still compare is to zero instead of just clearing "n". If we add a critical section we get:
Code: Select all
while (n > 0) {
chSysLock();
n--;
chSysUnlock();
}
Code: Select all
while (n > 0) {
80030d2: 4bc3 ldr r3, [pc, #780] ; (80033e0 <main+0x310>)
80030d4: 681a ldr r2, [r3, #0]
80030d6: 2a00 cmp r2, #0
while (n > 0) {
80030da: dd0b ble.n 80030f4 <main+0x24>
80030dc: 2020 movs r0, #32
80030de: 2100 movs r1, #0
80030e0: f380 8811 msr BASEPRI, r0
chSysLock();
n--;
80030e4: 681a ldr r2, [r3, #0]
80030e6: 3a01 subs r2, #1
80030e8: 601a str r2, [r3, #0]
80030ea: f381 8811 msr BASEPRI, r1
while (n > 0) {
80030ee: 681a ldr r2, [r3, #0]
80030f0: 2a00 cmp r2, #0
80030f2: dcf5 bgt.n 80030e0 <main+0x10>
The compiler does not more optimize access to "n".
Giovanni
Re: Missing volatile keyword in documentation?
Thanks for the example Giovanni. How does the compiler know it shouldn't cache n in a register after an msr instruction? I don't see any memory or data barrier instructions generated.
This is the documentation of the MSR instruction I read:
http://infocenter.arm.com/help/index.js ... EEJCI.html
This is the documentation of the MSR instruction I read:
http://infocenter.arm.com/help/index.js ... EEJCI.html
- Giovanni
- Site Admin
- Posts: 14457
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: Missing volatile keyword in documentation?
It is not a runtime barrier, it is a logical barrier for the compiler. That " : memory" tells the compiler that the asm instruction can change memory (even if it is not true), the compiler can no more assume that "n" is unchanged across the asm statement.
Giovanni
Giovanni
Re: Missing volatile keyword in documentation?
Giovanni wrote:It is not a runtime barrier, it is a logical barrier for the compiler. That " : memory" tells the compiler that the asm instruction can change memory (even if it is not true), the compiler can no more assume that "n" is unchanged across the asm statement.
Giovanni
Cool, you just gave me a couple search terms to Google around with .
Now, having Googled it seems that the logical memor. y barrier would not be sufficient for those processors that have I-cache enabled, and thus may require memory barrier instructions. Just wondering if the current solution of a logical barrier is sufficient for the entire range of potential devices ChibiOS could support. The most general solution would be to declare all multi-thread objects as volatile?
- Giovanni
- Site Admin
- Posts: 14457
- Joined: Wed May 27, 2009 8:48 am
- Location: Salerno, Italy
- Has thanked: 1076 times
- Been thanked: 922 times
- Contact:
Re: Missing volatile keyword in documentation?
Hi,
I believe that from the CPU point of view it makes no difference if code is fetched directly from RAM or from the cache, same for data. Multi threading makes no difference, it is one CPU executing code sequentially, all that matters is that cache coherency with RAM/flash.
Cache has an impact when there are multiple bus masters (DMAs, other cores) and cache coherence is not handled in HW, see Cortex-M7.
Giovanni
I believe that from the CPU point of view it makes no difference if code is fetched directly from RAM or from the cache, same for data. Multi threading makes no difference, it is one CPU executing code sequentially, all that matters is that cache coherency with RAM/flash.
Cache has an impact when there are multiple bus masters (DMAs, other cores) and cache coherence is not handled in HW, see Cortex-M7.
Giovanni
Who is online
Users browsing this forum: No registered users and 71 guests