Missing volatile keyword in documentation? Topic is solved

[faisal]

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

[Giovanni]

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:

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

[faisal]

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?

[Giovanni]

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:

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]

I made a little test:

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

[faisal]

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

[Giovanni]

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

[faisal]

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]

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