-
Notifications
You must be signed in to change notification settings - Fork 133
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
Support mapping ranges #266
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for this PR (and like I said in #264, I'm very sorry for the long delay)!
This adds quite a lot of complex code. I tried to look through it in detail, but it's difficult to spot every possible bug without extensive testing. Ideally, we should add a lot of unit tests to ensure that everything is correct, but I understand that this would be a lot of work.
My proposal is the following: We merge this PR, but hide all the new methods behind an optional cargo feature named experimental
. We then document clearly that the methods are only sparsely tested and might still contain bugs. We should also link to a tracking issue where users can report bugs, but also positive feedback (if everything works as intended). What do you think about this approach?
Sounds good to me. |
This would be a breaking change, so I should set |
Why do you think that it would be a breaking change? |
It adds methods to a trait, but they all have default implementations, so I think this is a minor non-breaking change. Unless I'm missing something. |
Oops my bad, this isn't a breaking change. |
What's the status of this? In the PR description you mention that this depends on #136. Is this still true? Also, should this go into 0.15 or is this a non-breaking change that can be merged directly into |
Thanks for reminding me! The pr sort of depends on #136 because of This pr should not be a breaking change regardless of the I just rebased the branch onto master, I'd appreciate it if you could double check whether I did this correctly (I already fixed a few mistakes). |
b50a051
to
c10f5a7
Compare
I just rebased this with the current master branch and got rid of some commits that were accidentally included from the next branch. |
We could also use a cfg flag instead of a feature. This is done in other libraries such as |
This pr adds various methods for modifying a range of pages:
Mapper::map_to_range
Mapper::map_to_range_with_table_flags
Mapper::map_range
Mapper::map_range_with_table_flags
Mapper::unmap_range
Mapper::update_flags_range
Mapper::identity_map_range
All of those functions have a default implementation that just calls the non-ranged version for each page.
MappedPageTable
,OffsetPageTable
andRecursivePageTable
have specialized implementations that avoid looking up the P1-P3 tables multiple times which should improve performance (I haven't run any benchmarks to verify this, the kernel I used to test the pr doesn't work with timers yet lol).Closes #192
Sort of depends on #136: The default implementation for
Mapper::map_range_with_table_flags
should useMapper::map_with_table_flags
instead ofMapper::map_to_with_table_flags
once #136 is merged.