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.
On Monday, March 23, 2020 at 8:48:36 PM UTC-4, Nadav Har'El wrote: > > On Tue, Mar 24, 2020 at 12:05 AM Waldemar Kozaczuk <[email protected] > <javascript:>> 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 <[email protected] <javascript:>> >> --- >> 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 = ≺ >> + 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? > > >> +} >> + >> 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); >> > > 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. > > + 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); >> + } >> > > 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? > 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 [email protected] <javascript:>. >> To view this discussion on the web visit >> https://groups.google.com/d/msgid/osv-dev/20200323220551.29880-1-jwkozaczuk%40gmail.com >> . >> > -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/dd12f35e-bbbe-4f66-ad72-cd86909a679a%40googlegroups.com.
