Laszlo Ersek
2022-Jan-31 15:37 UTC
[Libguestfs] [PATCH nbdkit v2 4/4] common/allocators: Always align mlock buffer
On 01/27/22 17:39, Richard W.M. Jones wrote:> On platforms with 64K page sizes (RHEL + aarch64 or ppc64le) we have > hit a problem in test-memory-allocator-malloc-mlock.sh which appears > to be caused by: > > 1. The test requests a 2K mlocked buffer. > > 2. The allocated buffer straddles a page boundary sometimes. > > 3. Linux expands the mlock request to two pages, ie. the requested > size becomes 2 * 64K = 128K > > 4. The standard mlock limit on Linux is 64K so the mlock request is > rejected and the test fails. > > Also POSIX permits the implementation to require mlock parameters to > be page aligned (although Linux does not require this). > > This commit attempts to fix both problems by ensuring the mlock > request is always page aligned, using the new > vector_reserve_page_aligned function. > > Thanks: Laszlo Ersek > --- > common/allocators/malloc.c | 53 ++++++++++++++++++++++++++++++-------- > 1 file changed, 42 insertions(+), 11 deletions(-) > > diff --git a/common/allocators/malloc.c b/common/allocators/malloc.c > index 9a2dd549..19df666a 100644 > --- a/common/allocators/malloc.c > +++ b/common/allocators/malloc.c > @@ -81,11 +81,32 @@ m_alloc_free (struct allocator *a) > } > } > > -/* Extend the underlying bytearray if needed. mlock if requested. */ > +/* Extend the underlying bytearray if needed. */ > static int > -extend (struct m_alloc *ma, uint64_t new_size) > +extend_without_mlock (struct m_alloc *ma, uint64_t new_size) > +{ > + size_t old_size, n; > + > + if (ma->ba.cap < new_size) { > + old_size = ma->ba.cap; > + n = new_size - ma->ba.cap; > + > + if (bytearray_reserve (&ma->ba, n) == -1) { > + nbdkit_error ("realloc: %m"); > + return -1; > + } > + > + /* Initialize the newly allocated memory to 0. */ > + memset (ma->ba.ptr + old_size, 0, n); > + } > + > + return 0; > +} > + > +#ifdef HAVE_MLOCK > +static int > +extend_with_mlock (struct m_alloc *ma, uint64_t new_size) > { > - ACQUIRE_WRLOCK_FOR_CURRENT_SCOPE (&ma->lock); > size_t old_size, n; > > if (ma->ba.cap < new_size) { > @@ -100,7 +121,7 @@ extend (struct m_alloc *ma, uint64_t new_size) > munlock (ma->ba.ptr, ma->ba.cap); > #endif > > - if (bytearray_reserve (&ma->ba, n) == -1) { > + if (bytearray_reserve_page_aligned (&ma->ba, n) == -1) { > nbdkit_error ("realloc: %m"); > return -1; > } > @@ -108,18 +129,28 @@ extend (struct m_alloc *ma, uint64_t new_size) > /* Initialize the newly allocated memory to 0. */ > memset (ma->ba.ptr + old_size, 0, n); > > -#ifdef HAVE_MLOCK > - if (ma->use_mlock) { > - if (mlock (ma->ba.ptr, ma->ba.cap) == -1) { > - nbdkit_error ("allocator=malloc: mlock: %m"); > - return -1; > - } > + if (mlock (ma->ba.ptr, ma->ba.cap) == -1) { > + nbdkit_error ("allocator=malloc: mlock: %m"); > + return -1; > } > -#endif > } > > return 0; > } > +#endif /* HAVE_MLOCK */ > + > +static int > +extend (struct m_alloc *ma, uint64_t new_size) > +{ > + ACQUIRE_WRLOCK_FOR_CURRENT_SCOPE (&ma->lock); > + > +#ifdef HAVE_MLOCK > + if (ma->use_mlock) > + return extend_with_mlock (ma, new_size); > +#endif > + > + return extend_without_mlock (ma, new_size); > +} > > static int > m_alloc_set_size_hint (struct allocator *a, uint64_t size_hint) >Reviewed-by: Laszlo Ersek <lersek at redhat.com>
Richard W.M. Jones
2022-Jan-31 16:14 UTC
[Libguestfs] [PATCH nbdkit v2 4/4] common/allocators: Always align mlock buffer
Thanks - I pushed this as: 521e69f7..24217480 master -> master I changed patch 3 to include your recommendations. But also I noticed there was a mistake in the memcpy which I hope I fixed and it's worth reading the comment about this: https://gitlab.com/nbdkit/nbdkit/-/blob/2421748073b71ab600db50f748a53b8f36d30add/common/utils/vector.c#L147 Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html
Laszlo Ersek
2022-Feb-01 07:22 UTC
[Libguestfs] [PATCH nbdkit v2 4/4] common/allocators: Always align mlock buffer
On 01/31/22 17:14, Richard W.M. Jones wrote:> > Thanks - I pushed this as: > > 521e69f7..24217480 master -> master > > I changed patch 3 to include your recommendations. But also I noticed > there was a mistake in the memcpy which I hope I fixed and it's worth > reading the comment about this: > > https://gitlab.com/nbdkit/nbdkit/-/blob/2421748073b71ab600db50f748a53b8f36d30add/common/utils/vector.c#L147/* How much to copy here? * * vector_reserve always makes the buffer larger so we don't have to * deal with the case of a shrinking buffer. * * Like the underlying realloc we do not have to zero the newly * reserved elements. * * However (like realloc) we have to copy the existing elements even * those that are not allocated and only reserved (between 'len' and * 'cap'). That's strange; I'd say any " offset >= len" access to a vector is an out-of-bounds bug in the client code. From "common/utils/vector.h": * where ?names.ptr[]? will be an array of strings and ?names.len? * will be the number of strings. There are no get/set accessors. To * iterate over the strings you can use the ?.ptr? field directly: * * for (size_t i = 0; i < names.len; ++i) * printf ("%s\n", names.ptr[i]); Then, * * The spec of vector is not clear on the last two points, but * existing code depends on this undocumented behaviour. */ To me the spec ("common/utils/vector.h") seemed clear: /* Reserve n elements at the end of the vector. Note space is \ * allocated and capacity is increased, but the vector length \ * is not increased and the new elements are not initialized. \ */ \ If length is not increased, then elements between "len" and "old cap" remain undefined as before, and elements between "old cap" and "new cap" are new, and also not initialized. I'm slightly concerned about accessing uninitialized (only allocated) storage with memcpy(); maybe it can be shown to be undefined behavior, maybe not, but I compilers have been exploiting UB more aggressively over time. Laszlo> > Rich. >