Alarm Syscall: fix computed dt when both unshifted reference and dt from userspace have low-order bits.
#4201
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
lschuermann
previously approved these changes
Oct 15, 2024
Comment on lines
-174
to
+177
| expired.reference.wrapping_add(expired.dt).into_usize(), | ||
| expired | ||
| .reference | ||
| .wrapping_add(expired.dt) | ||
| .into_u32_left_justified() as usize, |
There was a problem hiding this comment.
It's unfortunate that we don't yet have a reliable way to test this part of the code. 😞
Computing unshifted dt independently of unshifted reference can lead to too low of a final expiration if reference and dt from userspace both have low-order bits. Instead, compute unshifted dt by first computing the expected expirtion, and right-shifting it to make sure the final reference + dt expires at or after the expected expiration.
lschuermann
previously approved these changes
Oct 15, 2024
Co-authored-by: Leon Schuermann <leon@is.currently.online>
|
@lschuermann last one I promise, just fixes formatting |
lschuermann
approved these changes
Oct 15, 2024
|
Is there anything in terms of stability guarantees that we need to watch out for in this PR? My guess is no: the previous alarm changes (#3975) were almost entirely backwards compatible, except for the callback arguments that was left unscaled, which this PR fixes. Thus, after this PR, we should be back to being compliant to the alarm interface as it's specified and included in the previous release. |
bradjc
approved these changes
Oct 16, 2024
|
Agree, I don't there are anything compatibility issues |
tyler-potyondy
approved these changes
Oct 16, 2024
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.
Pull Request Overview
This pull request fixes two issues in the alarm syscall driver, both necessary to correctly handle alarms that overflow in userspace. Each issue separated into its own commit:
dtfor a userspace alarm when userspace values are left-shifted to be u32 (e.g. on the 24-bit nRF52 RTC implementation) underestimateddtin some cases where bothreferenceanddthad low-order bits that were rounded off. It is necessary to computedtconsidering thereferencevalue when it is provided, so this PR computesdtfrom the computed expiration in the shifted domain.Testing Strategy
The second commit adds test cases for 24-bit alarms with provided reference counters. Running
cargo testwithout the changes in the second commit fail, and succeed with the changes. The first commit is tested via changes to libtock-c in a different PR to that repo.TODO or Help Wanted
It would be good to be able to test upcall values in unit tests, but I decided to avoid figuring out how to do that for this PR since the relevant change is pretty trivially correct, and was unnoticed simply because that value was never used.
Documentation Updated
/docs, or no updates are required.Formatting
make prepush.