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.

Sign up for GitHub

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

Jump to bottom

deprecate and remove old k_mem_pool / sys_mem_pool APIs #24358

Closed
andrewboie opened this issue Apr 14, 2020 · 11 comments · Fixed by #28611
Closed

deprecate and remove old k_mem_pool / sys_mem_pool APIs #24358

andrewboie opened this issue Apr 14, 2020 · 11 comments · Fixed by #28611
Assignees
@andyross
Labels
area: API Changes to public APIs area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Milestone
v2.4.0

Comments

@andrewboie
Copy link
Contributor

We have replaced the mem pool APIs with the new k_heap/sys_heap APIs. We don't need to keep maintaining two APIs for doing the same thing, and should aim to remove the k_mem_pool and sys_mem_pool APIs from the kernel.

The first step is to __deprecate them all, and change any use within Zephyr to use k_heap/sys_heap instead.

The k_pipe_block_put() API will need to be adjusted as it takes a k_mem_block as an argument. I couldn't find any other public APIs which would need to be changed.
@andrewboie andrewboie added bug The issue is a bug, or the PR is fixing a bug area: API Changes to public APIs area: Kernel labels Apr 14, 2020
@carlescufi carlescufi added Enhancement Changes/Updates/Additions to existing features and removed area: API Changes to public APIs labels Apr 21, 2020
@carlescufi carlescufi added this to the v2.3.0 milestone Apr 21, 2020
@carlescufi carlescufi added area: API Changes to public APIs and removed bug The issue is a bug, or the PR is fixing a bug labels Apr 21, 2020
@carlescufi carlescufi mentioned this issue Apr 21, 2020
Closed
@carlescufi carlescufi added this to v2.3 in Release Plan Apr 28, 2020
@carlescufi
Copy link
Member

@andyross and @andrewboie we need this for 2.3.0. Any timeline for a PR?

@carlescufi carlescufi added bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug and removed Enhancement Changes/Updates/Additions to existing features labels May 12, 2020
@andyross
Copy link
Contributor

I was honestly expecting this to wait for a release before deprecation, to reduce churn. The original idea, I thought, was that in 2.3 apps would have the new backend by default but no requirement to move away from the old APIs, then later we'd start moving things away.

Some of this, like the direct mem_pool APIs, is straightforward. Also we could rework k_malloc/k_free in terms of the immediate API and not a double-wrapper. The thread memory pools are coded to the old API but could easily be moved to the new one.

But the old pool scheme has its fingers in other places too, like the k_mem_block abstraction used as a handle, which would be an API change if we want to "fix" that (though for a first cut we could just add a constructor for one from a k_heap+pointer tuple) Also both mailbox and pipe will allocate on the behalf of their users, so they need porting. There are probably others I forget.

The early stuff in that list is easy and can be done this week. The later bits may take some thought and argument.

@andrewboie
Copy link
Contributor Author

I don't think this should be a high priority bug for 2.3 I think this can wait until 2.4. @carlescufi can you share some details on your reasoning for making this a blocker?

@nashif
Copy link
Member

nashif commented May 12, 2020

reasoning for making this a blocker?
reasoning is the fact that if we need to deprecate, then it has to happen in 2.3, otherwise might end up with old APIs.

have the new backend by default but no requirement to move away from the old APIs, then later we'd start moving things away.

Ok, if we deprecate this in 2.4, then we should be able to drop legacy in 2.6 (LTS), i think this works fine.

@carlescufi lets move this to 2.4..

@carlescufi
Copy link
Member

Agreed, let's move this to 2.4.

@carlescufi carlescufi removed the priority: high High impact/importance bug label May 13, 2020
@carlescufi carlescufi modified the milestones: v2.3.0, v2.4.0 May 13, 2020
@carlescufi carlescufi moved this from v2.3 to v2.4 in Release Plan May 13, 2020
@carlescufi carlescufi added Enhancement Changes/Updates/Additions to existing features and removed bug The issue is a bug, or the PR is fixing a bug labels May 13, 2020
@nashif nashif added the priority: high High impact/importance bug label Aug 13, 2020
@andrewboie andrewboie mentioned this issue Sep 19, 2020
Merged
@carlescufi
Copy link
Member

This should be a bug and be fixed for 2.4. It was already something that had to be fixed for 2.3 and never happened.

@nashif @MaureenHelm do you agree?

@nashif nashif added bug The issue is a bug, or the PR is fixing a bug and removed priority: high High impact/importance bug labels Sep 19, 2020
@nashif nashif removed the Enhancement Changes/Updates/Additions to existing features label Sep 19, 2020
@andyross
Copy link
Contributor

OK, sounds good. I'll provide the deprecation patch as soon as I get another spin of the SOF PR up, that one is easy. Note it will end up needing to remove a few tests though. Post-deprecation, there needs to be a new API to convert a heap block to an old-style "mem block" for the oddball APIs that use those.

@MaureenHelm MaureenHelm added the priority: high High impact/importance bug label Sep 22, 2020
@nashif nashif mentioned this issue Sep 22, 2020
Merged
andyross pushed a commit to andyross/zephyr that referenced this issue Sep 22, 2020
kernel: Deprecate k_mem_pool APIs
be1417e 
Mark all k_mem_pool APIs deprecated for future code.  Remaining
internal usage now uses equivalent "z_mem_pool" symbols instead.

Fixes zephyrproject-rtos#24358

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

Consensus in #28611 is that this should be pushed to 2.5

@MaureenHelm
Copy link
Member

Consensus in #28611 is that this should be pushed to 2.5

Nevertheless, thank you @andyross for putting together this PR so quickly.

@maksimmasalski
Copy link
Collaborator

@andyross Please update milestone, I see 2.4.0

@andyross andyross moved this from v2.4 - API Freeze to v2.5 - Feature Freeze in Release Plan Sep 29, 2020
@github-actions
Copy link

This issue has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this issue will automatically be closed in 14 days. Note, that you can always re-open a closed issue at any time.

@github-actions github-actions bot added the Stale label Nov 28, 2020
@MaureenHelm MaureenHelm removed the Stale label Nov 30, 2020
andyross pushed a commit to andyross/zephyr that referenced this issue Dec 7, 2020
kernel: Deprecate k_mem_pool APIs
c47e15c 
Mark all k_mem_pool APIs deprecated for future code.  Remaining
internal usage now uses equivalent "z_mem_pool" symbols instead.

Fixes zephyrproject-rtos#24358

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
nashif pushed a commit that referenced this issue Dec 8, 2020
@nashif
kernel: Deprecate k_mem_pool APIs
6965cf5 
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>
bwasim pushed a commit to bwasim/zephyr that referenced this issue Jan 5, 2021
@bwasim
kernel: Deprecate k_mem_pool APIs
d34e7f1 
Mark all k_mem_pool APIs deprecated for future code.  Remaining
internal usage now uses equivalent "z_mem_pool" symbols instead.

Fixes zephyrproject-rtos#24358

Signed-off-by: Andy Ross <andrew.j.ross@intel.com>
@s-kelvin s-kelvin mentioned this issue Jan 11, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@andyross andyross

Labels
area: API Changes to public APIs area: Kernel bug The issue is a bug, or the PR is fixing a bug priority: high High impact/importance bug
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging a pull request may close this issue.

Deprecate k_mem_pool API, remove sys_mem_pool allocator andyross/zephyr
6 participants
@nashif @andyross @andrewboie @MaureenHelm @carlescufi @maksimmasalski