On Sunday, November 18, 2018 at 11:37:49 AM UTC-5, Nadav Har'El wrote:
>
> 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]
> <javascript:>> 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] <javascript:>>
>> ---
>> 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:
>
> Your observation is correct about two types of sub-scenarios: 1) size <=
alignment and 2) alignment <=size
> unsigned n = ilog2_roundup(std::max(size, memory::pool::min_object_size);
> if (alignment <= n) {
>
I am not sure you meant to compare log2 of size with alignment, did you?
> // .. 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.
> }
>
I have sent new patch that addresses your observation. I have also greatly
enhanced the unit test to cover more scenarios.
>
>
>> 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] <javascript:>.
>> 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.
