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

Implement frame metadata and refactor most usages of frames #836

Merged
merged 10 commits into from
Jun 3, 2024

Conversation

junyang-zh
Copy link
Collaborator

@junyang-zh junyang-zh commented May 11, 2024

Found a patch named frame_meta_v2.patch on my pants. Seems that it implements the frame metadata.

EDIT: I am trying to make commits in this PR atomic.

  • Implement basic FrameMeta with one counter and use it for VmFrame;
  • Use the metadata to improve the page table;
  • Expose the cursor to the framework user, addressing The APIs of VmSpace are vulnerable to race conditions #681;
  • Remove VmFrameVec;
  • Rename VmFrame to Frame and refactor PAGE_SIZE usages;
  • Refactor VmSegment to Segment;
  • Rename aster_frame::vm to aster_frame::mm;
  • Mark the memory region information in the metadata.

EDIT: I am marking the first commit ready for review. I may continue working on the following commits.

EDIT: The page table is improved with the metadata now. However I didn't observe significant performance improvement and it is even slightly worse (the time mapping 273,255 untracked frames (~1G) at a time in release mode is ~73ms compared to original ~56ms). Only the memory usage is improved. I suspect that the original overhead behaves like a cache from reading PTEs.

EDIT: I refactored the cursor. After heavy manual optimization, the time mapping 273,255 untracked frames in release mode dropped from ~73ms to ~42ms, and is faster than the original ~56ms. Yes!

EDIT: I removed some goals of this PR.

@junyang-zh junyang-zh force-pushed the frame_meta branch 11 times, most recently from 93fa7b0 to ae9fe2d Compare May 15, 2024 14:50
@junyang-zh junyang-zh marked this pull request as ready for review May 15, 2024 14:50
@junyang-zh junyang-zh force-pushed the frame_meta branch 7 times, most recently from 11fea2e to 425a76a Compare May 17, 2024 08:45
@junyang-zh junyang-zh force-pushed the frame_meta branch 9 times, most recently from e03c077 to abc6458 Compare May 19, 2024 07:53
@junyang-zh
Copy link
Collaborator Author

I may post the justification for some utter changes here:

Removal of the VmIo implementation of VmSpace

Efficient Reading/Writing in a virtual memory space can only be done if it is activated. This behavior is not aligned with the definition of VmIo, which don't have this extra requirement. Also it is not actually useful now, removing it is not a big problem. We should provide an efficient R/W abstraction to the user space in the future.

Frame handle as an any-level frame

Well, you can get a handle to a mapped frame while querying over the page table by virtual addresses. If Frames are only base pages, you need an extra enum for returning the results. The current design provides a set of inherently elegant APIs. We can also ensure Frame being mappable if virtual address is aligned. But if use Segment for huge pages, there would be extra checks and the user may not sure if the page is mapped as a huge one.

The level is also encoded in the virtual address of the metadata, so there's no overhead in the handle. The users can always make static assertions of the size of the frame if they are clear about how it is created managed. Although the aster-nix is currently ignorant about huge frames, it also works well and no extra overhead is brought since page_size(1) is a constant and aster-nix can safely regard all of them as base pages (frame.size() == page_size(1) && frame.level == 1 as an invariant).

Removal of VmFrameVec

Honestly the only 2 pro of this is doing batch mapping/IO(R/W). But you have a cursor to do actual batch mapping. The R/W implementation is trivial also. So why not Vec<Frame> when needed?

@junyang-zh junyang-zh force-pushed the frame_meta branch 2 times, most recently from d5b257c to e0bfeaa Compare May 28, 2024 15:24
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page/mod.rs Show resolved Hide resolved
framework/aster-frame/src/mm/page_table/frame.rs Outdated Show resolved Hide resolved
framework/aster-frame/src/mm/page_table/frame.rs Outdated Show resolved Hide resolved
@tatetian
Copy link
Contributor

tatetian commented Jun 1, 2024

Most of the code looks good. But the Page::drop method needs to be revised extensively, I suppose. See my detailed comments above.

@junyang-zh junyang-zh force-pushed the frame_meta branch 4 times, most recently from 8c96390 to 968cfcb Compare June 2, 2024 08:04
@junyang-zh
Copy link
Collaborator Author

junyang-zh commented Jun 2, 2024

Why is adding a copy_handle method more preferable than implementing the Clone trait?

Because there are two behaviors of a clone in the page table context. The "deep copy" is to allocate a new page and recursively copy all the children; while the "shallow copy" copies the handle and increments the reference count. Implementing the Clone trait makes this distinction dubious. So I choose to implement both deep and shallow copy by methods.

And yes. I am skeptical about all code locations where the concept of "handle" is mentioned.

The concept of handle is good but as the confusion it has brought to you I think further comments in the code is needed. Perhaps there would be naming problems. Arc is definitely a handle to the underlying data type, with a good naming. But PageRc(PageTableNodeRc, SegmentRc, ...) seems an awkward copy cat naming. Developers won't use other things than Page to access a page either. So Page as a page handle would seemingly be a nice concept. There may be better alternatives but I haven't thought of one yet.

@junyang-zh
Copy link
Collaborator Author

junyang-zh commented Jun 2, 2024

I think the names forget and restore should be re-considered.

For forgetting and restoring the page handle, the naming is indeed not appropriate. I've renamed them into into_raw and from_raw. They still operate on Paddr for the following reasons: Arc::<T>::from_raw and Arc::<T>::into_raw operates also on *const T rather than *const ArcInner. A Paddr points to a page, which is sound.

Also, the Page::from_raw and Page::into_raw on Paddr are very helpful for page table implementations, since you need to into_raw a page table node and store the Paddr in a PTE/cr3.

For xarray FrameRef implementations, it's now behaving like a transmute, giving out *const MetaSlots.

@junyang-zh junyang-zh force-pushed the frame_meta branch 2 times, most recently from 87402ee to 04ae00e Compare June 2, 2024 12:53
@tatetian
Copy link
Contributor

tatetian commented Jun 3, 2024

I think the names forget and restore should be re-considered.

For forgetting and restoring the page handle, the naming is indeed not appropriate. I've renamed them into into_raw and from_raw. They still operate on Paddr for the following reasons: Arc::<T>::from_raw and Arc::<T>::into_raw operates also on *const T rather than *const ArcInner. A Paddr points to a page, which is sound.

Also, the Page::from_raw and Page::into_raw on Paddr are very helpful for page table implementations, since you need to into_raw a page table node and store the Paddr in a PTE/cr3.

For xarray FrameRef implementations, it's now behaving like a transmute, giving out *const MetaSlots.

I am satisfied with the refactored version.

@junyang-zh junyang-zh force-pushed the frame_meta branch 2 times, most recently from f1265ba to 2093886 Compare June 3, 2024 11:58
In this commit, the frame metadata storage schema is implemented. The bootstrap process is refactored
and a boot page table is introduced to perform early stage metadata mapping. The metadata is then used
to track `VmFrame`s instead of the former `Arc` approach.
This PR also refactored the page table cursor, distinguishing `Cursor` from `CursorMut`, and split
a lot of functions to reduce dynamic condition checking.

There are also other sanitizations performed, including refactoring PTE's `is_huge` API to `is_last`,
hardening tracked mapping checks, and making `VmFrame` any size.
Copy link
Contributor

@tatetian tatetian left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@tatetian tatetian merged commit 232e62b into asterinas:main Jun 3, 2024
7 checks passed
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.

None yet

2 participants