Concerns about the atomicity of vTaskSuspendAll()

Kernel
1

kilo-sierra wrote on Tuesday, September 17, 2013:

vTaskSuspendAll()

is used to lock taskswitches without disabling interrupts. Pretty good.
But this function works by just incrementing a global variable

++uxSchedulerSuspended;

On many platforms including AVR and ARM Cortex M this operation compiles to something like

load uxSchedulerSuspendeded into register
increment register
store register into uxSchedulerSuspendeded

which is obviously NOT ATOMIC.

Therefore the variable uxSchedulerSuspendeded may get
a wrong value which can lead to unintended unlocking of
the kernel with catastrophic consequences.

I suggest to modify the locking function like this

void vTaskSuspendAll( void )
{
	portATOMIC_INCREMENT( uxSchedulerSuspended);
}

and to implement a macro portATOMIC_INCREMENT()
for all platforms that lack an atomic increment
machine instruction.

2

destremps wrote on Wednesday, September 18, 2013:

Well he sure might be right. I suppose there is only such a thing as an atomic read. Not an atomic read-increment-write to a global. Native size or not. Does this need to be fixed?

3

jdurand wrote on Wednesday, September 18, 2013:

When I modify globals I use something like this

void FlagClear(int i) {			// safely clear a flag
	vPortEnterCritical();		// disable FreeRTOS interrupts
	Flags &= ~i;			// clear the flag
	vPortExitCritical();	        // put interrupts back like they were
}

Of course that has the problem of Enter and Exit not working right…

4

rtel wrote on Wednesday, September 18, 2013:

Sorry not to answer this sooner - I’ve been thrown out somewhat by the forum software being updated and then having to re-register to get notifications of people posting.

This topic has been discussed a few times, you should be able to find it in the archive on the FreeRTOS.org website.

Basically the key to this is that each task maintains its own context, and a context switch cannot occur if the variable is non zero. So, as long as the writing from the register back into the memory is atomic, it is not a problem.

In your example, consider the following scenario, which starts with uxSchedulerSuspended at zero.

/* This load stores the variable's value in a register. */
load uxSchedulerSuspendeded into register

 ... Now a context switch causes another task to run, and
the other task uses the same variable.  The other task 
will see the variable as zero because the variable has 
not yet been updated by the original task... Eventually 
the original task runs again.  **That can only happen 
when uxSchedulerSuspended is once again zero**, and when 
the original task runs again the contents of the CPU 
registers are restored to exactly how they were when it 
was switched out - therefore the value it read into the 
register still matches the value of the 
xuSchedulerSuspended variable ... 

/* The value will be incremented to be equal to 1. */
increment register

/* The value restored to uxSchedulerSuspended will be the 
correct value of 1, even though the variable was used 
by other tasks in the mean time. */
store register into uxSchedulerSuspendeded

Regards.

5

destremps wrote on Wednesday, September 18, 2013:

Thanks Richard. Okay. That explains it. This particular variable ‘self protects’ itself.

6

kilo-sierra wrote on Wednesday, September 18, 2013:

YUP - got it … good news!
uxSchedulerSuspended is binary, not counting
because of it’s special use.
Especially it is not used whithin one of the
…fromISR functions.
So everything is fine.

Thank you Richard!