Re: [PATCH v4 20/26] sched/deadline: Credit a rebuild to the CPU recorded with the task's bandwidth

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] sched/deadline: stale dl_bw_cpu used by dl_task_rebuild_cpu()
  after fallback migration
- [Medium] sched/deadline: dl_task_rebuild_cpu() disagrees with
  select_fallback_rq() on isolated CPUs
--

commit 95575db84831c783a0bb061a74c9e5615adce1b2
Author: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>

sched/deadline: Credit a rebuild to the CPU recorded with the task's bandwidth

This commit adds dl_task_rebuild_cpu() so root domain bandwidth rebuilds
credit each deadline task to the CPU recorded with its bandwidth when active
and allowed, and updates dl_task_offline_migration() to add bandwidth even
when a prior rebuild left the task out.

> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index f169cf35e3136..ed951246e3d2d 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c

[ ... ]

> @@ -3656,6 +3662,47 @@ static void dl_get_task_effective_cpus(struct task_struct *p, struct cpumask *cp
>  	cpuset_cpus_allowed_locked(p, cpus);
>  }
>  
> +/*
> + * A CPU of the root domain that a rebuild of the accounting counts @p's
> + * bandwidth in. The one recorded with it, where it is active and the
> + * cpuset of @p allows it: that of the root domain that admitted the
> + * bandwidth, or that the last move put it in, which the cpuset alone does
> + * not tell where it holds CPUs of two root domains, housekeeping ones
> + * beside the ones isolcpus="domain" leaves to the default root domain. The
> + * record answers as well for a task whose own CPUs went offline, moved to
> + * an active one by dl_task_offline_migration(). Else the first active CPU
> + * the cpuset allows, a housekeeping one where it has any; and nr_cpu_ids
> + * where it allows none, which the isolated CPUs allow, no deadline task
> + * of theirs keeping the last of them online.
> + */
> +static unsigned int dl_task_rebuild_cpu(struct task_struct *p)
> +{
> +	struct cpumask *msk = this_cpu_cpumask_var_ptr(local_cpu_mask_dl);
> +	int recorded = p->dl.dl_bw_cpu;
> +	unsigned int cpu;
> +
> +	/*
> +	 * The task's own CPUs first: a change of cpusets sets the mask of
> +	 * each task before the rebuild it makes, where the housekeeping mask
> +	 * that dl_get_task_effective_cpus() reads lags that change. With
> +	 * none of them active, the CPU recorded is where
> +	 * dl_task_offline_migration() moved the task; else its cpuset says.
> +	 */
> +	cpumask_and(msk, &p->cpus_mask, cpu_active_mask);
> +	if (cpumask_empty(msk)) {
> +		if (recorded >= 0 && cpu_active(recorded))
> +			return recorded;
> +		dl_get_task_effective_cpus(p, msk);
> +		cpumask_and(msk, msk, cpu_active_mask);
> +	}
> +	if (recorded >= 0 && cpumask_test_cpu(recorded, msk))
> +		return recorded;
> +	cpu = cpumask_first_and(msk, housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT));
> +	if (cpu < nr_cpu_ids)
> +		return cpu;
> +	return cpumask_first(msk);
> +}

[Severity: Medium]
Can dl_task_rebuild_cpu() credit a task's bandwidth to a different root
domain than the one select_fallback_rq() places the task on when multiple
CPUs are isolated with isolcpus=domain?

Suppose CPUs 0 and 1 are housekeeping and CPUs 2 and 3 are isolated in
def_root_domain, and a sleeping deadline task is pinned to CPU 2 when CPU 2
goes offline.  Because p->cpus_mask has no active CPU and p->dl.dl_bw_cpu
(2) is inactive, dl_task_rebuild_cpu() falls back to
dl_get_task_effective_cpus(), which sees p->cpus_ptr disjoint from hk_msk
and returns the remaining active isolated CPU 3, so dl_add_task_root_domain()
credits def_root_domain and sets p->dl.dl_bw_cpu = 3.  When the task later
wakes up, select_fallback_rq() -> cpuset_cpus_allowed_fallback() ->
set_cpus_allowed_force() widens p->cpus_mask to all possible CPUs {0..3} and
selects the first active CPU 0 in the housekeeping root domain, while
p->dl.dl_bw_cpu remains 3 (now in p->cpus_mask) so subsequent rebuilds keep
the bandwidth in def_root_domain.

Conversely, if CPUs 0 and 1 are isolated and CPUs 2 and 3 are housekeeping,
and a running task pinned to CPU 0 is migrated by balance_push() ->
select_fallback_rq() when CPU 0 goes offline, select_fallback_rq() widens
p->cpus_mask to {0..3} and moves the task to the first active CPU 1 in
def_root_domain.  When dl_task_rebuild_cpu() then runs with recorded = 0
(inactive) and msk = {1, 2, 3}, cpumask_first_and(msk,
housekeeping_cpumask(HK_TYPE_DOMAIN_BOOT)) picks housekeeping CPU 2 instead
of task_cpu(p) (CPU 1), crediting the housekeeping root domain while the
task runs in def_root_domain.

>  /* From dl_rebuild_rd_accounting(), under the locks it asserts. */
>  void dl_add_task_root_domain(struct task_struct *p)
>  {
> @@ -3663,7 +3710,6 @@ void dl_add_task_root_domain(struct task_struct *p)
>  	struct rq *rq;
>  	struct dl_bw *dl_b;
>  	unsigned int cpu;
> -	struct cpumask *msk;
>  
>  	raw_spin_lock_irqsave(&p->pi_lock, rf.flags);
>  	/*
> @@ -3677,10 +3723,16 @@ void dl_add_task_root_domain(struct task_struct *p)
>  		return;
>  	}
>  
> -	msk = this_cpu_cpumask_var_ptr(local_cpu_mask_dl);
> -	dl_get_task_effective_cpus(p, msk);
> -	cpu = cpumask_first_and(cpu_active_mask, msk);
> -	BUG_ON(cpu >= nr_cpu_ids);
> +	cpu = dl_task_rebuild_cpu(p);
> +	if (cpu >= nr_cpu_ids) {
> +		/*
> +		 * No active CPU to count this task's bandwidth: leave it out
> +		 * of this rebuild, where dl_bw_gen keeps its release out as
> +		 * well, until one has a CPU for it.
> +		 */
> +		raw_spin_unlock_irqrestore(&p->pi_lock, rf.flags);
> +		return;
> +	}

[Severity: Medium]
When a task has no active CPU and is left out of the rebuild here, does
leaving p->dl.dl_bw_cpu untouched cause a later rebuild to credit its
bandwidth back to the old isolated CPU after fallback migration?

If a sleeping task confined to isolated CPU 2 is skipped here when CPU 2
goes offline, p->dl.dl_bw_cpu stays 2.  When the task wakes up while CPU 2
is still offline, try_to_wake_up() -> select_task_rq() ->
select_fallback_rq() calls set_cpus_allowed_force() without
SCA_DL_BW_RESERVED, widening p->cpus_mask to all possible CPUs and moving
the task to a housekeeping CPU without updating p->dl.dl_bw_cpu.  Once CPU 2
is brought back online, dl_task_rebuild_cpu() sees recorded == 2 set in
p->cpus_mask & cpu_active_mask and returns CPU 2, crediting the task's
bandwidth to def_root_domain even though the task was left out of the
previous generation and is now running in the housekeeping root domain.

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