Re: [PATCH v3 14/24] sched/deadline: Record the root domain holding a task's bandwidth

[sashiko-bot@kernel.org]

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_rq() root domain check on sleeping moved tasks
- [Medium] sched/deadline: missing special and zero-bw checks in dl_task_needs_bw_move()
--

commit 529159218137dba9eec548b4daee830c808791a5
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

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

This commit records a CPU of the root domain holding a deadline task's
bandwidth in sched_dl_entity::dl_bw_cpu so that bandwidth updates,
migrations, and releases operate on the root domain that actually accounted
for the bandwidth.

Link: https://lore.kernel.org/all/20261103-b4-sched-dl-bw-fixes-v1-1-dd761fe005b4@redhat.com/

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d9bd01a1bac0e..6a9b9791ddad7 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c

[ ... ]

> @@ -3474,17 +3499,30 @@ static void set_cpus_allowed_dl(struct task_struct *p,
>  	 * domain (see cpuset_can_attach()).
>  	 */
>  	if (dl_task_needs_bw_move(p, ctx->new_mask)) {
> -		struct dl_bw *src_dl_b;
> +		unsigned int src_cpu = dl_task_root_domain_cpu(p);
> +		struct dl_bw *src_dl_b = &cpu_rq(src_cpu)->rd->dl_bw;
> +		unsigned int cpu;
>  
> -		src_dl_b = dl_bw_of(cpu_of(rq));
>  		/*
>  		 * We now free resources of the root_domain we are migrating
>  		 * off. In the worst case, sched_setattr() may temporary fail
> -		 * until we complete the update.
> +		 * until we complete the update. A rebuild that dropped the
> +		 * bandwidth of @p leaves nothing to free.
> +		 */
> +		if (p->dl.dl_bw_gen == dl_bw_gen) {
> +			raw_spin_lock(&src_dl_b->lock);
> +			__dl_sub(src_dl_b, p->dl.dl_bw, dl_bw_cpus(src_cpu));
> +			raw_spin_unlock(&src_dl_b->lock);
> +		}
> +
> +		/*
> +		 * The room in the root domain of the mask to come was made
> +		 * by cpuset_can_attach(): that one counts the bandwidth from
> +		 * here on.
>  		 */
> -		raw_spin_lock(&src_dl_b->lock);
> -		__dl_sub(src_dl_b, p->dl.dl_bw, dl_bw_cpus(task_cpu(p)));
> -		raw_spin_unlock(&src_dl_b->lock);
> +		cpu = cpumask_any_and(ctx->new_mask, cpu_active_mask);
> +		p->dl.dl_bw_cpu = cpu < nr_cpu_ids ? (int)cpu : -1;
> +		p->dl.dl_bw_gen = dl_bw_gen;
>  	}
>  
>  	set_cpus_allowed_common(p, ctx);
> @@ -3493,10 +3531,13 @@ static void set_cpus_allowed_dl(struct task_struct *p,
>  bool dl_task_needs_bw_move(struct task_struct *p,
>  			   const struct cpumask *new_mask)
>  {
> +	unsigned int cpu;
> +
>  	if (!dl_task(p))
>  		return false;
>  
> -	return !cpumask_intersects(task_rq(p)->rd->span, new_mask);
> +	cpu = dl_task_root_domain_cpu(p);
> +	return !cpumask_intersects(cpu_rq(cpu)->rd->span, new_mask);
>  }

[Severity: Medium]
Should dl_task_needs_bw_move() also return false when
dl_entity_is_special(&p->dl) or !p->dl.dl_bw (or p->dl.dl_bw_cpu < 0)?

In include/linux/sched.h, dl_bw_cpu is documented as a CPU of the root
domain that counts dl_bw, and -1 where none does. Consistent with that,
dl_add_task_root_domain() checks:

    if (!dl_task(p) || dl_entity_is_special(&p->dl) || !p->dl.dl_bw)

so special entities and tasks boosted into the class via priority
inheritance with no bandwidth of their own do not get counted or given a
dl_bw_cpu record.

However, dl_task_needs_bw_move() only checks !dl_task(p), and
dl_task_root_domain_cpu(p) falls back to task_cpu(p) when dl_bw_cpu < 0.

Even with the follow-up commits "sched/deadline: Move a task's bandwidth
only where room was made" and "cgroup/cpuset: Give back the bandwidth an
attach did not move" applied, if a non-deadline task with p->dl.dl_bw == 0
and p->dl.dl_bw_cpu == -1 is priority-inheritance boosted into
dl_sched_class and moved across root domains via cpuset_can_attach() ->
cpuset_attach_task(), cpuset_attach_task() evaluates:

    dl_task(task) && dl_task_needs_bw_move(task, cs->effective_cpus) &&
    cs->sum_moved_dl_bw + task->dl.dl_bw <= cs->sum_migrate_dl_bw

as true (since 0 <= 0) and calls set_cpus_allowed_bw_reserved() ->
set_cpus_allowed_dl(), which sets a non-negative p->dl.dl_bw_cpu on the
0-bandwidth task.

When that task is later deboosted in switched_from_dl(), if dl_runtime == 0,
dl_task_release_bw() is not called and the task returns to SCHED_NORMAL with
a stale non-negative dl_bw_cpu. If the task previously left SCHED_DEADLINE
while asleep past its 0-lag time (where switched_from_dl() called
dl_task_release_bw() and __dl_clear_state(), clearing dl_bw = 0 and
dl_bw_cpu = -1 while keeping dl_runtime > 0), deboosting it before its 0-lag
time causes dl_task_depart() to see dl_bw_cpu >= 0 and link the 0-bandwidth
task onto dl_departed_list.

Similarly, for a SCHED_FLAG_SUGOV kthread (where sched_dl_overflow() returns
0 early leaving dl_bw_cpu = -1 while __setparam_dl() sets dl_bw > 0, and
sugov_kthread_create() uses set_cpus_allowed_ptr() without PF_NO_SETAFFINITY
when policy->dvfs_possible_from_any_cpu is true), migrating the kthread
across root domains causes dl_task_needs_bw_move() to return true, charging
its uncounted dl_bw into root-domain total_bw accounting and setting
dl_bw_cpu >= 0.

> @@ -3568,7 +3609,11 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	struct cpumask *msk;
>  
>  	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
> -	if (!dl_task(p) || dl_entity_is_special(&p->dl)) {
> +	/*
> +	 * Nothing to count for a special entity, nor for a task boosted into
> +	 * the class with no bandwidth of its own, which gets no record.
> +	 */
> +	if (!dl_task(p) || dl_entity_is_special(&p->dl) || !p->dl.dl_bw) {
>  		raw_spin_unlock_irqrestore(&p->pi_lock, rf.flags);
>  		return;
>  	}
> @@ -3590,10 +3635,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;
>  	} else if (READ_ONCE(p->__state) == TASK_DEAD) {
>  		/*
>  		 * Dead and not departed: its bandwidth was released at its
> @@ -3610,6 +3657,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);

[ ... ]

> @@ -3872,13 +3920,10 @@ static void switched_to_dl(struct rq *rq, struct task_struct *p)
>  	/*
>  	 * 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.
> +	 * timer will not remove it, but its leaving the class again, from
> +	 * the root domain of the CPU recorded, which it keeps.
>  	 */
> -	if (p->dl.dl_departed_cpu >= 0)
> -		dl_task_undepart(p);
> +	dl_task_undepart(p);

[Severity: High]
As reported by Sashiko on v2 [1], 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 in root domain RD0 switches to SCHED_NORMAL
before its 0-lag time, switched_from_dl() arms inactive_timer and calls
dl_task_depart(p), linking p->dl.dl_departed_node onto dl_departed_list with
p->dl.dl_bw_cpu set to RD0's CPU and p->dl.dl_bw > 0.

If p is then boosted into dl_sched_class via priority inheritance
(rt_mutex_setprio() -> switched_to_dl()), switched_to_dl() cancels
inactive_timer and calls dl_task_undepart(p), which unlinks
p->dl.dl_departed_node from dl_departed_list while keeping p->dl.dl_bw_cpu
set to RD0's CPU.

If a root domain rebuild (dl_rebuild_rd_accounting()) runs while p is
PI-boosted, dl_clear_root_domains() does not restore p's bandwidth because p
is no longer on dl_departed_list, and in dl_add_task_root_domain(p),
!list_empty(&p->dl.dl_departed_node) is false. Doesn't this lead to three
issues:

1. If p was moved to another cpuset partition RD1 while in SCHED_NORMAL
   (where no deadline admission check or bandwidth reservation was performed
   because !dl_task(p)), neither dl_add_task_root_domain(p) here nor
   dl_task_rebuild_cpu(p) in the follow-up commit "sched/deadline: Credit a
   rebuild to the CPU recorded with the task's bandwidth" preserves
   p->dl.dl_bw_cpu (since RD0's CPU is not in RD1's effective cpumask).
   Instead, the rebuild adds p->dl.dl_bw to RD1 and overwrites
   p->dl.dl_bw_cpu with RD1's CPU.

2. If p's departing bandwidth was already dropped by an earlier rebuild
   (p->dl.dl_bw_gen < dl_bw_gen - 1 or p->dl.dl_bw_cpu went offline),
   dl_add_task_root_domain(p) lacks the dl_bw_gen == dl_bw_gen - 1 and
   cpu_active(recorded) checks present in dl_clear_root_domains() and
   resurrects p->dl.dl_bw into p's effective cpuset.

3. 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 dl_clear_root_domains() misses it) and
   then deboost via rt_mutex_setprio() -> switched_from_dl() before
   dl_add_task_root_domain(p) inspects it. 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 from RD0.

[ ... ]

> @@ -4091,9 +4136,10 @@ int sched_dl_overflow(struct task_struct *p, int policy,
>  	u64 period = attr->sched_period ?: attr->sched_deadline;
>  	u64 runtime = attr->sched_runtime;
>  	u64 new_bw = dl_policy(policy) ? to_ratio(period, runtime) : 0;
> -	int cpus, err = -1, cpu = task_cpu(p);
> -	struct dl_bw *dl_b = dl_bw_of(cpu);
> +	int cpus, err = -1, cpu;
> +	struct dl_bw *dl_b;
>  	unsigned long cap;
> +	bool counted;
>  
>  	if (attr->sched_flags & SCHED_FLAG_SUGOV)
>  		return 0;
> @@ -4102,6 +4148,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]
As reported by Sashiko on v2 [2], does updating a task's deadline parameters
or affinity while it is asleep after moving to a new cpuset partition still
check the stale task_rq(p) root domain in __sched_setscheduler() and
dl_task_check_affinity(), or fall back to the stale task_cpu(p) in
sched_dl_overflow() when counted is false?

When a task p sleeping on a CPU in cpuset partition A is moved via
cgroup.procs (cpuset_attach_task() -> set_cpus_allowed_ptr()) to a disjoint
cpuset partition B, affine_move_task() leaves task_cpu(p) and task_rq(p) on
partition A's CPU until wakeup, while set_cpus_allowed_common() updates
p->cpus_ptr to partition B's CPUs and (for a SCHED_DEADLINE task)
set_cpus_allowed_dl() moves its bandwidth to partition B and sets
p->dl.dl_bw_cpu to a CPU in partition B.

Although sched_dl_overflow() is updated here to use p->dl.dl_bw_cpu when
counted is true, its caller __sched_setscheduler() in
kernel/sched/syscalls.c still checks rq = task_rq(p) (in 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) fails and __sched_setscheduler() returns
-EPERM before reaching sched_dl_overflow().

Similarly, dl_task_check_affinity() in kernel/sched/syscalls.c checks:

    guard(rcu)();
    if (!cpumask_subset(task_rq(p)->rd->span, mask))
        return -EBUSY;

against the stale task_rq(p)->rd->span rather than the root domain of
dl_task_root_domain_cpu(p).

In addition, in sched_dl_overflow(), when counted is false (for example,
when a sleeping non-deadline task moved to partition B enters SCHED_DEADLINE
with dl_bandwidth_enabled() disabled or via sched_setattr_nocheck(), or
after a rebuild dropped its bandwidth), cpu = counted ? p->dl.dl_bw_cpu :
task_cpu(p) falls back to task_cpu(p) in partition A, crediting new_bw and
recording p->dl.dl_bw_cpu in partition A even though the task's cpuset and
p->cpus_ptr belong to partition B.

[1] https://lore.kernel.org/r/sashiko-outbox-198299@kernel.org
[2] https://lore.kernel.org/r/sashiko-outbox-198299@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105193359.1450487-1-mathieu.desnoyers@efficios.com?part=14