-
Notifications
You must be signed in to change notification settings - Fork 2k
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/malloc_thread_safe: new module #15606
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
maribu
requested review from
aabadie,
bergzand,
fjmolinas,
kaspar030,
kYc0o,
leandrolanzieri,
MichelRottleuthner,
miri64,
roberthartung and
smlng
as code owners
December 10, 2020 09:46
maribu
added
Area: sys
Area: System
CI: run tests
If set, CI server will run tests on hardware for the labeled PR
Type: bug
The issue reports a bug / The PR fixes a bug (including spelling errors)
Type: enhancement
The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
CI: ready for build
If set, CI server will compile all applications for all available boards for the labeled PR
labels
Dec 10, 2020
maribu
force-pushed
the
malloc-newlib-picolibc
branch
from
December 15, 2020 12:31
bb70a59
to
26d5b45
Compare
benpicco
approved these changes
Dec 17, 2020
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me.
The overhead is minimal and only has to be payed if malloc
& friends are used at all.
Tests did all pass, so that should be good.
codespell found some typos - please squash those in directly.
This test checks whether calling malloc in more than one thread is safe.
Split out Gunar Schorcht's clever approach to provide thread safe malloc for AVR into a system module and make AVR depend on this. This allows other platforms to also use this.
Disabling IRQs during malloc() provides mutually exclusive access and even is safe from IRQ context, but is suboptimal for real time scenarios. Instead, the implementation is changed to use a mutex to provide mutually exclusive access. As a result, calls to malloc() and free() from IRQ context no longer is possible. But this this is a really horrible idea to begin with, the impact should be minimal and the improved real time properties of the system should make it a good trade-off. An assert() is added to allow easy detection of regressions and, hence, aid users to fix their code.
maribu
force-pushed
the
malloc-newlib-picolibc
branch
from
December 17, 2020 14:39
26d5b45
to
09b41d2
Compare
Thanks :-) |
This was referenced Jan 10, 2021
This was referenced Apr 14, 2021
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
CI: run tests
If set, CI server will run tests on hardware for the labeled PR
Type: bug
The issue reports a bug / The PR fixes a bug (including spelling errors)
Type: enhancement
The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Background
The AVR libc in
master
has special wrappers formalloc()
,calloc()
,realloc()
andfree()
provided by @gschorcht to allow thread-safe calls tomalloc()
and friends. Using some linker magic, all calls to the real functions are linked against those wrappers. Hence, neither the C library nor the user code need to be touched to provide thread safety this way.Other C libraries like newlib or picolibc optionally provide hooks that get called when entering and leaving
malloc()
and friends. Similar hooks exist for other functions as well. #8619 provides those hooks, so that thread safe versions of various syscalls are made available. However, presumably due to the increased per-thread RAM requirements, this PR still didn't made it tomaster
.@gschorcht
malloc()
wrappers on the other side require no additional RAM, only some ROM. And the concept easily applies to any c library. So this PR makes them available as separate module.Contribution description
malloc()
and friends fromcpu/atmega_common
tosys/malloc_thread_safe
mutex
instead of disabling IRQ during critical coresizeof(mutex_t)
(2 - 4 bytes) and calls tomalloc()
and friends from IRQ context becomes impossibleassert()
s are added to easily detect calls tomalloc()
from IRQ contextcpu/atmega_common
andcpu/cortexm_common
depend on the new moduleFuture Steps
USEMODULE += malloc_thread_safe
, but splitting this eases testing.malloc()
and friends can be optional. This allows application developers to decide which side of the trade-off (RAM vs. thread-safety) they prefer. E.g. most of the time I don't care about garbage from STDOUT, or being able to only access files from a single thread.malloc_thread_safe
could be dropped whennewlib_thread_safe
is usedmalloc
wrappers get garbage collected, if unused. So only applications doing dynamic memory management have to pay the toll.Testing procedure
The test provided by #15598 is included in this PR and should now pass for Cortex-M boards.
Issues/PRs references
Includes #15598
Provides in part what #8619 aims to do. (And should not conflict with it. User should be able to pick #8619 over this.)