Consider the following global variable foo, and its get() and set() functions:
int32_t foo;
mutexType fooMutex;
int32_t getfoo(void)
{
int32_t aux;
MutexLock(fooMutex);
aux = foo;
MutexUnlock(fooMutex);
return aux;
}
void setfoo(int32_t value)
{
MutexLock(fooMutex);
foo = value;
MutexUnlock(fooMutex);
}
The following tasks modify foo:
void ResetTask()
{
while(1)
{
setfoo(resetvalue);
AccessPeripheral2();
wait(resetPeriod);
}
}
void ActTask()
{
while(1)
{
foocopy = getfoo();
if (foocopy > 0)
{
x = AccessPeripheral1();
setfoo( foocopy - x);
}
wait(actPeriod);
}
}
The accessPeripheralX() functions have critical sections inside, although protecting different resources.
If the if condition (foocopy > 0) is achieved, but foo is reset between getfoo() and setfoo() at the ActTask(), it will then be set to an outdated, invalid value at the end of the cycle, producing erroneous computation.
How do I prevent this race condition?
Rewriting the ActTask() to:
if (foocopy > 0)
{
x = AccessPeripheral1();
setfoo( getfoo() - x);
}
in practice eliminated it, but theorically the task can still be preempted between getting and setting foo.
I also thought about introducing another critical section when handling foo:
[Enter critical section]
foocopy = getfoo();
if (foocopy > 0)
{
x = AccessPeripheral1();
setfoo( foocopy - x);
}
[Leave critical section]
and
[Enter critical section]
setfoo(resetvalue);
AccessPeripheral2();
wait(resetPeriod);
[Leave critical section]
but I tend to avoid that alternative because I find having nested critical sections greatly increases the complexity (and the likehood of bugs).
You only have one critical section (see assumption below), it spans from reading foo to writing it, because the writing depends on the read value. Not the written value depends on the read value, the fact that it is written.
As long as the read value does not influence the writing (neither the value nor the writing itself) there is no race condition in the reading itself.
E.g. it would be harmless to read the value and then print it to console for humans to contemplate.
As long as the value to be written is not depending on the current value, there is no race condition in the writing itself. E.g. the attempt to write a 5 to a variable containing a 4 is harmless. If however the idea is that after the write, the variable should be one higher than just before the write, then you have race condition with any other increment access.
E.g. two preempting increments on a 4, both of which are expected to have an effect, i.e. the correct result should be 6, can end up in 5, if not protected.
I am assuming that reading/writing a single variable is atomic. I.e. if you attempt to write a given value and preemption happens with a different value to be written, then either value ends up in the variable, not a mixture of both, like e.g. high byte from one value, low byte from the other. Same for reading, i.e. reading with a preempting write access will either yield the old consistent value or the new.
Strictly speaking this is not true, i.e. a standard conforming compiler is not required to guarantee that. If you are worried about this special atomicity problem then keep the protection inside the getter and setter or use mechanisms for writing which guarantee this implicitly.
The racing condition the problem you describe starts at the read access
foocopy = getfoo();
because the value read here affects the write access
setfoo( foocopy - x);
via the intention that the new value should only be written if the variable contains a value greater than zero, as is obvious from
if (foocopy > 0)
I.e. the actual critical section to be protected is this
foocopy = getfoo();
if (foocopy > 0)
{
x = AccessPeripheral1();
setfoo( foocopy - x);
}
Which is of course exactly the part which you protected in your second to last code fragment.
It is however not necessary to protect all of the very last code fragment. Only the writing to foo needs to be prevented during the vulnerable critical section. This is not because the writing itself is a vulnerable critical section; it is just the "attacker", the potential problem. It needs to be locked out, in order to prevent the vulnerable critical section described above.
Too put it all together, I propose the following:
int32_t foo;
mutexType paranoiaMutex;
/* only needed to protect the unrelated critical section of reading or writing,
separatly, if that is not atomic */
int32_t getfoo(void)
{
int32_t aux;
MutexLock(paranoiaMutex); /* or use atomic mechanism */
aux = foo;
MutexUnlock(paranoiaMutex); /* or use atomic mechanism */
/* note this by the way,
you might have overlooked something in your design here */
return aux; /* not foo */
}
void setfoo(int32_t value)
{
MutexLock(paranoiaMutex); /* or use atomic mechanism */
foo = value;
MutexUnlock(paranoiaMutex); /* or use atomic mechanism */
/* Using fooMutex additionally here is imaginable,
but in order to minimise the confusion of nested mutexes,
I propose to use that mutex on the same "layer" of coding.
Note that only one mutex and one layer of mutex nesting
is occuring inside this code part.
You ARE right about being careful with that...
*/
}
and
mutexType fooMutex;
/* Note that only one mutex and one layer of mutex nesting
is occuring inside this code part. */
void ResetTask()
{
while(1)
{
MutexLock(fooMutex);
setfoo(resetvalue);
MutexUnlock(fooMutex);
AccessPeripheral2();
wait(resetPeriod);
}
}
void ActTask()
{
while(1)
{
MutexLock(fooMutex);
foocopy = getfoo();
if (foocopy > 0)
{
x = AccessPeripheral1();
setfoo( foocopy - x);
}
MutexUnlock(fooMutex);
wait(actPeriod);
}
}
There is a possible optimisation for the duration of a locked mutex, which is "expensive".
This optimisation assumes that
AccessPeripheral1() is slow (most peripheral accesses are)AccessPeripheral1(); if foo<=0 (it might be undesired)AccessPeripheral1(); earlier,
especially earlier than reading foo (it might be undesired)These assumptions are important and need to be verified, which I can of course not do.
But if the assumptions are applicable, then change like below, to get a much shorter critical section.
void ActTask()
{
while(1)
{
x = AccessPeripheral1();
MutexLock(fooMutex);
foocopy = getfoo();
if (foocopy > 0)
{
setfoo( foocopy - x);
}
MutexUnlock(fooMutex);
wait(actPeriod);
}
}