Test sched family#967
Conversation
There was a problem hiding this comment.
PR #967 代码审查
注意:此 PR 已关闭且未合并,同一作者已提交 PR #970("sched: fix ABI issues and add full sched-family test suite",状态 open,1 commit squash 版本)作为后续。以下审查意见同时适用于 PR #970。
变更概要
本 PR 为 StarryOS 实现了 sched 系列调用的语义化支持:
os/arceos/modules/axtask/src/task.rs:在TaskInner中新增sched_policy(AtomicI32)和sched_priority(AtomicI32)字段,提供 getter/setter 接口,初始化默认值 0(对应SCHED_NORMAL)。os/StarryOS/kernel/src/syscall/task/schedule.rs:- 新增
SchedParam结构体(#[repr(C)], bytemuck Pod) - 新增
check_sched_permission权限检查函数(CAP_SYS_NICE / euid 匹配) sys_sched_getscheduler:从硬编码SCHED_RR改为返回task.sched_policy()sys_sched_setscheduler:解析SCHED_RESET_ON_FORK标志,验证 policy 和 priority 范围sys_sched_getparam:从用户空间读取 task 的sched_priority并写回sys_sched_setaffinity:新增check_sched_permission调用
- 新增
- 测试:新增
test-suit/starryos/normal/qemu-smp4/test-sched-family/,包含 x86_64/riscv64/aarch64/loongarch64 四架构 QEMU 配置及 C 测试用例
验证结果
cargo fmt --check:通过- CI check-runs:无(分支无 CI 报告)
- 无法在当前环境运行 QEMU 测试和 cargo clippy(目标平台交叉编译)
代码质量分析
实现整体结构清晰,策略/优先级验证逻辑正确(SCHED_NORMAL/BATCH/IDLE → prio 必须为 0,SCHED_FIFO/RR → prio 须在 1..=99),bytemuck::Pod 用于安全的用户空间参数反序列化是合理的。
以下为具体的代码问题(详见 inline comments):
-
SCHED_RESET_ON_FORK仅为存根:标志被解析和剥离,但_reset_on_fork变量未使用,实际 fork-reset 行为未实现。建议添加// TODO注释。 -
get_task_by_sched_pid重复调用:check_sched_permission内部调用一次,外部函数又调用一次。可考虑让权限检查返回 task 引用以避免冗余查找。 -
sys_sched_getparam缺少权限检查:Linux 上读取其它进程的调度参数需要 CAP_SYS_NICE 或 uid/euid 匹配。 -
sched_policy/sched_priority 存而不用:字段已添加到
TaskInner但实际调度器未使用这些值做调度决策。作为分阶段实现可接受,但应明确记录。
重复/重叠分析
- PR #970(同一作者,open):包含完全相同的内核/axtask 修改和几乎相同的测试,使用 squash commit(1 commit vs 9 commits),x86_64 toml 更精简。PR #970 是本 PR 的替代版本,建议关闭本 PR 并审查 PR #970。
- dev 分支不包含
sched_policy/sched_priority字段或相关 syscall 实现,不构成重复。
测试覆盖评估
测试用例覆盖了 sched 系列调用的主要边界:
sched_setaffinity/getaffinity正常流程 + EFAULT/EINVAL/EPERM/ESRCHsched_setscheduler/getscheduler正常流程 + EINVAL(负 pid、NULL param、非法 policy、非法 priority)/EPERM/ESRCHsched_getparam+ ESRCHgetpriority+ EINVAL/ESRCH- 使用直接
syscall()调用避免 libc wrapper 干扰
测试设计合理,success_regex/fail_regex 能正确分类结果。
Powered by glm-5.1
| let user_param = user_param[0]; | ||
| let mut policy = _policy as u32; | ||
| const SCHED_RESET_ON_FORK: u32 = 0x40000000; | ||
| let _reset_on_fork = (policy & SCHED_RESET_ON_FORK) != 0; |
There was a problem hiding this comment.
SCHED_RESET_ON_FORK 标志被正确解析和剥离,但 _reset_on_fork 变量以 _ 前缀标记为未使用。在 Linux 中,此标志意味着子进程应回退到 SCHED_NORMAL 而非继承父进程的实时调度策略。
当前实现接受此标志但不执行实际的 reset-on-fork 行为,即 fork/clone 时不会重置子进程的调度策略。建议在此处添加 // TODO: implement reset-on-fork behavior in fork/clone path 注释,明确标注这是已知未完成的语义。
| } | ||
|
|
||
| pub fn sys_sched_setaffinity(pid: i32, cpusetsize: usize, user_mask: *const u8) -> AxResult<isize> { | ||
| check_sched_permission(pid)?; |
There was a problem hiding this comment.
check_sched_permission(pid)? 内部已经调用 get_task_by_sched_pid(pid) 获取 task 引用做权限校验,但该 task 引用在 check_sched_permission 返回后被丢弃。紧接着第 137 行又调用 get_task_by_sched_pid(pid) 重新获取同一个 task。
同样的问题存在于 sys_sched_setscheduler(第 174-175 行)。
建议:将 check_sched_permission 改为返回 (AxTaskRef, ()) 或单独封装一个 get_task_with_permission(pid) -> AxResult<AxTaskRef>,避免冗余查找。
| Ok(0) | ||
| } | ||
|
|
||
| pub fn sys_sched_getparam(_pid: i32, _param: *mut ()) -> AxResult<isize> { |
There was a problem hiding this comment.
在 Linux 中,sched_getparam 对目标进程有权限检查:调用者需要 CAP_SYS_NICE 或调用者的 real/effective UID 匹配目标进程的 real/saved-set UID。
当前实现允许任意进程读取任意其它进程的调度参数,没有权限检查。虽然 sched_getparam 对安全性影响较低(只读操作),但若追求 Linux ABI 兼容性,建议添加与 sched_setparam 类似的权限检查。
No description provided.