Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

libc/picolibc: Don't enable malloc by default #63259

Closed

Conversation

keith-packard
Copy link
Collaborator

@keith-packard keith-packard commented Sep 28, 2023

The common malloc implementation uses a heap which is initialized at system startup time. That gets linked into the application even if there are no references to malloc. This wastes memory and slows application startup.

When used with the minimal C library, this is "fixed" in the default configuration by setting the common malloc arena size to zero, removing the arena initialization code and providing stubs for malloc and free which are no-ops.

Picolibc adopted a different plan where a default malloc arena was provided when malloc was enabled. This ensures that malloc will be usable when it is available, which avoids a user trap where the application links but fails as soon as it tries to allocate any memory. However, that means that any application using picolibc will include the malloc arena initialization even if it doesn't use malloc. That costs a few hundred bytes of memory and a bit of startup time.

As an alternative, this patch series disables malloc entirely in the default configuration; appplications needing it will need to explicitly enable it in their configuration. All of the samples and tests which use malloc have their configurations updated.

Subsystems, drivers, libraries and modules which use malloc internally now signal their need for an allocator where necessary by selecting the REQUIRES_MALLOC Kconfig parameter.

nashif
nashif previously approved these changes Sep 28, 2023
@keith-packard
Copy link
Collaborator Author

I've hacked this up to add a new config entry, REQUIRES_MALLOC and making that drive whether COMMON_LIBC_MALLOC is selected by both picolibc and the minimal C library. This will allow libraries, drivers, modules and other components to indicate a dependence on malloc without reference to the common malloc implementation -- directly enabling COMMON_LIBC_MALLOC will pull in that code even if it isn't the right malloc implementation.

@kartben
Copy link
Collaborator

kartben commented Sep 29, 2023

This will allow libraries, drivers, modules and other components to indicate a dependence on malloc

And I think this PR should add such dependency to the likes of Thrift, TfLite, etc rather than doing it at the sample level, no?

@keith-packard
Copy link
Collaborator Author

This will allow libraries, drivers, modules and other components to indicate a dependence on malloc

And I think this PR should add such dependency to the likes of Thrift, TfLite, etc rather than doing it at the sample level, no?

It probably won't be perfect, but yeah, it would be better to at least get started with this.

@keith-packard
Copy link
Collaborator Author

rebased to fix conflict.

cfriedt
cfriedt previously approved these changes Nov 17, 2023
@@ -6,6 +6,7 @@
menuconfig NET_SOCKETS
bool "BSD Sockets compatible API"
select FDTABLE
select REQUIRES_MALLOC if DNS_RESOLVER || NET_IP
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

technically, getaddrinfo and freeaddrinfo could maybe use a slab if malloc is unavailable. What do you think @rlubos ? Not a change required here, but maybe a good future enhancement. Even malloc-less systems should be able to resolve.

@@ -7,6 +7,7 @@ config ZEPHYR_THRIFT_MODULE
menuconfig THRIFT
bool "Support for Thrift [EXPERIMENTAL]"
select EXPERIMENTAL
select REQUIRES_MALLOC
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day, It would be cool to not require a global allocator for Thrift

@keith-packard
Copy link
Collaborator Author

Seems like this might uncover more malloc dependencies. Having malloc exist but always fail (with a zero-sized arena) seems like a user trap to me?

Now that picolibc is the default C library, tests which want to check
the minimal library need to explictly select it.

Signed-off-by: Keith Packard <[email protected]>
Make sure these tests don't run when the underlying target requires a full
C library, and hence won't end up using the minimal C library anyways.

Signed-off-by: Keith Packard <[email protected]>
Instead of making applications use C library specific settings to enable
malloc support, provide this indirect symbol which then detects which C
library is in use and selects the correct configuration for each.

Signed-off-by: Keith Packard <[email protected]>
Instead of making applications use C library specific settings to specify
the desired minimum malloc arena size, provide this indirect symbol which
then detects which C library is in use and selects the correct
configuration for each.

Signed-off-by: Keith Packard <[email protected]>
Most C++ applications will end up using various language features which
involve allocating memory, so most applications will need
malloc. Applications not needing it may still set REQUIRES_MALLOC=n if
they don't.

Signed-off-by: Keith Packard <[email protected]>
The getaddrinfo function uses calloc when either a DNS resolver or an IP
stack is available. Indicate that by selecting REQUIRES_MALLOC when
necessary.

Signed-off-by: Keith Packard <[email protected]>
This module uses various malloc APIs internally.

Signed-off-by: Keith Packard <[email protected]>
These modules use malloc APIs internally. Add REQUIRES_MALLOC to their
Kconfig settings to ensure the C library provides an implementation.

Signed-off-by: Keith Packard <[email protected]>
This module uses malloc in the common code.

Signed-off-by: Keith Packard <[email protected]>
The da1469x_pd driver uses calloc when reading trimv information.

Signed-off-by: Keith Packard <[email protected]>
These hash_map tests all use malloc directly to prepare various
test elements.

Signed-off-by: Keith Packard <[email protected]>
This sample uses malloc for a local buffer.

Signed-off-by: Keith Packard <[email protected]>
strftime deals with timezones and the picolibc (and newlib) implementations
of that code currently uses malloc. That will get fixed upstream
eventually, but for now, we need malloc to use it.

Signed-off-by: Keith Packard <[email protected]>
These tests allocate output buffers at runtime.

Signed-off-by: Keith Packard <[email protected]>
This test uses malloc for temporary buffers.

Signed-off-by: Keith Packard <[email protected]>
This test uses malloc for output buffers.

Signed-off-by: Keith Packard <[email protected]>
Instead of relying on the default setting of `MINIMAL_LIBC_MALLOC` to pull
in the common malloc implementation, enable it whenever the application
indicates that it needs malloc through the REQUIRES_MALLOC setting.

Signed-off-by: Keith Packard <[email protected]>
When malloc is included in the build, the heap initialization code will be
pulled in even if the application doesn't actually use malloc. This
increases the size of executables by a few hundred bytes and delays system
startup.

Select COMMON_LIBC_MALLOC only when applications indicate that they require
malloc by setting REQUIRES_MALLOC.

Signed-off-by: Keith Packard <[email protected]>
Copy link
Member

@cfriedt cfriedt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still LGTM

@keith-packard
Copy link
Collaborator Author

I've just posted PR #65434 which is an alternative to this PR that avoids including the malloc initialization code by not using --whole-archive on the common libc library. Would that be a better choice to reduce memory usage by applications not using malloc? Or would it be better to have applications explicitly request malloc when they need it?

@keith-packard
Copy link
Collaborator Author

Ok, I think #65434 is the right way forward here -- applications do not have to declare whether they use malloc or not. When the don't, the malloc initialization code will not be included in the application.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

8 participants