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

lib: os: p4wq: fix K_P4WQ_DELAYED_START mode #82551

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

kv2019i
Copy link
Collaborator

@kv2019i kv2019i commented Dec 4, 2024

When the PRESTART thread state was removed, this changed the semantics
of k_thread_start() when thread was created with a K_FOREVER timeout,
suspended and then started with k_thread_start().

This sequence is used in p4wq to implement K_P4WQ_DELAYED_START
(which again is needed by K_P4WQ_USER_CPU_MASK).

With PRESTART removed, the following sequence:
z_mark_thread_as_not_suspended(thread);
k_thread_start(thread);

.. no longer starts the thread. As a result, p4wq users like SOF
multicore configurations, hit errors as p4wq threads never start.

Fix the implementation by removing the calls to change thread
suspended state explicitly, but rather rely on the new
k_thread_create() and k_thread_start() semantics.

Fixes: 7cdf405 ("kernel/sched: Eliminate PRESTART thread state")

When the PRESTART thread state was removed, this changed the semantics
of k_thread_start() when thread was created with a K_FOREVER timeout,
suspended and then started with k_thread_start().

This sequence is used in p4wq to implement K_P4WQ_DELAYED_START
(which again is needed by K_P4WQ_USER_CPU_MASK).

With PRESTART removed, the following sequence:
  z_mark_thread_as_not_suspended(thread);
  k_thread_start(thread);

.. no longer starts the thread. As a result, p4wq users like SOF
multicore configurations, hit errors as p4wq threads never start.

Fix the implementation by removing the calls to change thread
suspended state explicitly, but rather rely on the new
k_thread_create() and k_thread_start() semantics.

Fixes: 7cdf405 ("kernel/sched: Eliminate PRESTART thread state")
Signed-off-by: Kai Vehmanen <[email protected]>
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 4, 2024

@andyross @peter-mitsis Not sure if #81684 is better, but pushed this PR as a more immediate fix as SOF is fatally wounded after the PRESTART removal... Local test passes, a bigger test run ongoing at thesofproject/sof#9709

@kv2019i kv2019i force-pushed the 202412-p4wq-fix branch 2 times, most recently from afc757e to a8997cb Compare December 4, 2024 14:30
@kv2019i
Copy link
Collaborator Author

kv2019i commented Dec 4, 2024

New version pushed:

  • fix the language in commit message, no other changes

Copy link
Contributor

@andyross andyross left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch.

@andyross
Copy link
Contributor

andyross commented Dec 4, 2024

Just to be That Guy: this does indicate that we have some incomplete test coverage of p4wq delayed start, forcing SOF to be the guinea pig. Wouldn't hurt to add a test case that exercises it in a way this would have broken.

@nashif nashif added the Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc. label Dec 5, 2024
@nashif nashif merged commit 4b49d7c into zephyrproject-rtos:main Dec 5, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Base OS Base OS Library (lib/os) Trivial Changes that can be reviewed by anyone, i.e. doc changes, minor build system tweaks, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants