On Tue, Jan 16, 2018 at 12:31 AM, Player, Timmons <
timmons.pla...@spirent.com> wrote:
> So, it is possible to reproduce the issue with a unit test, however it
> requires specifying an explicit memory size to the run script.
>
> The issue only occurs when the bitset size is an exact multiple of
> mmu::huge_page_size. In that case, the first mmap that is greater than or
> equal to mmu::huge_page_size will be allocated from the top most range.
> When that allocation is munmapped, the system will hang.
>
> Please see the patch below which adds a tst-page-range-allocator.cc unit
> test.
>
> After applying and rebuilding the test image you should be able to trigger
> the hang by calling:
>
> timmons@rtp-skink osv[0]> scripts/run.py -m512.15M -c2 -e
> tests/tst-page-range-allocator.so
> warning: TCG doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
> warning: TCG doesn't support requested feature: CPUID.01H:ECX.vmx [bit 5]
> OSv v0.24-474-g9cceaca
> eth0: 192.168.122.15
> _bitmap.size() = 131072, end = 131072
> [ hangs here… ]
>
> P.S. Since the unit test requires an explicit configuration, I’m not sure
> if it’s useful to add it to the patch… Let me know if you think otherwise.
>
>
Thanks.
I think it's useful, but the unit test's source code should explain how it
should be used.
But I wonder how stable this "512.15M" number is... Could we find a way for
the test to check if the bitmap size is the problematic one, somehow?
>
> diff --git a/core/mempool.cc b/core/mempool.cc
> index 569a0ac..437b54c 100644
> --- a/core/mempool.cc
> +++ b/core/mempool.cc
> @@ -767,6 +767,10 @@ void page_range_allocator::free(page_range* pr)
> pr2->size += pr->size;
> pr = pr2;
> }
> + auto end = get_bitmap_idx(*pr) + pr->size / page_size;
> + if (end >= _bitmap.size()) {
>
+ printf("_bitmap.size() = %zu, end = %zu\n", _bitmap.size(), end);
>
Is this case an indication of an error? If so, should we abort() here? Or
is this an ok case - in which case it's probably not ok to print a message
every ime it happens... (since you added this printout to the free() code,
not to the test...).
+ }
>
if (_bitmap[get_bitmap_idx(*pr) + pr->size / page_size]) {
> auto pr2 = static_cast<page_range*>(static_cast<void*>(pr) +
> pr->size);
> remove(*pr2);
> diff --git a/modules/tests/Makefile b/modules/tests/Makefile
> index b8348dd..715a7c1 100644
> --- a/modules/tests/Makefile
> +++ b/modules/tests/Makefile
> @@ -87,7 +87,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-calloc.so tst-page-range-allocator.so
>
> # libstatic-thread-variable.so tst-static-thread-variable.so \
>
> diff --git a/tests/tst-page-range-allocator.cc b/tests/tst-page-range-
> allocator.cc
> new file mode 100644
> index 0000000..bf2a9aa
> --- /dev/null
> +++ b/tests/tst-page-range-allocator.cc
> @@ -0,0 +1,28 @@
> +#include <osv/mmu.hh>
> +#include <sys/mman.h>
> +#include <string.h>
> +
> +int tests = 0, fails = 0;
> +
> +constexpr int bufsize = mmu::huge_page_size;
> +
> +static void report(bool ok, const char *msg)
> +{
> + ++tests;
> + fails += !ok;
> + debug("%s: %s\n", (ok ? "PASS" : "FAIL"), msg);
>
Can you please use printf() instead of debug()? Otherwise the test is
mysteriously silent if not running the kernel in verbose mode.
+}
> +
> +int main()
> +{
> + void *buffer = mmap(NULL, bufsize, PROT_READ|PROT_WRITE,
> MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> + report(buffer != nullptr, "mmap");
> +
> + // Force the buffer to get mapped.
> + memset(buffer, 0, bufsize);
> +
> + int error = munmap(buffer, bufsize);
> + report(error == 0, "mumap");
> +
> + debug("SUMMARY: %d tests, %d failures\n", tests, fails);
> +}
>
> From: OSv Gopher <osv-dev@googlegroups.com> on behalf of Timmons Player <
> timmons.pla...@spirent.com>
> Date: Monday, January 15, 2018 at 3:05 PM
> To: Waldek Kozaczuk <jwkozac...@gmail.com>, OSv Gopher <
> osv-dev@googlegroups.com>
> Subject: Re: [PATCH] memory: respect _bitset length bounds in
> page_range_allocator
>
> I don’t know. I could only ever reproduce it on 1 CPU setups on a
> specific hypervisor. I’ll investigate reproducing it via a unit test.
>
> Timmons
>
>
> From: OSv Gopher <osv-dev@googlegroups.com> on behalf of Waldek Kozaczuk <
> jwkozac...@gmail.com>
> Date: Monday, January 15, 2018 at 2:19 PM
> To: OSv Gopher <osv-dev@googlegroups.com>
> Subject: Re: [PATCH] memory: respect _bitset length bounds in
> page_range_allocator
>
> How difficult would it be to create a unit test around this bug to prove
> what it fixes?
>
> On Monday, January 15, 2018 at 12:52:24 PM UTC-5, Timmons C. Player wrote:
> The page_range_allocator attempts to merge neighbor page ranges
> when a page range is freed in order to minimize memory fragmentation.
>
> However, the original implementation didn't validate indexes before
> checking the _bitset to determine whether a page range had a neighbor
> or not. This change make sure that indexes stay within bounds.
>
> This is the correct fix for the fix attempted in
> 44f8324e3f2e53f55022a1d93c8b072514e7ac07
>
> Signed-off-by: Timmons C. Player <timmons...@spirent.com>
> ---
> core/mempool.cc | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/core/mempool.cc b/core/mempool.cc
> index e0d0867..1ac3f67 100644
> --- a/core/mempool.cc
> +++ b/core/mempool.cc
> @@ -761,13 +761,15 @@ page_range* page_range_allocator::alloc_aligned(size_t
> size, size_t offset,
>
> void page_range_allocator::free(page_range* pr)
> {
> - if (_bitmap[get_bitmap_idx(*pr) - 1]) {
> + auto idx = get_bitmap_idx(*pr);
> + if (idx && _bitmap[idx - 1]) {
> auto pr2 = *(reinterpret_cast<page_range**>(pr) - 1);
> remove(*pr2);
> pr2->size += pr->size;
> pr = pr2;
> }
> - if (_bitmap[get_bitmap_idx(*pr) + pr->size / page_size - 1]) {
> + auto next_idx = get_bitmap_idx(*pr) + pr->size / page_size;
> + if (next_idx < _bitmap.size() && _bitmap[next_idx]) {
> auto pr2 = static_cast<page_range*>(static_cast<void*>(pr) +
> pr->size);
> remove(*pr2);
> pr->size += pr2->size;
> --
> 2.7.4
> --
> 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+unsubscr...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>
>
>
>
>
> Spirent Communications e-mail confidentiality.
> ------------------------------------------------------------------------
> This e-mail contains confidential and / or privileged information
> belonging to Spirent Communications plc, its affiliates and / or
> subsidiaries. If you are not the intended recipient, you are hereby
> notified that any disclosure, copying, distribution and / or the taking of
> any action based upon reliance on the contents of this transmission is
> strictly forbidden. If you have received this message in error please
> notify the sender by return e-mail and delete it from your system.
>
> Spirent Communications plc
> Northwood Park, Gatwick Road, Crawley, West Sussex, RH10 9XN, United
> Kingdom.
> Tel No. +44 (0) 1293 767676
> Fax No. +44 (0) 1293 767677
>
> Registered in England Number 470893
> Registered at Northwood Park, Gatwick Road, Crawley, West Sussex, RH10
> 9XN, United Kingdom.
>
> Or if within the US,
>
> Spirent Communications,
> 27349 Agoura Road, Calabasas, CA, 91301, USA.
> Tel No. 1-818-676- 2300
> --
> 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+unsubscr...@googlegroups.com.
> 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 osv-dev+unsubscr...@googlegroups.com.
> 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 osv-dev+unsubscr...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
