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

kerncore: Add tests for bad or uncontained memory contained in a contiguous slice #1747

Merged

Conversation

aapoalas
Copy link
Contributor

@aapoalas aapoalas commented Apr 11, 2024

The recent Oxide and Friends episode mentioned this memory region spanning slices issue, and the code was interesting. I thought I saw a chance to optimise it by omitting the start address containment checking once the start region is found. (Based on benchmarking against the test impls, it isn't faster.) The obvious issue with that was of course validity of the intermediate regions but I realised that wouldn't be a problem since I still need to check the validity of each region between start and end.

I figured I'd write tests to show that the alternative solution works but then realised the actual issue: My "optimised" algorithm allows for uncontained (unmapped?) memory to be accessed as long as it is found between two good regions. I presume this makes my improvement worthless, but at least the tests are worthwhile additions. The second added test shows the error in my idea.

I've still left the original modification in this PR's commits to at least show what the idea was. There may still be some worth to it.

Side note: The idea for the two loops came from Vec::retain_mut's code which uses a similar logic to find the first item that isn't retained.

@aapoalas
Copy link
Contributor Author

After running some benchmarks, my version does not outperform the original loop even in its best-case scenario which is also probably highly unlikely: A long slice spanning over multiple contiguous good regions.

I'll revert the algorithm changes from the PR and leave behind the tests.

cbiffle added a commit to aapoalas/hubris that referenced this pull request Apr 11, 2024
@cbiffle
Copy link
Collaborator

cbiffle commented Apr 11, 2024

Thanks for the tests! (And the experimentation on the algorithm, even if it wasn't clearly a win just yet.)

I've applied a couple small fixes and intend to merge this.

cbiffle added a commit to aapoalas/hubris that referenced this pull request Apr 11, 2024
@cbiffle cbiffle force-pushed the kern-add-interleaved-bad-memory-test branch from ba6922e to 685b128 Compare April 11, 2024 23:00
@cbiffle cbiffle force-pushed the kern-add-interleaved-bad-memory-test branch from 685b128 to 826d42c Compare April 11, 2024 23:02
@cbiffle cbiffle merged commit e21558e into oxidecomputer:master Apr 11, 2024
103 checks passed
@aapoalas aapoalas deleted the kern-add-interleaved-bad-memory-test branch April 12, 2024 03:18
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