public inbox for newlib@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
@ 2021-04-26  8:57 Ola Olsson
  2021-04-26  9:41 ` Corinna Vinschen
  2021-04-26 15:31 ` Keith Packard
  0 siblings, 2 replies; 12+ messages in thread
From: Ola Olsson @ 2021-04-26  8:57 UTC (permalink / raw)
  To: newlib; +Cc: ola1olsson, Ola Olsson

The only reason why it is tough for us to use nano malloc
is because of the small shortcoming where nano_malloc()
splits a bigger chunk from the free list into two pieces
while handing back the second one (the tail) to the user.
This is error prone and especially bad for smaller heaps,
where nano malloc is supposed to be superior. The normal
malloc doesn't have this issue and we need to use it even
though it costs us ~2k bytes compared to nano-malloc.

The problem arise especially after giving back _every_
malloced memory to the heap and then starting to exercise
the heap again by allocating something small. This small
item might split the whole heap in two equally big parts
depending on how the heap has been exercised before.

I have uploaded the smallest possible application
(only tested on ST and Nordic devices) to show the issue
while the real customer applications are far more complicated:
https://drive.google.com/file/d/1kfSC2KOm3Os3mI7EBd-U0j63qVs8xMbt/view?usp=sharing

The application works like the following pseudo code,
where we assume a heap of 100 bytes
(I haven't taken padding and other nitty and gritty
details into account. Everything to simplify understanding):

void *ptr = malloc(52); // We get 52 bytes and we have
                        // 48 bytes to use.
free(ptr); // Hand back the 52 bytes to nano_malloc
           // This is the magic line that shows the issue of
           // nano_malloc
ptr = malloc(1); // Nano malloc will split the 52 bytes
                 // in the free list and hand you a pointer
                 // somewhere in the
                 // middle of the heap.
ptr2 = malloc(52); // Out of memory...

I have done a fix which hands back the first part of the
splitted chunk. Once this is fixed we obviously
have the 1 byte placed in position 0 of the heap instead
of somewhere in the middle.

However, this won't let us malloc 52 new bytes even though
we potentially have 99 bytes left to use in the heap. The
reason is that when we try to do the allocation,
nano-malloc looks into the free list and sees a 51 byte
chunk to be used.
This is not big enough so nano-malloc decides to call
sbrk for _another_ 52 bytes which is not possible since
there is only 48 bytes left to ask for.

The solution for this problem is to check if the last
item in the free list is adjacent to sbrk(0). If it is,
as it is in this case, we can just ask sbrk for the
remainder of what is needed. In this case 1 byte.

NB! I have only tested the solution on our ST device.
---
 newlib/libc/stdlib/nano-mallocr.c | 70 +++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 9 deletions(-)

diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
index 1e0703948..1c7ab390b 100644
--- a/newlib/libc/stdlib/nano-mallocr.c
+++ b/newlib/libc/stdlib/nano-mallocr.c
@@ -264,11 +264,22 @@ void * nano_malloc(RARG malloc_size_t s)
         {
             if (rem >= MALLOC_MINCHUNK)
             {
-                /* Find a chunk that much larger than required size, break
-                * it into two chunks and return the second one */
-                r->size = rem;
-                r = (chunk *)((char *)r + rem);
-                r->size = alloc_size;
+                if (p == r)
+                {
+                    /* First item in the list, break it into two chunks
+                    *  and return the first one */
+                    r->size = alloc_size;
+                    free_list = (chunk *)((char *)r + alloc_size);
+                    free_list->size = rem;
+                    free_list->next = r->next;
+                } else {
+                    /* Any other item in the list. Split and return
+                    * the first one */
+                    r->size = alloc_size;
+                    p->next = (chunk *)((char *)r + alloc_size);
+                    p->next->size = rem;
+                    p->next->next = r->next;
+                }
             }
             /* Find a chunk that is exactly the size or slightly bigger
              * than requested size, just return this chunk */
@@ -297,11 +308,52 @@ void * nano_malloc(RARG malloc_size_t s)
         /* sbrk returns -1 if fail to allocate */
         if (r == (void *)-1)
         {
-            RERRNO = ENOMEM;
-            MALLOC_UNLOCK;
-            return NULL;
+            /* sbrk didn't have the requested amount. Let's check
+             * if the last item in the free list is adjacent to the 
+             * current heap end (sbrk(0)). In that case, only ask
+             * for the difference in size and merge them */
+            p = free_list;
+            r = p;
+
+            while (r)
+            {
+                p=r;
+                r=r->next;
+            }
+
+            if ((char *)p + p->size == (char *)_SBRK_R(RCALL 0))
+            {
+               /* The last free item has the heap end as neighbour.
+                * Let's ask for a smaller amount and merge */
+               alloc_size -= p->size;
+               alloc_size = ALIGN_SIZE(alloc_size, CHUNK_ALIGN); /* size of aligned data load */
+               alloc_size += MALLOC_PADDING; /* padding */
+               alloc_size += CHUNK_OFFSET; /* size of chunk head */
+               alloc_size = MAX(alloc_size, MALLOC_MINCHUNK);
+
+               if (sbrk_aligned(RCALL alloc_size) != (void *)-1)
+               {
+                   p->size += alloc_size;
+                   r = p;
+               }
+               else
+               {
+                   RERRNO = ENOMEM;
+                   MALLOC_UNLOCK;
+                   return NULL;
+               }
+            }
+            else
+            {
+                RERRNO = ENOMEM;
+                MALLOC_UNLOCK;
+                return NULL;
+            }
+        }
+        else
+        {
+            r->size = alloc_size;
         }
-        r->size = alloc_size;
     }
     MALLOC_UNLOCK;
 
