-
Notifications
You must be signed in to change notification settings - Fork 349
DP: Fork "native" and "proxy" versions (part 4.1 of #10287) #10398
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR refactors the Zephyr DP (Data Processing) scheduler by creating separate "native" and "proxy" implementations, which is part of a larger effort (issue #10287) to support different userspace modes. The refactoring extracts common data structures and function declarations into a shared header file while maintaining two distinct implementation files.
Key Changes
- Extracted scheduler data structures and function declarations from
zephyr_dp_schedule.cinto a new shared headerzephyr_dp_schedule.h - Created two variant implementations:
zephyr_dp_schedule_proxy.candzephyr_dp_schedule_native.c, selected viaCONFIG_SOF_USERSPACE_PROXY - Removed
__sparse_forceand__sparse_cachecast annotations from stack allocation functions inuserspace_helper.c
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| zephyr/lib/userspace_helper.c | Cosmetic cleanup removing sparse annotation casts from stack allocation/deallocation functions |
| src/schedule/zephyr_dp_schedule.h | New shared header containing scheduler data structures and function declarations |
| src/schedule/zephyr_dp_schedule.c | Refactored to use shared header and removed scheduler_dp_recalculate/dp_thread_fn implementations |
| src/schedule/zephyr_dp_schedule_proxy.c | New proxy implementation with recalculation logic in thread function |
| src/schedule/zephyr_dp_schedule_native.c | New native implementation without recalculation in thread function |
| src/schedule/CMakeLists.txt | Updated build configuration to conditionally compile native or proxy scheduler based on CONFIG_SOF_USERSPACE_PROXY |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -0,0 +1,41 @@ | |||
| /* SPDX-License-Identifier: BSD-3-Clause */ | |||
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing header guards. This header file should include preprocessor guards (e.g., #ifndef ZEPHYR_DP_SCHEDULE_H, #define ZEPHYR_DP_SCHEDULE_H, and #endif) to prevent multiple inclusion.
| * see dp_startup_delay comment for details | ||
| */ | ||
| mod->dp_startup_delay = false; | ||
|
|
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Extra blank line before closing brace. This is inconsistent with the proxy version (line 44-45 in zephyr_dp_schedule_proxy.c), where there is no blank line.
| /* if true exit the while loop, terminate the thread */ | ||
| task_stop = task->state == SOF_TASK_STATE_COMPLETED || | ||
| task->state == SOF_TASK_STATE_CANCEL; | ||
|
|
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing scheduler_dp_recalculate(dp_sch, false) call before unlocking. The proxy version (lines 153-157 in zephyr_dp_schedule_proxy.c) includes this recalculation, which ensures all DP tasks' readiness and deadlines are updated. This appears to be a functional difference that should be intentional for the native version, but if the two versions are meant to have the same core logic, this should be added.
| scheduler_dp_recalculate(dp_sch, false); |
|
|
||
| #include "zephyr_dp_schedule.h" | ||
|
|
||
| /* Go through all DP tasks and recalculate their readness and dedlines |
Copilot
AI
Nov 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spelling errors in comment: "readness" should be "readiness" and "dedlines" should be "deadlines".
| /* Go through all DP tasks and recalculate their readness and dedlines | |
| /* Go through all DP tasks and recalculate their readiness and deadlines |
softwarecki
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The main difference is the missing scheduler_dp_recalculate call, which could be handled with an ifdef. Removing deadline recalculation after DP processing will likely degrade EDF. While recalculation is not possible in userspace context now, it should rather be solved differently, for example through a syscall, instead of being removed.
We need to introduce an appropriate ifdef related to running all modules in userspace as well as entire pipelines. I would call this experimental or all-userspace, not native. Selecting CONFIG_SOF_USERSPACE_PROXY to switch between implementations does not seem correct. It is not that the proxy requires something different, non-standard, and is not native. It is the "all-userspace" implementation that imposes its own limitations, which need special handling. Additionally, the CONFIG_SOF_USERSPACE_PROXY definition depends on CONFIG_USERSPACE. So if userspace is not enabled, the "native" version will be used, which lacks DP deadline recalculation even though it would be possible without userspace.
It would also be nice to fix the same typos and formatting in that unwanted proxy file :)
@softwarecki no, it isn't the main difference. If you check the full (as of now, and it will likely grow) PR #10386 it adds a few more differences after forking. While maybe the current Kconfig implementation isn't perfect, it certainly can be improved, but I really don't think keeping both versions use the same sources with just build- or run-time switching is plausible. I would be inevitably breaking the version that I'm not using (and that isn't tested by the CI), and the same would happen in the opposite direction. |
@softwarecki this isn't what "native" is about. Native is about processing IPC commands on the DP thread "natively" instead of a proxy thread. It isn't moving all modules or entire pipelines to userspace, it is only moving all DP threads there. |
|
v2: restored non-userspace builds to use the original DP version by default |
|
@lyakh It's hard to understand the actual scope because #10386 contains a lot of changes and I need some time to review it. That PR will definitely need to be split into smaller ones with separate functional parts. |
@lyakh @softwarecki I am going to merge 3 userspace modes (and we will need to make user we partition and kconfig as needed) as each mode will target different OSes/configurations and use cases: e.g.
So lets make sure we use Kconfig and partitioning to make sure we dont break each other or kernel mode DP. |
lgirdwood
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not seeing any changes that could break kernel DP or proxy DP here.
|
@lyakh @lgirdwood "Proxy" was introduced as an intermediate component to simplify implementation. It is compact and does not require changes across many places in SOF. A The main difference is that "proxy" was designed with a dedicated thread for IPC handling. The alternative is handling IPC in the DP thread. A configuration switch could control whether IPC is processed in DP or a separate thread. Similarly to how the "native" solution uses userspace changes introduced in the DP scheduler for "proxy". The real issue is unnecessary separation of kernel DP and proxy DP. They should be merged with configuration options such as which thread processes IPC. Splitting code instead of merging is not a good approach. I noticed "native"/"kernel" modifies DP deadline calculation function and some dp structures. "Proxy" does not use these structures so splitting are unneccessary. Changing this structures only in one variant will break the other. I suggest:
Edit: |
|
@softwarecki @lgirdwood yes, a unified version would be preferable, but unifying the two versions would take too long. Yes, Adrian, we did manage to unify memory allocation, but it's a much smaller difference and also its unification took significant effort. I'd rather not do the unification because I cannot directly test your version, so I'll definitely break it, then you'll test and report, then I'll need to fix etc. OTOH, once both versions are in "main" and working, you'd be quite welcome to try to unify them - at least at that point both versions should be tested by the CI, so it would be easier to make sure not to break anything. My main goal now is to get my version running properly again (after rebasing) and to merge it. Then we can try to bring the two closer again. |
|
So we are all on the same page in terms of modes :) Please correct me if I'm wrong.
|
scheduler_dp_recalculate() and dp_thread_fn() will have significant difference between the "proxy" and "native" versions. Extract them into a separate file for easy forking. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Fork a version of the deviating DP functions for a native (proxy- free) implementation. The first deviation is the removal of task recalculation at the end of DP task runs. DP tasks are recalculated on every LL tick (typically every 1ms). DP threads then run with varying periodicity and duration. They can take 10 or even 100ms to run. Also recalculating tasks in the end of those runs doesn't improve anything and only adds load and a need to protect data. This is even worse when DP threads run in user space. Skip recalculation for the native version. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Fix wrong sparse type-casting. Signed-off-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
In the current variant, proxy works as you described. However, this is the first version, and its main goal was to enable userspace. Whether memory is shared per driver class or each module has its own heap is not critical. I can easily change this or make it configurable. @lyakh |
Key concern was addressed, remaining points can be handled in future PRs
Fork the DP scheduler, plus an additional cosmetic fix