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

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

On 28/10/26 09:06, K Prateek Nayak wrote:
> 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;

But a task can be blocked/sleeping and non_contending if
task_non_contending() was called after 0-lag; in that case running_bw
was subtracted already (see zerolag_time < 0 -> dl_task case) but ..

> -	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);

.. we still need to sub_rq_bw for a migrated task, and I believe what
you are proposing would skip that?