Re: [PATCH v5 18/29] sched/deadline: Move a task's bandwidth only where room was made

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] sched/deadline: skipped bandwidth move in
  __set_cpus_allowed_ptr_locked()
--

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

sched/deadline: Move a task's bandwidth only where room was made

This commit adds the SCA_DL_BW_RESERVED flag and
set_cpus_allowed_bw_reserved() so that set_cpus_allowed_dl() only moves a
deadline task's bandwidth across root domains when the caller has reserved
space in the destination root domain.

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index bf722f5ff97e5..030106b3d97f4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3272,6 +3272,22 @@ int set_cpus_allowed_ptr(struct task_struct *p, const struct cpumask *new_mask)
>  }
>  EXPORT_SYMBOL_GPL(set_cpus_allowed_ptr);
>  
> +/*
> + * Like set_cpus_allowed_ptr(), for a caller that has made room for the
> + * deadline bandwidth of @p in the root domain of @new_mask. Move that
> + * bandwidth there with @p. Return 0 on success, or a negative error
> + * code.
> + */
> +int set_cpus_allowed_bw_reserved(struct task_struct *p, const struct cpumask *new_mask)
> +{
> +	struct affinity_context ac = {
> +		.new_mask  = new_mask,
> +		.flags     = SCA_DL_BW_RESERVED,
> +	};
> +
> +	return __set_cpus_allowed_ptr(p, &ac);
> +}

[Severity: Medium]
Can __set_cpus_allowed_ptr_locked() skip set_cpus_allowed_dl() when
SCA_DL_BW_RESERVED is set and p->cpus_mask already equals ctx->new_mask?

kernel/sched/core.c:__set_cpus_allowed_ptr_locked() {
	...
	if (!(ctx->flags & SCA_MIGRATE_ENABLE)) {
		if (cpumask_equal(&p->cpus_mask, ctx->new_mask)) {
			if (ctx->flags & SCA_USER)
				swap(p->user_cpus_ptr, ctx->user_mask);
			goto out;
		}
	...
}

With this patch, an affinity change without SCA_DL_BW_RESERVED (such as
sched_setaffinity() when sched_rt_runtime_us is -1 on a cpuset spanning
two root domains) updates p->cpus_mask to CPUs in root domain B while
leaving p->dl.dl_bw_cpu in root domain A.  If the task is then attached to
a cpuset in root domain B whose effective affinity mask matches
p->cpus_mask, cpuset_can_attach() reserves bandwidth in root domain B and
cpuset_attach_task() calls set_cpus_allowed_bw_reserved(), but
__set_cpus_allowed_ptr_locked() takes the cpumask_equal() early exit
without calling do_set_cpus_allowed() -> set_cpus_allowed_dl().

That leaves p->dl.dl_bw counted in source root domain A, keeps
p->dl.dl_bw_cpu pointing to root domain A, and leaks the destination
reservation in root domain B.  Later commits in this series ("cgroup/cpuset:
Give back the bandwidth an attach did not move" and "sched/deadline: Move
a task's bandwidth when it must leave its CPU") only partially mitigate
this: if the task was sleeping during the earlier unreserved affinity
change, p->dl.dl_bw_cpu stays in root domain A, the cpumask_equal() early
exit still skips set_cpus_allowed_dl() on attach, and cpuset_free_dl_bw()
refunds root domain B while the sleeping task's bandwidth remains counted
in root domain A.

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