Re: [PATCH] sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args

[email protected]
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
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.