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

vm: Copy on write #1392

Merged
merged 54 commits into from
Jul 26, 2023
Merged

vm: Copy on write #1392

merged 54 commits into from
Jul 26, 2023

Conversation

franciscozdo
Copy link
Collaborator

@franciscozdo franciscozdo commented Jun 8, 2023 •
edited
Loading

Implementation of copy on write.

The most important changes:

  • I modify the flow of page fault handling. Now after checking if access to some address was forbidden we have to check if it is possible according to protection bits stored in VM map. There is a case when we handle write access fault because the region of memory was marked read-only due to Copy-on-write.
  • I add vm_anon structure to make it possible to share single pages between two processes amaps.

@franciscozdo franciscozdo added the WiP not ready for code review label Jun 8, 2023
@franciscozdo franciscozdo added review please review this PR and removed WiP not ready for code review labels Jul 1, 2023
@cahirwpz
Copy link
Owner

cahirwpz commented Jul 6, 2023

Could we limit the scope of this PR be moving following changes to another PR?

  • bin/utest/mmap.c
  • sys/debug/virtmem.py
  • sys/tests/utest.c

@franciscozdo
Copy link
Collaborator Author

I have moved the new test to new PR (#1401) and the debugging improvement is already included in (#1396)

pj1031999
pj1031999 reviewed Jul 7, 2023
View reviewed changes
bin/utest/mmap.c Outdated Show resolved Hide resolved
bin/utest/mmap.c Outdated Show resolved Hide resolved
bin/utest/mmap.c Outdated Show resolved Hide resolved
include/sys/vm_amap.h Outdated Show resolved Hide resolved
include/sys/vm_amap.h Outdated Show resolved Hide resolved
sys/kern/vm_amap.c Outdated Show resolved Hide resolved
sys/kern/vm_amap.c Outdated Show resolved Hide resolved
sys/kern/vm_amap.c Outdated Show resolved Hide resolved
sys/kern/vm_amap.c Outdated Show resolved Hide resolved
sys/kern/vm_amap.c Outdated Show resolved Hide resolved
@franciscozdo franciscozdo added the blocked depends on feature that has not been merged label Jul 9, 2023
@franciscozdo
Copy link
Collaborator Author

We should merge #1401 before these changes. I will remove blocked label after it will be ready to merge again.

pj1031999
pj1031999 approved these changes Jul 24, 2023
View reviewed changes
Copy link
Collaborator

@pj1031999 pj1031999 left a comment

Choose a reason for hiding this comment

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

LGTM

cahirwpz
cahirwpz reviewed Jul 24, 2023
View reviewed changes
Copy link
Owner

@cahirwpz cahirwpz left a comment

Choose a reason for hiding this comment

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

Looks much better, but could use another round of cleanups to make it really good and actually ready for merge :)

sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
cahirwpz
cahirwpz reviewed Jul 26, 2023
View reviewed changes
include/sys/vm_amap.h Outdated Show resolved Hide resolved
include/sys/vm_amap.h Outdated Show resolved Hide resolved
include/sys/vm_amap.h Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Outdated Show resolved Hide resolved
sys/kern/vm_map.c Show resolved Hide resolved
cahirwpz
cahirwpz approved these changes Jul 26, 2023
View reviewed changes
Copy link
Owner

@cahirwpz cahirwpz left a comment

Choose a reason for hiding this comment

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

LGTM

include/sys/vm_amap.h Outdated Show resolved Hide resolved
sys/kern/vm_map.c Show resolved Hide resolved
@cahirwpz cahirwpz added accepted accepted by the maintainer for code review and removed review please review this PR labels Jul 26, 2023
@cahirwpz cahirwpz merged commit 8146359 into cahirwpz:master Jul 26, 2023
9 checks passed
@franciscozdo franciscozdo deleted the uvm-cow branch July 27, 2023 20:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pj1031999 pj1031999 pj1031999 approved these changes

@cahirwpz cahirwpz cahirwpz approved these changes

Assignees
No one assigned
Labels
accepted accepted by the maintainer for code review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@franciscozdo @cahirwpz @pj1031999