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: Make thread-safe #15218

Merged
merged 1 commit into from
Oct 20, 2020
Merged

sys/tsrb: Make thread-safe #15218

merged 1 commit into from
Oct 20, 2020

Conversation

maribu
Copy link
Member

@maribu maribu commented Oct 13, 2020

Contribution description

Drop the requirement of having only one writer and one reader, as the name of the ring-buffer does not indicate any limitation on the thread-safety. The two-threads-one-buffer kind of ring buffer can be reintroduced with a different name.

Testing procedure

On e.g. STM32 or SAM0 boards, have two threads continuously produce stdio output. The thread with higher priority should sleep for random intervals.

Expected output:

  • The higher priority threads interrupts the lower priority thread and stdio output gets mixed up. But no characters are lost or printed more than once

Actual output:

  • Occasionally, chars get lost or printed more than once

Issues/PRs references

Fixes #9882

Split out of #15103

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Drop the requirement of having only one writer and one reader, as the name of
the ring-buffer does not indicate any limitation on the thread-safety. The
two-threads-one-buffer kind of ring buffer can be reintroduced with a different
name.
@maribu maribu requested a review from kaspar030 as a code owner October 13, 2020 08:38
@miri64 miri64 added Area: sys Area: System Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) labels Oct 13, 2020
@miri64 miri64 added this to the Release 2020.10 milestone Oct 13, 2020
@miri64 miri64 requested a review from bergzand October 13, 2020 09:31
@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 14, 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.

Makes sense, especially when you add the old version under a different name for the one reader one writer case.

The 'thread safe' ring buffer actually not being thread safe at all has thrown me off already too.

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR CI: run tests If set, CI server will run tests on hardware for the labeled PR labels Oct 20, 2020
@miri64
Copy link
Member

miri64 commented Oct 20, 2020

pkg_libhydrogen test fail is unrelated (see #15066)

@miri64 miri64 merged commit 9e4c508 into RIOT-OS:master Oct 20, 2020
@maribu
Copy link
Member Author

maribu commented Oct 20, 2020

Thanks :-)

@maribu
Copy link
Member Author

maribu commented Oct 20, 2020

Backport provided in #15254

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 Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch 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.

sys/tsrb is not thread safe
3 participants