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

Add newlib retargetable locking implementation and tests #36201

Merged

Conversation

stephanosio
Copy link
Member

@stephanosio stephanosio commented Jun 12, 2021

This series adds the newlib retargetable locking interface function implementations, as well as the tests to verify that the retargetable locking interface is functional and used by the internal lock functions.

This commit adds the newlib retargetable locking interface function
implementations in order to make newlib functions thread safe.

The newlib retargetable locking interface is internally called by the
standard C library functions provided by newlib to synchronise access
to the internal shared resources.

By default, the retargetable locking interface functions defined within
the newlib library are no-op. When multi-threading is enabled (i.e.
`CONFIG_MULTITHREADING=y`), the Zephyr-side retargetable locking
interface implementations override the default newlib implementation
and provide locking mechanism.

The retargetable locking interface may be called with either a static
(`__lock__...`) or a dynamic lock.

The static locks are statically allocated and initialised immediately
after kernel initialisation by `newlib_locks_prepare`.

The dynamic locks are allocated and de-allocated through the
`__retargetable_lock_init[_recursive]` and
`__retarget_lock_close_[recurisve]` functions as necessary by the
newlib functions. These locks are allocated in the newlib heap using
the `malloc` function when userspace is not enabled -- this is safe
because the internal multi-threaded malloc lock implementations
(`__malloc_lock` and `__malloc_unlock`) call the retargetable locking
interface with a static lock (`__lock__malloc_recursive_mutex`). When
userspace is enabled, the dynamic locks are allocated and freed through
`k_object_alloc` and `k_object_release`.

Note that the lock implementations used here are `k_mutex` and `k_sem`
instead of `sys_mutex` and `sys_sem` because the Zephyr kernel does not
currently support dynamic allocation of the latter. These locks should
be updated to use `sys_mutex` and `sys_sem` when the Zephyr becomes
capable of dynamically allocating them in the future.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>

Fixes the "Retarget Locking" portion of #21519.

NOTE1: The reentrancy-related fixes will be addressed in an upcoming (separate) PR.
NOTE2: The tests will fail because the Zephyr SDK newlib is currently compiled with --disable-newlib-multithread. To be merged after zephyrproject-rtos/sdk-ng#344 is mainlined in the CI.

Fixes #21519.

@stephanosio stephanosio added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug area: C Library C Standard Library labels Jun 12, 2021
@stephanosio stephanosio added this to the v2.7.0 milestone Jun 12, 2021
@github-actions github-actions bot added the area: Tests Issues related to a particular existing or missing test label Jun 12, 2021
@carlescufi carlescufi requested a review from dcpleung June 12, 2021 15:27
@stephanosio
Copy link
Member Author

UPDATE:

@carlescufi carlescufi added the area: newlib Newlib C Standard Library label Jun 14, 2021
@stephanosio stephanosio marked this pull request as ready for review July 20, 2021 17:39
@stephanosio stephanosio requested a review from nashif as a code owner July 20, 2021 17:39
@stephanosio
Copy link
Member Author

Marking it as "ready for review" since the PR itself is ready. Please note that the CI failure will be gone as soon as the SDK 0.13.0 is mainlined.

@nashif nashif added the DNM This PR should not be merged (Do Not Merge) label Aug 3, 2021
This commit adds the newlib retargetable locking interface function
implementations in order to make newlib functions thread safe.

The newlib retargetable locking interface is internally called by the
standard C library functions provided by newlib to synchronise access
to the internal shared resources.

By default, the retargetable locking interface functions defined within
the newlib library are no-op. When multi-threading is enabled (i.e.
`CONFIG_MULTITHREADING=y`), the Zephyr-side retargetable locking
interface implementations override the default newlib implementation
and provide locking mechanism.

The retargetable locking interface may be called with either a static
(`__lock__...`) or a dynamic lock.

The static locks are statically allocated and initialised immediately
after kernel initialisation by `newlib_locks_prepare`.

The dynamic locks are allocated and de-allocated through the
`__retargetable_lock_init[_recursive]` and
`__retarget_lock_close_[recurisve]` functions as necessary by the
newlib functions. These locks are allocated in the newlib heap using
the `malloc` function when userspace is not enabled -- this is safe
because the internal multi-threaded malloc lock implementations
(`__malloc_lock` and `__malloc_unlock`) call the retargetable locking
interface with a static lock (`__lock__malloc_recursive_mutex`). When
userspace is enabled, the dynamic locks are allocated and freed through
`k_object_alloc` and `k_object_release`.

