Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mbed::CircularBuffer interrupt safe but not thread safe? #15497

Open
schnoberts1 opened this issue Mar 8, 2024 · 4 comments
Open

mbed::CircularBuffer interrupt safe but not thread safe? #15497

schnoberts1 opened this issue Mar 8, 2024 · 4 comments

Comments

@schnoberts1
Copy link

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
Copy link
Contributor

0xc0170 commented May 21, 2024

`> 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
Copy link
Author

schnoberts1 commented May 21, 2024

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
Copy link
Contributor

agausmann commented Sep 25, 2024

(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
Copy link
Author

Ok, that makes sense for single core. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Untriaged
Status: Needs Triage
Development

No branches or pull requests

4 participants