-
Notifications
You must be signed in to change notification settings - Fork 14
Fix vmmap change_prot fragmentation and alignment bugs #513
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
Fix vmmap change_prot fragmentation and alignment bugs #513
Conversation
- Refactor change_prot to use two-phase approach avoiding fragmentation - Add optimization to skip splitting when protection unchanged - Fix find_map_space_with_hint alignment calculation - Addresses all 8 failing unit tests from issue #498
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
This is great, thank you @ChinmayShringi Could we possibly add a full system integration test to confirm this as well? |
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
CI test failed. Should we fix this before merge? |
I'll rerun this but I'm 99% certain that it's unrelated to this PR and due to this: #489 |
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| size_t large_size = 100 * PAGESIZE; | ||
| unsigned char *large = mmap(NULL, large_size, PROT_READ | PROT_WRITE, | ||
| MAP_PRIVATE | MAP_ANONYMOUS, -1, 0); | ||
| if (large == MAP_FAILED) { |
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.
think all of these checking statements would be better off as asserts across all these tests
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
End-to-End Test ReportTest PreviewTest Report Deterministic TestsSummary
Test Results by Category
Non Deterministic TestsSummary
Test Results by Category
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
rennergade
left a comment
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.
this looks great to me now. approved.
Purpose
Fixes vmmap memory protection change logic and alignment calculation bugs that caused 8 unit test failures.
Issues
Fixes #498
Changes
1. Fixed
change_protFunctionThe previous implementation modified entries in-place during iteration while scheduling overlapping intervals for insertion, causing excessive fragmentation.
Root cause:
ent_startbut not the actual interval tree entryinsert_overwritecallsSolution:
page_numandnpagesfields for each split part2. Fixed
find_map_space_with_hintAlignmentThe previous implementation calculated
aligned_end - rounded_num_pageswhich did not guarantee start alignment.Solution:
rounded_num_pagesis already a multiple ofpages_per_mapaligned_end_pageis also a multiple, their difference is guaranteed to be alignedTesting
All 8 previously failing tests now pass:
test_change_prot_end_of_regiontest_change_prot_exact_boundariestest_change_prot_middle_of_regiontest_change_prot_multiple_timestest_change_prot_spanning_multiple_regionstest_change_prot_to_nonetest_change_prot_to_same_valuetest_find_map_space_with_hint_alignment_in_pages