lib: libc: common: Export z_malloc_heap #79123
Conversation
The heap used by malloc/free is a static, private to the malloc implementation. Unfortunately, this makes it impossible to use tools like `sys_heap_runtime_stats_get()`, as they need access to the heap variable. Remedy this by removing the static from this declaration. Signed-off-by: David Brown <david.brown@linaro.org>
zephyrbot
added
size: XS
|
Personally, I would like to see Would that help? |
Having mallinfo() would definitely work for my purposes. I think you might only be able to fill in some of the fields, though. |
I'll whip up a PR |
|
a discord conversation concluded that mallinfo is not sufficient for David's purposes, so I'll stamp this as well. |
|
The problem with mallinfo is that is basically a dump of glibc malloc internals. I think we're better of just providing the Zephyr heap stats. Exporting this allows someone to do that on their own, or we could write wrappers. But, the symbol already has the prefix. |
|
With the heap listener API, I think the last time I looked at it, it only allowed for one listener. It might have been more of an implementation detail though, because the API itself looks capable of supporting many. If that limitation weren't present, it would be trivial to support any number of decent frontends for recording / tracing allocation events (for various definitions of decent). |
@d3zd3z What prevents us from defining our own |
| Z_LIBC_DATA static struct sys_heap z_malloc_heap; | ||
| Z_LIBC_DATA struct sys_heap z_malloc_heap; |
There was a problem hiding this comment.
I understand this can be useful for debugging purposes; but, this more or less creates an implicit contract between an internal implementation detail of common libc and whatever will be using this (another Zephyr component? user application?), and I anticipate we will be seeing bugs like "unresolved symbol z_malloc_heap" (or even worse "sys_heap_runtime_stats_get crashing because z_malloc_heap is no longer struct sys_heap") in the future if we do this.
I would say we should define and implement Zephyr-flavoured mallinfo with the fields we need if we want to make the internal heap stats available to the components external to the common libc; otherwise, patches/hacks like these should stay in one's local branch.
Technically, we could also make accessor functions to abstract things, removing the need for duplicate code everywhere. E.g. // struct mallinfo could have custom fields appended (e.g. uintptr_t).
// These could all be either real functions or inline
size_t mallinfo_get_avail_buckets(const struct mallinfo *info);
size_t mallinfo_get_free_bytes(const struct mallinfo *info);
size_t mallinfo_get_allocated_bytes(const struct mallinfo *info);
size_t mallinfo_get_max_allocated_bytes(const struct mallinfo *info);
// maybe a bucket accessor for testing or debug?
struct z_heap_bucket* mallinfo_get_bucket(can nst struct mallinfo *info, size_t index); |
|
I don't see a lot of benefit to adding the accessors. @cfriedt what do you think of pushing your mallinfo change, but just with fields that make sense for us (I'm even ok with the ifdefs, especially since it will make it clear to someone using the header what features they need to be able to get the information). |
Well, to "make progress", I just remove the 'static', but I'd rather not keep that patch locally. I'm inclined to define our own mallinfo. The main problem with separate accessors is that they aren't atomic, and might return inconsistent data if there are allocation operations between the calls (say between threads). |
|
Frankly, I'd like to see a general migration from custom malloc implementations to the common one. In that case, exposing the fact that it uses the standard Zephyr heap manager seems completely reasonable to me. Trying to construct new interfaces to hide the implementation doesn't seem valuable for the long term. Do we need patches that replace the newlib allocator with the common one? Are there other C library allocators in common use? |
Stephanos wouldn't approve that though, so let's find the next best compromise.
Accessors to a copy of a mallinfo on the stack don't need to be atomic but, in retrospect, that would be kind of annoying anyway to have one accessor per field. Not a great suggestion. My bad.
Let's start there - think that would make sense as long as there isn't a naming conflict. We don't need to support the GNU interface, but if someone else wants to implement it, they can in the future. What about this? // return a copy of the heap on the stack.
// copy is made atomically.
// mallinfoZ == z_heap, or some other derivative
struct mallinfoZ mallinfoZ(void);I'm not crazy about giving users all of the heap bucket pointers. Ideally, we could minimize the surface that footgun points at. Would it make sense to provide a subset of struct mallinfoZ {
size_t size_bytes;
size_t free_bytes;
size_t allocated_bytes;
size_t max_allocated_bytes;
};If users want something similar to the gnu mallinfo2, it's more of a future problem. As long as there is separation, it's fine. |
I'm not particularly fond of My usage is pretty simple. I have something that every 10 minutes or so prints out the malloc infomation, namely to make sure I don't have memory leaks. |
|
That works for me. |
The heap used by malloc/free is a static, private to the malloc implementation. Unfortunately, this makes it impossible to use tools like
sys_heap_runtime_stats_get(), as they need access to the heap variable.Remedy this by removing the static from this declaration.
Other options are to add an additional API to make the desired queries.