serenity icon indicating copy to clipboard operation
serenity copied to clipboard

Kernel/Memory: Make mmap objects track dirty and clean pages

Open brody-qq opened this issue 1 year ago • 2 comments

This PR fixes this issue https://github.com/SerenityOS/serenity/issues/15951

  • Pages of file mmap regions with no writes to them are marked as clean
  • Writing to a clean page will mark the page as dirty
  • When msync() is called:
    • Only dirty pages are flushed to storage
    • The flushed pages are now synced with storage, so they are marked clean and remapped
    • NOTE: This only applies to SharedInodeVMObjects
  • When purge() is called, only clean pages are evicted from memory

This PR also adds a few more test cases for private and shared file mmaps

brody-qq avatar Jun 12 '24 16:06 brody-qq

A lot of my testing was done by running the following 2 programs in userspace and checking the debug logs for anything unusual:

  • https://github.com/brody-qq/serenity/blob/debug-mmap-dirty-pages/Userland/Utilities/test_mmap_dirty_pages.cpp
  • https://github.com/brody-qq/serenity/blob/debug-mmap-dirty-pages/Userland/Utilities/test_mmap_dirty_pages_parallel.cpp

If anyone has advice on better ways to test this I would appreciate it!

brody-qq avatar Jun 12 '24 16:06 brody-qq

I updated this to remove a FIXME comment that is fixed by this PR

--- a/Kernel/Memory/Region.cpp
+++ b/Kernel/Memory/Region.cpp
@@ -56,10 +56,8 @@ Region::Region(VirtualRange const& range, NonnullLockRefPtr<VMObject> vmobject,
 
 Region::~Region()
 {
-    if (is_writable() && vmobject().is_shared_inode()) {
-        // FIXME: This is very aggressive. Find a way to do less work!
+    if (is_writable() && vmobject().is_shared_inode())
         (void)static_cast<SharedInodeVMObject&>(vmobject()).sync_before_destroying();
-    }
 
     m_vmobject->remove_region(*this);

brody-qq avatar Jun 20 '24 03:06 brody-qq