All of lore.kernel.org
 help / color / mirror / Atom feed
From: sashiko-bot@kernel.org
To: "Tao Cui" <cui.tao@linux.dev>
Cc: bpf@vger.kernel.org, sched-ext@lists.linux.dev
Subject: Re: [PATCH] sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args
Date: Mon, 24 Aug 2026 13:55:56 +0000	[thread overview]
Message-ID: <20260824135557.2CC371F000E9@smtp.kernel.org> (raw)
In-Reply-To: <20260824133954.561956-1-cui.tao@linux.dev>

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 <cuitao@kylinos.cn>

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/20260824133954.561956-1-cui.tao@linux.dev?part=1

  reply	other threads:[~2026-08-24 13:55 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-24 13:39 [PATCH] sched_ext: pass the initial cpu.idle state in scx_cgroup_init_args Tao Cui
2026-08-24 13:55 ` sashiko-bot [this message]
2026-08-24 14:11 ` Andrea Righi
2026-08-24 14:16   ` Tao Cui

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260824135557.2CC371F000E9@smtp.kernel.org \
    --to=sashiko-bot@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=cui.tao@linux.dev \
    --cc=sashiko-reviews@lists.linux.dev \
    --cc=sched-ext@lists.linux.dev \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.