[PATCH] mempool: preliminary support of mmap-backed large allocations

[Waldemar Kozaczuk]

This patch partially addresses the issue #854. In essence it changes malloc_large()
to use mmu::map_anon() if requested memory is >= 2MB and it does not have be
contiguous in physical memory. It is supposed to help OSv memory allocation
behave better when memory in free_page_ranges is heavily fragmented and
large allocations (>=2MB) cannot be satisfied with straight contiguous
page range.

Please not this patch does NOT address scenario where allocation
requests > 4K and < 2MB cannot be satisfied because mamory is fragmented
at that level.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
core/mempool.cc | 41 ++++++++++++++++++++++++++++++++++++-----
core/mmu.cc | 9 +++++----
2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/core/mempool.cc b/core/mempool.cc
index d902eea8..0d82f145 100644
--- a/core/mempool.cc
+++ b/core/mempool.cc
@@ -702,10 +702,13 @@ page_range* page_range_allocator::alloc(size_t size)
for (auto&& pr : _free[exact_order - 1]) {
if (pr.size >= size) {
range = &pr;
+ remove_list(exact_order - 1, *range);
break;
}
}
- return nullptr;
+ if (!range) {
+ return nullptr;
+ }
} else if (order == max_order) {
range = &*_free_huge.rbegin();
if (range->size < size) {
@@ -820,7 +823,9 @@ void page_range_allocator::for_each(unsigned min_order, Func f)
}
}

-static void* malloc_large(size_t size, size_t alignment, bool block = true)
+static size_t huge_page_size = 0x200000;
+
+static void* malloc_large(size_t size, size_t alignment, bool block = true, bool contiguous = true)
{
auto requested_size = size;
size_t offset;
@@ -832,6 +837,16 @@ static void* malloc_large(size_t size, size_t alignment, bool block = true)
size += offset;
size = align_up(size, page_size);

+ if (size >= huge_page_size && !contiguous) {
+ // Map memory if requested memory greater than 2MB and does not need to be contiguous
+ //TODO: For now pre-populate the memory, in future consider turning it off
+ void* obj = mmu::map_anon(nullptr, size, mmu::mmap_populate, mmu::perm_read | mmu::perm_write);
+ page_range* ret_header = reinterpret_cast<page_range*>(obj);
+ ret_header->size = size;
+ trace_memory_malloc_large(obj + offset, requested_size, size, alignment);
+ return obj + offset;
+ }
+
while (true) {
WITH_LOCK(free_page_ranges_lock) {
reclaimer_thread.wait_for_minimum_memory();
@@ -1061,10 +1076,21 @@ static void free_page_range(void *addr, size_t size)
free_page_range(static_cast<page_range*>(addr));
}

+static inline bool is_addr_memory_mmapped(void* addr)
+{
+ return (ulong)addr >= 0x200000000000 && (ulong)addr < 0x800000000000;
+}
+
static void free_large(void* obj)
{
obj = align_down(obj - 1, page_size);
- free_page_range(static_cast<page_range*>(obj));
+ auto range = static_cast<page_range *>(obj);
+ if (is_addr_memory_mmapped(obj)) {
+ mmu::munmap(obj, range->size);
+ }
+ else {
+ free_page_range(range);
+ }
}

static unsigned large_object_size(void *obj)
@@ -1689,7 +1715,7 @@ static inline void* std_malloc(size_t size, size_t alignment)
memory::alloc_page());
trace_memory_malloc_page(ret, size, mmu::page_size, alignment);
} else {
- ret = memory::malloc_large(size, alignment);
+ ret = memory::malloc_large(size, alignment, true, false);
}
memory::tracker_remember(ret, size);
return ret;
@@ -1760,6 +1786,11 @@ void free(void* object)
return;
}
memory::tracker_forget(object);
+ if (memory::is_addr_memory_mmapped(object)) {
+ memory::free_large(object);
+ return;
+ }
+
switch (mmu::get_mem_area(object)) {
case mmu::mem_area::page:
object = mmu::translate_mem_area(mmu::mem_area::page,
@@ -1969,7 +2000,7 @@ void* alloc_phys_contiguous_aligned(size_t size, size_t align, bool block)
assert(is_power_of_two(align));
// make use of the standard large allocator returning properly aligned
// physically contiguous memory:
- auto ret = malloc_large(size, align, block);
+ auto ret = malloc_large(size, align, block, true);
assert (!(reinterpret_cast<uintptr_t>(ret) & (align - 1)));
return ret;
}
diff --git a/core/mmu.cc b/core/mmu.cc
index ff3fab47..d184541c 100644
--- a/core/mmu.cc
+++ b/core/mmu.cc
@@ -115,10 +115,11 @@ phys virt_to_phys(void *virt)
}
#endif

- // For now, only allow non-mmaped areas. Later, we can either
- // bounce such addresses, or lock them in memory and translate
- assert(virt >= phys_mem);
- return reinterpret_cast<uintptr_t>(virt) & (mem_area_size - 1);
+ if (virt >= phys_mem) {
+ return reinterpret_cast<uintptr_t>(virt) & (mem_area_size - 1);
+ } else {
+ return virt_to_phys_pt(virt);
+ }
}

template <int N, typename MakePTE>
--
2.20.1

