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 and remove old k_mem_pool / sys_mem_pool APIs #24358

Open
andrewboie opened this issue Apr 14, 2020 · 11 comments · May be fixed by #28611
Open

deprecate and remove old k_mem_pool / sys_mem_pool APIs #24358

andrewboie opened this issue Apr 14, 2020 · 11 comments · May be fixed by #28611

Comments

@andrewboie
Copy link
Contributor

@andrewboie andrewboie commented Apr 14, 2020

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.

@carlescufi
Copy link
Member

@carlescufi carlescufi commented May 12, 2020

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

@andyross
Copy link
Contributor

@andyross andyross commented May 12, 2020

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

@andrewboie andrewboie commented May 12, 2020

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

@carlescufi carlescufi commented May 13, 2020

Agreed, let's move this to 2.4.

@carlescufi
Copy link
Member

@carlescufi carlescufi commented Sep 19, 2020

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 and removed priority: high labels Sep 19, 2020
@nashif nashif removed the Enhancement label Sep 19, 2020
@andyross
Copy link
Contributor

@andyross andyross commented Sep 21, 2020

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.

andyross added a commit to andyross/zephyr that referenced this issue Sep 22, 2020
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 added a commit to andyross/zephyr that referenced this issue Sep 22, 2020
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

@andyross andyross commented Sep 23, 2020

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

@MaureenHelm
Copy link
Member

@MaureenHelm MaureenHelm commented Sep 23, 2020

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

@maksimmasalski maksimmasalski commented Sep 28, 2020

@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
andyross added a commit to andyross/zephyr that referenced this issue Oct 21, 2020
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 added a commit to andyross/zephyr that referenced this issue Oct 23, 2020
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 added a commit to andyross/zephyr that referenced this issue Oct 26, 2020
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>
cfriedt added a commit to cfriedt/zephyr that referenced this issue Nov 26, 2020
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>
@github-actions
Copy link

@github-actions github-actions bot commented Nov 28, 2020

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
cfriedt added a commit to cfriedt/zephyr that referenced this issue Dec 1, 2020
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Release Plan
v2.5 - Feature Freeze
Linked pull requests

Successfully merging a pull request may close this issue.

6 participants
You can’t perform that action at this time.