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

RFC: Provide k_realloc() #41151

Closed
brgl opened this issue Dec 14, 2021 · 9 comments · Fixed by #72483
Closed

RFC: Provide k_realloc() #41151

brgl opened this issue Dec 14, 2021 · 9 comments · Fixed by #72483
Assignees
Labels
area: Kernel RFC Request For Comments: want input from the community

Comments

@brgl
Copy link
Collaborator

brgl commented Dec 14, 2021

Introduction

Problem description

Zephyr provides k_malloc(), k_calloc() and k_free() but not k_realloc(). This RFC is about providing it or finding a different solution for the first potential user that is LVGL v8.

Background: Zephyr currently uses LVGL v7. I'm working on a changeset that will bump the LVGL version to v8 (link). This version however requires some realloc() implementation. Currently lvgl's Kconfig options allow to choose which allocator to use: the kernel heap, the libc implementation or the internal allocator in lvgl.

Proposed change

If we want to keep this triple choice then the kernel heap must introduce k_realloc().

Detailed RFC

Proposed change (Detailed)

Implement k_realloc() that would be the analog of the libc's realloc().

The prototype would look like this:

void *k_realloc(void *ptr, size_t size);

Internally it would check if size if bigger than the currently allocated chunk. If yes, it would allocate a new chunk, copy the contents into it, then free the current one. Otherwise it would just return the current pointer.

Dependencies

In linux we have the ksize() function that returns the actual size of the allocated block. We'd need some helper like this in zephyr in order to not have to look too deep into the heap code.

Concerns and Unresolved Questions

Is there any reason why k_realloc() is not already provided in zephyr?

Alternatives

Accept that lvgl cannot work with the kernel heap and move on.

@brgl brgl added the RFC Request For Comments: want input from the community label Dec 14, 2021
@carlocaione
Copy link
Collaborator

@npitre

@npitre
Copy link
Collaborator

npitre commented Dec 14, 2021

Is there any reason why k_realloc() is not already provided in zephyr?

Yes, most likely because no one needed it so far.

In linux we have the ksize() function that returns the actual size of the
allocated block. We'd need some helper like this in zephyr in order to not
have to look too deep into the heap code

The low-level heap layer has that. It is sys_heap_usable_size().

However k_malloc() is a layer above k_heap_alloc() which is a layer
above sys_heap_alloc() (and the aligned variation). You'd have to cross
the same layers to implement the equivalent of k_usable_size().

Also, the low-level heap layer already implements sys_heap_realloc().
It is highly recommended to use it and not try to be smart with
sys_heap_usable_size() as the heap allocator may extend the allocation
in-place even if sys_heap_usable_size() doesn't show it. In other words,
sys_heap_realloc() tries to be smarter for you.

Yet, if your memory needs are well characterized, you could create a heap
for your own usage and skip one or both top layers and the overhead
associated with them, and you wouldn't need a k_realloc().

@brgl
Copy link
Collaborator Author

brgl commented Mar 22, 2022

Idea was abandoned.

@ycsin
Copy link
Member

ycsin commented Dec 28, 2023

reopening this issue, as there are use cases that require it

@ycsin ycsin reopened this Dec 28, 2023
@npitre
Copy link
Collaborator

npitre commented Dec 28, 2023

Please describe those use cases so potential alternatives to k_realloc()
could be explored. Implementing k_realloc() comes with its share of
complexity and costs that, so far, weren't justified.

It is unfortunate that past discussions were not reflected here.

@ycsin
Copy link
Member

ycsin commented Dec 28, 2023

Please describe those use cases

The k_realloc() implementation would make the implementation in #67007 looks nicer, see #67007 (comment). I wouldn't be surprised if there are other similar patterns exist in the current codebase due to the lack of k_realloc(), and can be made simpler after its implementation.

@npitre
Copy link
Collaborator

npitre commented Jan 6, 2024

Sorry for the delay - I was on vacation.

The k_realloc() implementation would make the implementation
in #67007 looks nicer, see
#67007 (comment)

No, it won't. You simply cannot use any realloc() at all there.

  • Your struct nsem_obj contains a sys_snode_t which is a pointer, and
    more importantly being pointed to by another sys_snode_t elsewhere. Since
    *_realloc() may move memory content to a different memory block, you'll
    then have to find and fix up potential dangling list pointers.

  • Same goes for the sem_t member which many references based on its memory
    address are taken by the rest of the system. You won't be able to fix up
    those if the semaphore is live.

Of course, one could say that, in the discussion linked above, a realloc
would occur only when sem_unlink() is invoked meaning that the memory area
would shrink, and with the current allocator implementation, the memory area
would not be moved. But this is not guaranteed by the spec and therefore
a bad idea to rely on such internal implementation details.

@nashif nashif closed this as completed Feb 9, 2024
@maass-hamburg
Copy link
Collaborator

reopening this issue again, as there is a use cases that require it.

hawkBit is currently using realloc() at

rsp_tmp = realloc(hb_context.response_data, response_buffer_size);

not having a k_realloc() is hampering the move to zephyr specific heap functions in the hawkBit subsystem

@andyross
Copy link
Contributor

andyross commented May 8, 2024

As was pointed out upthread, this is mostly just an exercise in forwarding of APIs. Someone needs to:

  • Write a k_heap_realloc() in terms of sys_heap_aligned_realloc() (which already exists)
  • Write a k_realloc() in terms of a k_heap_realloc() call that uses the system heap

Neither is more than a few lines of code, and the existing code can be used as a guide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Kernel RFC Request For Comments: want input from the community
Projects
Status: Done
Development

Successfully merging a pull request may close this issue.

8 participants