Bladeren bron

Make font loading use mmap().

This exposed a serious race condition in page_in_from_inode().
Reordered the logic and added a paging lock to VMObject.
Now, only one process can page in from a VMObject at a time.
There are definitely ways to optimize this, for instance by making
the locking be per-page instead. It's not something that I'm going
to worry about right now though.
Andreas Kling 6 jaren geleden
bovenliggende
commit
abe3f515b1
4 gewijzigde bestanden met toevoegingen van 59 en 33 verwijderingen
  1. 29 10
      Kernel/MemoryManager.cpp
  2. 2 0
      Kernel/MemoryManager.h
  3. 26 23
      SharedGraphics/Font.cpp
  4. 2 0
      SharedGraphics/Font.h

+ 29 - 10
Kernel/MemoryManager.cpp

@@ -301,30 +301,49 @@ bool MemoryManager::page_in_from_inode(Region& region, unsigned page_index_in_re
     auto& vmo = region.vmo();
     auto& vmo = region.vmo();
     ASSERT(!vmo.is_anonymous());
     ASSERT(!vmo.is_anonymous());
     ASSERT(vmo.inode());
     ASSERT(vmo.inode());
-    auto& inode = *vmo.inode();
+
     auto& vmo_page = vmo.physical_pages()[region.first_page_index() + page_index_in_region];
     auto& vmo_page = vmo.physical_pages()[region.first_page_index() + page_index_in_region];
-    ASSERT(vmo_page.is_null());
-    vmo_page = allocate_physical_page(ShouldZeroFill::No);
-    if (vmo_page.is_null()) {
-        kprintf("MM: page_in_from_inode was unable to allocate a physical page\n");
-        return false;
+
+    bool interrupts_were_enabled = are_interrupts_enabled();
+
+    if (!interrupts_were_enabled)
+        sti();
+
+    LOCKER(vmo.m_paging_lock);
+
+    if (!interrupts_were_enabled)
+        cli();
+
+    if (!vmo_page.is_null()) {
+        kprintf("MM: page_in_from_inode was served by someone else while lock was held\n");
+        remap_region_page(region, page_index_in_region, true);
+        return true;
     }
     }
-    remap_region_page(region, page_index_in_region, true);
-    byte* dest_ptr = region.laddr().offset(page_index_in_region * PAGE_SIZE).as_ptr();
+
 #ifdef MM_DEBUG
 #ifdef MM_DEBUG
     dbgprintf("MM: page_in_from_inode ready to read from inode, will write to L%x!\n", dest_ptr);
     dbgprintf("MM: page_in_from_inode ready to read from inode, will write to L%x!\n", dest_ptr);
 #endif
 #endif
     sti(); // Oh god here we go...
     sti(); // Oh god here we go...
-    auto nread = inode.read_bytes(vmo.inode_offset() + ((region.first_page_index() + page_index_in_region) * PAGE_SIZE), PAGE_SIZE, dest_ptr, nullptr);
+    byte page_buffer[PAGE_SIZE];
+    auto& inode = *vmo.inode();
+    auto nread = inode.read_bytes(vmo.inode_offset() + ((region.first_page_index() + page_index_in_region) * PAGE_SIZE), PAGE_SIZE, page_buffer, nullptr);
     if (nread < 0) {
     if (nread < 0) {
         kprintf("MM: page_in_from_inode had error (%d) while reading!\n", nread);
         kprintf("MM: page_in_from_inode had error (%d) while reading!\n", nread);
         return false;
         return false;
     }
     }
     if (nread < PAGE_SIZE) {
     if (nread < PAGE_SIZE) {
         // If we read less than a page, zero out the rest to avoid leaking uninitialized data.
         // If we read less than a page, zero out the rest to avoid leaking uninitialized data.
-        memset(dest_ptr + nread, 0, PAGE_SIZE - nread);
+        memset(page_buffer + nread, 0, PAGE_SIZE - nread);
     }
     }
     cli();
     cli();
+    vmo_page = allocate_physical_page(ShouldZeroFill::No);
+    if (vmo_page.is_null()) {
+        kprintf("MM: page_in_from_inode was unable to allocate a physical page\n");
+        return false;
+    }
+    remap_region_page(region, page_index_in_region, true);
+    byte* dest_ptr = region.laddr().offset(page_index_in_region * PAGE_SIZE).as_ptr();
+    memcpy(dest_ptr, page_buffer, PAGE_SIZE);
     return true;
     return true;
 }
 }
 
 

+ 2 - 0
Kernel/MemoryManager.h

