Re: [PATCH v4 16/26] sched/deadline: Record the root domain holding a task's bandwidth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] sched/deadline: dl_task_undepart() loses departed state on PI
  boost
- [Medium] sched/deadline: stale dl_bw_cpu when dl_bw is zero misroutes
  bandwidth
--

commit 750b37778a0ba278821539fc3ca7bca85e0c4a0a
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

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

This commit records a CPU from the root domain accounting for a deadline
task's bandwidth in sched_dl_entity::dl_bw_cpu so that subsequent moves,
parameter updates, rebuilds, and releases operate on the root domain that
took 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 3f3de4b74a2dc..7a7101e6f7a03 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c

[ ... ]

> @@ -3537,10 +3575,18 @@ 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)
>  {
> -	if (!dl_task(p))
> +	unsigned int cpu;
> +
> +	/*
> +	 * Nothing to move for a special entity, nor for a task boosted into
> +	 * the class with no bandwidth of its own: neither is counted, as
> +	 * dl_add_task_root_domain() leaves them out.
> +	 */
> +	if (!dl_task(p) || dl_entity_is_special(&p->dl) || !p->dl.dl_bw)
>  		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);
>  }
>  
>  /* Assumes rq->lock is held */
> @@ -3614,8 +3660,9 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
>  	/*
>  	 * Nothing to count for a special entity, nor for a task with no
> -	 * bandwidth, such as an exited one whose bandwidth went with its
> -	 * parameters, which the walk of the task list finds too.
> +	 * bandwidth: an exited one whose bandwidth went with its
> +	 * parameters, which the walk of the task list finds too, and a task
> +	 * boosted into the class with none 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);
> @@ -3639,10 +3686,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) {

[ ... ]

> @@ -3942,13 +3992,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: Medium]
When a task that left SCHED_DEADLINE before its 0-lag time is boosted back
into dl_sched_class via priority inheritance, does calling
dl_task_undepart(p) lose the fact that its bandwidth is departing?

Unlinking p->dl.dl_departed_node while !task_has_dl_policy(p) makes
list_empty(&p->dl.dl_departed_node) true while dl_task(p) and p->dl.dl_bw
are both non-zero.  If the task's affinity or cpuset changed to another
root domain after it left SCHED_DEADLINE, a root-domain rebuild in
dl_add_task_root_domain() will skip the !list_empty(&p->dl.dl_departed_node)
branch, charge the departing bandwidth to the new root domain that never
admitted it, and overwrite p->dl.dl_bw_cpu.

For the same reason, dl_task_needs_bw_move() checks !dl_task(p) rather
than !task_has_dl_policy(p), so migrating that PI-boosted task across
exclusive cpusets moves its departing bandwidth into the destination root
domain.  If a prior rebuild had already dropped the departing bandwidth,
dl_add_task_root_domain() also puts it back because it does not check
dl_bw_gen == dl_bw_gen - 1 as dl_clear_root_domains() does.  And because
rt_mutex_setprio() does not serialize against dl_rebuild_rd_accounting(),
a departed task that is PI-boosted when dl_clear_root_domains() runs and
deboosted before the task-list walk reaches it is missed by both, dropping
its active 0-lag bandwidth from the root domain.

[ ... ]

> @@ -4155,9 +4202,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;
> @@ -4166,6 +4214,20 @@ 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, or the lock of dl_accounting_lock()
> +	 * without cpusets, 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);

[ ... ]

> @@ -4209,11 +4273,20 @@ int sched_dl_overflow(struct task_struct *p, int policy,
>  	 * run meanwhile: the caller holds cpuset_mutex, or the lock of
>  	 * dl_accounting_lock() without cpusets.
>  	 */
> -	if (!err && dl_policy(policy) && !task_has_dl_policy(p)) {
> -		lockdep_assert_cpuset_lock_held();
> -		lockdep_assert_dl_accounting_held();
> -		if (p->dl.dl_departed_cpu >= 0)
> -			dl_task_release_bw(p, -1);
> +	if (!err && dl_policy(policy)) {
> +		if (!task_has_dl_policy(p)) {
> +			lockdep_assert_cpuset_lock_held();
> +			lockdep_assert_dl_accounting_held();
> +			/*
> +			 * Whatever bandwidth the task still has counted
> +			 * somewhere, the new one replaces: on the departed
> +			 * list, or taken off it by a boost into the class
> +			 * (switched_to_dl()), the record says where.
> +			 */
> +			if (p->dl.dl_bw_cpu >= 0)
> +				dl_task_release_bw(p);
> +		}
> +		p->dl.dl_bw_cpu = cpu;
> +		p->dl.dl_bw_gen = dl_bw_gen;
>  	}

[Severity: Medium]
Can recording p->dl.dl_bw_cpu when new_bw is 0 cause a later sched_setattr()
to charge the task's bandwidth to a stale root domain?

__checkparam_dl() permits parameters where (runtime << BW_SHIFT) < period
(for example, runtime = 1024 ns and period = 2 s), for which to_ratio()
returns new_bw == 0.  sched_dl_overflow() (and dl_task_offline_migration())
still sets p->dl.dl_bw_cpu to a CPU in the current root domain RD0.  If
the task is then moved to another exclusive cpuset partition RD1,
dl_task_needs_bw_move() returns false because !p->dl.dl_bw, leaving
p->dl.dl_bw_cpu pointing at RD0.  When the task runs in RD1 and calls
sched_setattr() with new_bw > 0, sched_dl_overflow() sees counted == true
and validates and adds new_bw to RD0's dl_bw instead of RD1's, bypassing
admission control in RD1.

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