tests: posix: semaphores: ensure test is not skipped #88769
Merged
+18
−18
Conversation
This file contains hidden or 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
cfriedt
added
the
area: Tests
ycsin
previously approved these changes
Apr 17, 2025
| @@ -3,3 +3,7 @@ CONFIG_ZTEST=y | |||
|
|
|||
| CONFIG_POSIX_AEP_CHOICE_BASE=y | |||
| CONFIG_POSIX_SEMAPHORES=y | |||
|
|
|||
| CONFIG_DYNAMIC_THREAD=y | |||
| CONFIG_DYNAMIC_THREAD_POOL_SIZE=2 | |||
There was a problem hiding this comment.
Same as #88767, we can make it bulletproof by BUILD_ASSERTing CONFIG_DYNAMIC_THREAD_POOL_SIZE>=2
cfriedt
force-pushed
the
tests-posix-semaphors-skipped
branch
from
650aa24 to
c1df1b3
Compare
ycsin
previously approved these changes
Apr 18, 2025
jukkar
previously approved these changes
Apr 19, 2025
JordanYates
previously approved these changes
Apr 19, 2025
|
This needs another change unfortunately - it should be visible after a rebase, but due to #88538, the default heap-add with I'll add another commit ahead of this one to create a heap-add for posix semaphores. The above failure can be traced to zephyr/lib/posix/options/semaphore.c Line 275 in e41909a |
cfriedt
dismissed stale reviews from JordanYates, jukkar, and ycsin
via
4b5ae9e
cfriedt
force-pushed
the
tests-posix-semaphors-skipped
branch
2 times, most recently
from
4b5ae9e to
61f1411
Compare
|
|
@nashif - this should reduce test execution time for semaphores by a good amount (for cortex-a53, and likely x86-64 as well) |
cfriedt
force-pushed
the
tests-posix-semaphors-skipped
branch
from
61f1411 to
4e6a92d
Compare
The implementation of POSIX_SEMAPHORES historically used heap allocation and has not yet been transitioned to a pool allocator. However, since 590258b, the default heap-add with CONFIG_POSIX_API has been reduced from 1 kiB which causes tests/posix/semaphores to fail due to NULL being returned from a call to k_calloc(). Create a minimal heap-add for the POSIX_SEMAPHORES Option Group. This can be removed at a future date if semaphores are changed to use a pooled allocator and fixed-size name, rather than heap allocation. Signed-off-by: Chris Friedt <[email protected]>
cfriedt
force-pushed
the
tests-posix-semaphors-skipped
branch
2 times, most recently
from
bec2865 to
e462778
Compare
|
ycsin
reviewed
Apr 20, 2025
| @@ -51,11 +54,10 @@ static void semaphore_test(sem_t *sem) | |||
|
|
|||
| zassert_equal(clock_gettime(CLOCK_REALTIME, &abstime), 0, "clock_gettime failed"); | |||
|
|
|||
| abstime.tv_sec += 5; | |||
| abstime.tv_sec += WAIT_TIME_MS / MSEC_PER_SEC; | |||
There was a problem hiding this comment.
Is this supposed to be in the second commit?
There was a problem hiding this comment.
Yeah, probably not.
I've actually been working on a timespec_util.h header as well that includes a few useful functions. That's been long overdue.
There is no reason to use a 5s wait time in tests. This can add up quite significantly across multiple platforms that execute in real-time under qemu (or even just on real hardware). Speedup observed with qemu_cortex_a53/qemu_cortex_a53/smp Before: ``` START - test_named_semaphore PASS - test_named_semaphore in 5.688 seconds ``` After: ``` START - test_named_semaphore PASS - test_named_semaphore in 0.783 seconds ``` Signed-off-by: Chris Friedt <[email protected]>
Just a formatting change, in a separate commit. Signed-off-by: Chris Friedt <[email protected]>
Commit f7633a5 moved the tests for the POSIX_SEMAPHORES Option Group from the tests/posix/common testsuite to its own dedicated testsuite. However, there was a copy-paste error. Previously, tests would have been run only once when dynamic threads were enabled, and then skipped when dynamic threads were disabled, since that follows the posix programming model better. However, dynamic threads were never actually enabled after moving to the new testsuite. So all tests were effectively skipped. Add the necessary options to prj.conf in order to ensure that there are sufficient dynamic threads available to run the testsuite. Signed-off-by: Chris Friedt <[email protected]>
cfriedt
force-pushed
the
tests-posix-semaphors-skipped
branch
from
e462778 to
a6a106c
Compare
jukkar
approved these changes
Apr 22, 2025
ycsin
approved these changes
Apr 22, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Commit f7633a5 moved the tests for the
POSIX_SEMAPHORESOption Group from thetests/posix/commontestsuite totests/posix/semaphores.However, there was a copy-paste error. Previously, tests would have been run only once when dynamic threads were enabled, and then skipped when dynamic threads were disabled, since that follows the posix programming model better. However, dynamic threads were never actually enabled after moving to the new testsuite. So all tests were effectively skipped.
Add the necessary options to prj.conf in order to ensure that there are sufficient dynamic threads available to run the testsuite.
Fixes #88768