Thanks! This is a very welcome patch :-) I'm committing your patch because it is correct, but I have a comment below maybe it could have been done slightly differently and cover yet another corner case, so please consider my comments below for a followup patch - if you agree with my comments.
On Sat, Nov 17, 2018 at 5:49 AM Waldemar Kozaczuk <[email protected]> wrote: > This patch adjusts std_malloc() to handle small > allocations (smaller than 16-bytes) more efficiently. > More specifically instead of allocating whole page > for each such request, adjusted std_malloc() uses memory > pool adequate to requested size and thus saves almost > entire page (4K) of memory. > > Fixes #1011 > > Signed-off-by: Waldemar Kozaczuk <[email protected]> > --- > core/mempool.cc | 5 +++-- > modules/tests/Makefile | 2 +- > tests/tst-small-malloc.cc | 45 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 49 insertions(+), 3 deletions(-) > create mode 100644 tests/tst-small-malloc.cc > > diff --git a/core/mempool.cc b/core/mempool.cc > index 1ac3f676..deb7a0bb 100644 > --- a/core/mempool.cc > +++ b/core/mempool.cc > @@ -1546,8 +1546,9 @@ static inline void* std_malloc(size_t size, size_t > alignment) > if ((ssize_t)size < 0) > return libc_error_ptr<void *>(ENOMEM); > void *ret; > - if (size <= memory::pool::max_object_size && alignment <= size && > smp_allocator) { > - size = std::max(size, memory::pool::min_object_size); > + size_t minimum_size = std::max(size, memory::pool::min_object_size); > + if (size <= memory::pool::max_object_size && alignment <= > minimum_size && smp_allocator) { > + size = minimum_size; > Here you're modifying the original "size" parameter anyway, so I don't think you really need this "minimum_size" variable, you could have just done above if (size < minimum_size) { size = minimum_size; } And then kept the original if(). Alternatively, maybe it makes sense not to modify size at all, and instead set the new variable n (defind below with a roundup). This will have allowed the original "size" to be remembered for the memory leak tracker, for example. But now that I think of this code, I think it misses another, rarer, possibility: What if someone allocates size=15 alignment=16 (that's easy to add a test for it in your test)? The if() will fail and fall back to a full page, but because ! 16 <= 15, but actually when we do the roundup, we anyway change the size (n actually) to 16. So maybe the easiest way to do this patch is to remove the "alignment" check from the above if, and instead do something like this in the code below: unsigned n = ilog2_roundup(std::max(size, memory::pool::min_object_size); if (alignment <= n) { // .. allocate from a pool } else if (alignment <= max_object_size ) { // a new case not handled before // ... allocate from a pool of size "alignment" instead of "n" (assuming alignment is a power of 2 ;-)) } else { // do the large allocation, with the correct alignment. } > unsigned n = ilog2_roundup(size); > ret = memory::malloc_pools[n].alloc(); > ret = translate_mem_area(mmu::mem_area::main, > mmu::mem_area::mempool, > diff --git a/modules/tests/Makefile b/modules/tests/Makefile > index b7042a1f..ece5f846 100644 > --- a/modules/tests/Makefile > +++ b/modules/tests/Makefile > @@ -116,7 +116,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so > tst-bsd-evh.so \ > tst-ifaddrs.so tst-pthread-affinity-inherit.so > tst-sem-timed-wait.so \ > tst-ttyname.so tst-pthread-barrier.so tst-feexcept.so tst-math.so \ > tst-sigaltstack.so tst-fread.so tst-tcp-cork.so tst-tcp-v6.so \ > - tst-calloc.so tst-crypt.so tst-non-fpic.so > + tst-calloc.so tst-crypt.so tst-non-fpic.so tst-small-malloc.so > > # libstatic-thread-variable.so tst-static-thread-variable.so \ > > diff --git a/tests/tst-small-malloc.cc b/tests/tst-small-malloc.cc > new file mode 100644 > index 00000000..b0c97c03 > --- /dev/null > +++ b/tests/tst-small-malloc.cc > @@ -0,0 +1,45 @@ > +/* > + * Copyright (C) 2018 Waldemar Kozaczuk > + * > + * This work is open source software, licensed under the terms of the > + * BSD license as described in the LICENSE file in the top-level > directory. > + */ > +#include <stdio.h> > +#include <stdlib.h> > +#include <cassert> > +#include <osv/trace-count.hh> > + > +int main() { > + tracepoint_counter *memory_malloc_mempool_counter = nullptr, > + *memory_malloc_page_counter = nullptr; > + > + for (auto & tp : tracepoint_base::tp_list) { > + if ("memory_malloc_mempool" == std::string(tp.name)) { > + memory_malloc_mempool_counter = new tracepoint_counter(tp); > + } > + if ("memory_malloc_page" == std::string(tp.name)) { > + memory_malloc_page_counter = new tracepoint_counter(tp); > + } > + } > + assert(memory_malloc_mempool_counter != nullptr); > + assert(memory_malloc_page_counter != nullptr); > + > + auto memory_malloc_mempool_counter_now = > memory_malloc_mempool_counter->read(); > + auto memory_malloc_page_counter_now = > memory_malloc_page_counter->read(); > + > + const int allocation_count = 1024; > + for( int i = 0; i < allocation_count; i++) { > + void *addr = malloc(7); > + assert(addr); > + free(addr); > + > + addr = malloc(17); > + assert(addr); > + free(addr); > + } > + > + // Verify all allocations above were handled by malloc_pool > + assert(memory_malloc_mempool_counter->read() - > memory_malloc_mempool_counter_now >= 2 * allocation_count); > + // Verify that NO allocations were handled by alloc_page > + assert(memory_malloc_page_counter->read() - > memory_malloc_page_counter_now == 0); > +} > \ No newline at end of file > -- > 2.19.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]. > For more options, visit https://groups.google.com/d/optout. > -- 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]. For more options, visit https://groups.google.com/d/optout.