@@ -77,6 +77,7 @@ private:
 };
 };
 
 
 class VMObject : public Retainable<VMObject> {
 class VMObject : public Retainable<VMObject> {
+    friend class MemoryManager;
 public:
 public:
     static RetainPtr<VMObject> create_file_backed(RetainPtr<Inode>&&, size_t);
     static RetainPtr<VMObject> create_file_backed(RetainPtr<Inode>&&, size_t);
     static RetainPtr<VMObject> create_anonymous(size_t);
     static RetainPtr<VMObject> create_anonymous(size_t);
@@ -108,6 +109,7 @@ private:
     size_t m_size { 0 };
     size_t m_size { 0 };
     RetainPtr<Inode> m_inode;
     RetainPtr<Inode> m_inode;
     Vector<RetainPtr<PhysicalPage>> m_physical_pages;
     Vector<RetainPtr<PhysicalPage>> m_physical_pages;
+    Lock m_paging_lock;
 };
 };
 
 
 class Region : public Retainable<Region> {
 class Region : public Retainable<Region> {

+ 26 - 23
SharedGraphics/Font.cpp

@@ -10,6 +10,7 @@
 #include <LibC/stdio.h>
 #include <LibC/stdio.h>
 #include <LibC/fcntl.h>
 #include <LibC/fcntl.h>
 #include <LibC/errno.h>
 #include <LibC/errno.h>
+#include <LibC/mman.h>
 #endif
 #endif
 
 
 #define DEFAULT_FONT_NAME Liza8x10
 #define DEFAULT_FONT_NAME Liza8x10
@@ -101,43 +102,26 @@ struct FontFileHeader {
     char name[64];
     char name[64];
 } PACKED;
 } PACKED;
 
 
-RetainPtr<Font> Font::load_from_file(const String& path)
+RetainPtr<Font> Font::load_from_memory(const byte* data)
 {
 {
-    int fd = open(path.characters(), O_RDONLY, 0644);
-    if (fd < 0) {
-        dbgprintf("open(%s) got fd=%d, failed: %s\n", path.characters(), fd, strerror(errno));
-        perror("open");
-        return nullptr;
-    }
-
-    FontFileHeader header;
-    ssize_t nread = read(fd, &header, sizeof(FontFileHeader));
-    if (nread != sizeof(FontFileHeader)) {
-        dbgprintf("nread != sizeof(FontFileHeader)=%u\n", sizeof(FontFileHeader));
-        return nullptr;
-    }
-
+    auto& header = *reinterpret_cast<const FontFileHeader*>(data);
     if (memcmp(header.magic, "!Fnt", 4)) {
     if (memcmp(header.magic, "!Fnt", 4)) {
-        dbgprintf("header.magic != '!Fnt'\n");
+        dbgprintf("header.magic != '!Fnt', instead it's '%c%c%c%c'\n", header.magic[0], header.magic[1], header.magic[2], header.magic[3]);
         return nullptr;
         return nullptr;
     }
     }
-
     if (header.name[63] != '\0') {
     if (header.name[63] != '\0') {
         dbgprintf("Font name not fully null-terminated\n");
         dbgprintf("Font name not fully null-terminated\n");
         return nullptr;
         return nullptr;
     }
     }
 
 
+    auto* glyphs_ptr = reinterpret_cast<const unsigned*>(data + sizeof(FontFileHeader));
+
     char** new_glyphs = static_cast<char**>(kmalloc(sizeof(char*) * 256));
     char** new_glyphs = static_cast<char**>(kmalloc(sizeof(char*) * 256));
     for (unsigned glyph_index = 0; glyph_index < 256; ++glyph_index) {
     for (unsigned glyph_index = 0; glyph_index < 256; ++glyph_index) {
         new_glyphs[glyph_index] = static_cast<char*>(kmalloc(header.glyph_width * header.glyph_height));
         new_glyphs[glyph_index] = static_cast<char*>(kmalloc(header.glyph_width * header.glyph_height));
         char* bitptr = new_glyphs[glyph_index];
         char* bitptr = new_glyphs[glyph_index];
         for (unsigned y = 0; y < header.glyph_height; ++y) {
         for (unsigned y = 0; y < header.glyph_height; ++y) {
-            unsigned pattern;
-            ssize_t nread = read(fd, &pattern, sizeof(unsigned));
-            if (nread != sizeof(unsigned)) {
-                // FIXME: free() things and return nullptr.
-                ASSERT_NOT_REACHED();
-            }
+            unsigned pattern = *(glyphs_ptr++);
             for (unsigned x = 0; x < header.glyph_width; ++x) {
             for (unsigned x = 0; x < header.glyph_width; ++x) {
                 if (pattern & (1u << x)) {
                 if (pattern & (1u << x)) {
                     *(bitptr++) = '#';
                     *(bitptr++) = '#';
@@ -151,6 +135,25 @@ RetainPtr<Font> Font::load_from_file(const String& path)
     return adopt(*new Font(String(header.name), new_glyphs, header.glyph_width, header.glyph_height, 0, 255));
     return adopt(*new Font(String(header.name), new_glyphs, header.glyph_width, header.glyph_height, 0, 255));
 }
 }
 
 
+RetainPtr<Font> Font::load_from_file(const String& path)
+{
+    int fd = open(path.characters(), O_RDONLY, 0644);
+    if (fd < 0) {
+        dbgprintf("open(%s) got fd=%d, failed: %s\n", path.characters(), fd, strerror(errno));
+        perror("open");
+        return nullptr;
+    }
+
+    auto* mapped_file = (byte*)mmap(nullptr, 4096 * 3, PROT_READ, MAP_SHARED, fd, 0);
+    if (mapped_file == MAP_FAILED)
+        return nullptr;
+
+    auto font = load_from_memory(mapped_file);
+    int rc = munmap(mapped_file, 4096 * 3);
+    ASSERT(rc == 0);
+    return font;
+}
+
 bool Font::write_to_file(const String& path)
 bool Font::write_to_file(const String& path)
 {
 {
     int fd = open(path.characters(), O_WRONLY | O_CREAT, 0644);
     int fd = open(path.characters(), O_WRONLY | O_CREAT, 0644);

+ 2 - 0
SharedGraphics/Font.h

@@ -12,6 +12,8 @@ public:
 
 
     RetainPtr<Font> clone() const;
     RetainPtr<Font> clone() const;
 
 
+    static RetainPtr<Font> load_from_memory(const byte*);
+
 #ifdef USERLAND
 #ifdef USERLAND
     static RetainPtr<Font> load_from_file(const String& path);
     static RetainPtr<Font> load_from_file(const String& path);
     bool write_to_file(const String& path);
     bool write_to_file(const String& path);