[PATCH sched_ext/for-7.3] sched_ext: skip ops.set_weight() for disabled tasks

[PATCH sched_ext/for-7.3] sched_ext: skip ops.set_weight() for disabled tasks

[Kuba Piecuch]

When switching a task's sched_class away from sched_ext, we get the
following sequence of events in __sched_setscheduler():

sched_change_begin()
  switched_from_scx()
    scx_disable_task(p)
      ops.disable(p)
__setscheduler_params()
  set_load_weight()
    reweight_task_scx(p)
      ops.set_weight(p)
p->sched_class = next_class;
sched_change_end()
  ...

Notably, ops.set_weight() is called _after_ ops.disable().
This violates the expected semantics of the callbacks, the expectation
being that ops.disable() can only be followed by ops.exit_task() or
ops.enable().

Skipping the weight adjustment for disabled tasks should be harmless
since the weight will be recalculated in scx_enable_task() if the task
ever rejoins SCX.

Signed-off-by: Kuba Piecuch <jpiecuch@google.com>
---
 kernel/sched/ext/ext.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
index 5241b55a58ec..b75d8d71ef4d 100644
--- a/kernel/sched/ext/ext.c
+++ b/kernel/sched/ext/ext.c
@@ -3673,6 +3673,17 @@ static void reweight_task_scx(struct rq *rq, struct task_struct *p,
 	if (task_dead_and_done(p))
 		return;
 
+	/*
+	 * When switching sched_class away from SCX, reweight_task_scx()
+	 * is called _after_ scx_disable_task(). Skip calling ops.set_weight()
+	 * since the BPF scheduler may have already forgotten the task in
+	 * ops.disable().
+	 * p->scx.weight will be recalculated in scx_enable_task() if the task
+	 * ever returns to SCX class.
+	 */
+	if (scx_get_task_state(p) != SCX_TASK_ENABLED)
+		return;
+
 	p->scx.weight = sched_weight_to_cgroup(scale_load_down(lw->weight));
 	if (SCX_HAS_OP(sch, set_weight))
 		SCX_CALL_OP_TASK(sch, set_weight, rq, p, p->scx.weight);
-- 
2.55.0.795.g602f6c329a-goog

Re: [PATCH sched_ext/for-7.3] sched_ext: skip ops.set_weight() for disabled tasks

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] reweight_task_scx() fails to update p->se.load, causing tasks to have permanently stale load weights when their priority changes or when transitioning from sched_ext back to sched_fair (CFS).
--

--- Patch [1]: [PATCH sched_ext/for-7.3] sched_ext: skip ops.set_weight() for disabled tasks ---
commit 009696d9cb0a5605e8759533b00b9204ec011c72
Author: Kuba Piecuch <jpiecuch@google.com>

sched_ext: skip ops.set_weight() for disabled tasks

When switching a task's sched_class away from sched_ext, ops.set_weight()
is currently called after ops.disable(). This commit skips the weight
adjustment for disabled tasks to respect the expected callback semantics,
allowing the weight to be recalculated later if the task rejoins SCX.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 691d53fe0f648..ff22f31594c5c 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -3925,6 +3925,17 @@ static void reweight_task_scx(struct rq *rq, struct task_struct *p,
>  	if (task_dead_and_done(p))
>  		return;
>  
> +	/*
> +	 * When switching sched_class away from SCX, reweight_task_scx()
> +	 * is called _after_ scx_disable_task(). Skip calling ops.set_weight()
> +	 * since the BPF scheduler may have already forgotten the task in
> +	 * ops.disable().
> +	 * p->scx.weight will be recalculated in scx_enable_task() if the task
> +	 * ever returns to SCX class.
> +	 */
> +	if (scx_get_task_state(p) != SCX_TASK_ENABLED)
> +		return;
> +
>  	p->scx.weight = sched_weight_to_cgroup(scale_load_down(lw->weight));
>  	if (SCX_HAS_OP(sch, set_weight))
>  		SCX_CALL_OP_TASK(sch, set_weight, rq, p, p->scx.weight);

[Severity: High]
This is a pre-existing issue, but does reweight_task_scx() need to update
p->se.load as well?

Looking at set_load_weight() in kernel/sched/core.c, it delegates the load
weight update to the current scheduling class's callback if available:

kernel/sched/core.c:set_load_weight() {
	...
	if (update_load && p->sched_class->reweight_task)
		p->sched_class->reweight_task(task_rq(p), p, &lw);
	else
		p->se.load = lw;
	...
}

When a task's priority is modified while it is in the sched_ext class,
set_load_weight() calls reweight_task_scx(). Because reweight_task_scx()
only updates p->scx.weight and entirely skips updating p->se.load, does the
CFS entity end up with a permanently stale load weight?

If the task's scheduling policy is later switched back to sched_fair, it
seems it would enter CFS with this incorrect load weight. Could this bypass
nice limits and break CFS fairness since the load weight dictates the task's
share of CPU time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710144342.3802587-1-jpiecuch@google.com?part=1

Re: [PATCH sched_ext/for-7.3] sched_ext: skip ops.set_weight() for disabled tasks

[Tejun Heo]

Hello,

Applied to sched_ext/for-7.2-fixes, thanks.

Added a Fixes: tag and stable Cc. The reversed set_weight()/disable()
ordering is a regression from the commit that moved ops.disable() ahead
of reweight_task_scx(), so it goes back to v6.19:

  Fixes: 637b0682821b ("sched: Fold sched_class::switch{ing,ed}_{to,from}() into the change pattern")
  Cc: stable@vger.kernel.org # v6.19+

Thanks.

--
tejun