[Nadav Har'El]

On Tue, Mar 24, 2020 at 12:05 AM Waldemar Kozaczuk <jwkoz...@gmail.com> wrote:

This patch partially addresses the issue #854. In essence it changes malloc_large()
to use mmu::map_anon() if requested memory is >= 2MB and it does not have be
contiguous in physical memory. It is supposed to help OSv memory allocation
behave better when memory in free_page_ranges is heavily fragmented and
large allocations (>=2MB) cannot be satisfied with straight contiguous
page range.

Please not this patch does NOT address scenario where allocation
requests > 4K and < 2MB cannot be satisfied because mamory is fragmented
at that level.

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
 core/mempool.cc | 41 ++++++++++++++++++++++++++++++++++++-----
 core/mmu.cc     |  9 +++++----
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/core/mempool.cc b/core/mempool.cc
index d902eea8..0d82f145 100644
--- a/core/mempool.cc
+++ b/core/mempool.cc
@@ -702,10 +702,13 @@ page_range* page_range_allocator::alloc(size_t size)
         for (auto&& pr : _free[exact_order - 1]) {

The comment just above this line mentioning non-contiguous allocation, is it still relevant with your patch?

Unfortunately, I don't understand what that comment was trying to say :-(

             if (pr.size >= size) {
                 range = &pr;
+                remove_list(exact_order - 1, *range);

I'm not familiar with this code, and I don't understand what I'm seeing here. What does the change here and a few lines below change in this function?

                 break;
             }
         }
-        return nullptr;
+        if (!range) {
+            return nullptr;
+        }
     } else if (order == max_order) {
         range = &*_free_huge.rbegin();
         if (range->size < size) {
@@ -820,7 +823,9 @@ void page_range_allocator::for_each(unsigned min_order, Func f)
     }
 }

-static void* malloc_large(size_t size, size_t alignment, bool block = true)
+static size_t huge_page_size = 0x200000;

we already have mmu::huge_page_size, perhaps we don't need another one?

If it's just an arbitrary limit and doesn't really need to be the same as mmu::huge_page_size, it should get a different name.

 

+
+static void* malloc_large(size_t size, size_t alignment, bool block = true, bool contiguous = true)
 {
     auto requested_size = size;
     size_t offset;
@@ -832,6 +837,16 @@ static void* malloc_large(size_t size, size_t alignment, bool block = true)
     size += offset;
     size = align_up(size, page_size);

+    if (size >= huge_page_size && !contiguous) {
+        // Map memory if requested memory greater than 2MB and does not need to be contiguous

Please use the constant name here, not "2MB". I'm not sure what it should be, and if we ever change it,

it would be a shame to have to hunt down all these references.

+        //TODO: For now pre-populate the memory, in future consider turning it off
+        void* obj = mmu::map_anon(nullptr, size, mmu::mmap_populate, mmu::perm_read | mmu::perm_write);

+        page_range* ret_header = reinterpret_cast<page_range*>(obj);
+        ret_header->size = size;

Wow, if this code wasn't confusing enough already ;-) So now we have a "page_range" structure but it isn't a normal page_range,

and can't be freed with free_page_range() as usual (I see you already handled that correctly below). I guess it's fine, but sad.

+        trace_memory_malloc_large(obj + offset, requested_size, size, alignment);
+        return obj + offset;
+    }
+
     while (true) {
         WITH_LOCK(free_page_ranges_lock) {
             reclaimer_thread.wait_for_minimum_memory();
@@ -1061,10 +1076,21 @@ static void free_page_range(void *addr, size_t size)
     free_page_range(static_cast<page_range*>(addr));
 }

+static inline bool is_addr_memory_mmapped(void* addr)
+{
+    return (ulong)addr >= 0x200000000000 && (ulong)addr < 0x800000000000;

Where did you get these numbers? Don't we already have numeric constants or functions to check these

things? Functions are probably better, and would need to be implemented differently for x86 and aarch64?

I see we already have a function is_linear_mapped(), would using its negation be enough? Or am

I missing some other options?

This was confusing for me to see. Above you changed free_large() to handle either the "normal" (contiguous) large range,

and the mmap case. But in this place, you call it *only* for the mmap case, and a few lines below, you call it again *only*

for the normal (contiguous) case. So it didn't have to be a single function at all... You could have left free_large() unchanged

as it was, and only in te line above, call mmu::unmap() - not free_large.

I am not sure I understand this change. Do we really want to do this? The caller of this function

won't know how many bytes after the start physical address are actually contiguous, and if one calls

this on an mmap(), it's not even guaranteed that the mapping remains in place or doesn't change...

Note that if I understand correctly, your code doesn't change alloc_phys_contiguous_aligned()

and virt_to_phys() can still be used on what it returns. So maybe it doesn't need to be fixed at all?

 }

 template <int N, typename MakePTE>
--
2.20.1

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/20200323220551.29880-1-jwkozaczuk%40gmail.com.

[Waldek Kozaczuk]

I think I have addressed all the issues you pointed out and sent 2 followup patches. As a matter of fact, the latest one should fully address issue 854 (also for smaller allocations). But see my other comment below.

I got rid of that change in my latest patch. 

But I think the original issue #854 you created mentioned the need for this change. I think you had in mind the case when drivers or other code in kernel possibly uses straight malloc() (instead of alloc_phys_contiguous_aligned()) and then calls virt_to_phys() on it. This would be an issue. Based on my cursory look most uses of virt_to_phys() are on memory returned by alloc_phys_contiguous_aligned() or other linearly mapped memory. But I am not 100% sure. 

 }

 template <int N, typename MakePTE>
--
2.20.1

--
You received this message because you are subscribed to the Google Groups "OSv Development" group.

To unsubscribe from this group and stop receiving emails from it, send an email to osv...@googlegroups.com.