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.

Sign up for GitHub

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

Jump to bottom

posix: use sys_sem instead of k_spinlock for pool synch #71832

Merged
merged 4 commits into from
Oct 3, 2024
Merged

posix: use sys_sem instead of k_spinlock for pool synch #71832

merged 4 commits into from
Oct 3, 2024
+298 −199

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Apr 23, 2024 •
edited
Loading

This is a repeat of #71718.

changelog:

  • initialize a variable used in a print statement that was not previously initialized
  • initialize all return values to be robust even in the case that assertions are disabled and the statically declared sys_sem (extremely improbably) fails to be taken
  • use a common platform_key field and do away with integration_platforms for userspace tests, providing easily scalable test coverage across architectures

Fixes #79278

@zephyrbot zephyrbot added area: Base OS Base OS Library (lib/os) area: Coding Guidelines Coding guidelines and style area: POSIX POSIX API Library labels Apr 23, 2024
@cfriedt cfriedt force-pushed the posix-use-sys-sem-for-pools branch 2 times, most recently from a69dcc7 to d4b9019 Compare April 23, 2024 17:49
@cfriedt cfriedt force-pushed the posix-use-sys-sem-for-pools branch 2 times, most recently from 5ece3aa to b25d612 Compare April 29, 2024 12:57
@cfriedt
Copy link
Member Author

cfriedt commented Apr 29, 2024

  • rebased: ci errors seemed to be transient, cannot repro on latest main

@cfriedt cfriedt force-pushed the posix-use-sys-sem-for-pools branch from b25d612 to b32cb40 Compare April 30, 2024 05:59
andyross
andyross previously approved these changes May 1, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Still +1 from me. Blocking IPC definitely seems like a better choice here.

@cfriedt
Copy link
Member Author

cfriedt commented May 1, 2024

I enabled userspace in the test suite so there are a bunch of fun new corner cases, hehe.

Will get to them shortly. It makes things a bit more interesting at least, but it should be more robust in the end, which is good.

@cfriedt cfriedt mentioned this pull request Jun 18, 2024
Open
@github-actions GitHub Actions
Copy link

github-actions bot commented Jul 1, 2024

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Jul 1, 2024
@cfriedt cfriedt removed the Stale label Jul 1, 2024
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Aug 31, 2024
@cfriedt cfriedt marked this pull request as draft August 31, 2024 08:59
@cfriedt cfriedt removed the Stale label Aug 31, 2024
Chris Friedt added 3 commits October 1, 2024 07:07
sys_sem: add a for-loop-like macro similar to spinlocks
52c9875 
Add SYS_SEM_LOCK() and SYS_SEM_LOCK_BREAK which are mostly the
same as K_SPINLOCK() and K_SPINLOCK_BREAK.

Signed-off-by: Chris Friedt <[email protected]>
clang-format: add entry for SYS_SEM_LOCK()
3966346 
SYS_SEM_LOCK() is a for-loop-like macro similar inspired by
K_SPINLOCK().

Add an entry to .clang-format so that autoformatting works as
expected.

Signed-off-by: Chris Friedt <[email protected]>
posix: use sys_sem instead of k_spinlock for pool synch
7eb0b85 
Based on Andy's talk at eoss 2024, use the sys/sem.h api instead
of the spinlock.h api to synchronize pooled elements since it
has minimal overhead like semaphores but also works from
userspace.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the posix-use-sys-sem-for-pools branch from b32cb40 to 60c8a88 Compare October 1, 2024 11:10
tests: posix: common: test userspace as well
ebf29ab 
Ensure that userspace is tested in the common posix testsuite.

Signed-off-by: Chris Friedt <[email protected]>
@cfriedt cfriedt force-pushed the posix-use-sys-sem-for-pools branch from 60c8a88 to ebf29ab Compare October 1, 2024 13:03
@cfriedt cfriedt added backport v3.7-branch Request backport to the v3.7-branch backport v3.6-branch Request backport to the v3.6-branch and removed backport v3.6-branch Request backport to the v3.6-branch backport v3.7-branch Request backport to the v3.7-branch labels Oct 1, 2024
@cfriedt
Copy link
Member Author

cfriedt commented Oct 1, 2024

Will get to them shortly.

5 months and 2 releases later 😅

There was maybe a minor time warp, but I hope that @andyross will re-approve 🙏 😁

@cfriedt cfriedt marked this pull request as ready for review October 1, 2024 14:51
@cfriedt cfriedt requested a review from andyross October 1, 2024 14:52
@zephyrbot zephyrbot requested a review from simhein October 1, 2024 14:52
andyross
andyross approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Still looks good here

fgrandel
fgrandel approved these changes Oct 1, 2024
View reviewed changes
Copy link
Contributor

@fgrandel fgrandel left a comment

Choose a reason for hiding this comment

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

Nice.

include/zephyr/sys/sem.h
* @param sem Semaphore (@ref sys_sem) used to guard the enclosed code block.
*/
#define SYS_SEM_LOCK(sem) \
for (int __rc SYS_SEM_LOCK_ONEXIT = sys_sem_take((sem), K_FOREVER); ({ \
Copy link
Contributor

Choose a reason for hiding this comment

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

@andyross Your pattern is getting popular. :-) Maybe we need some macro soon to generate it w/o repeating ourselves?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, though (and I know I introduced it), I worry about the "deadlock on break/continue" problem with non-GNU toolchains, which do exist. We don't really have a clean answer for how to prevent/detect that misuse. When it was just the one helper macro in the scheduler it seemed maintainable, but now that it's everywhere I'm getting cold feet and mostly hoping someone else decides what to do :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that an LLVM issue too then? And is the root cause use of statement expressions?

ycsin
ycsin approved these changes Oct 1, 2024
View reviewed changes
Copy link
Member

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

@fabiobaltieri fabiobaltieri merged commit d11a0c2 into zephyrproject-rtos:main Oct 3, 2024
34 checks passed
@cfriedt cfriedt deleted the posix-use-sys-sem-for-pools branch October 3, 2024 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyross andyross andyross approved these changes

@aescolar aescolar aescolar left review comments

@fgrandel fgrandel fgrandel approved these changes

@ycsin ycsin ycsin approved these changes

@dcpleung dcpleung Awaiting requested review from dcpleung

@peter-mitsis peter-mitsis Awaiting requested review from peter-mitsis

@carlescufi carlescufi Awaiting requested review from carlescufi

@jfischer-no jfischer-no Awaiting requested review from jfischer-no

@keith-zephyr keith-zephyr Awaiting requested review from keith-zephyr

@nashif nashif Awaiting requested review from nashif

@simhein simhein Awaiting requested review from simhein

Assignees

@cfriedt cfriedt

Labels
area: Base OS Base OS Library (lib/os) area: Coding Guidelines Coding guidelines and style area: POSIX POSIX API Library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

posix: key, mutex, thread, rwlock use spinlocks (possibly from userspace)
7 participants
@cfriedt @fgrandel @andyross @ycsin @aescolar @fabiobaltieri @zephyrbot