implementation of vTaskSuspendAll() in 5.20,

kolodko1 wrote on Thursday, June 11, 2009:

Hi

Port PIC24, ver, 5.20
Compilator:  pic30-gcc v3.12 from microchip, opt. flag O2 + some minors flags

Disassembly listing for vTaskSuspendAll()
--------------------------------------------------------------------------------------------------------
1015:             
1016:              void vTaskSuspendAll( void )
1017:              {
1018:                  /* A critical section is not required as the variable is of type
1019:                  portBASE_TYPE. */
1020:                  ++uxSchedulerSuspended;
1399A  81D890     mov.w 0x3b12,0x0000
1399C  E80000     inc.w 0x0000,0x0000
1399E  89D890     mov.w 0x0000,0x3b12
1021:              }
139A0  060000     return
--------------------------------------------------------------------------------------------------------

So it takes 3 instruction: load uxSchedulerSuspended to WREG0, increment WREG0 and store results.
So I do not see reason for comment:
    /* A critical section is not required as the variable is of type
    portBASE_TYPE. */

Please comment.
Best Regards
/Greg

edwards3 wrote on Thursday, June 11, 2009:

That does not look right!

rtel wrote on Thursday, June 11, 2009:

That is a very recent change, and I agree it looks wrong.  I will investigate.  There will be a new release on the 22nd so it can get fixed quickly.

Thanks for pointing this out.

Regards.

richard_damon wrote on Thursday, June 11, 2009:

Actually, I think it is ok, let me explain.

If uxSchedulerSuspended starts off non-zero, then we don’t need a critical section, as an interrupt during the execution of the code will not change the thread of execution, so the non-atomic sequence will execute ok (a key fact is that no interrupt will change the value of this variable and not set it back to what it started as).

If uxSchedulerSuspended starts of zero, then it is possible for the thread of execution to be interrupted, but if we do execute a task switch, then when we get switched back, by definition the variable is zero again (if not, we couldn’t have been been switched back to).

The key fact is that due to the nature of this variable, a task can not see it changed by another task, it will always be zero if something else can run (that task can change it, but must restore it before you get control back).

In fact I think it will work even if the read and writes to uxSchedulerSuspended aren’t atomic in most practical situations (the most likely would be hitting a nesting depth suspensions of 256 on a machine that writes 1 byte at a time, then the counting from 255 to 256 might go 255->0(write low byte) -> 256 (write high byte) and if you get in interrupt for the moment when it is 0 you will have a problem.

rtel wrote on Thursday, June 11, 2009:

Excellent.  Thanks for the clarity.

I would like to say "that was exactly what I was thinking", but um…well its getting late.

Regards.

kolodko1 wrote on Friday, June 12, 2009:

Hi

Hmmm…, yes, that’s reasonable explanation. Of course based on assumption that read and write is atomic.

Regards
/Greg

richard_damon wrote on Friday, June 12, 2009:

Yes, the access to the count needs to be atomic (or at least atomic "enough", the bits that might be non-zero need to be accessed atomically, depending on how many layerd of vTaskSuspendAll() might be nested, that may just be the bottom couple of bits.