Is self increment in C thread safe?

Question

I found some code in FreeRTOS (FreeRTOSV7.4.0\FreeRTOS\Source\tasks.c):

void vTaskSuspendAll( void )
{
    /* A critical section is not required as the variable is of type
    portBASE_TYPE. */
    ++uxSchedulerSuspended;
}

It is explicitly said no need to protect due to the type is "portBASE_TYPE", which is a "long" type. My understood is that it assumes the self-increment to this type is atomic. But after I disassembled it I could not find any proof, its a plain load->add->store. Then is it a problem?

void vTaskSuspendAll( void )
{
        /* A critical section is not required as the variable is of type
        portBASE_TYPE. */
        ++uxSchedulerSuspended;
 4dc:   4b03            ldr     r3, [pc, #12]   ; (4ec <vTaskSuspendAll+0x10>)
 4de:   f8d3 2118       ldr.w   r2, [r3, #280]  ; 0x118
 4e2:   1c50            adds    r0, r2, #1
 4e4:   f8c3 0118       str.w   r0, [r3, #280]  ; 0x118
 4e8:   4770            bx      lr
 4ea:   bf00            nop
 4ec:   00000000        .word   0x00000000

000004f0 <xTaskGetTickCount>:
        return xAlreadyYielded;
}

It's not defined by C. It depends on the compiler/hardware platform. — Oliver Charlesworth, Mar 20 '13 at 01:23
A good policy to adopt when dealing with thread-safety is this: If there is even the tiniest shred of a chance that a race condition will occur, then it WILL occur. This doesn't answer your question, but it's useful to reassure yourself that locking is important if you ever find yourself thinking "this operation is so nearly atomic that I don't need to lock". — paddy, Mar 20 '13 at 02:38
Finally, I find the reason why it is safe here. Thanks all! [link](http://www.freertos.org/FreeRTOS_Support_Forum_Archive/February_2010/freertos_portBASE_TYPE_amp_critical_section_3560405.html) — user1603164, Mar 20 '13 at 04:42
FreeRTOS code is NOT thread safe when it runs under posix simulator. I have checked and the above shit results in hundreds of concurrency errors reported by valgrind. However interestingly it still continues to work most of the time. I guess it does so more by chance than by design.. (however on arm that would be fine. I still think though that something should be added to make it thread safe on platforms that require explicit safety) — Martin, Jan 20 '17 at 12:41
FreeRTOS has many ports and this one may be wrong. If the portBASE_TYPE required to be atomic - try using char (it only holds true and false anyway). — lkanab, Nov 01 '18 at 03:49

score 9 · Answer 1 · answered Mar 20 '13 at 01:55

9

It's not atomic, as you've documented. But it could still be "thread safe" in a less strict sense: a long can't be in an inconsistent state. The extent of the danger here is that if n threads call vTaskSuspendAll then uxSchedulerSuspended will be incremented by anywhere between 1 and n.

But this could be perfectly fine if the variable is something that doesn't need to be perfect, like a tracker for how many times the user asked to suspend. There's "thread safe" meaning "this operation produces the same result, no matter how its calls are interleaved" and there's "thread safe" meaning "nothing explodes if you call this from multiple threads".

answered Mar 20 '13 at 01:55

Nathaniel Waisbrot

23,261
7
71
99

+1 for most complete answer. I think the comment in the original FreeRTOS code is confusing. I bet that all that FreeRTOS requires is that uxSchedulerSuspended be non zero to halt, so it does not matter if it is incremented 1 or n. – Mark Lakata Sep 24 '13 at 16:45
Wow! Great answer – lkanab Nov 01 '18 at 03:38

score 8 · Answer 2 · answered Mar 20 '13 at 01:24

8

No, incrementing values in C is not guaranteed to be atomic. You need to provide synchronization, or use a system-specific library to perform atomic increments/decrements.

answered Mar 20 '13 at 01:24

Sergey Kalinichenko

714,442
84
1,110
1,523

So do you mean it can be a bug? Even in a single core environment? – user1603164 Mar 20 '13 at 01:29
1

@user1603164 Absolutely: if your code is preempted between loading and storing the value, the other thread can modify the value from under you, causing hard-to-find errors. This would happen even in single-CPU, single-core, non-hyperthreaded environments. – Sergey Kalinichenko Mar 20 '13 at 01:36
1

Yes, even in a single-core environment. There is no reason that `x++;` needs to generate machine code that performs a single-instruction read-modify-write on memory. The compiler is free to implement `++` with separate loads and stores, and very well might for better scheduling, etc. Also, many machines **don't even have** read-modify-write instructions. In any such case, the thread could be preempted after the read but before the write. – R.. GitHub STOP HELPING ICE Mar 20 '13 at 01:38
Any BKM to use the compiler from https://launchpad.net/gcc-arm-embedded to generate atomic++? Target for cortex-m4. Otherwise I have to disable/enable irq to protect? – user1603164 Mar 20 '13 at 01:41
@user1603164 It sounds like the exact value of the `uxSchedulerSuspended` variable is not allowed to change between the suspension and re-activation of the task, so you do not need to modify anything. – Sergey Kalinichenko Mar 20 '13 at 09:51

score 6 · Answer 3 · edited Jun 01 '18 at 20:07

The operation is not atomic, but nowhere does it say it is. However, the code is thread safe, but you would have to be very familiar with what the code was doing, and how it fitted into the design of the scheduler to know that. It does not matter if other tasks modify the variable between the load and store because when the executing task next runs it will find the variable in the same state as when the original load was performed (so the modify and write portions are still consistent and valid).

As a previous posted notes, the long cannot be in an inconsistent state because it is the base type of the architecture on which it is running. Consider however what would happen if the code was running on an 8 bit machine (or 16 bit) and the variable was 32 bit. Then it would not be thread safe because the full 32 bits would be modified byte or word at a time, rather than all at once. In that scenario, one byte might be loaded into a register, modified, then written back to RAM (leaving the other three bytes unmodified) when a context switch occurs. If the next task that executed read the same variable it would read one byte that had been modified and three bytes that had not - and you have a major problem.

+1 If the variable is guaranteed to be in the same state when the task next runs, why would that same assumption not hold for incrementing a multi-byte type? — Sergey Kalinichenko, Mar 20 '13 at 09:54

Is self increment in C thread safe?

3 Answers3