Re: [PATCH v5 08/29] sched/deadline: Take a deadline boost from the waiter's rt_mutex node

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

Hello,

On 08/11/26 13:53, Mathieu Desnoyers wrote:
> When a SCHED_DEADLINE waiter boosts the owner of an rt_mutex, the owner
> runs on the waiter's live parameters, through dl.pi_se. The waiter can
> leave SCHED_DEADLINE, and have them cleared, before the owner is
> deboosted. When the owner's runtime runs out, replenish_dl_entity()
> loops forever on a dl_runtime of 0. It holds the rq lock, with
> interrupts off. The owner's CPU warns there and locks up.
> 
> rt_mutex_setprio() also reads the waiter's live priority, not the one in
> its rt_mutex node. Commit acd58620e415 ("sched/rtmutex: Refactor
> rt_mutex_setprio()") found this fine: if they differ, a PI chain update
> is imminent.
> 
> But the update can come too late. Leaving SCHED_DEADLINE clears the
> parameters, so that admission control counts the task again when it
> comes back. __sched_setscheduler() walks the chain only after it drops
> the waiter's rq lock. In the change itself, task_non_contending() clears
> the parameters of a waiter woken past its 0-lag time.
> inactive_task_timer() clears them later, when a sleeping waiter reaches
> that time.
> 
> O is a SCHED_OTHER task that holds the rt_mutex L. W is a SCHED_DEADLINE
> task. T is any other task:
> 
>   CPU 0 (O)                         CPU 1 (W)                     CPU 2 (T)
> 
>   locks L
>                                     blocks on L
>                                       rt_mutex_setprio(O, W)
>                                         O->dl.pi_se = &W->dl
>                                     sleeps
>                                       task_non_contending()
>                                         arms W's inactive timer
                                               ^^^
We know that PI is broken with DEADLINE and this adds to the count of
broken points. :)

>                                                                   sched_setscheduler(W)
>                                                                     W leaves SCHED_DEADLINE
>                                                                     task_rq_unlock(W)
>                                     <W's inactive timer>
>                                     inactive_task_timer()
>                                       __dl_clear_params()
>                                         W->dl.dl_runtime = 0
>   <tick>
>   update_curr_dl_se()
>     replenish_dl_entity()
>       runtime += W->dl.dl_runtime
>       loops forever
>                                                                     rt_mutex_adjust_pi(W)
>                                                                       rt_mutex_setprio(O, W)
>                                                                         spins on O's rq lock

The hang of course is real though.

> A copy of the parameters at the boost is not enough. rt_mutex_setprio()
> would read the waiter's priority, then its parameters. A change of the
> waiter can clear them in between. A stress test switches the waiter in
> and out of SCHED_DEADLINE. With such a copy, the owner's CPU locked up
> within a second. With this patch, the test runs for 600 seconds.
> 
> The rt_mutex node records the waiter's priority and deadline under the
> waiter's pi_lock, which every change of its parameters holds. Fix this
> by recording the reservation there too. A deadline boost then takes the
> priority, the deadline and a copy of the reservation from the node of
> the top pi waiter. rt_effective_prio() now reads that node too. It used
> p->pi_top_task, which try_to_take_rt_mutex() does not update. The chain
> walk and rt_mutex_adjust_pi() also compare the reservation, so that a
> change of it alone reaches the owner.
> 
> This needs "locking/rtmutex: Clone the sort keys in
> rt_mutex_enqueue_pi()". struct rt_mutex_waiter grows from 112 to 160
> bytes on 64-bit. With PREEMPT_RT, every sleeping lock waiter has one on
> its stack.
> 
> A 300ms debug delay before rt_mutex_adjust_pi() locks up v7.4-rc1 in
> 8-CPU VMs, but not a kernel with this patch. The locking API, ww_mutex,
> locktorture and futex tests still pass with and without PREEMPT_RT.
> 
> Found by the Sashiko AI review of v3. Before v4, a second model found
> the window of a copy, and the stale node of a waiter that becomes the
> top waiter.
> 
> Fixes: a5e7be3b28a2 ("sched/deadline: Clear dl_entity params when setscheduling to different class")
> Cc: stable@vger.kernel.org
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
> ---

I am not completely sure that we want the additinal machinery below
though, if we can find a simpler stop-gap solution until proxy execution
makes things right.

I was thinking 2 alternative potential bandaids:

 1. Don't clear the parameters of a blocked waiter. Every live donor is,
 by construction, a task blocked on an rt_mutex. Guard the
 __dl_clear_params() calls in task_non_contending() and
 inactive_task_timer() with p->pi_blocked_on (both sites hold pi_lock,
 which is also what pi_blocked_on is written under). The bandwidth
 release still happens at the 0-lag time; only the parameter wipe waits
 until the task is no longer blocked. 
 
 2. Even simpler, considering that the above is of course still far from
 ideal: replenish_dl_entity() already WARNs on pi_of(dl_se)->dl_runtime
 <= 0, and then hangs. Make the condition bail instead. The deboost is
 already in flight: the sched_setscheduler() that clears the donor's
 parameters ends with rt_mutex_adjust_pi(), which walks the chain and
 deboosts the owner through rt_mutex_setprio() and needs the owner's rq
 lock to do so, which is exactly what the infinite loop holds. Bailing
 releases it; the owner then sits throttled until the walk lands
 (dl_task_timer() is a no-op for boosted entities) and moves it back to
 fair. The stall is bounded by the walk's latency. 

Don't get me wrong, the proposed fix looks OK to me, but it also looks
more complex to backport to stable, it will be anyway ripped apart by
proxy exec and doesn't unfortunately make DEADLINE PI less broken (yes
of course the system won't hang, which is very desirable :).

Thoughts?

Thanks,
Juri