Note that the lock implementations used here are `k_mutex` and `k_sem`
instead of `sys_mutex` and `sys_sem` because the Zephyr kernel does not
currently support dynamic allocation of the latter. These locks should
be updated to use `sys_mutex` and `sys_sem` when the Zephyr becomes
capable of dynamically allocating them in the future.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
This commit adds the `static` keyword to the test functions that are
not intended to be globally available.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio stephanosio force-pushed the newlib_retargetable_locking branch 2 times, most recently from e0adc1e to cb4be75 Compare August 4, 2021 07:38
@stephanosio stephanosio removed the DNM This PR should not be merged (Do Not Merge) label Aug 4, 2021
@stephanosio
Copy link
Member Author

SDK 0.13.0 has been mainlined. This PR is ready for final review and merging.

Please note that the checkpatch failure is wrong and should be ignored.

Copy link
Member

@carlescufi carlescufi left a comment

Choose a reason for hiding this comment

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

This looks good to me, with my limited knowledge of newlib's retargetable locking system.

This commit adds the tests for the newlib retargetable locking
interface, as well as the tests for the internal lock functions that
are supposed to internally invoke the retargetable locking interface.

All of these tests must pass when the toolchain newlib is compiled with
the `retargetable-locking` and `multithread` options, which are
required to ensure that the newlib is thread-safe, enabled. If the
toolchain newlib is compiled with either of these options disabled,
this test will fail.

This commit also adds the userspace testcases to ensure that the newlib
is thread-safe in the user mode.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
Copy link
Collaborator

@galak galak left a comment

Choose a reason for hiding this comment

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

We need to update the TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION in the cmakefiles

In order to use the newlib retagetable locking interface (for thread
safety), which requires the newlib multi-threading feature to be
enabled, the Zephyr SDK 0.13.0 or above must be used.

Signed-off-by: Stephanos Ioannidis <root@stephanos.io>
@stephanosio
Copy link
Member Author

We need to update the TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION in the cmakefiles

@galak Done

@stephanosio stephanosio requested a review from galak August 4, 2021 14:28
@cfriedt
Copy link
Member

cfriedt commented Aug 6, 2021

CI says there is a whitespace issue. I'm not sure I see what the problem is though. @galak - could that be a false positive?

@stephanosio
Copy link
Member Author

CI says there is a whitespace issue. I'm not sure I see what the problem is though. @galak - could that be a false positive?

@cfriedt checkpatch has a bug in which it fails to recognise variable declarations with volatile pointers. Yes, it is a false positive.

@galak galak merged commit b973cdc into zephyrproject-rtos:main Aug 9, 2021
@galak
Copy link
Collaborator

galak commented Aug 9, 2021

@cfriedt I went ahead and merged this.

@cfriedt
Copy link
Member

cfriedt commented Aug 9, 2021

@cfriedt I went ahead and merged this.

Thanks - I don't seem to have merge rights to manually merge PR's in case of false positives, but I'm also kind of ok with that.

@stephanosio stephanosio added the Release Notes To be mentioned in the release notes label Aug 16, 2021
saininav added a commit to saininav/meta-zephyr that referenced this pull request Dec 20, 2022
Build newlib library to be thread-safe in multithreaded environment.

zephyrproject-rtos/zephyr#21518
zephyrproject-rtos/zephyr#21519
zephyrproject-rtos/zephyr#36201

https://sourceware.org/legacy-ml/newlib/2016/msg01165.html
https://sourceware.org/git/?p=newlib-cygwin.git;a=commit;h=bd54749095ee45d7136b6e7c8a1e5218749c87b6

Error log:
newlib/libc-hooks.c:310:1: note: in expansion of macro 'BUILD_ASSERT'
BUILD_ASSERT(IS_ENABLED(_RETARGETABLE_LOCKING), "Retargetable locking must be enabled");

Signed-off-by: Naveen Saini <naveen.kumar.saini@intel.com>
Tested-by: Jon Mason <jon.mason@arm.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Build System area: C Library C Standard Library area: newlib Newlib C Standard Library area: Tests Issues related to a particular existing or missing test bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RFC: libc: thread-safe newlib
6 participants