Re: [Minios-devel] [UNIKRAFT PATCH] lib/ukalloc: uk_getmallocsize should return size not order

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Radu,

thank you for your patch. Some comments below:


On 07/20/2018 06:32 PM, Radu Nicolau wrote:
> Fixed uk_getmallocsize in order to get the allocated size
> instead of the order. The allocated size is needed by
> uk_realloc_ifpages in order to determine how much to copy
> from the old allocation to the new allocated area.
>
> Signed-off-by: Radu Nicolau <radunicolau102@xxxxxxxxx>
As a general rule when you create unikraft patches, please add the 
subsystem you're touching as the first part, then describe your changes 
in imperative mode, e.g.:
"lib/ukalloc: Change return value of uk_getmallocsize()

Change uk_getmallocsize() to return the allocated size in bytes instead 
of page order. Frooble the bazzlenitz for the springenwerk."
> ---
>   lib/ukalloc/alloc.c | 20 ++++++++++++++++++--
>   1 file changed, 18 insertions(+), 2 deletions(-)
>
> diff --git a/lib/ukalloc/alloc.c b/lib/ukalloc/alloc.c
> index 3260554..52e9a77 100644
> --- a/lib/ukalloc/alloc.c
> +++ b/lib/ukalloc/alloc.c
> @@ -124,7 +124,7 @@ static void *uk_get_real_start(const void *ptr)
>                                      (uintptr_t) __PAGE_SIZE);
>         if (intptr == ptr) {
>                 /* special case: the memory was page-aligned.
> -                * In this cas,e the size information lies at the start of the
> +                * In this case the size information lies at the start of the
>                  * previous page, with the rest of that page unused.
>                  */
>                 intptr -= __PAGE_SIZE;
Obviously no complaint from my side here. ;-) Always good to "drive-by 
patch" typos.
> @@ -135,8 +135,24 @@ static void *uk_get_real_start(const void *ptr)
>   static size_t uk_getmallocsize(const void *ptr)
>   {
>         size_t *intptr = uk_get_real_start(ptr);
> +       size_t mallocsize = __PAGE_SIZE << (*intptr);
>   
> -	return *intptr;
> +       if (((uintptr_t) ptr & (~__PAGE_MASK)) == 0) {
> +               /*
> +                * special case: the memory was page-aligned
> +                * In this case the allocated size should not account for the
> +                * previous page which was used for storing the order
> +                */
> +               mallocsize -= __PAGE_SIZE;
> +       } else {
> +               /*
> +                * If pointer is not page aligned it means the header is
> +                * on the same page. This will break if metadata size increases
> +                */
> +               mallocsize -= sizeof(*intptr);
> +       }
> +
> +       return mallocsize;
>   }
I agree that it probably makes sense for uk_getmallocsize() to return 
the size and not the page order. Plus, it indeed fixes a bug in the 
comparison in uk_realloc_ifpages(). However, I don't understand checks 
you do to change mallocsize. uk_get_real_start() already takes care of 
this, so you're overcompensating, aren't you?
>   
>   /* return the smallest order (1<<order pages) that can fit size bytes */

Cheers,
Florian

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel