Xen project Mailing List

Hi Florian,

I've responded below

On Mon, Jul 23, 2018 at 5:52 PM, Florian Schmidt <Florian.Schmidt@xxxxxxxxx> wrote:

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."

I'll format the patches I send like this from now on :)

---
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?

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.

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

Cheers,

Radu

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