Re: [PATCH 2/9] sched_ext: Split curr|donor references properly

[sashiko-bot@kernel.org]

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

New issues:
- [High] update_curr_scx() unconditionally consumes delta_exec for non-EXT donor tasks when an EXT owner task goes to sleep, improperly charging the EXT DL server and breaking CPU time accounting for the donor task.
- [Medium] The detailed task state dump in `scx_dump_cpu()` incorrectly assumes `rq->curr` is the only active task to dump, causing the actual BPF-scheduled EXT task (`rq->donor`) to be omitted when proxy execution is active and the contexts differ.

Pre-existing issues:
- [High] scx_can_stop_tick() evaluates the tick dependency of the outgoing task rather than the incoming task during set_next_task_scx(), leaving the tick improperly stopped on NOHZ_FULL CPUs.
--

commit 176c10999ef611ede6c4d2387d4c2485699560c2
Author: John Stultz <jstultz@google.com>

sched_ext: Split curr|donor references properly

This patch modifies sched_ext to use rq->donor instead of rq->curr for slice
accounting, preemption, task picking, tick handling, and CPU kicks. This
separation prepares the scheduler for proxy execution, where the task
supplying the scheduling context (donor) can differ from the physically
executing task.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 1a0ec985da77d..242f6ffbba350 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c
> @@ -1145,17 +1145,17 @@ static void touch_core_sched_dispatch(struct rq *rq, struct task_struct *p)
>  
>  static void update_curr_scx(struct rq *rq)
>  {
> -	struct task_struct *curr = rq->curr;
> +	struct task_struct *donor = rq->donor;
>  	s64 delta_exec;
>  
>  	delta_exec = update_curr_common(rq);
>  	if (unlikely(delta_exec <= 0))
>  		return;
>  
> -	if (curr->scx.slice != SCX_SLICE_INF) {
> -		curr->scx.slice -= min_t(u64, curr->scx.slice, delta_exec);
> -		if (!curr->scx.slice)
> -			touch_core_sched(rq, curr);
> +	if (donor->scx.slice != SCX_SLICE_INF) {
> +		donor->scx.slice -= min_t(u64, donor->scx.slice, delta_exec);

[Severity: High]
Does this code apply EXT-specific accounting to a non-EXT donor task when the
owner task goes to sleep?

If proxy execution is active, where the donor is a FAIR task and the owner
(currently executing) is an EXT task, update_curr_scx() will be called
because the executing task belongs to the EXT class.

Without a check ensuring the donor is actually in the EXT class
(e.g., if (donor->sched_class != &ext_sched_class) return;), could the EXT
DL server be erroneously charged for the FAIR task's time?

[ ... ]

> @@ -4029,7 +4030,7 @@ static void run_deferred(struct rq *rq)
>  #ifdef CONFIG_NO_HZ_FULL
>  bool scx_can_stop_tick(struct rq *rq)
>  {
> -	struct task_struct *p = rq->curr;
> +	struct task_struct *p = rq->donor;
>  	struct scx_sched *sch = scx_task_sched(p);
>  
>  	if (p->sched_class != &ext_sched_class)

[Severity: High]
This is a pre-existing issue, but does scx_can_stop_tick() evaluate the tick
dependency of the outgoing task rather than the incoming task?

When switching to an EXT task on a NOHZ_FULL CPU, set_next_task_scx()
triggers a check:

set_next_task_scx()
  sched_update_tick_dependency()
    scx_can_stop_tick()

At this point, rq->donor (and previously rq->curr) has not yet been updated
to the newly selected task, which happens later in __schedule().

If the previous task was not an EXT task (e.g., idle), won't
scx_can_stop_tick() return true and wrongly bypass the BPF scheduler's
request for ticks?

[ ... ]

> @@ -6007,6 +6008,9 @@ static void scx_dump_cpu(struct scx_sched *sch, struct seq_buf *s,
>  	dump_line(&ns, "          curr=%s[%d] class=%ps",
>  		  rq->curr->comm, rq->curr->pid,
>  		  rq->curr->sched_class);
> +	dump_line(&ns, "          donor=%s[%d] class=%ps",
> +		  rq->donor->comm, rq->donor->pid,
> +		  rq->donor->sched_class);
>  	if (!cpumask_empty(rq->scx.cpus_to_kick))
>  		dump_line(&ns, "  cpus_to_kick   : %*pb",
>  			  cpumask_pr_args(rq->scx.cpus_to_kick));

[Severity: Medium]
Will this miss dumping the detailed state of the EXT donor task if rq->donor
and rq->curr differ?

Later in scx_dump_cpu(), the detailed task dump only checks rq->curr:

	if (rq->curr->sched_class == &ext_sched_class &&
	    (dump_all_tasks || scx_task_on_sched(sch, rq->curr)))
		scx_dump_task(sch, s, dctx, rq, rq->curr, '*');

If proxy execution is active and rq->donor is an EXT task but rq->curr is a
FAIR task, the actual BPF-scheduled EXT task (the donor) would be completely
omitted from the detailed dump during a crash or sysrq.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260706070410.282826-1-arighi@nvidia.com?part=2