Re: [PATCH 1/3] sched_ext: Add ops.sub_cid_sched_updated() to report the sched running on a cid

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] sched_ext: skipped NONE notification for the last active scheduler
--

commit 24e8930bf2fb59df3460d85d232da97c8cc5102f
Author: Tejun Heo <tj@kernel.org>

sched_ext: Add ops.sub_cid_sched_updated() to report the sched running on a cid

This commit adds ops.sub_cid_sched_updated() to report changes in the
scheduler running on a specific cgroup ID. It maintains the notification
state using a static key that is disabled when the last interested scheduler
goes away.

> diff --git a/kernel/sched/ext/sub.c b/kernel/sched/ext/sub.c
> index 6b964743cab9b..59fc506af817e 100644
> --- a/kernel/sched/ext/sub.c
> +++ b/kernel/sched/ext/sub.c

[ ... ]

> @@ -92,6 +99,161 @@ struct scx_sched *scx_next_descendant_pre(struct scx_sched *pos, struct scx_sche

[ ... ]

> +void scx_cid_sched_update(struct rq *rq, struct scx_sched *sch)
> +{
> +	struct scx_sched *prev = rq->scx.sched;
> +	s32 level, common = -1;
> +
> +	lockdep_assert_rq_held(rq);
> +
> +	if (!static_branch_unlikely(&__scx_ops_cid_sched_updated_enabled) || prev == sch)
> +		return;

[ ... ]

> +void scx_ops_cid_sched_updated_disable(struct scx_sched *sch)
> +{
> +	bool last;
> +	s32 cpu;
> +
> +	lockdep_assert_held(&scx_enable_mutex);
> +
> +	last = SCX_HAS_OP(sch, sub_cid_sched_updated) && !--scx_nr_ops_cid_sched_updated;
> +	if (last)
> +		static_branch_disable(&__scx_ops_cid_sched_updated_enabled);
> +
> +	/*
> +	 * Re-homing restarts a running task's session under its new sched. A
> +	 * task that blocked while its cpu's dispatch had the rq lock dropped is
> +	 * put and set next again without a restart, so rq->scx.sched keeps @sch
> +	 * until the pick completes:
> +	 *
> +	 *   cpu 0, __schedule()                 cpu 1, scx_sub_disable(@sch)
> +	 *   T of @sch blocks, stops running
> +	 *   dispatch drops the rq lock
> +	 *                                       re-home T, nothing to restart
> +	 *                                       this sweep
> +	 *   pick N, N's session opens
> +	 *
> +	 * Closing the session here reports NONE to @sch and its ancestors now
> +	 * and lets the pick report N's sched from NONE instead of from @sch.
> +	 */
> +	for_each_possible_cpu(cpu) {
> +		struct rq *rq = cpu_rq(cpu);
> +
> +		guard(rq_lock_irqsave)(rq);
> +		if (rq->scx.sched == sch)
> +			scx_cid_sched_update(rq, NULL);

[Severity: Medium]
When disabling the last active scheduler, does this disable the static branch
too early?

In scx_ops_cid_sched_updated_disable(), if this is the last scheduler, the
static branch is turned off before the loop:

    last = SCX_HAS_OP(sch, sub_cid_sched_updated) && !--scx_nr_ops_cid_sched_updated;
    if (last)
        static_branch_disable(&__scx_ops_cid_sched_updated_enabled);

Then, the loop calls scx_cid_sched_update(rq, NULL). However,
scx_cid_sched_update() starts with an early return if the static branch is
disabled:

    if (!static_branch_unlikely(&__scx_ops_cid_sched_updated_enabled) || prev == sch)
        return;

Will this cause the final NONE state notification to be silently dropped for
the last BPF scheduler unloaded?

> +		if (last)
> +			rq->scx.sched = NULL;
> +	}
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005175520.2756986-1-tj@kernel.org?part=1