Re: [PATCH v5 23/29] sched/deadline: Move a task's bandwidth when it must leave its CPU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] sched/deadline: stale dl_bw_cpu on same-rd return in
  dl_task_move_bw()
- [Medium] sched/deadline: missed bw move in migrate_task_rq_dl() on
  task wakeup
--

commit 83356714a4dd1213bf9ceba0a7dd4eb657180f4e
Author: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>

sched/deadline: Move a task's bandwidth when it must leave its CPU

This commit moves a deadline task's bandwidth to the destination CPU's root
domain in migrate_task_rq_dl() when the task leaves an inactive or
disallowed CPU.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index bc31045e4686f..a35609a10ec5a 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -2873,11 +2873,54 @@ select_task_rq_dl(struct task_struct *p, int cpu, int flags)
>  	return cpu;
>  }
>  
> -static void migrate_task_rq_dl(struct task_struct *p, int new_cpu __maybe_unused)
> +/*
> + * Move the bandwidth of @p to the root domain of @new_cpu. Count it there
> + * also if a rebuild left @p out. Add it even without room, since @p runs
> + * there. The caller holds the rq lock of @p, or its pi_lock for a wakeup.
> + */
> +static void dl_task_move_bw(struct task_struct *p, int new_cpu)
> +{
> +	struct dl_bw *dl_b;
> +	int cpu;
> +
> +	if (dl_entity_is_special(&p->dl) || !p->dl.dl_bw)
> +		return;
> +
> +	guard(raw_spinlock)(&dl_departed_lock);
> +	cpu = p->dl.dl_bw_cpu;
> +	if (cpu < 0)
> +		return;
> +	if (p->dl.dl_bw_gen == dl_bw_gen) {
> +		if (cpu_rq(cpu)->rd == cpu_rq(new_cpu)->rd)
> +			return;

[Severity: Medium]
Does returning here without setting p->dl.dl_bw_cpu = new_cpu leave a stale
offline or disallowed CPU recorded when the task moves within the same root
domain?

During sched_cpu_deactivate(cpu), set_cpu_active(cpu, false) and
balance_push_set(cpu, true) run before cpuset_cpu_inactive(cpu) detaches
cpu from its root domain and rebuilds bandwidth accounting.  When
balance_push() or try_to_wake_up() moves a task from the deactivating cpu
to an active new_cpu in the same root domain during that window,
cpu_rq(cpu)->rd == cpu_rq(new_cpu)->rd still holds, so p->dl.dl_bw_cpu is
left pointing to the inactive cpu.  If the task then departs on new_cpu
before its 0-lag time, dl_clear_root_domains() and dl_add_task_root_domain()
see !cpu_active(p->dl.dl_bw_cpu) and drop the task's departing bandwidth
from new_cpu's root domain, or dl_task_release_bw() subtracts it from
def_root_domain once detach_destroy_domains() has run.

Updating p->dl.dl_bw_cpu = new_cpu before returning when the root domains
match would keep the recorded CPU valid.

> +		dl_b = dl_bw_of(cpu);
> +		raw_spin_lock(&dl_b->lock);
> +		__dl_sub(dl_b, p->dl.dl_bw, dl_bw_cpus(cpu));
> +		raw_spin_unlock(&dl_b->lock);
> +	}
> +	dl_b = dl_bw_of(new_cpu);
> +	raw_spin_lock(&dl_b->lock);
> +	__dl_add(dl_b, p->dl.dl_bw, dl_bw_cpus(new_cpu));
> +	raw_spin_unlock(&dl_b->lock);
> +	p->dl.dl_bw_cpu = new_cpu;
> +	p->dl.dl_bw_gen = dl_bw_gen;
> +}
> +
> +static void migrate_task_rq_dl(struct task_struct *p, int new_cpu)
>  {
> +	int cpu = task_cpu(p);
>  	struct rq_flags rf;
>  	struct rq *rq;
>  
> +	/*
> +	 * The task must leave a CPU that is no longer active, or that its
> +	 * mask no longer holds. The new CPU can then be in another root
> +	 * domain. Move the bandwidth there, as dl_task_offline_migration()
> +	 * does for a throttled task.
> +	 */
> +	if (unlikely(!cpu_active(cpu) || !cpumask_test_cpu(cpu, p->cpus_ptr)))
> +		dl_task_move_bw(p, new_cpu);

[Severity: Medium]
Can this check miss the bandwidth move on wakeup when task_cpu(p) is an
active CPU that is in p->cpus_ptr by the time set_task_cpu() runs?

If a task sleeps on an active housekeeping CPU c0 (task_cpu(p) == c0) and
has its affinity and bandwidth moved to an isolated CPU c1 while asleep,
taking c1 offline leaves the task out of the rebuild (or counted in
def_root_domain).  When the task wakes up in try_to_wake_up():

kernel/sched/core.c:select_fallback_rq() {
    ...
		case cpuset:
			if (cpuset_cpus_allowed_fallback(p)) {
				state = possible;
				break;
			}
			fallthrough;
		case possible:
			set_cpus_allowed_force(p, task_cpu_fallback_mask(p));
    ...
}

select_fallback_rq() widens p->cpus_ptr to include c0 before returning to
try_to_wake_up().  If it picks another housekeeping CPU c2, set_task_cpu()
calls migrate_task_rq_dl(p, c2) with cpu = c0, which is both active and now
in p->cpus_ptr, so dl_task_move_bw() is skipped.  If select_fallback_rq()
picks c0 itself, try_to_wake_up() sees task_cpu(p) == cpu and skips
set_task_cpu() and migrate_task_rq_dl() altogether.  The same happens when
sched_setaffinity() (with admission control disabled) moves a sleeping task
away from c0 across a rebuild and then back to a mask containing c0 before
wakeup.  In each case the waking task runs in a root domain that does not
count its bandwidth.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261108185406.1549770-1-mathieu.desnoyers@xxxxxxxxxxxx?part=23