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

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Radu,

On 07/24/2018 11:47 AM, Radu Nicolau wrote:
>         @@ -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?
>
> uk_getmallocsize() takes the order from uk_get_real_start() and does 
> (pagesize<<order). The problem is that the result of that shift will 
> include the size of the metadata. The check I introduces determines the 
> size of the metadata depending on whether the pointer is page aligned or 
> not which it then subtracts from the mallocsize. You need to do this as 
> realloc needs the size of the malloc (without metadata size).
> I agree that functions check for the same thing, but the actions differ 
> - uk_get_real_start() determines location of the metadata, 
> uk_getmallocsize() uses the metadata and determines the allocated size.
Oh, right. I misread your if-statement. This sounds correct indeed.

If you submit a v2 with the slightly updated commit message, I'll flag 
this as reviewed.
Thanks,
Florian

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