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

Deprecate k_mem_pool API, remove sys_mem_pool allocator #28611

Merged
merged 23 commits into from Dec 8, 2020

Conversation

andyross
Copy link
Contributor

@andyross andyross commented Sep 22, 2020

The k_heap/sys_heap code has been default for a release, so it's time to deprecate the old allocator.

This eliminates the older CONFIG_MEM_POOL_HEAP_BACKEND kconfig, effectively making it true always and removing the ability for applications to request the older pool engine. It then deprecates the older APIs, leaving them in place for internal users as a "Z" API.

There are still a few users of the older API internally that will need cleanup (edit: not anymore, it's 100% gone in the current version). I'll submit those separately I think, I'm not sure if those are in scope for 2.4 or not:

  • k_malloc and the system pool are still implemented in terms of mem_pool, even though those are just wrappers now. It should be directly on top of a k_heap.

  • The per-thread resource pools likewise are mem_pools still. This should be k_heap's instead.

  • Mailbox has a API that will retrieve a message into a newly-allocated heap block. This API is weird and itself deprecated, but if we decide we actually want to preserve the functionality it will need some kind of new variant.

  • Similarly pipe has block usage internally.

And there are two places where the underlying sys_heap allocator still gets used:

  • The minimal libc malloc() implementation is on top of sys_heap instead of k_malloc. I'm not sure why, but this needs some kind of port.

  • The lib/gui/lvgl code has an internal sys_mem_pool and needs a port to sys_heap (not hard to write, but I have no idea how to test it)

Fixes #24358

@andyross
Copy link
Contributor Author

Ugh, it collides. Sorry, I did this on a week-stale master. Will get that cleaned up.

nashif
nashif previously approved these changes Sep 22, 2020
@nashif nashif dismissed their stale review September 23, 2020 10:57

need CI to pass

@pabigot
Copy link
Collaborator

pabigot commented Sep 23, 2020

I'm not clear on how #24358 is a bug, let alone having a release-blocking priority, except as a means of getting this late change into 2.4.0. And I've been told before that stage of release cycle is not a valid reason for rejecting a PR.

But from policy discussion heard at the last TSC regarding criteria for accepting late addition of new boards I need to ask: If a PR like this came three days before release from anybody other than a platinum member company, would people be seriously considering merging it into the release when it passes?

@andrewboie
Copy link
Contributor

I'm not clear on how #24358 is a bug

It's a bug because we have two mechanisms in the kernel which do the same thing, and we do not want to maintain both, particularly not in the second LTS release.

Andy Ross added 11 commits December 7, 2020 12:30
This data structure is going away, and its replacement (sys_heap) has
tests already.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This code used a sys_mem_pool directly.  Use a new-style heap instead
to do the same thing.

(Note that the usage is a little specious -- it allocates from the
heap but doesn't appear to fill or check any data therein, just that
the heap memory can be copied from the two memory domains.  It's
unclear exactly what this is trying to demonstrate and we might want
to improve the sample to do something less trivial.)

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This has been replaced by sys_heap now and all dependencies are gone.
Remove.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The kernel resource pool is now a k_heap.  There is a compatibility
API still, but this is a core test that should be exercising the core
API.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
These two test cases were making whitebox assumptions of both the
block header size and memory layout of an old-style k_mem_pool that
aren't honored by the k_heap allocator.  They aren't testing anything
that isn't covered elsewhere.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This test was written to use a TINY system heap (64 bytes) from which
it has to allocate on behalf of a userspace process.  The change in
convention from mem_pool (where the byte count now includes metadata
overhead) means it runs out of space.  Bump to 192 bytes.  Still tiny.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
On userspace platforms, this test needs a little bit of kernel heap.
The old mem_pool number was specified without metadata overhead
(i.e. it reflected 128 bytes of actual data available and the metadata
was stored silently somewhere else), where the new heap specifies the
size of the contiguous buffer in memory that stores both data and
chunk headers, etc...

Increase to 256 bytes.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The sys_mem_pool data structure is going away.  And this test case
didn't actually do much.  All it did was create a sys_mem_pool in the
app data section (I guess that's the "mem_protect" part?) and validate
that it was usable.  We have tests for sys_heap to do that already
elsewhere anyway; no point in porting.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This tiny header uses non-builtin types but includes no headers that
would define them.  Recent header motion seems to have exposed a case
where this file can get built before its dependencies are included.
Add the header directly.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The mailbox and msgq utilities had API variants that could pass old
mem_pool blocks through the data structure.  That API is being
deprected (and the features were obscure), so remove the internal
support.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Remove test cases that exercise the deprecated mem_pool features of
the pipe utility.

Note that this leaves comparatively few cases left, we should probably
audit coverage after this merges and rewrite tests that aren't
interdependent.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@andyross
Copy link
Contributor Author

andyross commented Dec 7, 2020

Heh, usb_dc_nrfx was one of the blind changes, isn't part of a default sanitycheck, but is part of CI. Fixed typo, repushed.

@nashif
Copy link
Member

nashif commented Dec 7, 2020

nrf52_bsim is unrelated and caused by #29810

Andy Ross added 3 commits December 7, 2020 14:19
Use the core k_heap API pervasively within our tree instead of the
z_mem_pool wrapper that provided compatibility with the older mempool
implementation.

Almost all of this is straightforward swapping of one alloc/free call
for another.  In a few cases where code was holding onto an old-style
"mem_block" a local compatibility struct with a single field has been
swapped in to keep the invasiveness of the changes down.

Note that not all the relevant changes in this patch have in-tree test
coverage, though I validated that it all builds.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Remove the declarations for the older mempool API

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
These implemented a k_mem_pool in terms of the now universal k_heap
utility.  That's no longer necessary now that the k_mem_pool API has
been removed.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@andyross
Copy link
Contributor Author

andyross commented Dec 7, 2020

Next spin: now-unused variable warning in hid-cdc sample

@andrewboie andrewboie dismissed cvinayak’s stale review December 7, 2020 22:47

stale review, responded

@nashif nashif merged commit 3c2c1d8 into zephyrproject-rtos:master Dec 8, 2020
pabigot added a commit to pabigot/zephyr that referenced this pull request Dec 10, 2020
Rework in zephyrproject-rtos#28611 left an old argument in place and missed a type check.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
MaureenHelm pushed a commit that referenced this pull request Dec 10, 2020
Rework in #28611 left an old argument in place and missed a type check.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
bwasim pushed a commit to bwasim/zephyr that referenced this pull request Jan 5, 2021
Rework in zephyrproject-rtos#28611 left an old argument in place and missed a type check.

Signed-off-by: Peter Bigot <peter.bigot@nordicsemi.no>
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: C Library C Standard Library area: Documentation area: File System area: Kernel area: Networking area: Samples Samples area: Tests Issues related to a particular existing or missing test platform: NXP NXP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deprecate and remove old k_mem_pool / sys_mem_pool APIs
10 participants