Re: [PATCH v5 06/29] sched: Take cpuset_mutex for a deadline policy change only with pi

[Juri Lelli <juri.lelli@redhat.com>]

Hello,

On 08/11/26 13:53, Mathieu Desnoyers wrote:
> __sched_setscheduler() takes cpuset_mutex for a change that involves
> SCHED_DEADLINE. On success, it releases it only when pi is set.
> normalize_rt_tasks(), for sysrq-n, is the one caller with pi unset. It
> runs under read_lock(&tasklist_lock). So sysrq-n takes the mutex in
> atomic context for a deadline task, and never releases it. Every later
> change that involves SCHED_DEADLINE then blocks forever.
> 
> Commit 710da3c8ea7d ("sched/core: Prevent race condition between cpuset
> and __sched_setscheduler()") took the cpuset lock only when pi is set.
> normalize_rt_tasks() cannot take it under tasklist_lock. Its changelog
> accepts this: after sysrq-n, the deadline guarantees are gone anyway.
> Commit 111cd11bbc54 ("sched/cpuset: Bring back cpuset_mutex") then took
> the mutex only for deadline tasks. It replaced the pi test by a policy
> test, and left the release under pi.
> 
> Fix this by taking the mutex for a deadline change only when pi is
> set.
> 
> The policy test reads p->policy before the runqueue lock. It is checked
> again under the lock only when the caller gave no policy. Otherwise,
> the task can enter SCHED_DEADLINE in between. The call then takes it
> out of the class without the mutex. A rebuild of the root domain
> accounting could run at the same time. So check again under the
> runqueue lock, and retry with the mutex.
> 
> sysrq-n on v7.4-rc1, with a deadline task and lockdep on:
> 
>   sysrq: Nice All RT Tasks
>   BUG: sleeping function called from invalid context at kernel/locking/mutex.c:623
>   in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 4356, name: deadline_switch
>   locks held by deadline_switch/4356: 4, last CPU#3:
>    ...
>    #3: ffffffff83006098 (tasklist_lock){.+.+}-{3:3}, at: normalize_rt_tasks+0x34/0x150
> 
> A sched_setattr(SCHED_DEADLINE) after it never returned, in 2 VMs of 2.
> The sysrq_normalize case of deadline_switch_test, added later in this
> series, tests this with DEADLINE_TEST_SYSRQ=1. It passes with this
> series.
> 
> Fixes: 111cd11bbc54 ("sched/cpuset: Bring back cpuset_mutex")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---

The change in itself makes sense to me.

Reviewed-by: Juri Lelli <juri.lelli@redhat.com>

I only have cosmetic/minor comments below.

>  kernel/sched/syscalls.c | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/kernel/sched/syscalls.c b/kernel/sched/syscalls.c
> index 56ce2352e89e..0bc778c12af4 100644
> --- a/kernel/sched/syscalls.c
> +++ b/kernel/sched/syscalls.c
> @@ -561,9 +561,11 @@ int __sched_setscheduler(struct task_struct *p,
>  
>  	/*
>  	 * SCHED_DEADLINE bandwidth accounting relies on stable cpusets
> -	 * information.
> +	 * information. Do not take cpuset_mutex when pi is unset: the
> +	 * caller is normalize_rt_tasks(), which holds tasklist_lock and
> +	 * cannot sleep.
>  	 */
> -	if (dl_policy(policy) || dl_policy(p->policy)) {
> +	if (pi && (dl_policy(policy) || dl_policy(p->policy))) {
>  		cpuset_locked = true;
>  		cpuset_lock();
>  	}
> @@ -654,6 +656,15 @@ int __sched_setscheduler(struct task_struct *p,
>  		goto recheck;
>  	}
>  
> +	/*
> +	 * The task entered SCHED_DEADLINE after the unlocked read of its
> +	 * policy above, so cpuset_mutex was not taken. Retry with it.
> +	 */
> +	if (pi && !cpuset_locked && task_has_dl_policy(p)) {
> +		task_rq_unlock(rq, p, &rf);
> +		goto recheck;
> +	}
> +

For a cursory read this check and the one above look similar. Can we
maybe have more descriptive comments for them, e.g., for the first

 /*
  * The caller gave no policy (SETPARAM_POLICY): the request was
  * derived from an unlocked read of p->policy above. If the task
  * changed policy since, re-derive the request from scratch, and
  * release cpuset_mutex: the re-derived request re-decides whether
  * it is needed.
  */

and, for the second, that this patch adds

 /*
  * The caller gave an explicit policy, so the request needs no
  * re-derivation (oldpolicy is -1 above). But the decision to
  * take cpuset_mutex used the same unlocked read: the task
  * entered SCHED_DEADLINE since, and the change below would take
  * it out of the class. Retry to take the mutex.
  */

Also, maybe add an unlikely() to the second check as well?

Thanks,
Juri