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

Support mapping ranges #266

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Support mapping ranges #266

wants to merge 18 commits into from
+2,034 −0

Conversation

Freax13
Copy link
Member

@Freax13 Freax13 commented Jun 7, 2021

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 and RecursivePageTable 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 use Mapper::map_with_table_flags instead of Mapper::map_to_with_table_flags once #136 is merged.

@josephlr josephlr mentioned this pull request Jul 17, 2021
Open
@phil-opp phil-opp mentioned this pull request Aug 22, 2021
Open
@phil-opp phil-opp self-assigned this Sep 4, 2021
phil-opp
phil-opp reviewed Sep 4, 2021
View reviewed changes
Copy link
Member

@phil-opp phil-opp left a 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?

src/structures/paging/mapper/mod.rs Outdated Show resolved Hide resolved
src/structures/paging/mapper/mod.rs Outdated Show resolved Hide resolved
src/structures/paging/mapper/mod.rs Outdated Show resolved Hide resolved
src/structures/paging/mapper/mod.rs Show resolved Hide resolved
src/structures/paging/mapper/mod.rs Outdated Show resolved Hide resolved
src/structures/paging/mapper/mapped_page_table.rs Show resolved Hide resolved
src/structures/paging/mapper/mapped_page_table.rs Outdated Show resolved Hide resolved
@Freax13
Copy link
Member Author

Freax13 commented Sep 4, 2021

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.

@Freax13
Copy link
Member Author

Freax13 commented Sep 5, 2021

This would be a breaking change, so I should set next as the base branch, correct? Should I still merge master into my branch?

@phil-opp
Copy link
Member

Why do you think that it would be a breaking change?

@josephlr
Copy link
Contributor

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.

Reference: https://doc.rust-lang.org/cargo/reference/semver.html#possibly-breaking-adding-a-defaulted-trait-item

@Freax13
Copy link
Member Author

Freax13 commented Sep 13, 2021

Oops my bad, this isn't a breaking change.

@phil-opp
Copy link
Member

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 master (the recent comments seem to indicate the latter).

@Freax13 Freax13 changed the base branch from next to master October 17, 2021 17:24
@Freax13
Copy link
Member Author

Freax13 commented Oct 17, 2021

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 master (the recent comments seem to indicate the latter).

Thanks for reminding me!

The pr sort of depends on #136 because of Mapper::map_range_with_table_flags: Usually the default implementations for the *_range methods call the non-range versions, but Mapper::map_range_with_table_flags can't do that until #136 adds Mapper::map_with_table_flags. Currently Mapper::map_range_with_table_flags just allocates a frame and calls Mapper::map_to_with_table_flags. Other than that the pr should be ready to merge.

This pr should not be a breaking change regardless of the experimental flag. Adding methods with a default implementation to a trait is not a breaking change.

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).

@phil-opp phil-opp added the waiting-for-review Waiting for a review from the maintainers. label Nov 6, 2021
@Qubasa Qubasa mentioned this pull request Nov 6, 2021
Draft
@Freax13 Freax13 force-pushed the map-ranges branch 2 times, most recently from b50a051 to c10f5a7 Compare December 26, 2021 12:49
@Freax13
Copy link
Member Author

Freax13 commented Dec 26, 2021

I just rebased this with the current master branch and got rid of some commits that were accidentally included from the next branch.

@Freax13
Copy link
Member Author

Freax13 commented Apr 17, 2022

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?

We could also use a cfg flag instead of a feature. This is done in other libraries such as tokio and tracing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@phil-opp phil-opp phil-opp left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@phil-opp phil-opp

Labels
waiting-for-review Waiting for a review from the maintainers.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add Mapper methods to map PageRange to PhysFrameRange
3 participants
@Freax13 @phil-opp @josephlr