Re: [PATCH] sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args
| Newsgroups | dev.linux.lists.sched-ext,org.kernel.vger.bpf |
|---|---|
| Message-ID | <[email protected]> |
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] The patch fails to initialize the new `idle` field in `struct scx_cgroup_init_args` in the sub-scheduler paths, causing it to implicitly default to false and lose the correct idle state. -- --- Patch [1]: [PATCH] sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args --- commit 6b122f2bce26c31e18660129955114867f5a36b0 Author: Tao Cui <[email protected]> sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args This commit adds the idle state to scx_cgroup_init_args and initializes it in scx_tg_online() and scx_cgroup_init(). This ensures that cgroups already configured as idle before the scheduler is loaded are correctly presented as idle to the BPF scheduler. > diff --git a/kernel/sched/ext/internal.h b/kernel/sched/ext/internal.h > index 53e136a47924d..aa149a9c29f7b 100644 > --- a/kernel/sched/ext/internal.h > +++ b/kernel/sched/ext/internal.h > @@ -259,6 +259,9 @@ struct scx_cgroup_init_args { > u64 bw_period_us; > u64 bw_quota_us; > u64 bw_burst_us; > + > + /* whether the cgroup is configured idle via cpu.idle */ > + bool idle; > }; [Severity: Medium] When adding this new field, did we miss updating the struct initialization in the sub-scheduler paths? Looking at kernel/sched/ext/sub.c, both scx_cgroup_claim_subtree() and scx_cgroup_return_subtree() initialize the struct but omit the new field: kernel/sched/ext/sub.c:scx_cgroup_claim_subtree() { ... struct scx_cgroup_init_args args = { .weight = tg->scx.weight, .bw_period_us = tg->scx.bw_period_us, .bw_quota_us = tg->scx.bw_quota_us, .bw_burst_us = tg->scx.bw_burst_us, }; ... } The exact same pattern also exists in scx_cgroup_return_subtree(). Without explicitly setting the idle field in these paths, wouldn't it implicitly default to false, causing cgroups to incorrectly lose their idle state when a sub-scheduler is enabled or disabled? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1