On Tue, Jan 16, 2018 at 12:31 AM, Player, Timmons <
[email protected]> 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 <[email protected]> on behalf of Timmons Player <
> [email protected]>
> Date: Monday, January 15, 2018 at 3:05 PM
> To: Waldek Kozaczuk <[email protected]>, OSv Gopher <
> [email protected]>
> 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 <[email protected]> on behalf of Waldek Kozaczuk <
> [email protected]>
> Date: Monday, January 15, 2018 at 2:19 PM
> To: OSv Gopher <[email protected]>
> 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 <[email protected]>
> ---
> 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 [email protected].
> 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 [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.
>
--
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.
