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

Open
wants to merge 18 commits into
base: master
from

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. 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

@andyross andyross commented Sep 22, 2020

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

@andyross andyross force-pushed the andyross:mem-pool-deprecate branch 2 times, most recently from 18be030 to 97b706c Sep 22, 2020
@andyross andyross requested review from pabigot and vanwinkeljan as code owners Sep 22, 2020
@jukkar
jukkar approved these changes Sep 23, 2020
@nashif nashif dismissed their stale review Sep 23, 2020

need CI to pass

@pabigot
Copy link
Contributor

@pabigot 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

@andrewboie andrewboie commented Sep 23, 2020

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.

andyross added 17 commits Sep 22, 2020
Remove the MEM_POOL_HEAP_BACKEND kconfig, treating it as true always.
Now the legacy mem_pool cannot be enabled and all usage uses the
k_heap/sys_heap backend.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
This API is being deprecated, and the underlying sys_heap code has its
tests elsewhere.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Mark all k_mem_pool APIs deprecated for future code.  Remaining
internal usage now uses equivalent "z_mem_pool" symbols instead.

Fixes #24358

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The sys_mem_pool allocator is a legacy thing.  Use the standard heap
to reduce code size.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
These were implemented in terms of the mem_pool/block API directly
(for complicated reasons, the pointers returned from this API may have
been allocated from allocators other than the single system heap).
Have them use a k_heap instead.

Requires a tweak to one test which had hard-coded an assumption about
the header size.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The k_mem_pool allocator is no more, and the z_mem_pool compatibility
API is going away.  The internal allocator should be a k_heap always.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Add an optimized realloc() implementation that can successfully expand
allocations in place if there exists enough free memory after the
supplied block.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
The older sys_mem_pool is going away and being replaced by a new
allocator.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
Simple unit test for the realloc functionality, covering all state
transitions.

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
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>
@andyross andyross force-pushed the andyross:mem-pool-deprecate branch from 9b15ab4 to f0f60a5 Oct 26, 2020
@nashif nashif requested a review from andrewboie Oct 26, 2020
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>
@andyross
Copy link
Contributor Author

@andyross andyross commented Oct 26, 2020

I'd been assuming those weird BSIM errors (in code that has nothing to do with mem_pool) were an upstream thing, but they turned out to be a latent bug with a header (missing stdint.h) that the removal of mempool_sys.h exposed. Fixed.

Copy link
Contributor

@andrewboie andrewboie left a comment

one nit about an API name change. rest looks fine.

I think we will need to file bugs or otherwise figure out what to do about the remaining uses of z_mem_pool_* inside the kernel so that we can remove all the z_mem_pool code entirely for LTS2, but we have time.

thread->resource_pool = heap;
}

static inline void z_thread_resource_pool_assign(struct k_thread *thread,

This comment has been minimized.

@andrewboie

andrewboie Oct 30, 2020
Contributor

this will break existing users.
better I think to leave this function name as k_thread_resource_pool_assign and mark as __deprecated

@andrewboie andrewboie mentioned this pull request Oct 30, 2020
1 of 10 tasks complete
@@ -80,7 +80,7 @@ static struct buf_descriptor __aligned(512) bdt[(NUM_OF_EP_MAX) * 2 * 2];

#define EP_BUF_NUMOF_BLOCKS (NUM_OF_EP_MAX / 2)

K_MEM_POOL_DEFINE(ep_buf_pool, 16, 512, EP_BUF_NUMOF_BLOCKS, 4);
Z_MEM_POOL_DEFINE(ep_buf_pool, 16, 512, EP_BUF_NUMOF_BLOCKS, 4);

This comment has been minimized.

@nashif

nashif Nov 2, 2020
Member

shouldn we be using K_HEAP_DEFINE here?

This comment has been minimized.

@andyross

andyross Nov 2, 2020
Author Contributor

Yes, but I don't feel comfortable doing the port without a device with this USB controller to validate. The wrappers are near-zero-size and very low risk. Changing API usage blindly seems like more downside than up.

This comment has been minimized.

@MaureenHelm

MaureenHelm Nov 3, 2020
Member

@jfischer-phytec-iot ping

This comment has been minimized.

@jfischer-no

jfischer-no Nov 4, 2020
Collaborator

I did jfischer-no@7f48295 just for fun. IMHO K_HEAP_DEFINE is not an easy replacement for K_MEM_POOL_DEFINE as the overhead for the heap management has to be considered. However, we want to remove this intermediate buffers anyway, so I am fine to do minimal changes and use Z_MEM_POOL_DEFINE for now.

@@ -353,14 +353,14 @@ int usb_dc_ep_configure(const struct usb_dc_ep_cfg_data * const cfg)
}

if (bdt[idx_even].buf_addr) {
k_mem_pool_free(block);
z_mem_pool_free(block);

This comment has been minimized.

@nashif

nashif Nov 2, 2020
Member

why are we using this internal API and not k_heap_free?

@@ -4,6 +4,7 @@
*
* SPDX-License-Identifier: Apache-2.0
*/
#include <stdint.h>

This comment has been minimized.

@cvinayak

cvinayak Nov 21, 2020
Contributor

Suggested change
#include <stdint.h>

Include #include <stdint.h> in c files where needed. This is not right to have here to satisfy files that include #include "hal/cntr.h"

That said, I will have a future action item on myself to clean up other redundant/incorrect includes in controller hal folder, like sys/dlist.h etc which is never used in cntr.c file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.