Re: Malloc: unusable area at the end of the heap section

[Jeff Johnston <jjohnstn@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 6703 bytes --]

Ok,

I am attaching the modified patch after discussing with Torbjorn about the
licensing.  Torbjorn, please
review and let me know if there are changes you would like, otherwise, I
will push.

-- Jeff J.


On Wed, Sep 14, 2022 at 4:52 PM Jeff Johnston <jjohnstn@redhat.com> wrote:

> Hi Torbjorn,
>
> I took a look at what would be needed to make this more generic.  I have a
> patch almost ready, but
> the one issue is that your libc/sys/arm/sysconf.c file does not have a
> license.  Could you please append a
> newer version of the file with a license and I will complete the patch for
> you to review?
>
> I decided to go with a simple HAVE_xxxx macro rather than a configure
> option so any platform can
> simply set the flag in configure.host.
>
> -- Jeff J.
>
> On Tue, Jun 7, 2022 at 12:03 PM Jeff Johnston <jjohnstn@redhat.com> wrote:
>
>> Hi Torbjorn,
>>
>> I think it would be useful.  Do you want to modify the patch to be more
>> generic?  I am thinking of a var set in configure.host that sets a compile
>> flag at the end such as _USE_SYSCONF_FOR_PAGESIZE.  Then,
>> the default sysconf.c can be put in the newlib/libc/unix directory.  It
>> is then a straight-forward exercise to add this as an enablement
>> configuration option.  What do you think?
>>
>> -- Jeff J.
>>
>> On Tue, Jun 7, 2022 at 2:18 AM Torbjorn SVENSSON via Newlib <
>> newlib@sourceware.org> wrote:
>>
>>> Hello,
>>>
>>> A while back, I provided a patch[1] to newlib that would allow the
>>> application to override the pagesize for malloc, but the patch got stalled.
>>> Maybe this would be a good time to take another look at the patch and see
>>> if it would actually fix the generic newlib usage or if it's still
>>> something that is only applicable for small embedded targets.
>>>
>>> [1] https://ecos.sourceware.org/ml/newlib/current/017616.html
>>>
>>> Kind regards,
>>> Torbjörn
>>>
>>> > -----Original Message-----
>>> > From: Newlib <newlib-
>>> > bounces+torbjorn.svensson=st.com@sourceware.org> On Behalf Of Jerome
>>> > Leroux
>>> > Sent: den 6 juni 2022 23:18
>>> > To: newlib@sourceware.org
>>> > Subject: Malloc: unusable area at the end of the heap section
>>> >
>>> > Hello Newlib developers,
>>> >
>>> > I am a user of Newlib in a project that runs on an NXP MCU.
>>> > I am using MCUXpressoIDE_11.3.0_5180_prc3, which comes with GCC “arm-
>>> > none-eabi-gcc.exe (GNU Arm Embedded Toolchain
>>> > 9-2020-q2-update) 9.3.1 20200408 (release)” and Newlib 3.3.0.
>>> >
>>> > I have identified an issue in malloc, and I think the problem is still
>>> present in
>>> > the latest version of Newlib. I could
>>> > not see any changes in the incriminated code since Newlib 3.3.0.
>>> >
>>> > I noticed this issue only in the standard malloc implementation and
>>> not in the
>>> > nano-malloc version.
>>> >
>>> > Here is a description of the problem:
>>> > The allocator splits the heap into pages. When a page is full, it
>>> increases the
>>> > heap size by reserving a new page in the
>>> > heap section. When reserving a new page, the allocator keeps the page
>>> end
>>> > address aligned with malloc_getpagesize, which
>>> > is set to 4096 by default. If there is not enough space to reserve the
>>> full page,
>>> > the allocation fails even if there is
>>> > enough space in the heap to allocate the chunk of memory.
>>> > Because the issue is related to the heap end address and how the linker
>>> > positions the heap, the same sequence of
>>> > allocations may lead to different results (failure or success)
>>> depending on the
>>> > location of the heap, even if the heap
>>> > size is constant. Typically, adding a new C global variable can shift
>>> the start
>>> > address of the heap section and cause a
>>> > malloc error.
>>> >
>>> > For example, with a heap section of 4096 bytes (0x1000 bytes):
>>> > If the heap section address is 0x20100-0x21100, during the
>>> initialization, the
>>> > page end address is set to 0x21000
>>> > (aligned on 4096). We will be able to allocate until the address
>>> 0x21000. After
>>> > that, the allocator will try to reserve
>>> > a new page, but it will fail because it won’t be able to reserve a
>>> 4096 bytes
>>> > page from 0x21000 to 0x22000. The
>>> > following allocations will fail. The usable heap size is 3840 bytes
>>> (0x21000 -
>>> > 0x20100) instead of 4096.
>>> > If the heap section address is 0x20F00-0x21F00 (same size), with the
>>> same
>>> > scenario, the usable heap size is 256 bytes
>>> > (0x21000 - 0x20F00).
>>> > Here are two examples of heap configurations:
>>> > https://gist.github.com/jerome-
>>> > leroux/759159fbd3e7bb5e189dbceb04636914?permalink_comment_id=4191
>>> > 266#gistcomment-4191266
>>> >
>>> > I did not dig into the implementation so much. From my understanding,
>>> the
>>> > problem comes from the usage of
>>> > "malloc_getpagesize" (see
>>> > https://github.com/bminor/newlib/blob/830a9b707caa5e343b6ffce7fcb2d3c
>>> > a97e3259c/newlib/libc/stdlib/_mallocr.c#L198) in
>>> > "malloc_extend_top" (probably here
>>> > https://github.com/bminor/newlib/blob/830a9b707caa5e343b6ffce7fcb2d3c
>>> > a97e3259c/newlib/libc/stdlib/_mallocr.c#L2166).
>>> > I can understand it makes sense to keep the pages aligned when running
>>> in a
>>> > system that implements virtual memory.
>>> > Still, on an MCU, the heap is just a contiguous chunk of memory
>>> allocated at
>>> > link time. Furthermore, the heap size is
>>> > usually pretty small (a few kilobytes), so potentially wasting 4 KB of
>>> memory
>>> > is unacceptable. Using the default
>>> > implementation of "sbrk" documented at
>>> > https://sourceware.org/newlib/libc.html#index-sbrk will lead to the
>>> > problem.
>>> >
>>> > I have written a simple example that demonstrates the issue (see
>>> > https://gist.github.com/jerome-
>>> > leroux/759159fbd3e7bb5e189dbceb04636914 ). To reproduce the problem,
>>> > define the macros
>>> > HEAP_SECTION_START_SYMBOL and HEAP_SECTION_END_SYMBOL, which
>>> > are specific to your environment. Then call the function
>>> > "test_malloc()".
>>> >
>>> > I tried to find someone with the same issue, but I couldn’t. The
>>> related
>>> > commits/discussions I found are:
>>> > -
>>> > https://github.com/bminor/newlib/commit/4a3d0a5a5d829c05868a34658eb
>>> > 45731dbb5112b
>>> > -
>>> https://stackoverflow.com/questions/39088598/malloc-in-newlib-does-it-
>>> > waste-memory-after-one-big-failure-allocation
>>> >
>>> > Can anyone confirm what I have noticed?
>>> >
>>> > Thanks.
>>> >
>>> > --
>>> > Jerome Leroux
>>>
>>>

[-- Attachment #2: 0001-Implement-sysconf-for-Arm.patch --]
[-- Type: text/x-patch, Size: 3643 bytes --]

From aae3831e6334eed571e8c2eb04295e53cf24f024 Mon Sep 17 00:00:00 2001
From: Jeff Johnston <jjohnstn@redhat.com>
Date: Fri, 16 Sep 2022 16:04:21 -0400
Subject: [PATCH] Implement sysconf for Arm

- add support for using sysconf to get page size in _mallocr.c via
  HAVE_SYSCONF_PAGESIZE flag set in configure.host
- set flag in configure.host for arm and add a default sysconf implementation
  in libc/sys/arm that returns the page size
- the default implementation can be overridden outside newlib to allow a
  different page size to improve malloc on devices with a small footprint
  without needing to rebuild newlib
- this patch is based on a contribution from Torbjorn Svensson and
  Niklas Dahlquist (https://ecos.sourceware.org/ml/newlib/current/017616.html)
---
 newlib/configure.host            |  2 ++
 newlib/libc/stdlib/_mallocr.c    |  2 ++
 newlib/libc/sys/arm/Makefile.inc |  2 +-
 newlib/libc/sys/arm/sysconf.c    | 30 ++++++++++++++++++++++++++++++
 4 files changed, 35 insertions(+), 1 deletion(-)
 create mode 100644 newlib/libc/sys/arm/sysconf.c

diff --git a/newlib/configure.host b/newlib/configure.host
index 98ce07d..32d1436 100644
--- a/newlib/configure.host
+++ b/newlib/configure.host
@@ -628,6 +628,7 @@ newlib_cflags="${newlib_cflags} -DCLOCK_PROVIDED -DMALLOC_PROVIDED -DEXIT_PROVID
 	;;
   arm*-*-pe)
 	syscall_dir=syscalls
+	newlib_cflags="${newlib_cflags} -DHAVE_SYSCONF_PAGESIZE"
 	;;
   arm*-*-*)
 	syscall_dir=syscalls
@@ -642,6 +643,7 @@ newlib_cflags="${newlib_cflags} -DCLOCK_PROVIDED -DMALLOC_PROVIDED -DEXIT_PROVID
 #         newlib_cflags="${newlib_cflags} -DARM_RDP_MONITOR"
 	  newlib_cflags="${newlib_cflags} -DARM_RDI_MONITOR"
 	fi
+	newlib_cflags="${newlib_cflags} -DHAVE_SYSCONF_PAGESIZE"
 	;;
   avr*)
 	newlib_cflags="${newlib_cflags} -DNO_EXEC -DSMALL_MEMORY -DMISSING_SYSCALL_NAMES"
diff --git a/newlib/libc/stdlib/_mallocr.c b/newlib/libc/stdlib/_mallocr.c
index 4b53997..1997b6d 100644
--- a/newlib/libc/stdlib/_mallocr.c
+++ b/newlib/libc/stdlib/_mallocr.c
@@ -320,12 +320,14 @@ extern "C" {
 #endif
 
 #ifndef _WIN32
+#ifndef HAVE_SYSCONF_PAGESIZE
 #ifdef SMALL_MEMORY
 #define malloc_getpagesize (128)
 #else
 #define malloc_getpagesize (4096)
 #endif
 #endif
+#endif
 
 #if __STD_C
 extern void __malloc_lock(struct _reent *);
diff --git a/newlib/libc/sys/arm/Makefile.inc b/newlib/libc/sys/arm/Makefile.inc
index 490a963..0122956 100644
--- a/newlib/libc/sys/arm/Makefile.inc
+++ b/newlib/libc/sys/arm/Makefile.inc
@@ -1,6 +1,6 @@
 AM_CPPFLAGS_%C% = -I$(srcdir)/libc/machine/arm
 
-libc_a_SOURCES += %D%/access.c %D%/aeabi_atexit.c
+libc_a_SOURCES += %D%/access.c %D%/aeabi_atexit.c %D%/sysconf.c
 if MAY_SUPPLY_SYSCALLS
 libc_a_SOURCES += %D%/libcfunc.c %D%/trap.S %D%/syscalls.c
 endif
diff --git a/newlib/libc/sys/arm/sysconf.c b/newlib/libc/sys/arm/sysconf.c
new file mode 100644
index 0000000..dbed7d7
--- /dev/null
+++ b/newlib/libc/sys/arm/sysconf.c
@@ -0,0 +1,30 @@
+/* libc/sys/arm/sysconf.c - The sysconf function */
+
+/* Copyright 2020, STMicroelectronics
+ *
+ * All rights reserved.
+ *
+ * Redistribution, modification, and use in source and binary forms is permitted
+ * provided that the above copyright notice and following paragraph are
+ * duplicated in all such forms.
+ *
+ * This file is distributed WITHOUT ANY WARRANTY; without even the implied
+ * warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ */
+
+#include <unistd.h>
+#include <errno.h>
+
+long sysconf(int name)
+{
+  switch (name)
+  {
+  case _SC_PAGESIZE:
+    return 4096;
+
+  default:
+    errno = EINVAL;
+    return -1;
+  }
+  return -1; /* Can't get here */
+}
-- 
1.8.3.1