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
@stephanosio stephanosio force-pushed the newlib_retargetable_locking branch from 246d3a1 to f282bc4 Compare Jun 12, 2021
@carlescufi carlescufi requested a review from dcpleung Jun 12, 2021
lib/libc/newlib/libc-hooks.c Outdated Show resolved Hide resolved
@stephanosio stephanosio requested a review from ioannisg Jun 14, 2021
@stephanosio stephanosio force-pushed the newlib_retargetable_locking branch from f282bc4 to 484d1c2 Compare Jun 14, 2021
@stephanosio
Copy link
Member Author

@stephanosio stephanosio commented Jun 14, 2021

UPDATE:

@stephanosio stephanosio force-pushed the newlib_retargetable_locking branch from 484d1c2 to c59d1c2 Compare Jun 14, 2021
@carlescufi carlescufi added the area: newlib Newlib C Standard Library label Jun 14, 2021
@stephanosio stephanosio marked this pull request as ready for review Jul 20, 2021
@stephanosio stephanosio requested a review from nashif as a code owner Jul 20, 2021
@stephanosio
Copy link
Member Author

@stephanosio stephanosio commented Jul 20, 2021

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
stephanosio added 2 commits Aug 4, 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 Aug 4, 2021
@stephanosio stephanosio removed the DNM This PR should not be merged (Do Not Merge) label Aug 4, 2021
@stephanosio
Copy link
Member Author

@stephanosio stephanosio commented Aug 4, 2021

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

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>
@stephanosio stephanosio force-pushed the newlib_retargetable_locking branch from cb4be75 to 712e605 Compare Aug 4, 2021
Copy link
Contributor

@galak galak left a comment

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

@stephanosio stephanosio commented Aug 4, 2021

We need to update the TOOLCHAIN_ZEPHYR_MINIMUM_REQUIRED_VERSION in the cmakefiles

@galak Done

@stephanosio stephanosio requested a review from galak Aug 4, 2021
galak
galak approved these changes Aug 4, 2021
@cfriedt
Copy link
Collaborator

@cfriedt 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

@stephanosio stephanosio commented Aug 7, 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?

@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
16 of 17 checks passed
@galak
Copy link
Contributor

@galak galak commented Aug 9, 2021

@cfriedt I went ahead and merged this.

@cfriedt
Copy link
Collaborator

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

6 participants