Conversation
…munication kthread Signed-off-by: Sven-Ola Tuecke <sven-ola@gmx.de>
… communication kthread Signed-off-by: Sven-Ola Tuecke <sven-ola@gmx.de>
github-actions
bot
added
size/medium
📝 WalkthroughWalkthroughThese patches introduce a new kernel API ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
bot
added
Needs review
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
patch/kernel/archive/spacemit-6.18/017-Spacemit-lower-kthread-loop-speed-to-1-sec.patch (1)
19-20:⚠️ Potential issue | 🟠 MajorThis current-kernel patch carries the same RT-priority risk.
Line 20 raises the worker to
MAX_RT_PRIO - 1(max FIFO priority) without any corresponding change to loop timing or wait cadence. The patch title claims "lower kthread loop speed" but only escalates scheduler priority—a large and orthogonal scheduling behavior change for a mailbox thread that could degrade system responsiveness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@patch/kernel/archive/spacemit-6.18/017-Spacemit-lower-kthread-loop-speed-to-1-sec.patch` around lines 19 - 20, The patch incorrectly escalates the mailbox kthread to real-time by setting struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1 }, which is unnecessary and risky; revert the priority change to a safe default (e.g., .sched_priority = 0) and implement the intended "lower loop speed" by adding an explicit sleep/wait in the kthread's main loop (use msleep(), schedule_timeout_interruptible(), or similar) to enforce a ~1s cadence; update the code paths that call set_scheduler/setparam so they use the restored param and place the delay inside the mailbox worker loop to achieve the lower frequency without changing scheduler priority.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@patch/kernel/archive/spacemit-6.6/015-Spacemit-lower-kthread-loop-speed-to-1-sec.patch`:
- Around line 19-20: The change sets struct sched_param param from non-real-time
(0) to MAX_RT_PRIO - 1, which promotes the kthread to the highest FIFO RT
priority; revert this by restoring a non-RT priority (e.g., .sched_priority = 0
or use SCHED_OTHER) for the mailbox kthread so it does not run at MAX_RT_PRIO -
1; update the initialization of param (the struct sched_param named param) and
any related sched_setscheduler call to keep the thread non-realtime rather than
elevating it to MAX_RT_PRIO - 1.
---
Outside diff comments:
In
`@patch/kernel/archive/spacemit-6.18/017-Spacemit-lower-kthread-loop-speed-to-1-sec.patch`:
- Around line 19-20: The patch incorrectly escalates the mailbox kthread to
real-time by setting struct sched_param param = {.sched_priority = MAX_RT_PRIO -
1 }, which is unnecessary and risky; revert the priority change to a safe
default (e.g., .sched_priority = 0) and implement the intended "lower loop
speed" by adding an explicit sleep/wait in the kthread's main loop (use
msleep(), schedule_timeout_interruptible(), or similar) to enforce a ~1s
cadence; update the code paths that call set_scheduler/setparam so they use the
restored param and place the delay inside the mailbox worker loop to achieve the
lower frequency without changing scheduler priority.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3dfd49c0-18ea-4239-95a3-750c4ce045bc
📒 Files selected for processing (5)
patch/kernel/archive/spacemit-6.18/017-Spacemit-lower-kthread-loop-speed-to-1-sec.patchpatch/kernel/archive/spacemit-6.18/019-Add-a-wait_for_completion_idle_timeout-to-normalize-.patchpatch/kernel/archive/spacemit-6.6/015-Spacemit-lower-kthread-loop-speed-to-1-sec.patchpatch/kernel/archive/spacemit-6.6/016-Spacemit-prevent-uninitialized-thread-shutdown.patchpatch/kernel/archive/spacemit-6.6/017-Add-a-wait_for_completion_idle_timeout-to-normalize-.patch
| - struct sched_param param = {.sched_priority = 0 }; | ||
| + struct sched_param param = {.sched_priority = MAX_RT_PRIO - 1 }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Linux sched_setscheduler(..., SCHED_FIFO, ...), what is the valid sched_priorityrange, and what effective priority doesMAX_RT_PRIO - 1 represent?
💡 Result:
- Valid
sched_priorityrange forsched_setscheduler(..., SCHED_FIFO, ...)(user space): 1–99 inclusive on Linux (1 = lowest RT priority, 99 = highest). [1][2] - What
MAX_RT_PRIO - 1represents:- In the kernel headers,
MAX_RT_PRIOis 100, soMAX_RT_PRIO - 1is 99. [3] - If you mean the user-visible POSIX RT priority (
param.sched_priority):MAX_RT_PRIO - 1corresponds tosched_priority = 99(highest RT priority). [1][2][3] - If you mean the kernel’s internal
task_struct->prionumbering: RT tasks occupyprio0..99, and lowerpriomeans higher priority, soprio = 99is the lowest RT internal priority (whereas the highest RT internal priority isprio = 0). [3][4]
- In the kernel headers,
Sources:
[1] sched_setscheduler(2) man page (man7.org) — Linux returns min/max RT priorities 1 and 99 for FIFO/RR. (man7.org)
[2] sched(7) man page (man7.org) — SCHED_FIFO/SCHED_RR priorities range 1 (low) to 99 (high). (man7.org)
[3] Linux kernel header include/linux/sched/prio.h — MAX_USER_RT_PRIO 100, MAX_RT_PRIO and note that lower p->prio means higher priority. (android.googlesource.com)
[4] Oracle Linux blog on task priorities — shows mapping between rt_priority and effective kernel prio. (blogs.oracle.com)
Citations:
- 1: https://man7.org/linux/man-pages/man2/sched_setscheduler.2.html
- 2: https://man7.org/linux/man-pages/man7/sched.7.html
- 3: https://android.googlesource.com/kernel/common/%2B/8a8431507f8f5910db5ac85b72dbdc4ed8f6b30/include/linux/sched/prio.h
- 4: https://blogs.oracle.com/linux/task-priority?utm_source=openai
Avoid pushing this worker to the top FIFO priority.
Line 20 changes scheduling priority from 0 (non-real-time) to MAX_RT_PRIO - 1 (99, the highest FIFO priority). This is a dramatic scheduling behavior change that does not affect the timeout governing how often the loop wakes. For a mailbox kthread, pushing to the top RT priority is high-risk when landed as part of a load-normalization fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@patch/kernel/archive/spacemit-6.6/015-Spacemit-lower-kthread-loop-speed-to-1-sec.patch`
around lines 19 - 20, The change sets struct sched_param param from
non-real-time (0) to MAX_RT_PRIO - 1, which promotes the kthread to the highest
FIFO RT priority; revert this by restoring a non-RT priority (e.g.,
.sched_priority = 0 or use SCHED_OTHER) for the mailbox kthread so it does not
run at MAX_RT_PRIO - 1; update the initialization of param (the struct
sched_param named param) and any related sched_setscheduler call to keep the
thread non-realtime rather than elevating it to MAX_RT_PRIO - 1.
1 participant
Description
As stated by @pyavitz in the RV2 Armbian forum thread: the legacy spacemit kernel also needs the load < 2 fix. See https://forum.armbian.com/topic/56846-orange-pi-rv2/page/4/#findComment-233778
This PR also corrects the respective patches for current spacemit kernel: moved wait_for_completion_timeout() -> wait_for_completion_idle_timeout() change to the correct patch file
Tested with a local compile (legacy and current kernel)
Summary by CodeRabbit
New Features
Bug Fixes
Performance