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/malloc_thread_safe: new module #15606

Merged
merged 4 commits into from
Dec 17, 2020
Merged

Conversation

maribu
Copy link
Member

@maribu maribu commented Dec 10, 2020

Background

The AVR libc in master has special wrappers for malloc(), calloc(), realloc() and free() provided by @gschorcht to allow thread-safe calls to malloc() 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 to master.

@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

  • Move thread safe implementation of malloc() and friends from cpu/atmega_common to sys/malloc_thread_safe
  • Use mutex instead of disabling IRQ during critical core
    • This has two downsides: RAM consumption is increased by sizeof(mutex_t) (2 - 4 bytes) and calls to malloc() and friends from IRQ context becomes impossible
    • However: This greatly improves real time properties of the system. IMO a good trade-off
    • assert()s are added to easily detect calls to malloc() from IRQ context
  • Make both cpu/atmega_common and cpu/cortexm_common depend on the new module

Future Steps

  • More platforms like MSP430, RISC-V, MIPS likely should also use this. But let's do this in follow ups. It is only a matter of adding USEMODULE += malloc_thread_safe, but splitting this eases testing.
  • newlib: add thread safe implementation #8619 remains useful for thread-safe implementations of all other system calls.
    • IMHO, thread safety for other system calls than 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.
    • The dependency to malloc_thread_safe could be dropped when newlib_thread_safe is used
    • This malloc 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.)

@maribu 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
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.

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 maribu merged commit c8d16d2 into RIOT-OS:master Dec 17, 2020
@maribu maribu deleted the malloc-newlib-picolibc branch December 17, 2020 18:55
@maribu
Copy link
Member Author

maribu commented Dec 17, 2020

Thanks :-)

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants