mbed::CircularBuffer interrupt safe but not thread safe?

[schnoberts1]

Description of defect

The ARMbed synchronisation description is as follows:

• Interrupt safe - safe for use from multiple threads and interrupts; operation is done atomically or in a critical section. The behavior is well defined when used from both interrupts and threads.
• Thread safe - safe for use from multiple threads; operation is protected by an RTOS primitive and can be used from multiple threads, but will cause problems if used from an interrupt service routine.
• Not protected - operation does not protect against concurrent access and needs to be synchronized externally. If you call from multiple threads without some other form of synchronization, data can become corrupted and behavior is undefined.

e.g. If it's marked Interrupt safe it's also thread safe.

mbed::CircularBuffer is marked thread safe but it's incrementCounter method is not thread safe. There's also a lack of fences, perhaps this isn't needed, but if that's the case it should be commented.

I think there's either a defect in the buffer or there's a defect in tagging this method as interrupt safe when that is defined as also being safe for use on multiple threads. As far as I can tell being in a critical section is not the same as an operation being atomic. Am I wrong and a critical section acts as a global lock, it looks to me like all it does is provide re-entrant disabling of IRQs?

Target(s) affected by this defect ?

Any developer using this class to communicate between threads.

Toolchain(s) (name and version) displaying this defect ?

All

What version of Mbed-os are you using (tag or sha) ?

mbed-os-6.17.0

What version(s) of tools are you using. List all that apply (E.g. mbed-cli)

N/A

How is this defect reproduced ?

I simply read the code, but any multi-thread high throughput test case would work, as would reading the resulting assembler.

[0xc0170]

`> mbed::CircularBuffer is marked thread safe but it's incrementCounter method is not thread safe. There's also a lack of fences, perhaps this isn't needed, but if that's the case it should be commented.

It's private method. Whoever changes the class implementation should follow the synchronization promise (invoking private methods that are not protected must be in the protected block otherwise its breaking its promise). Or what have I missed?

[schnoberts1]

It was an example and I should have provided context.

My confusion leads from this :

void push(const T &data)
    {
        core_util_critical_section_enter();

        _buffer[_head] = data;

        _head = incrementCounter(_head);

        if (_full) {
            _tail = _head;
        } else if (_head == _tail) {
            _full = true;
        }

        core_util_critical_section_exit();
    } 

It calls incrementCounter. It’s wrapped in these critical sections calls which call hal_critical_section_enter which call __disable_irq but this isn’t a serialisation point (a lock) or a fence is it? If I am right then I expected incrementCounter to be thread safe but it doesn’t seem to be.

Sorry for the confusing problem statement.

I don’t understand how this circular buffer is thread safe. I’d like use it so if I am mistaken that’s awesome :)

[agausmann]

(disclaimer: I may have some details wrong - if you have better information, please let me know!)

I believe critical sections implicitly provide thread safety for single-core systems (which is partly why interrupt-safe implies thread-safe). Because no interrupts can be fired, the current thread cannot be preempted, so no other threads can run until the critical section ends.

Also if it is single-core, memory consistency is not an issue for thread synchronization, so memory barriers are not needed for that case.

[schnoberts1]

Ok, that makes sense for single core. Thanks.