From: Torbjorn SVENSSON <torbjorn.svensson@foss.st.com>
To: Jeff Johnston <jjohnstn@redhat.com>
Cc: Newlib <newlib@sourceware.org>
Subject: Re: [PATCH 2/2] Don't allocate another header when merging chunks
Date: Thu, 1 Sep 2022 21:04:15 +0200 [thread overview]
Message-ID: <c0606460-7835-060c-befe-0e147b128f0b@foss.st.com> (raw)
In-Reply-To: <CAOox84v+hWtFxXj4FVU=unxCgV7gTsb3QetHi0HC3zuQKaDujQ@mail.gmail.com>
Hi Jeff,
Thanks for the review.
No, I don't think the MALLOC_MINCHUNK should apply as I think is to
ensure that there is enough room for the chunk header.
/* size of smallest possible chunk. A memory piece smaller than this size
* won't be able to create a chunk */
#define MALLOC_MINCHUNK (CHUNK_OFFSET + MALLOC_PADDING + MALLOC_MINSIZE)
In this particular case, there is already a chunk, but it's a bit too
small to fit the requested size to malloc(). As a result, it should be
the requested size minus the size for the last chunk minus the size of
the chunk header. The remaining number of bytes is the require memory to
create a chunk (user data + chunk header) that is just the right size.
Think of it like this:
If there is enough free heap, then malloc(100) would just call
_sbrk(108) and return that space.
If that area is then free'ed, and a following malloc(200) is called,
then there are 3 scenarios:
a) there is enough free heap, so it will be like returning the space
returned by _sbrk(208).
b) these is not enough free heap, and the last chunk is still in use. In
this case, malloc would return 0 and errno should be ENOMEM.
c) there is not enough free heap, but the last chunk is no used, thus it
could be extended. If the the chunk was 108 bytes including the header,
then _sbrk(200-108) would be needed to create a single chunk that is 200
bytes + 8 bytes for the header.
If you look for the extreme, then the last chunk size could be 1 byte
less than what is requested of malloc, like the scenario above. The last
chunk size is 100 bytes (plus header) and the application calls
malloc(101). Then it would not make sense to apply MALLOC_MINCHUNK, as
that would call _sbrk(MALLOC_MINCHUNK) rather than _sbrk(1). In this
case, wasting MALLOC_MINCHUNK-1 bytes as they will never be access by
the application.
Does this make sense or am I overthinking this?
Kind regards,
Torbjörn
Ps. Header size for a chunk on arm-none-eabi is 8 bytes, for other
targets, the chunk header can be of different size, but I assumed
arm-none-eabi in my comment above to make it more clear.
Ps2. _sbrk and malloc is obviously more complicated than my example
above, but I tried to reduce the complexity to the involved logic for
extending a chunk size.
On 2022-09-01 20:44, Jeff Johnston wrote:
> I think that the check for MALLOC_MINCHUNK should still apply. Do you
> agree?
>
> -- Jeff J.
>
> On Tue, Aug 30, 2022 at 9:57 AM Torbjörn SVENSSON
> <torbjorn.svensson@foss.st.com <mailto:torbjorn.svensson@foss.st.com>>
> wrote:
>
> In the nano version of malloc, when the last chunk is to be extended,
> there is no need to acount for the header again as it's already taken
> into account in the overall "alloc_size" at the beginning of the
> function.
>
> Contributed by STMicroelectronics
>
> Signed-off-by: Torbjörn SVENSSON <torbjorn.svensson@foss.st.com
> <mailto:torbjorn.svensson@foss.st.com>>
> ---
> newlib/libc/stdlib/nano-mallocr.c | 4 ----
> 1 file changed, 4 deletions(-)
>
> diff --git a/newlib/libc/stdlib/nano-mallocr.c
> b/newlib/libc/stdlib/nano-mallocr.c
> index 43eb20e07..b2273ba60 100644
> --- a/newlib/libc/stdlib/nano-mallocr.c
> +++ b/newlib/libc/stdlib/nano-mallocr.c
> @@ -328,10 +328,6 @@ void * nano_malloc(RARG malloc_size_t s)
> /* 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)
> {
> --
> 2.25.1
>
next prev parent reply other threads:[~2022-09-01 19:04 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-30 13:56 [PATCH 1/2] Used chunk needs to be removed from free_list Torbjörn SVENSSON
2022-08-30 13:56 ` Torbjörn SVENSSON
2022-08-30 13:56 ` [PATCH 2/2] Don't allocate another header when merging chunks Torbjörn SVENSSON
2022-08-30 13:56 ` Torbjörn SVENSSON
2022-09-01 18:44 ` Jeff Johnston
2022-09-01 19:04 ` Torbjorn SVENSSON [this message]
2022-09-01 19:36 ` Jeff Johnston
2022-09-01 19:39 ` Jeff Johnston
2022-09-01 18:41 ` [PATCH 1/2] Used chunk needs to be removed from free_list Jeff Johnston
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=c0606460-7835-060c-befe-0e147b128f0b@foss.st.com \
--to=torbjorn.svensson@foss.st.com \
--cc=jjohnstn@redhat.com \
--cc=newlib@sourceware.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).