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

userlib: add a critical-section impl for task code. #1646

Merged
merged 2 commits into from
Mar 18, 2024

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Mar 6, 2024

The embedded ecosystem is starting to catch up with the fact that not all of us are running a single address space in privileged mode, and crates like portable-atomics may now be in a state where we could rely on them.

Implementing things like atomic polyfills on platforms without atomics requires some way to control concurrency, and crates seem to be standardizing on the critical-section crate -- which unlike the old critical section in cortex-m, can be customized for our purposes.

So, this commit adds a backing implementation to userlib to allow the critical-section crate to be used in tasks. I'm slightly alarmed to note that our dependency graph already includes a very old version of critical-section, but I'm pretty sure that should be safe since we've never provided an impl for it. It is presumably not being linked in.

I need to do more work to evaluate the code generation of crates like portable-atomics, but this part, at least, I think we can probably land.

@hawkw hawkw self-requested a review March 6, 2024 20:21
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

license-eye has totally checked 508 files.

Valid Invalid Ignored Fixed
507 1 0 0
Click to see the invalid file list
  • sys/userlib/src/critical_section.rs

sys/userlib/src/critical_section.rs Show resolved Hide resolved
Copy link
Member

@hawkw hawkw 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 --- i'm excited for the subsequent PR to get rid of armv6m-atomic-hack!

sys/userlib/src/critical_section.rs Outdated Show resolved Hide resolved
sys/userlib/src/critical_section.rs Show resolved Hide resolved
@hawkw
Copy link
Member

hawkw commented Mar 6, 2024

it occurs to me that a very nice side benefit of Hubris' use of separate compilation for tasks is that we can allow an individual task to opt in/out of global userlib behaviors using Cargo feature flags --- if we had instead implemented tasks as a bunch of crates that were all dependencies of a single top-level build, feature unification would mean that one task opting out of the default critical_section impl wouldn't work, as other tasks would still be enabling the feature flag. it might be worth discussing this in the benefits of the separate compilation model in docs/talks!

@cbiffle
Copy link
Collaborator Author

cbiffle commented Mar 18, 2024

it might be worth discussing this in the benefits of the separate compilation model in docs/talks!

@hawkw fwiw, this is also why we tend to appear in discussions of changing Cargo's feature unification behavior -- this is great behavior for our purposes, but is really annoying for some other people!

Explaining the use case publicly would probably help, you're right.

@cbiffle cbiffle enabled auto-merge (rebase) March 18, 2024 23:18
@cbiffle cbiffle merged commit 9ad7afe into master Mar 18, 2024
83 checks passed
@cbiffle cbiffle deleted the cbiffle/critical-section branch March 18, 2024 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants