Skip to content

Memory manager mmap#931

Merged
sporksmith merged 7 commits intoshadow:devfrom
sporksmith:memory-manager-mmap
Aug 20, 2020
Merged

Memory manager mmap#931
sporksmith merged 7 commits intoshadow:devfrom
sporksmith:memory-manager-mmap

Conversation

@sporksmith
Copy link
Copy Markdown
Contributor

@sporksmith sporksmith commented Aug 18, 2020 •
edited
Loading

  • Updates our understanding of the plugin's memory space; all accesses
    should now be in a region we know about.
  • Moves anonymous mappings into our shared memory file for fast access.
    Notably allocators typically use such mappings for large allocation
    requests (e.g. 100 MB).

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 18, 2020 •
edited
Loading

Codecov Report

Merging #931 into dev will increase coverage by 0.34%.
The diff coverage is 69.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##              dev     #931      +/-   ##
==========================================
+ Coverage   54.56%   54.90%   +0.34%     
==========================================
  Files         132      132              
  Lines       18637    18900     +263     
  Branches     4716     4782      +66     
==========================================
+ Hits        10169    10377     +208     
+ Misses       5375     5374       -1     
- Partials     3093     3149      +56
Flag Coverage Δ
#tests 54.90% <69.00%> (+0.34%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/main/host/descriptor/file.c 25.28% <0.00%> (+0.49%) ⬆️
src/main/host/thread.rs 85.71% <0.00%> (+3.79%) ⬆️
src/shim/preload_syscalls.c 17.64% <50.00%> (+1.07%) ⬆️
src/main/host/memory_manager.rs 74.59% <67.04%> (-3.52%) ⬇️
src/test/memory/test_mmap.c 67.85% <73.52%> (+17.85%) ⬆️
src/main/host/syscall/mman.c 61.61% <75.75%> (+0.07%) ⬆️
src/main/utility/interval_map.rs 79.57% <83.33%> (+0.33%) ⬆️
src/main/host/syscall_handler.c 50.25% <100.00%> (+0.50%) ⬆️
src/main/utility/proc_maps.rs 65.16% <100.00%> (ø)
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8eefa1d...fcd99e0. Read the comment docs.

@sporksmith sporksmith force-pushed the memory-manager-mmap branch 2 times, most recently from ed11df8 to ef071b7 Compare August 18, 2020 21:56
@sporksmith sporksmith marked this pull request as ready for review August 18, 2020 22:04
@sporksmith sporksmith linked an issue Aug 18, 2020 that may be closed by this pull request
Make thread_getWriteablePointer and thread_getReadablePointer zero-copy #888
Closed
stevenengler
stevenengler approved these changes Aug 19, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I won't claim to understand everything that's going on here, but looks okay to me :) My only suggestion is maybe a comment for handle_mutations describing its purpose. Edit: And now that the code is fairly large, I think maybe a top-level comment describing the MemoryManager (what it does and what it's purpose is) would also be helpful.

@robgjansen
Copy link
Copy Markdown
Member

robgjansen commented Aug 19, 2020

Reminder: maybe add mmap2 to this PR.

@sporksmith
Tell clang-format to leave bindings.h alone
5fe0a3e
@sporksmith
Handle mmap, munmap, and mremap, updating internal mappings
b0ff0c9 
* Updates our understanding of the plugin's memory space; all accesses
should now be in a region we know about.
* Moves anonymous mappings into our shared memory file for fast access.
Notably allocators typically use such mappings for large allocation
requests (e.g. 100 MB).
@sporksmith
Document handle_mutations
e462fc7
@sporksmith
Extend mmap tests
fcb248b 
* Add test for mremap clobbering part of an existing mapping
* Validate that shadow can access the effected memory
@sporksmith sporksmith force-pushed the memory-manager-mmap branch from ef071b7 to 59fb191 Compare August 20, 2020 14:31
@sporksmith sporksmith force-pushed the memory-manager-mmap branch from be12695 to fcd99e0 Compare August 20, 2020 15:35
@sporksmith
Copy link
Copy Markdown
Contributor Author

sporksmith commented Aug 20, 2020

When adding the requested documentation for handle_mutations I realized there was a bug in the mremap handling. I extended the tests to more thoroughly cover mremap and ended up changing it a fair bit.

I also added support support and testing for mmap2.

Could you take another look? Normally GH has an option to view the diff since your last review, but I might have broken that by rebasing on the latest dev; if so, you should still be able to manually select the range of commits to look at (you want "Document handle_mutations" onward)

stevenengler
stevenengler approved these changes Aug 20, 2020
View reviewed changes
Copy link
Copy Markdown
Contributor

@stevenengler stevenengler left a comment

Choose a reason for hiding this comment

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

Looks okay to me.

@sporksmith sporksmith merged commit 99ec943 into shadow:dev Aug 20, 2020
@sporksmith sporksmith deleted the memory-manager-mmap branch August 20, 2020 16:48
@sporksmith sporksmith mentioned this pull request Aug 20, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stevenengler stevenengler stevenengler approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

Minimum viable prototype of process-b...

Development

Successfully merging this pull request may close these issues.

Make thread_getWriteablePointer and thread_getReadablePointer zero-copy

3 participants

@sporksmith @robgjansen @stevenengler