[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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 */


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



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.