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

kernel: add k_heap_aligned_alloc #30003

Merged
merged 1 commit into from
Dec 8, 2020
Merged

kernel: add k_heap_aligned_alloc #30003

merged 1 commit into from
Dec 8, 2020

Conversation

maxbachmann
Copy link
Contributor

@maxbachmann maxbachmann commented Nov 13, 2020

k_heap did not have an aligned alloc function, even though
this is supported by the internal sys_heap. (implements #29631)

@andrewboie in #29029 you wrote that k_heap has a facility for aligned allocations you should use that instead of k_malloc to reserve memory, but apparently this is not possible for k_heap yet

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Definitely useful. Not loving the cut/paste though, can we fix?

kernel/kheap.c Outdated
@@ -51,6 +51,31 @@ void *k_heap_alloc(struct k_heap *h, size_t bytes, k_timeout_t timeout)
return ret;
}

void *k_heap_aligned_alloc(struct k_heap *h, size_t align, size_t bytes, k_timeout_t timeout)
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than cutting and pasting the synchronization as boilerplate from k_heap_alloc(), could you please refactor the two so they use the same implementation? One simple option would be to implement k_heap_alloc() in terms of k_heap_aligned_alloc(). Or if we care about the mild performance overhead (IMHO we don't) implement both in terms of an inline function where the alignment code optimizes away.

@andrewboie
Copy link
Contributor

Looks good now. CI failure looks like some infrastructure problem, can you rebase and push again, see if it goes away?

@andrewboie andrewboie requested a review from cfriedt November 13, 2020 20:40
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.

Looks good to me. Thanks for getting this in!

k_heap did not have an aligned alloc function, even though
this is supported by the internal sys_heap.

Signed-off-by: Maximilian Bachmann <m.bachmann@acontis.com>
@maxbachmann
Copy link
Contributor Author

@andyross can you re-review this

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

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

Sorry, my concerns were addressed, this looks great.

@nashif nashif merged commit 34d7c78 into zephyrproject-rtos:master Dec 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: API Changes to public APIs area: Kernel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants