Re: [PATCH] sched/deadline: Fix race in push_dl_task

[Juri Lelli]

Hi,

On 12/03/25 18:46, Harshit Agarwal wrote:
> Thanks Juri, for taking a look.

Of course! Thanks to you for working on this.

> > On Mar 12, 2025, at 2:42 AM, Juri Lelli <juri.lelli@xxxxxxxxxx> wrote:
> > 
> > Hi Harshit,
> > 
> > Thanks for this!
> > 
> > I don't think we want this kind of URLs in the changelog, as URL might
> > disappear while the history remains (at least usually a little longer
> > :). Maybe you could add a very condensed version of the description of
> > the problem you have on the other fix?
> 
> Sorry about this and thanks for pointing it out. I will fix it in the
> next version of the patch.

No worries and thanks.

> >> In this fix we bail out or retry in the push_dl_task, if the task is no
> >> longer at the head of pushable tasks list because this list changed
> >> while trying to lock the runqueue of the other CPU.
> >> 
> >> Signed-off-by: Harshit Agarwal <harshit@xxxxxxxxxxx>
> >> Cc: stable@xxxxxxxxxxxxxxx
> >> ---
> >> kernel/sched/deadline.c | 25 +++++++++++++++++++++----
> >> 1 file changed, 21 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> >> index 38e4537790af..c5048969c640 100644
> >> --- a/kernel/sched/deadline.c
> >> +++ b/kernel/sched/deadline.c
> >> @@ -2704,6 +2704,7 @@ static int push_dl_task(struct rq *rq)
> >> {
> >> struct task_struct *next_task;
> >> struct rq *later_rq;
> >> + struct task_struct *task;
> >> int ret = 0;
> >> 
> >> next_task = pick_next_pushable_dl_task(rq);
> >> @@ -2734,15 +2735,30 @@ static int push_dl_task(struct rq *rq)
> >> 
> >> /* Will lock the rq it'll find */
> >> later_rq = find_lock_later_rq(next_task, rq);
> >> - if (!later_rq) {
> >> - struct task_struct *task;
> >> + task = pick_next_pushable_dl_task(rq);
> >> + if (later_rq && (!task || task != next_task)) {
> >> + /*
> >> + * We must check all this again, since
> >> + * find_lock_later_rq releases rq->lock and it is
> >> + * then possible that next_task has migrated and
> >> + * is no longer at the head of the pushable list.
> >> + */
> >> + double_unlock_balance(rq, later_rq);
> >> + if (!task) {
> >> + /* No more tasks */
> >> + goto out;
> >> + }
> >> 
> >> + put_task_struct(next_task);
> >> + next_task = task;
> >> + goto retry;
> > 
> > I fear we might hit a pathological condition that can lead us into a
> > never ending (or very long) loop. find_lock_later_rq() tries to find a
> > later_rq for at most DL_MAX_TRIES and it bails out if it can't.
> 
> This pathological case exists today as well and will be there even
> if we move this check inside find_lock_later_rq. This check is just
> broadening the scenarios where we would retry, where we would
> have panicked otherwise (the bug).
> If this check is moved inside find_lock_later_rq then function will
> return null and then the caller here will do the same which is retry
> or bail out if no tasks are available. Specifically, tt will execute
> the if (!later_rq) block here.
> The number of retries will be bound by the number of tasks in 
> the pushable tasks list.
> 
> > 
> > Maybe to discern between find_lock_later_rq() callers we can use
> > dl_throttled flag in dl_se and still implement the fix in find_lock_
> > later_rq()? I.e., fix similar to the rt.c patch in case the task is not
> > throttled (so caller is push_dl_task()) and not rely on pick_next_
> > pushable_dl_task() if the task is throttled.
> > 
> 
> Sure I can do this as well but like I mentioned above I don’t think
> it will be any different than this patch unless we want to
> handle the race for offline migration case or if you prefer
> this in find_lock_later_rq just to keep it more inline with the rt
> patch. I just found the current approach to be less risky :)

What you mean with "handle the race for offline migration case"?

And I am honestly conflicted. I think I like the encapsulation better if
we can find a solution inside find_lock_later_rq(), as it also aligns
better with rt.c, but you fear it's more fragile?

Best,
Juri