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/minimal: provide aligned allocators #29529

Closed
wants to merge 2 commits into from
Closed

libc/minimal: provide aligned allocators #29529

wants to merge 2 commits into from

Conversation

cfriedt
Copy link
Member

@cfriedt cfriedt commented Oct 25, 2020

This change adds support for standardized aligned allocation APIs via posix_memalign(3) and aligned_alloc(3) in the minimal libc.

Fixes #29519

@cfriedt cfriedt marked this pull request as draft October 25, 2020 13:49
@cfriedt cfriedt self-assigned this Oct 25, 2020
@github-actions github-actions bot added area: API Changes to public APIs area: C Library C Standard Library area: Kernel area: Tests Issues related to a particular existing or missing test labels Oct 25, 2020
@cfriedt cfriedt marked this pull request as ready for review October 25, 2020 19:06
@maxbachmann
Copy link
Contributor

The biggest issue that I can see is that sys_mem_pool does not support aligned allocations, and it is non-obvious to me how to make it support aligned allocations efficientlly. The easiest way to support aligned allocations is to use the k_heap API (which uses sys_heap* under the hood.

In #28611 the sys_mem_pool allocator gets removed

@andrewboie andrewboie closed this Oct 25, 2020
@andrewboie andrewboie reopened this Oct 25, 2020
@andrewboie
Copy link
Contributor

mis-click sorry

can you set this as a Draft PR since it is still WIP

@cfriedt cfriedt marked this pull request as draft October 25, 2020 21:10
@cfriedt
Copy link
Member Author

cfriedt commented Oct 26, 2020

The biggest issue that I can see is that sys_mem_pool does not support aligned allocations, and it is non-obvious to me how to make it support aligned allocations efficientlly. The easiest way to support aligned allocations is to use the k_heap API (which uses sys_heap* under the hood.

In #28611 the sys_mem_pool allocator gets removed

Now it all makes sense! Thanks for pointing that out @maxbachmann

@cfriedt cfriedt changed the title WIP: kernel: provide aligned variants for allocators libc/minimal: provide aligned allocators Nov 20, 2020
@cfriedt
Copy link
Member Author

cfriedt commented Nov 20, 2020

I cherry-picked from #30003 #28611, so please wait until after those are merged before reviewing.
cc @maxbachmann @andyross

@cfriedt cfriedt marked this pull request as ready for review November 20, 2020 18:15
@cfriedt cfriedt marked this pull request as draft November 20, 2020 18:20
@cfriedt
Copy link
Member Author

cfriedt commented Nov 20, 2020

Leaving this as a draft until #30003 and #28611 are merged.

Provide a standard aligned allocation API via posix_memalign(3)
and aligned_alloc(3) in the minimal libc.

Fixes #29519

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
This change adds tests for aligned allocators posix_memalign(3)
and aligned_alloc(3) in the minimal libc.

Fixes #29519

Signed-off-by: Christopher Friedt <chrisfriedt@gmail.com>
@cfriedt
Copy link
Member Author

cfriedt commented Dec 1, 2020

Closing this in favour of #30363 for a couple of reasons.

  • Newlib does not provide us with aligned_alloc(3) or posix_memalign(3) so it's pointless to offer in the minimal libc
  • @andrewboie said I should just provide a k_malloc_aligned() instead and I probably should have just listened to him in the first place 😉 but it's hard to remember random github comments on old PRs when jumping all over the stack.

In any case, #30363 is really just recycling the aligned_alloc(3) implementation and test case from here and applying them to k_malloc() instead, so I didn't really waste time / code... right? 🤷‍♂️

@cfriedt cfriedt closed this Dec 1, 2020
@pfalcon
Copy link
Contributor

pfalcon commented Dec 1, 2020

Newlib does not provide us with aligned_alloc(3) or posix_memalign(3) so it's pointless to offer in the minimal libc

I don't agree with the "pointless" valuation: POSIX provides them, that's pretty good a reason on its own.

That said, you're designer of this feature.

@cfriedt
Copy link
Member Author

cfriedt commented Dec 1, 2020

Newlib does not provide us with aligned_alloc(3) or posix_memalign(3) so it's pointless to offer in the minimal libc

I don't agree with the "pointless" valuation: POSIX provides them, that's pretty good a reason on its own.

Sorry, - it's definitely not "pointless". You're right. For my current purposes though (#29029) I unfortunately can't use posix_memalign() because it's not both in newlib and in the minimal libc.

I'll keep the branch around just in case we want to add this in the future. Personally, I think supporting standards are important.

@pfalcon
Copy link
Contributor

pfalcon commented Dec 1, 2020

I unfortunately can't use posix_memalign() because it's not both in newlib and in the minimal libc.

That at least should lead to traces of ideas for some kind of "polyfills" (curse JavaScript for abusing thru the entire industry), where we can add functionality for both newlib and minlibs. Actually, we do things like that already - newlib doesn't ship e.g. select() implementation, we do.

(That's not call for action, just call for thinking and maybe planning ahead.)

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: Bluetooth area: C Library C Standard Library area: Documentation area: Kernel area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kernel: provide aligned variants for allocators
4 participants