Skip to content

Conversation

@lyakh
Copy link
Collaborator

@lyakh lyakh commented Nov 27, 2025

Fork the DP scheduler, plus an additional cosmetic fix

Copy link

Copilot AI left a 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.c into a new shared header zephyr_dp_schedule.h
  • Created two variant implementations: zephyr_dp_schedule_proxy.c and zephyr_dp_schedule_native.c, selected via CONFIG_SOF_USERSPACE_PROXY
  • Removed __sparse_force and __sparse_cache cast annotations from stack allocation functions in userspace_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 */
Copy link

Copilot AI Nov 27, 2025

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.

Copilot uses AI. Check for mistakes.
* see dp_startup_delay comment for details
*/
mod->dp_startup_delay = false;

Copy link

Copilot AI Nov 27, 2025

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.

Suggested change

Copilot uses AI. Check for mistakes.
/* if true exit the while loop, terminate the thread */
task_stop = task->state == SOF_TASK_STATE_COMPLETED ||
task->state == SOF_TASK_STATE_CANCEL;

Copy link

Copilot AI Nov 27, 2025

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.

Suggested change
scheduler_dp_recalculate(dp_sch, false);

Copilot uses AI. Check for mistakes.

#include "zephyr_dp_schedule.h"

/* Go through all DP tasks and recalculate their readness and dedlines
Copy link

Copilot AI Nov 27, 2025

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

Suggested change
/* Go through all DP tasks and recalculate their readness and dedlines
/* Go through all DP tasks and recalculate their readiness and deadlines

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@softwarecki softwarecki left a 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 :)

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 1, 2025

The main difference is the missing scheduler_dp_recalculate call, which could be handled with an ifdef.

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

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 1, 2025

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.

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

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 1, 2025

v2: restored non-userspace builds to use the original DP version by default

@softwarecki
Copy link
Collaborator

@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.
For now, I only had a quick look and I see changes not related to IPC command processing in the DP thread. In that case, please add an option to choose whether IPC should be processed in the DP thread.
It seems you are looking at the proxy as some unwanted competing solution. Instead of splitting the code and intentionally making part of it break because changes are not even tested with the other option enabled, these solutions should be merged into one configurable approach. What is the real difference? Only whether IPC is processed in the DP thread or in a separate worker. In that case, IPC handling can be moved to a separate function and called depending on configuration either in the DP thread or in a worker. The code preparing requests for the userspace thread and handling those requests can be the same and that should be the goal. Also, in this context, I would understand "native" as processing in the IPC thread. Moving it to DP is not native, it's DP IPC processing.

@lgirdwood
Copy link
Member

@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. For now, I only had a quick look and I see changes not related to IPC command processing in the DP thread. In that case, please add an option to choose whether IPC should be processed in the DP thread. It seems you are looking at the proxy as some unwanted competing solution. Instead of splitting the code and intentionally making part of it break because changes are not even tested with the other option enabled, these solutions should be merged into one configurable approach. What is the real difference? Only whether IPC is processed in the DP thread or in a separate worker. In that case, IPC handling can be moved to a separate function and called depending on configuration either in the DP thread or in a worker. The code preparing requests for the userspace thread and handling those requests can be the same and that should be the goal. Also, in this context, I would understand "native" as processing in the IPC thread. Moving it to DP is not native, it's DP IPC processing.

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

  1. Proxy DP mode - using CONFIG_USERSPACE_PROXY kconfig and file naming.

  2. Native DP mode - using CONFIG_USERSPACE_NATIVE kconfig and file naming.

  3. App userspace mode - using CONFIG_USERSPACE_APP Kconfig depends 2)

So lets make sure we use Kconfig and partitioning to make sure we dont break each other or kernel mode DP.

Copy link
Member

@lgirdwood lgirdwood left a 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.

@softwarecki
Copy link
Collaborator

softwarecki commented Dec 2, 2025

@lyakh @lgirdwood
I think the term "native" is misleading. "Kernel" proposed by Liam is slightly better. The "native" approach is not fundamentally different from proxy. IPC calls to modules are still wrapped and passed to a userspace thread. This behaves like a proxy regardless of naming.

"Proxy" was introduced as an intermediate component to simplify implementation. It is compact and does not require changes across many places in SOF. A llext modules could use it if there was such a will. When userspace implementation spreads across SOF, "proxy" will only provide userspace processing for iadk modules and will likely become scattered as well. As I understand, @lyakh added functions to pack IPC requests for "native proxy". This is similar to "proxy" but with a shorter path. "Proxy" could use these prepared requests.

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:

  • do not split files,
  • extract IPC processing into a separate function reusable by proxy,
  • add a configuration option to process IPC in DP thread,
  • call this function from DP thread.
  • I will use the same option to conditionally create a thread for IPC module processing and reuse the same function.

Edit:
In a previous PR (#10302) there was an initial attempt to separate memory allocation with #ifdef. After discussion, we agreed to share the heap usage instead. This shows that introducing conditional code is not always necessary and common mechanisms can be reused.

@lyakh
Copy link
Collaborator Author

lyakh commented Dec 3, 2025

@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.
As for differences - I personally hold this for a rather important and significant difference. We want to be able to guarantee, that code, belonging to a certain module, only runs in that module's process (thread) context. When somebody writes a module and submits it to be run with SOF, all the code in that module will only ever run in that thread's context. I think this is a rather clear and worthy goal to pursue.

@lgirdwood
Copy link
Member

So we are all on the same page in terms of modes :) Please correct me if I'm wrong.

  1. Kernel mode: What we have today with no MMU. i.e. all DP, IPC and LL runs in same kernel context as RTOS. We dont want any changes to flow here to retain existing legacy behaviour.

  2. Proxy mode: DP runs in a separate userspace context, memory can be shared per driver class and IPC is managed by a proxy IPC thread for DP user modules. DP module IPC processing and audio processing are different threads (and different contexts ?).

  3. Application mode: (renamed from native mode to avoid confusion). Infra and DP run in same or different userspace context. IPC handling split into 2 threads - 1 kernel and the other user thread with same context as user infra. DP does IPC in its own thread/context. Memory shared between DP/infra user domains as defined by topology.

lyakh added 3 commits December 4, 2025 17:40
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>
@softwarecki
Copy link
Collaborator

@lgirdwood

2. Proxy mode: DP runs in a separate userspace context, memory can be shared per driver class and IPC is managed by a proxy IPC thread for DP user modules. DP module IPC processing and audio processing are different threads (and different contexts ?).

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.
Similarly, in the first variant IPC is handled in a separate thread. I plan to add a switch to control whether IPC should be processed in a separate proxy thread or in module's DP thread. This is just about where the handler function is called, but I want to keep that choice available. DP threads run in a specific configuration that might cause issues with IPC handling. We will be able to test different configurations and pick the better one.

@lyakh
In the original PR review (#10386) I suggested naming files based on their actual content rather than the configuration option name. Using config names is not meaningful because the _proxy.c file is also used when proxy is disabled and userspace is not supported. The most important part is that we dropped the misleading "native" naming. The rest is cosmetic and can be addressed in future PRs.

@softwarecki softwarecki self-requested a review December 4, 2025 21:11
@softwarecki softwarecki dismissed their stale review December 4, 2025 21:13

Key concern was addressed, remaining points can be handled in future PRs

@lgirdwood lgirdwood merged commit 33079fd into thesofproject:main Dec 5, 2025
37 of 42 checks passed
@lyakh lyakh deleted the user-p3_1 branch December 5, 2025 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants