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

sys/{tsrb, oorb}: Thread-safe ringbuffers #15103

Closed
wants to merge 3 commits into from
Closed

Conversation

maribu
Copy link
Member

@maribu maribu commented Sep 28, 2020

Contribution description

  • Rename the current tsrb implementation to oorb
    • This highlights that this is a one reader, one writer thread-safe ringbuffer
  • Re-add the original tsrb code, but insert irq_disable() ... irq_restore() where needed to provide thready-safety for multiple readers and writers
    • Existing users will just use this instead, as the API is untouched
  • Add a note of warning to oorb
    • This issues this warns about are hopefully addressed soonish

Testing procedure

E.g. stdio over UART on STM32 should still work. And now concurrent stdio should not result in corrupting the output (only in mixing it).

Issues/PRs references

Addresses #9882. (But the same issues still apply to the new oorb. However, with the documentation pointing those issues out, I think we can consider them as features :-D. And those "features" should be addressed in a follow up soonish.)

The thread-safe ringbuffer assumes that only one thread writes to it and only
one thread reads from it. This rename makes it more obvious and allows to
additionally add a (less efficient) thread-safe ringbuffer implementation that
works with multiple readers and/or writers.
Based on the tsrb code just renamed to oorb, implement a thread-safe ring
buffer that works for multiple readers and/or writers by disabling IRQs during
critical phases.
@maribu maribu added Area: sys Area: System Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Sep 28, 2020
@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Sep 30, 2020
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the TSRB could just do e.g.

static inline int tsrb_empty(const oorb_t *rb)
{
    int retval;
    unsigned status = irq_disable();
    retval = oorb_empty(rb);
    irq_restore(status);
    return retval;
}

to avoid code duplication

Comment on lines +51 to +52
volatile unsigned reads; /**< total number of reads */
volatile unsigned writes; /**< total number of writes */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do those need to be volatie?`

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, those should be atomic. Or more precisely, semi-atomic :-D

(As this is the two-threads-one-buffer mode, only the reader will write to reads and only the writer will write to writes. So as long as the read and the write access to them is atomically, everything is fine. This means explicitly that a read-modify-write operation could consist of three operations with only the read and write being atomic. Real atomic accesses would make the whole read-modify-write atomically. The atomic utils helper functions introduce just the semi-atomic functions this needs - which at least for word sized variables should be lock free.)

But I intentionally left the implementation of oorb unchanged. Fixing it efficiently would require the atomic utils PR being merged. And fixing it inefficiently is done in tsrb.

@maribu
Copy link
Member Author

maribu commented Oct 21, 2020

Btw: As boths contributions have been split out, there is no need to keep this open.

@maribu maribu closed this Oct 21, 2020
@maribu maribu deleted the tsrb branch October 21, 2020 06:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: sys Area: System CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants