Re: [PATCH v2 15/21] sched/deadline: Record the root domain holding a task's bandwidth

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] sched/deadline: broken rd rebuild for PI-boosted departed tasks
- [Medium] sched/deadline: stale task_cpu() root domain on sleeping moved tasks
- [Low] sched/deadline: stale comments in switched_to_dl() and dl_bw_cpu helpers
--

commit 90cd336704a1119534fc7c444edd810ae1302642
Author: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>

sched/deadline: Record the root domain holding a task's bandwidth

This commit replaces dl_departed_cpu and cpumask-based root domain lookups
with a recorded dl_bw_cpu in struct sched_dl_entity so that deadline
bandwidth is released or moved from the same root domain where it was
accounted.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index 85df2b0044bed..1dbde0fd199b4 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -252,15 +252,14 @@ static unsigned int dl_bw_gen;
>   * @p leaves SCHED_DEADLINE, or dies, its bandwidth kept for now in the root
>   * domain of @cpu. Under its rq lock.
>   */
> -static void dl_task_depart(struct task_struct *p, int cpu)
> +static void dl_task_depart(struct task_struct *p)
>  {
>  	struct sched_dl_entity *dl_se = &p->dl;
>  
>  	lockdep_assert_irqs_disabled();
> -	if (dl_se->dl_departed_cpu >= 0)
> -		return;
>  	guard(raw_spinlock)(&dl_departed_lock);
> -	dl_se->dl_departed_cpu = cpu;
> +	if (!list_empty(&dl_se->dl_departed_node) || dl_se->dl_bw_cpu < 0)
> +		return;
>  	list_add(&dl_se->dl_departed_node, &dl_departed_list);
>  }
>  
> @@ -273,9 +272,10 @@ static void dl_task_undepart(struct task_struct *p)
>  	struct sched_dl_entity *dl_se = &p->dl;
>  
>  	lockdep_assert_irqs_disabled();
> +	if (list_empty(&dl_se->dl_departed_node))
> +		return;
>  	guard(raw_spinlock)(&dl_departed_lock);
> -	dl_se->dl_departed_cpu = -1;
> -	list_del(&dl_se->dl_departed_node);
> +	list_del_init(&dl_se->dl_departed_node);
>  }
>  
>  /*
> @@ -284,24 +284,25 @@ static void dl_task_undepart(struct task_struct *p)
>   * when a rebuild of the accounting has dropped it; and no rebuild running
>   * counts it again. Under @p's rq lock.
>   */
> -static void dl_task_release_bw(struct task_struct *p, int cpu)
> +static void dl_task_release_bw(struct task_struct *p)

[ ... ]

> @@ -3475,11 +3488,14 @@ static void task_woken_dl(struct rq *rq, struct task_struct *p)
>   */
>  static unsigned int dl_task_root_domain_cpu(struct task_struct *p)
>  {
> -	unsigned int cpu;
> +	int cpu = p->dl.dl_bw_cpu;
>  
> -	cpu = cpumask_first_and(&p->cpus_mask, cpu_active_mask);
> -	if (cpu >= nr_cpu_ids)
> -		cpu = task_cpu(p);
> +	/*
> +	 * No bandwidth of @p's counted anywhere: nothing to take back or to
> +	 * move, and the CPU it is on answers for its root domain.
> +	 */
> +	if (cpu < 0)
> +		return task_cpu(p);
>  	return cpu;
>  }

[ ... ]