-- 
2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-26  8:57 [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation Ola Olsson
@ 2021-04-26  9:41 ` Corinna Vinschen
  2021-04-26 15:31 ` Keith Packard
  1 sibling, 0 replies; 12+ messages in thread
From: Corinna Vinschen @ 2021-04-26  9:41 UTC (permalink / raw)
  To: newlib


The patch looks good to me at first glance, but I'd like to
get some review(s) from other nano-malloc stakeholders.


Thanks,
Corinna


On Apr 26 10:57, Ola Olsson wrote:
> The only reason why it is tough for us to use nano malloc
> is because of the small shortcoming where nano_malloc()
> splits a bigger chunk from the free list into two pieces
> while handing back the second one (the tail) to the user.
> This is error prone and especially bad for smaller heaps,
> where nano malloc is supposed to be superior. The normal
> malloc doesn't have this issue and we need to use it even
> though it costs us ~2k bytes compared to nano-malloc.
> 
> The problem arise especially after giving back _every_
> malloced memory to the heap and then starting to exercise
> the heap again by allocating something small. This small
> item might split the whole heap in two equally big parts
> depending on how the heap has been exercised before.
> 
> I have uploaded the smallest possible application
> (only tested on ST and Nordic devices) to show the issue
> while the real customer applications are far more complicated:
> https://drive.google.com/file/d/1kfSC2KOm3Os3mI7EBd-U0j63qVs8xMbt/view?usp=sharing
> 
> The application works like the following pseudo code,
> where we assume a heap of 100 bytes
> (I haven't taken padding and other nitty and gritty
> details into account. Everything to simplify understanding):
> 
> void *ptr = malloc(52); // We get 52 bytes and we have
>                         // 48 bytes to use.
> free(ptr); // Hand back the 52 bytes to nano_malloc
>            // This is the magic line that shows the issue of
>            // nano_malloc
> ptr = malloc(1); // Nano malloc will split the 52 bytes
>                  // in the free list and hand you a pointer
>                  // somewhere in the
>                  // middle of the heap.
> ptr2 = malloc(52); // Out of memory...
> 
> I have done a fix which hands back the first part of the
> splitted chunk. Once this is fixed we obviously
> have the 1 byte placed in position 0 of the heap instead
> of somewhere in the middle.
> 
> However, this won't let us malloc 52 new bytes even though
> we potentially have 99 bytes left to use in the heap. The
> reason is that when we try to do the allocation,
> nano-malloc looks into the free list and sees a 51 byte
> chunk to be used.
> This is not big enough so nano-malloc decides to call
> sbrk for _another_ 52 bytes which is not possible since
> there is only 48 bytes left to ask for.
> 
> The solution for this problem is to check if the last
> item in the free list is adjacent to sbrk(0). If it is,
> as it is in this case, we can just ask sbrk for the
> remainder of what is needed. In this case 1 byte.
> 
> NB! I have only tested the solution on our ST device.
> ---
>  newlib/libc/stdlib/nano-mallocr.c | 70 +++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 9 deletions(-)
> 
> diff --git a/newlib/libc/stdlib/nano-mallocr.c b/newlib/libc/stdlib/nano-mallocr.c
> index 1e0703948..1c7ab390b 100644
> --- a/newlib/libc/stdlib/nano-mallocr.c
> +++ b/newlib/libc/stdlib/nano-mallocr.c
> @@ -264,11 +264,22 @@ void * nano_malloc(RARG malloc_size_t s)
>          {
>              if (rem >= MALLOC_MINCHUNK)
>              {
> -                /* Find a chunk that much larger than required size, break
> -                * it into two chunks and return the second one */
> -                r->size = rem;
> -                r = (chunk *)((char *)r + rem);
> -                r->size = alloc_size;
> +                if (p == r)
> +                {
> +                    /* First item in the list, break it into two chunks
> +                    *  and return the first one */
> +                    r->size = alloc_size;
> +                    free_list = (chunk *)((char *)r + alloc_size);
> +                    free_list->size = rem;
> +                    free_list->next = r->next;
> +                } else {
> +                    /* Any other item in the list. Split and return
> +                    * the first one */
> +                    r->size = alloc_size;
> +                    p->next = (chunk *)((char *)r + alloc_size);
> +                    p->next->size = rem;
> +                    p->next->next = r->next;
> +                }
>              }
>              /* Find a chunk that is exactly the size or slightly bigger
>               * than requested size, just return this chunk */
> @@ -297,11 +308,52 @@ void * nano_malloc(RARG malloc_size_t s)
>          /* sbrk returns -1 if fail to allocate */
>          if (r == (void *)-1)
>          {
> -            RERRNO = ENOMEM;
> -            MALLOC_UNLOCK;
> -            return NULL;
> +            /* sbrk didn't have the requested amount. Let's check
> +             * if the last item in the free list is adjacent to the 
> +             * current heap end (sbrk(0)). In that case, only ask
> +             * for the difference in size and merge them */
> +            p = free_list;
> +            r = p;
> +
> +            while (r)
> +            {
> +                p=r;
> +                r=r->next;
> +            }
> +
> +            if ((char *)p + p->size == (char *)_SBRK_R(RCALL 0))
> +            {
> +               /* The last free item has the heap end as neighbour.
> +                * Let's ask for a smaller amount and merge */
> +               alloc_size -= p->size;
> +               alloc_size = ALIGN_SIZE(alloc_size, CHUNK_ALIGN); /* size of aligned data load */
> +               alloc_size += MALLOC_PADDING; /* padding */
> +               alloc_size += CHUNK_OFFSET; /* size of chunk head */
> +               alloc_size = MAX(alloc_size, MALLOC_MINCHUNK);
> +
> +               if (sbrk_aligned(RCALL alloc_size) != (void *)-1)
> +               {
> +                   p->size += alloc_size;
> +                   r = p;
> +               }
> +               else
> +               {
> +                   RERRNO = ENOMEM;
> +                   MALLOC_UNLOCK;
> +                   return NULL;
> +               }
> +            }
> +            else
> +            {
> +                RERRNO = ENOMEM;
> +                MALLOC_UNLOCK;
> +                return NULL;
> +            }
> +        }
> +        else
> +        {
> +            r->size = alloc_size;
>          }
> -        r->size = alloc_size;
>      }
>      MALLOC_UNLOCK;
>  
> -- 
> 2.25.1


^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-26  8:57 [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation Ola Olsson
  2021-04-26  9:41 ` Corinna Vinschen
@ 2021-04-26 15:31 ` Keith Packard
  2021-04-28 16:54   ` Ola Olsson
  1 sibling, 1 reply; 12+ messages in thread
From: Keith Packard @ 2021-04-26 15:31 UTC (permalink / raw)
  To: Ola Olsson, newlib; +Cc: Ola Olsson, ola1olsson

[-- Attachment #1: Type: text/plain, Size: 1258 bytes --]

Ola Olsson <ola1olsson@gmail.com> writes:

> The solution for this problem is to check if the last
> item in the free list is adjacent to sbrk(0). If it is,
> as it is in this case, we can just ask sbrk for the
> remainder of what is needed. In this case 1 byte.

I've already implemented both of these suggestions in the nano malloc
version included as part of picolibc. This version has some significant
additional work which fixes issues with memalign (does anyone use this
on embedded systems?).

https://github.com/picolibc/picolibc/blob/main/newlib/libc/stdlib/nano-mallocr.c

Growing the chunk adjacent to the brk is useful for both malloc and
realloc as this allows the common pattern of a buffer which grows
incrementally to be realloc'd in place.

I was hoping to get some time to push this upstream to newlib as you're
quite correct that having these two changes is very useful on limited
memory devices. However, I have been redirected away from this work.

There are also a couple of tests included with picolibc that uncovered
the issues with the original implementation:

https://github.com/picolibc/picolibc/blob/main/test/malloc.c
https://github.com/picolibc/picolibc/blob/main/test/malloc_stress.c

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-26 15:31 ` Keith Packard
@ 2021-04-28 16:54   ` Ola Olsson
  2021-04-28 18:19     ` Keith Packard
  0 siblings, 1 reply; 12+ messages in thread
From: Ola Olsson @ 2021-04-28 16:54 UTC (permalink / raw)
  To: Keith Packard; +Cc: newlib, Ola Olsson

Hi again,

I am sorry but I have no idea what picolibc is and I haven't seen any
errata/todo or anything in newlib indicating that this issue will be fixed.
I am just a simple programmer; I pulled latest and greatest of newlib, used
"git log" and inspected the last 7 commits of nano-mallocr.c and then I was
back in year 2017...Hence, I pretty much came to the conclusion that this
issue won't be fixed unless I fix it myself.

Please tell me if there is anything I can do or if something is expected
from me regarding the patch before it's getting merged.

Btw, there are some cleanup I would like to do in this file as well, mostly
regarding comments and tabs->spaces but I guess it's okay to send a new
patch for that.

On Mon, Apr 26, 2021 at 5:31 PM Keith Packard <keithp@keithp.com> wrote:

> Ola Olsson <ola1olsson@gmail.com> writes:
>
> > The solution for this problem is to check if the last
> > item in the free list is adjacent to sbrk(0). If it is,
> > as it is in this case, we can just ask sbrk for the
> > remainder of what is needed. In this case 1 byte.
>
> I've already implemented both of these suggestions in the nano malloc
> version included as part of picolibc. This version has some significant
> additional work which fixes issues with memalign (does anyone use this
> on embedded systems?).
>
>
> https://github.com/picolibc/picolibc/blob/main/newlib/libc/stdlib/nano-mallocr.c
>
> Growing the chunk adjacent to the brk is useful for both malloc and
> realloc as this allows the common pattern of a buffer which grows
> incrementally to be realloc'd in place.
>
> I was hoping to get some time to push this upstream to newlib as you're
> quite correct that having these two changes is very useful on limited
> memory devices. However, I have been redirected away from this work.
>
> There are also a couple of tests included with picolibc that uncovered
> the issues with the original implementation:
>
> https://github.com/picolibc/picolibc/blob/main/test/malloc.c
> https://github.com/picolibc/picolibc/blob/main/test/malloc_stress.c
>
> --
> -keith
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-28 16:54   ` Ola Olsson
@ 2021-04-28 18:19     ` Keith Packard
  2021-04-28 20:02       ` Ola Olsson
  0 siblings, 1 reply; 12+ messages in thread
From: Keith Packard @ 2021-04-28 18:19 UTC (permalink / raw)
  To: Ola Olsson; +Cc: newlib, Ola Olsson

[-- Attachment #1: Type: text/plain, Size: 1450 bytes --]

Ola Olsson <ola1olsson@gmail.com> writes:

> Hi again,
>
> I am sorry but I have no idea what picolibc is and I haven't seen any
> errata/todo or anything in newlib indicating that this issue will be fixed.
> I am just a simple programmer; I pulled latest and greatest of newlib, used
> "git log" and inspected the last 7 commits of nano-mallocr.c and then I was
> back in year 2017...Hence, I pretty much came to the conclusion that this
> issue won't be fixed unless I fix it myself.

Oh, sorry, picolibc is a newlib fork designed for embedded systems

        https://github.com/picolibc/picolibc

Picolibc release 1.5.1 has the updated nano-malloc code in place. This
is packaged in Debian unstable if you're using that. It also builds on
Windows platforms.

> Please tell me if there is anything I can do or if something is expected
> from me regarding the patch before it's getting merged.

I'd encourage you to take a look at the picolibc changes and see if
those look useful. I'm hoping to have some time to work on pushing them
back to newlib as they'd be useful there as well.

> Btw, there are some cleanup I would like to do in this file as well, mostly
> regarding comments and tabs->spaces but I guess it's okay to send a new
> patch for that.

I've been avoiding whitespace-only changes; I know the formatting is
terrible, but fixing it means back-porting changes becomes very difficult.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-28 18:19     ` Keith Packard
@ 2021-04-28 20:02       ` Ola Olsson
  2021-04-28 21:12         ` Keith Packard
  0 siblings, 1 reply; 12+ messages in thread
From: Ola Olsson @ 2021-04-28 20:02 UTC (permalink / raw)
  To: Keith Packard; +Cc: newlib, Ola Olsson

Okay, now I follow. Thanks for the explanation. I have only heard about
newlib and newlib nano since these are the options I get through most
UI/SDK from MCU manufacturers.

> I'd encourage you to take a look at the picolibc changes and see if
> those look useful. I'm hoping to have some time to work on pushing them
> back to newlib as they'd be useful there as well.

It's quite a big diff between the current newlib nano-malloc code and the
nano-malloc in picolibc even though the strategy is the same. The code
definitely looks okay from my point of view but it feels overkill for me to
bring in an overwrite merge (or a bigger patch) right now when the only
thing I need is the small patch I provided. At least since I haven't seen
any more issues with the newlib nano version of malloc.
My experience with the newlib nano-malloc is limited though, maybe you are
aware of more bugs, or maybe the picolibc version is easier to maintain? Or
what is the rational of bringing in the picolibc version of nano-malloc?

I am of course fine with you bringing in the nano-malloc of picolibc but
I'd suggest to merge my patch in the meantime. The first reason is that I
am afraid that it will take time to get the fixes backported from picolibc.
The other reason is that I want to have a commit that I can use as
reference for bug checking if I find something fishy in the picolibc
version of nano-malloc.

/Ola

On Wed, Apr 28, 2021 at 8:19 PM Keith Packard <keithp@keithp.com> wrote:

> Ola Olsson <ola1olsson@gmail.com> writes:
>
> > Hi again,
> >
> > I am sorry but I have no idea what picolibc is and I haven't seen any
> > errata/todo or anything in newlib indicating that this issue will be
> fixed.
> > I am just a simple programmer; I pulled latest and greatest of newlib,
> used
> > "git log" and inspected the last 7 commits of nano-mallocr.c and then I
> was
> > back in year 2017...Hence, I pretty much came to the conclusion that this
> > issue won't be fixed unless I fix it myself.
>
> Oh, sorry, picolibc is a newlib fork designed for embedded systems
>
>         https://github.com/picolibc/picolibc
>
> Picolibc release 1.5.1 has the updated nano-malloc code in place. This
> is packaged in Debian unstable if you're using that. It also builds on
> Windows platforms.
>
> > Please tell me if there is anything I can do or if something is expected
> > from me regarding the patch before it's getting merged.
>
> I'd encourage you to take a look at the picolibc changes and see if
> those look useful. I'm hoping to have some time to work on pushing them
> back to newlib as they'd be useful there as well.
>
> > Btw, there are some cleanup I would like to do in this file as well,
> mostly
> > regarding comments and tabs->spaces but I guess it's okay to send a new
> > patch for that.
>
> I've been avoiding whitespace-only changes; I know the formatting is
> terrible, but fixing it means back-porting changes becomes very difficult.
>
> --
> -keith
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-28 20:02       ` Ola Olsson
@ 2021-04-28 21:12         ` Keith Packard
  2021-04-29  5:11           ` Ola Olsson
  0 siblings, 1 reply; 12+ messages in thread
From: Keith Packard @ 2021-04-28 21:12 UTC (permalink / raw)
  To: Ola Olsson; +Cc: newlib, Ola Olsson

[-- Attachment #1: Type: text/plain, Size: 1969 bytes --]

Ola Olsson <ola1olsson@gmail.com> writes:

> Okay, now I follow. Thanks for the explanation. I have only heard about
> newlib and newlib nano since these are the options I get through most
> UI/SDK from MCU manufacturers.

Yeah, picolibc has only been around for a couple of years...

> It's quite a big diff between the current newlib nano-malloc code and the
> nano-malloc in picolibc even though the strategy is the same. The code
> definitely looks okay from my point of view but it feels overkill for me to
> bring in an overwrite merge (or a bigger patch) right now when the only
> thing I need is the small patch I provided. At least since I haven't seen
> any more issues with the newlib nano version of malloc.

The nano malloc in newlib doesn't handle the memalign call correctly,
which I suspect you aren't using. It's also has more overhead than the
picolibc version as it adds variable padding between the header and the
allocation.

> My experience with the newlib nano-malloc is limited though, maybe you are
> aware of more bugs, or maybe the picolibc version is easier to maintain? Or
> what is the rational of bringing in the picolibc version of
> nano-malloc?

The generated code is smaller, the allocation overhead is lower and the
API is more robust against invalid API usage.

> I am of course fine with you bringing in the nano-malloc of picolibc but
> I'd suggest to merge my patch in the meantime. The first reason is that I
> am afraid that it will take time to get the fixes backported from picolibc.
> The other reason is that I want to have a commit that I can use as
> reference for bug checking if I find something fishy in the picolibc
> version of nano-malloc.

If someone wants to work on your patches in newlib, that's fine; they're
definitely an improvement over the current code. My work is focused on
picolibc, and when I have some time, I try to get fixes merged into
newlib.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-28 21:12         ` Keith Packard
@ 2021-04-29  5:11           ` Ola Olsson
  2021-04-29 22:09             ` Keith Packard
  2021-04-30  2:11             ` Keith Packard
  0 siblings, 2 replies; 12+ messages in thread
From: Ola Olsson @ 2021-04-29  5:11 UTC (permalink / raw)
  To: Keith Packard; +Cc: newlib, Ola Olsson

Hi!

>The nano malloc in newlib doesn't handle the memalign call correctly,
>which I suspect you aren't using. It's also has more overhead than the
>picolibc version as it adds variable padding between the header and the
>allocation.
You are right. I missed the padding details. That's nice to get rid of.
And no, I am not using memalign at all. I only use newlib for embedded
systems.

> The generated code is smaller, the allocation overhead is lower and the
> API is more robust against invalid API usage.
Now when I skimmed through the code I couldn't find anything regarding the
API robustness.
Do you think you can point me in the right direction?
But yes, you are right here as well, the code is smaller and allocation
overhead lower so
the nano-malloc in picolibc is most likely an improvement.

>If someone wants to work on your patches in newlib, that's fine; they're
>definitely an improvement over the current code. My work is focused on
>picolibc, and when I have some time, I try to get fixes merged into
>newlib.

Great, I appreciate it. Tell me when you've hit the merge button and I'll
make a new GCC/Newlib version for my company
to use on our internal automatic integration system.

Finally I'd like to thank you for taking time to teach me about the
internals even though I assume you are busy enough as it is.

Take care

On Wed, Apr 28, 2021 at 11:12 PM Keith Packard <keithp@keithp.com> wrote:

> Ola Olsson <ola1olsson@gmail.com> writes:
>
> > Okay, now I follow. Thanks for the explanation. I have only heard about
> > newlib and newlib nano since these are the options I get through most
> > UI/SDK from MCU manufacturers.
>
> Yeah, picolibc has only been around for a couple of years...
>
> > It's quite a big diff between the current newlib nano-malloc code and the
> > nano-malloc in picolibc even though the strategy is the same. The code
> > definitely looks okay from my point of view but it feels overkill for me
> to
> > bring in an overwrite merge (or a bigger patch) right now when the only
> > thing I need is the small patch I provided. At least since I haven't seen
> > any more issues with the newlib nano version of malloc.
>
> The nano malloc in newlib doesn't handle the memalign call correctly,
> which I suspect you aren't using. It's also has more overhead than the
> picolibc version as it adds variable padding between the header and the
> allocation.
>
> > My experience with the newlib nano-malloc is limited though, maybe you
> are
> > aware of more bugs, or maybe the picolibc version is easier to maintain?
> Or
> > what is the rational of bringing in the picolibc version of
> > nano-malloc?
>
> The generated code is smaller, the allocation overhead is lower and the
> API is more robust against invalid API usage.
>
> > I am of course fine with you bringing in the nano-malloc of picolibc but
> > I'd suggest to merge my patch in the meantime. The first reason is that I
> > am afraid that it will take time to get the fixes backported from
> picolibc.
> > The other reason is that I want to have a commit that I can use as
> > reference for bug checking if I find something fishy in the picolibc
> > version of nano-malloc.
>
> If someone wants to work on your patches in newlib, that's fine; they're
> definitely an improvement over the current code. My work is focused on
> picolibc, and when I have some time, I try to get fixes merged into
> newlib.
>
> --
> -keith
>

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-29  5:11           ` Ola Olsson
@ 2021-04-29 22:09             ` Keith Packard
  2021-04-30  2:11             ` Keith Packard
  1 sibling, 0 replies; 12+ messages in thread
From: Keith Packard @ 2021-04-29 22:09 UTC (permalink / raw)
  To: Ola Olsson; +Cc: newlib, Ola Olsson

[-- Attachment #1: Type: text/plain, Size: 631 bytes --]

Ola Olsson <ola1olsson@gmail.com> writes:

> Now when I skimmed through the code I couldn't find anything regarding the
> API robustness.
> Do you think you can point me in the right direction?

Allocations of very large or negative amounts now fail appropriately;
there were a few corner cases which did the wrong thing before.

Oh, I also changed the behavior of 'malloc' to always set memory to
zero; that makes the system more consistent in that application failures
to initialize memory now always get the same result. It's a bit slower,
but I feel like the added layer of reliability is worth it.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-29  5:11           ` Ola Olsson
  2021-04-29 22:09             ` Keith Packard
@ 2021-04-30  2:11             ` Keith Packard
  2021-04-30 18:15               ` Ola Olsson
  1 sibling, 1 reply; 12+ messages in thread
From: Keith Packard @ 2021-04-30  2:11 UTC (permalink / raw)
  To: Ola Olsson; +Cc: newlib, Ola Olsson

[-- Attachment #1: Type: text/plain, Size: 374 bytes --]

Ola Olsson <ola1olsson@gmail.com> writes:

> Finally I'd like to thank you for taking time to teach me about the
> internals even though I assume you are busy enough as it is.

Oh, I should have linked the blog post I wrote about this a while ago:

        https://keithp.com/blogs/picolibc-news/

There's a long section on the nano malloc changes.

-- 
-keith

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-30  2:11             ` Keith Packard
@ 2021-04-30 18:15               ` Ola Olsson
  2021-05-03 11:02                 ` Corinna Vinschen
  0 siblings, 1 reply; 12+ messages in thread
From: Ola Olsson @ 2021-04-30 18:15 UTC (permalink / raw)
  To: Keith Packard, vinschen; +Cc: newlib, Ola Olsson

+Corinna

> If someone wants to work on your patches in newlib, that's fine; they're
>definitely an improvement over the current code. My work is focused on
> picolibc, and when I have some time, I try to get fixes merged into
>newlib.

Sorry Keith, English is not my native language and I don't know who to
contact in this newlib project. I guess that someone is Corinna?
Corinna, sorry for the long discussion. Keith has a superior nano-malloc in
his picolibc and I totally support it to be backported once he is ready.
Until then, I would like to get this patch merged if possible since it
could benefit us right now. Also, it can be used later for comparison if
(unlikely though) something goes haywire with that integration. That's my
two cents.

>Oh, I should have linked the blog post I wrote about this a while ago:
> https://keithp.com/blogs/picolibc-news/
> There's a long section on the nano malloc changes.

Keith. Thanks for the explanations regarding the API robustness and this
blog post. It's brilliant. Well written and nicely explained.

^ permalink raw reply	[flat|nested] 12+ messages in thread

* Re: [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation
  2021-04-30 18:15               ` Ola Olsson
@ 2021-05-03 11:02                 ` Corinna Vinschen
  0 siblings, 0 replies; 12+ messages in thread
From: Corinna Vinschen @ 2021-05-03 11:02 UTC (permalink / raw)
  To: newlib

On Apr 30 20:15, Ola Olsson wrote:
> +Corinna

Please don't CC me, I'm setting "Reply-To:" for a reason.  Thanks.

> > If someone wants to work on your patches in newlib, that's fine; they're
> >definitely an improvement over the current code. My work is focused on
> > picolibc, and when I have some time, I try to get fixes merged into
> >newlib.
> 
> Sorry Keith, English is not my native language and I don't know who to
> contact in this newlib project. I guess that someone is Corinna?

Patch pushed.


Thanks,
Corinna


^ permalink raw reply	[flat|nested] 12+ messages in thread

end of thread, other threads:[~2021-05-03 11:02 UTC | newest]

Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-26  8:57 [PATCH] Nano-malloc: Fix for unwanted external heap fragmentation Ola Olsson
2021-04-26  9:41 ` Corinna Vinschen
2021-04-26 15:31 ` Keith Packard
2021-04-28 16:54   ` Ola Olsson
2021-04-28 18:19     ` Keith Packard
2021-04-28 20:02       ` Ola Olsson
2021-04-28 21:12         ` Keith Packard
2021-04-29  5:11           ` Ola Olsson
2021-04-29 22:09             ` Keith Packard
2021-04-30  2:11             ` Keith Packard
2021-04-30 18:15               ` Ola Olsson
2021-05-03 11:02                 ` Corinna Vinschen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).