Re: [PATCH v33 7/7] sched/proxy: Add deactivated (sleeping) owner handling to find_proxy_task()

[K Prateek Nayak <kprateek.nayak@amd.com>]

Hello Juri,

On 10/27/2026 9:52 PM, Juri Lelli wrote:
> Hello!
> 
> On 26/10/26 09:56, K Prateek Nayak wrote:
>> On 10/24/2026 2:37 PM, Andrea Righi wrote:
>>>> +static void do_activate_blocked_waiter(struct rq *target_rq, struct task_struct *p, int en_flags)
>>>> +{
> 
> ...
> 
>>>> +		if (task_cpu(p) != target_cpu) {
>>>> +			en_flags |= ENQUEUE_MIGRATED;
>>>> +			psi_ttwu_dequeue(p);
>>>> +			proxy_set_task_cpu(p, target_cpu);
>>>> +		}
>>>
>>> Can this break SCHED_DEADLINE bw accounting?
>>>
>>> p is sleeping here (not TASK_WAKING, not TASK_ON_RQ_MIGRATING), so
>>> __set_task_cpu() skips migrate_task_rq_dl(): the old rq never does
>>> sub_rq_bw(), while ENQUEUE_MIGRATED makes the new rq do add_rq_bw(), so
>>> this_bw is leaked on the old rq. And if p is still dl_non_contending,
>>> task_contending() on the new rq just cancels the inactive timer, leaving
>>> running_bw on the old rq forever and making it underflow on the new rq
>>> the next time p blocks.
>>>
>>> set_task_cpu() doesn't help, because migrate_task_rq_dl() bails out
>>> unless TASK_WAKING, so I'm not sure how to fix this case...
>>
>> I wonder why "p->dl.dl_non_contending" is not sufficient to know if
>> there exists residual bandwidth on prev CPU. Seems to have been like
>> that since commit 209a0cbda7a0 ("sched/deadline: Improve the tracking
>> of active utilization").
>>
>> Only time task is non-contending is if it goes away with DEQUEUE_SLEEP
>> or switched_from_dl() with dl_runtime.
>>
>> The latter doesn't need migrating the timer as the timer will either
>> be cancelled if the task switches back to deadline, or will expire
>> where it was queued - set_task_cpu() has no effect in this case.
>>
>> Only other case that marks "dl_non_contending" is sleep + wakeup and
>> for that "p->dl.non_contending" should be sufficient. I don't think
>> we ever do a set_task_cpu() for a blocked task other than wakeup do
>> we?
>>
>> Juri, Peter, are we missing something subtle here?
> 
> I think Andrea's concern is valid, but I'm not sure I follow what you
> propose Prateek (apologies!) or are you saying you don't see an issue
> here?

Sorry for the confusion! I do see the problem and my question was why
can't the bailout in migrate_task_rq_dl() be something like:

  (Lightly tested)

diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
index c0ebdcde5fe5..6a081c2b7191 100644
--- a/kernel/sched/deadline.c
+++ b/kernel/sched/deadline.c
@@ -2654,19 +2654,23 @@ select_task_rq_dl(struct task_struct *p, int cpu, int flags)
 
 static void migrate_task_rq_dl(struct task_struct *p, int new_cpu __maybe_unused)
 {
-	struct rq_flags rf;
 	struct rq *rq;
 
-	if (READ_ONCE(p->__state) != TASK_WAKING)
+	/* Inactive timer is not armed. Nothing to migrate! */
+	if (task_on_rq_migrating(p) || !p->dl.dl_non_contending)
 		return;
 
-	rq = task_rq(p);
+	/* Task must be fully blocked if we reach here. */
+	WARN_ON_ONCE(!p->is_blocked);
+
 	/*
-	 * Since p->state == TASK_WAKING, set_task_cpu() has been called
-	 * from try_to_wake_up(). Hence, p->pi_lock is locked, but
-	 * rq->lock is not... So, lock it
+	 * set_task_cpu() has been called from try_to_wake_up().
+	 * Hence, p->pi_lock is locked, but rq->lock is not...
+	 * So, lock it
 	 */
-	rq_lock(rq, &rf);
+	rq = task_rq(p);
+	guard(rq_lock)(rq);
+
 	if (p->dl.dl_non_contending) {
 		update_rq_clock(rq);
 		sub_running_bw(&p->dl, &rq->dl);
@@ -2681,7 +2685,6 @@ static void migrate_task_rq_dl(struct task_struct *p, int new_cpu __maybe_unused
 		cancel_inactive_timer(&p->dl);
 	}
 	sub_rq_bw(&p->dl, &rq->dl);
-	rq_unlock(rq, &rf);
 }
 
 static void check_preempt_equal_dl(struct rq *rq, struct task_struct *p)
---

Isn't task_on_rq_migrating() || !p->dl.dl_non_contending sufficient
bail out checks given the inactive timer is only armed for fully
blocked (DEQUEUE_SLEEP) tasks?

deactivate_task() + activate_task() will not leave an inactive timer
so task_on_rq_migrating() should be enough to capture that and if we
are doing set_task_rq() on a blocked task, it must be during wakeup.

> 
> If the issue is confirmed, I wonder if relaxing the first check in
> migrate_task_rq_dl() to something like
> 
>     if (READ_ONCE(p->__state) != TASK_WAKING && !p->is_blocked)
>         return;

So, the problem is we we are doing an block_task() + activate_task()
for proxy donors and they do not see TASK_WAKING state - the task is
fully blocked (say TASK_UNINTERRUPTIBLE) for this whole duration.

block_task() can arm and inactive timer that needs cancelling before
the activate on a different rq but since we don't do TASK_WAKING,
migrate_task_rq_dl() will bail early and the timer is left on the
old CPU.
 
> might fix things, but then it's ugly to call it from do_activate_
> blocked_waiter(). We could teach set_task_cpu() about blocked waiters
> (otherwise it warns) and then call it instead of __set_task_cpu(), but
> I'm not sure it's any prettier.

If something like the suggested diff is alright, then proxy can
simply use set_task_cpu() (followed by correcting p->wake_cpu) and
everything should be alright.

-- 
Thanks and Regards,
Prateek