> @@ -3614,10 +3643,12 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	 */
>  	if (p->dl.dl_bw_gen == dl_bw_gen)
>  		goto unlock;
> -	if (p->dl.dl_departed_cpu >= 0) {
> -		cpu = p->dl.dl_departed_cpu;
> -		if (!cpu_active(cpu))
> +	if (!list_empty(&p->dl.dl_departed_node)) {
> +		int recorded = p->dl.dl_bw_cpu;
> +
> +		if (recorded < 0 || !cpu_active(recorded))
>  			goto unlock;
> +		cpu = recorded;
>  	}
>  	rq = cpu_rq(cpu);
>  	dl_b = &rq->rd->dl_bw;
> @@ -3625,6 +3656,7 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	raw_spin_lock(&dl_b->lock);
>  	__dl_add(dl_b, p->dl.dl_bw, cpumask_weight(rq->rd->span));
>  	raw_spin_unlock(&dl_b->lock);
> +	p->dl.dl_bw_cpu = cpu;
>  	p->dl.dl_bw_gen = dl_bw_gen;
>  unlock:
>  	raw_spin_unlock(&dl_departed_lock);

[Severity: High]
Can root domain bandwidth accounting be corrupted during a rebuild when a
departed task is boosted into dl_sched_class via priority inheritance before
its 0-lag time expires?

When a SCHED_DEADLINE task leaves the deadline policy before its 0-lag time,
switched_from_dl() arms inactive_timer and calls dl_task_depart(p) to link
p->dl.dl_departed_node onto dl_departed_list with p->dl.dl_bw_cpu recording
its root domain CPU. If p is then PI-boosted via rt_mutex_setprio() ->
switched_to_dl(), dl_task_undepart(p) unlinks p->dl.dl_departed_node while
preserving p->dl.dl_bw_cpu.

If dl_rebuild_rd_accounting() runs while p is PI-boosted,
dl_add_task_root_domain(p) sees list_empty(&p->dl.dl_departed_node) == true:

1. If p was moved to another cpuset partition while in SCHED_NORMAL (where no
deadline admission check ran because !dl_task(p)), dl_add_task_root_domain(p)
ignores p->dl.dl_bw_cpu, adds p->dl.dl_bw to the new cpuset's root domain via
dl_get_task_effective_cpus(p), and overwrites p->dl.dl_bw_cpu with cpu. Or,
if an earlier rebuild had already dropped p's departing bandwidth, this path
resurrects p->dl.dl_bw into p's effective cpuset.

2. Because rt_mutex_setprio() only holds p->pi_lock and rq->__lock (not
cpuset_mutex or dl_accounting_mutex), p can be PI-boosted when
dl_clear_root_domains() runs (so it is not on dl_departed_list and is not
restored there) and then deboost via rt_mutex_setprio() -> switched_from_dl()
before the task walk reaches p. switched_from_dl() puts p back onto
dl_departed_list after dl_clear_root_domains() has already finished, and
dl_add_task_root_domain(p) skips p because !dl_task(p) is now true, dropping
p's active 0-lag bandwidth completely.

3. For a regular non-deadline task with p->dl.dl_bw == 0 that is PI-boosted
during a rebuild, dl_add_task_root_domain(p) unconditionally sets
p->dl.dl_bw_cpu = cpu, leaving a non-negative dl_bw_cpu on a task with zero
bandwidth after it deboosts.

Should a departed task stay on dl_departed_list across PI boost/deboost until
its bandwidth is actually released, and should dl_add_task_root_domain(p) skip
tasks with p->dl.dl_bw == 0?

[ ... ]

> @@ -3890,8 +3922,7 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
>  	 * the task then removes the bandwidth from that one, as it did
>  	 * before departures were recorded.
>  	 */
> -	if (p->dl.dl_departed_cpu >= 0)
> -		dl_task_undepart(p);
> +	dl_task_undepart(p);

[Severity: Low]
This isn't a bug, but several comments in kernel/sched/deadline.c were not
updated to match the new dl_bw_cpu behavior:

1. In switched_to_dl(), the comment right above dl_task_undepart(p) still says:

	 * Boosted back into the class before its 0-lag time (a policy change
	 * released that bandwidth in sched_dl_overflow()): the inactive
	 * timer will not remove it, but its leaving the class again. That
	 * forgets the CPU recorded: moved to another root domain meanwhile,
	 * the task then removes the bandwidth from that one, as it did
	 * before departures were recorded.

However, dl_task_undepart(p) now preserves p->dl.dl_bw_cpu instead of
forgetting it.

2. Above dl_task_root_domain_cpu(), the function comment still states that it
uses p's current cpus_mask to find the root domain and falls back when
cpus_mask has no active CPU:

 * Find the root domain where a DL task's bandwidth is currently
 * accounted. For a sleeping task that was moved between cpusets
 * without running, task_rq(p)->rd may be stale. Use the task's
 * current cpus_mask (not yet updated to the new mask) to find the
 * actual root domain. Use cpus_mask rather than cpus_ptr, which
 * may be temporarily narrowed by migrate_disable().

whereas dl_task_root_domain_cpu() now reads p->dl.dl_bw_cpu directly.

3. Above dl_task_depart() and dl_task_release_bw(), the comments still refer
to the @cpu parameter and departed-CPU fallback that were removed from both
functions.

Could these comments be updated to reflect the new dl_bw_cpu logic?

[ ... ]

> @@ -4115,6 +4147,19 @@ int sched_dl_overflow(struct task_struct *p, int policy,
>  	if (new_bw == p->dl.dl_bw && task_has_dl_policy(p))
>  		return 0;
>  
> +	/*
> +	 * The bandwidth of a task of the class is counted in the root
> +	 * domain that took it, which the CPU the task is on does not tell:
> +	 * moved asleep, it has not run there yet. Change it where it is
> +	 * counted; a rebuild that dropped it, and a task that enters the
> +	 * class, have the root domain of that CPU take the new one. The
> +	 * caller holds cpuset_mutex, so no rebuild runs meanwhile.
> +	 */
> +	counted = task_has_dl_policy(p) && p->dl.dl_bw_cpu >= 0 &&
> +		  p->dl.dl_bw_gen == dl_bw_gen;
> +	cpu = counted ? p->dl.dl_bw_cpu : task_cpu(p);
> +	dl_b = dl_bw_of(cpu);

[Severity: Medium]
Does updating a task's deadline parameters while it is asleep after moving to
a new cpuset partition still use the stale task_cpu(p) root domain in
__sched_setscheduler() and sched_dl_overflow()?

When a task sleeping on a CPU in partition A is moved to a disjoint cpuset
partition B via cpuset_attach_task() -> set_cpus_allowed_ptr(),
affine_move_task() does not migrate the sleeping task's runqueue, so
task_cpu(p) remains in partition A while p->cpus_ptr and p->cpus_mask are
updated to partition B (and for a SCHED_DEADLINE task, set_cpus_allowed_dl()
updates p->dl.dl_bw_cpu to partition B).

If __sched_setscheduler() is then called on the still-sleeping task:

1. In kernel/sched/syscalls.c:__sched_setscheduler(), when
dl_bandwidth_enabled() is true, the affinity check before sched_dl_overflow()
still inspects rq->rd->span from task_rq(p) (partition A):

		if (dl_bandwidth_enabled() && dl_policy(policy) &&
				!(attr->sched_flags & SCHED_FLAG_SUGOV)) {
			cpumask_t *span = rq->rd->span;

			if (!cpumask_subset(span, p->cpus_ptr) ||
			    rq->rd->dl_bw.bw == 0) {
				retval = -EPERM;
				goto unlock;
			}
		}

Because partition A's span is disjoint from p->cpus_ptr (partition B),
!cpumask_subset(span, p->cpus_ptr) triggers and __sched_setscheduler() fails
with -EPERM before reaching sched_dl_overflow().

2. In sched_dl_overflow(), when counted is false (for example, when a sleeping
non-deadline task moved to partition B is switched to SCHED_DEADLINE while
still asleep, or when a prior rebuild dropped the task's bandwidth),
cpu = counted ? p->dl.dl_bw_cpu : task_cpu(p) falls back to task_cpu(p) in
partition A, crediting the bandwidth to partition A's root domain even though
inc_dl_tasks_cs(p) and the task's wakeup place it in partition B.

Should __sched_setscheduler() and the uncounted path in sched_dl_overflow()
resolve the destination root domain from the task's effective cpuset or
allowed mask rather than task_cpu(p)?

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