|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [UNIKRAFT PATCH v2] lib/ukalloc: add ifmalloc compatibility interface
On 30.07.20, 23:53, "Hugo Lefeuvre" <Hugo.Lefeuvre@xxxxxxxxx> wrote:
Hi Simon,
thanks a lot for your comments, answers inline.
regards,
Hugo
On Thu, 2020-07-30 at 18:20 +0200, Simon Kuenzer wrote:
>
> Hey, I found to open questions. Let me know what you think.
>
> Thanks,
>
> Simon
>
> On 30.07.20 16:48, Hugo Lefeuvre wrote:
> >
> >
> > Add ifmalloc, the malloc compatibility interface. This interface
> > implements
> > a POSIX compliant allocation interface on top of a simple malloc()
> > and
> > free() interface. This interface is similar to ifpages, which does
> > the same
> > for palloc() and pfree().
> >
> > ifmalloc will be used for the port of TLSF and tinyalloc which do
> > not
> > support (posix_)memalign().
> >
> > Signed-off-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> > ---
> > Changed since v1:
> > - update alloc.c file header
> > - use METADATA_IFMALLOC_SIZE_POW2 instead of sizeof(struct
> > metadata_ifmalloc)
> > - add unlikely where appropriate
> > - use align < sizeof(void *) instead of (align % sizeof(void *))
> > != 0
> > - minor optimizations and style-related changes
> >
> > lib/ukalloc/Config.uk | 5 ++
> > lib/ukalloc/alloc.c | 148
> > +++++++++++++++++++++++++++++++++++-
> > lib/ukalloc/exportsyms.uk | 4 +
> > lib/ukalloc/include/uk/alloc.h | 5 ++
> > lib/ukalloc/include/uk/alloc_impl.h | 27 +++++++
> > 5 files changed, 186 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/ukalloc/Config.uk b/lib/ukalloc/Config.uk
> > index 0de7464..c965d08 100644
> > --- a/lib/ukalloc/Config.uk
> > +++ b/lib/ukalloc/Config.uk
> > @@ -5,6 +5,11 @@ menuconfig LIBUKALLOC
> > select LIBUKDEBUG
> >
> > if LIBUKALLOC
> > + config LIBUKALLOC_IFMALLOC
> > + bool "Malloc compatibility interface"
> > + default n
> > + help
> > + Provide helpers for allocators defining
> > exclusively malloc and free
> > config LIBUKALLOC_IFSTATS
> > bool "Statistics interface"
> > default n
> > diff --git a/lib/ukalloc/alloc.c b/lib/ukalloc/alloc.c
> > index 127bc6f..160a0a4 100644
> > --- a/lib/ukalloc/alloc.c
> > +++ b/lib/ukalloc/alloc.c
> > @@ -1,8 +1,10 @@
> > /* SPDX-License-Identifier: BSD-3-Clause */
> > /*
> > * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> > + * Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>
> > *
> > - * Copyright (c) 2017, NEC Europe Ltd., NEC Corporation. All
> > rights reserved.
> > + * Copyright (c) 2017-2020, NEC Laboratories Europe GmbH, NEC
> > Corporation,
> > + * All rights reserved.
> > *
> > * Redistribution and use in source and binary forms, with or
> > without
> > * modification, are permitted provided that the following
> > conditions
> > @@ -28,8 +30,6 @@
> > * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR
> > OTHERWISE)
> > * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
> > ADVISED OF THE
> > * POSSIBILITY OF SUCH DAMAGE.
> > - *
> > - * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> > */
> >
> > /* This is a very simple, naive implementation of malloc.
> > @@ -52,6 +52,7 @@
> > #include <uk/essentials.h>
> > #include <uk/assert.h>
> > #include <uk/arch/limits.h>
> > +#include <uk/arch/lcpu.h>
> >
> > #define size_to_num_pages(size) \
> > (ALIGN_UP((unsigned long)(size), __PAGE_SIZE) /
> > __PAGE_SIZE)
> > @@ -267,6 +268,147 @@ int uk_posix_memalign_ifpages(struct uk_alloc
> > *a,
> > return 0;
> > }
> >
> > +#if CONFIG_LIBUKALLOC_IFMALLOC
> > +
> > +struct metadata_ifmalloc {
> > + size_t size;
> > + void *base;
> > +};
> > +
> > +#define METADATA_IFMALLOC_SIZE_POW2 16
> > +UK_CTASSERT(!(sizeof(struct metadata_ifmalloc) >
> > METADATA_IFMALLOC_SIZE_POW2));
> > +
> > +static struct metadata_ifmalloc *uk_get_metadata_ifmalloc(const
> > void *ptr)
> > +{
> > + return (struct metadata_ifmalloc *)((uintptr_t) ptr -
> > + METADATA_IFMALLOC_SIZE_POW2);
> > +}
> > +
> > +static size_t uk_getmallocsize_ifmalloc(const void *ptr)
> > +{
> > + struct metadata_ifmalloc *metadata =
> > uk_get_metadata_ifmalloc(ptr);
> > + return (size_t) ((uintptr_t) metadata->base + metadata-
> > >
> > > size -
> > + (uintptr_t) ptr);
> > +}
> > +
> > +void uk_free_ifmalloc(struct uk_alloc *a, void *ptr)
> > +{
> > + struct metadata_ifmalloc *metadata;
> > +
> > + UK_ASSERT(a);
> > + UK_ASSERT(a->free_backend);
> > + if (!ptr)
> > + return;
> > +
> > + metadata = uk_get_metadata_ifmalloc(ptr);
> > + a->free_backend(a, metadata->base);
> > +}
> > +
> > +void *uk_malloc_ifmalloc(struct uk_alloc *a, size_t size)
> > +{
> > + struct metadata_ifmalloc *metadata;
> > + size_t realsize = size + METADATA_IFMALLOC_SIZE_POW2;
> > + void *ptr;
> > +
> > + UK_ASSERT(a);
> > + UK_ASSERT(a->malloc_backend);
> > +
> > + /* check for overflow */
> > + if (unlikely(realsize < size))
> > + return NULL;
> > +
> > + ptr = a->malloc_backend(a, realsize);
> > + if (!ptr)
> > + return NULL;
> > +
> > + metadata = ptr;
> > + metadata->size = realsize;
> > + metadata->base = ptr;
> > +
> > + return (void *) ((uintptr_t) ptr +
> > METADATA_IFMALLOC_SIZE_POW2);
> > +}
> > +
> > +void *uk_realloc_ifmalloc(struct uk_alloc *a, void *ptr, size_t
> > size)
> > +{
> > + void *retptr;
> > + size_t mallocsize;
> > +
> > + UK_ASSERT(a);
> > + if (!ptr)
> > + return uk_malloc_ifmalloc(a, size);
> > +
> > + if (ptr && !size) {
> > + uk_free_ifmalloc(a, ptr);
> > + return NULL;
> > + }
> > +
> > + retptr = uk_malloc_ifmalloc(a, size);
> > + if (!retptr)
> > + return NULL;
> > +
> > + mallocsize = uk_getmallocsize_ifmalloc(ptr);
> > +
> > + memcpy(retptr, ptr, MIN(size, mallocsize));
> > +
> > + uk_free_ifmalloc(a, ptr);
> > + return retptr;
> > +}
> > +
> > +int uk_posix_memalign_ifmalloc(struct uk_alloc *a,
> > + void **memptr, size_t align,
> > size_t size)
> > +{
> > + struct metadata_ifmalloc *metadata;
> > + size_t realsize, padding;
> > + uintptr_t intptr;
> > +
> > + UK_ASSERT(a);
> > + if (((align - 1) & align) != 0
> > + || align < sizeof(void *))
> > + return EINVAL;
> > +
> > + if (!size) {
> > + *memptr = NULL;
> Should we touch *memptr in case of failures? You seem not to do it
> in
> the other cases.
> https://man7.org/linux/man-pages/man3/posix_memalign.3.html
Taking a look at https://pubs.opengroup.org/onlinepubs/9699919799/funct
ions/posix_memalign.html, specifically:
"If size is 0, either:
* posix_memalign() shall not attempt to allocate any space, in which
case either an implementation-defined error number shall be returned,
or zero shall be returned with a null pointer returned in memptr, or
* posix_memalign() shall attempt to allocate some space and, if the
allocation succeeds, zero shall be returned and a pointer to the
allocated space shall be returned in memptr. The application shall
ensure that the pointer is not used to access an object."
Indeed, I prefer to return an error code instead of setting memptr and
returning zero. My fear is that some users might dereference memptr
after observing a zero return value. Do you think EINVAL is fine in
this case? In this case we could just remove `*memptr = NULL;`.
Note that this bug was already present in uk_posix_memalign_ifpages, I
intentionally reproduced it in the ifmalloc wrapper. It was introduced
in https://github.com/unikraft/unikraft/commit/a323fbd67ac3cb21139633b3
00a024538167493e
Good point. Personally, I prefer returning an error and not touching memptr,
since we have choice. That feels a bit more well-defined than the other
behavior.
In case we will come across while porting an application that expect the other
behavior, we may think of adding a menu config option to switch between them.
I really hope it is rare that someone calls posix_memalign() with size 0...
Can you add a brief comment that two described behavior exist for size = 0?
In order to keep uk_posix_memalign_ifpages() equivalent, can you remove
setting memptr also there for this case? With having the comment it is
fine to do this in the same patch.
> > + return EINVAL;
> > +}
> > +
> > + /* Store size information preceeding the memory block.
> > Since we return
> > + * pointers aligned at `align` we need to reserve at least
> > that much
> > + * space for the size information.
> > + */
> > + if (align < METADATA_IFMALLOC_SIZE_POW2) {
> > + align = METADATA_IFMALLOC_SIZE_POW2;
> > + padding = 0;
> > + } else {
> > + padding = METADATA_IFMALLOC_SIZE_POW2;
> > + }
> > +
> > + realsize = size + padding + align;
> > +
> > + /* check for overflow */
> > + if (unlikely(realsize < size))
> > + return NULL;
> Hum, you should return an errno. I would choose ENOMEM, thats closer
> inline with malloc:
>
> return ENOMEM;
Sounds good.
>
> >
> >
> > +
> > + intptr = (uintptr_t) a->malloc_backend(a, realsize);
> > +
> > + if (!intptr)
> > + return ENOMEM;
> > +
> > + *memptr = (void *) ALIGN_UP(intptr +
> > METADATA_IFMALLOC_SIZE_POW2,
> > + (uintptr_t) align);
> > +
> > + metadata = uk_get_metadata_ifmalloc(*memptr);
> > +
> > + /* check for underflow */
> > + UK_ASSERT(intptr <= (uintptr_t) metadata);
> > +
> > + metadata->size = realsize;
> > + metadata->base = (void *) intptr;
> > +
> > + return 0;
> > +}
> > +
> > +#endif
> > +
> > void uk_pfree_compat(struct uk_alloc *a, void *ptr,
> > unsigned long num_pages __unused)
> > {
> > diff --git a/lib/ukalloc/exportsyms.uk b/lib/ukalloc/exportsyms.uk
> > index 2c8a90f..21c1996 100644
> > --- a/lib/ukalloc/exportsyms.uk
> > +++ b/lib/ukalloc/exportsyms.uk
> > @@ -4,6 +4,10 @@ uk_malloc_ifpages
> > uk_free_ifpages
> > uk_realloc_ifpages
> > uk_posix_memalign_ifpages
> > +uk_malloc_ifmalloc
> > +uk_realloc_ifmalloc
> > +uk_posix_memalign_ifmalloc
> > +uk_free_ifmalloc
> > uk_calloc_compat
> > uk_memalign_compat
> > uk_realloc_compat
> > diff --git a/lib/ukalloc/include/uk/alloc.h
> > b/lib/ukalloc/include/uk/alloc.h
> > index 5bfa2f2..1457922 100644
> > --- a/lib/ukalloc/include/uk/alloc.h
> > +++ b/lib/ukalloc/include/uk/alloc.h
> > @@ -85,6 +85,11 @@ struct uk_alloc {
> > uk_alloc_memalign_func_t memalign;
> > uk_alloc_free_func_t free;
> >
> > +#if CONFIG_LIBUKALLOC_IFMALLOC
> > + uk_alloc_free_func_t free_backend;
> > + uk_alloc_malloc_func_t malloc_backend;
> > +#endif
> > +
> > /* page allocation interface */
> > uk_alloc_palloc_func_t palloc;
> > uk_alloc_pfree_func_t pfree;
> > diff --git a/lib/ukalloc/include/uk/alloc_impl.h
> > b/lib/ukalloc/include/uk/alloc_impl.h
> > index 8bcca94..b15f49e 100644
> > --- a/lib/ukalloc/include/uk/alloc_impl.h
> > +++ b/lib/ukalloc/include/uk/alloc_impl.h
> > @@ -62,6 +62,14 @@ int uk_posix_memalign_ifpages(struct uk_alloc
> > *a, void **memptr,
> > size_t align, size_t size);
> > void uk_free_ifpages(struct uk_alloc *a, void *ptr);
> >
> > +#if CONFIG_LIBUKALLOC_IFMALLOC
> > +void *uk_malloc_ifmalloc(struct uk_alloc *a, size_t size);
> > +void *uk_realloc_ifmalloc(struct uk_alloc *a, void *ptr, size_t
> > size);
> > +int uk_posix_memalign_ifmalloc(struct uk_alloc *a, void **memptr,
> > + size_t align, size_t size);
> > +void uk_free_ifmalloc(struct uk_alloc *a, void *ptr);
> > +#endif
> > +
> > /* Functionality that is provided based on malloc() and
> > posix_memalign() */
> > void *uk_calloc_compat(struct uk_alloc *a, size_t num, size_t
> > len);
> > void *uk_realloc_compat(struct uk_alloc *a, void *ptr, size_t
> > size);
> > @@ -88,6 +96,25 @@ void uk_pfree_compat(struct uk_alloc *a, void
> > *ptr, unsigned long num_pages);
> > uk_alloc_register((a));
> > \
> > } while (0)
> >
> > +#if CONFIG_LIBUKALLOC_IFMALLOC
> > +#define uk_alloc_init_malloc_ifmalloc(a, malloc_f, free_f,
> > addmem_f) \
> > + do {
> > \
> > + (a)->malloc = uk_malloc_ifmalloc;
> > \
> > + (a)->calloc = uk_calloc_compat;
> > \
> > + (a)->realloc = uk_realloc_ifmalloc;
> > \
> > + (a)->posix_memalign = uk_posix_memalign_ifmalloc;
> > \
> > + (a)->memalign = uk_memalign_compat;
> > \
> > + (a)->malloc_backend = (malloc_f);
> > \
> > + (a)->free_backend = (free_f);
> > \
> > + (a)->free = uk_free_ifmalloc;
> > \
> > + (a)->palloc = uk_palloc_compat;
> > \
> > + (a)->pfree = uk_pfree_compat;
> > \
> > + (a)->addmem = (addmem_f);
> > \
> > +
> > \
> > + uk_alloc_register((a));
> > \
> > + } while (0)
> > +#endif
> > +
> > /* Shortcut for doing a registration of an allocator that only
> > * implements palloc(), pfree(), addmem()
> > */
Thanks,
Simon
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |