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

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

On 13/03/25 19:38, Harshit Agarwal wrote:
> 
> 
> >>> 
> >>> 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"?
> 
> By offline migration I meant dl_task_offline_migration path which
> calls find_lock_later_rq. So unless we think the same race that this
> fix is trying to address for push_dl_task can happen for
> dl_task_offline_migration, there is one less reason to encapsulate
> this in find_lock_later_rq.
> 
> > 
> > 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?
> > 
> 
> Yes I agree that encapsulation in find_lock_later_rq will be ideal
> but by keeping it limited to push_dl_task I wanted to keep the change
> more targeted to avoid any possible side effect on
> dl_task_offline_migration call path.
> 
> Let’s say if we go ahead with making the change in find_lock_later_rq
> itself then we will have to fallback to current checks for throttled case
> and for remaining we will use the task != pick_next_pushable_dl_task(rq)
> check. Below is the diff of how it will be:
> 
>                 /* Retry if something changed. */
>                 if (double_lock_balance(rq, later_rq)) {
> -                       if (unlikely(task_rq(task) != rq ||
> +                       if (unlikely(is_migration_disabled(task) ||
>                                      !cpumask_test_cpu(later_rq->cpu, &task->cpus_mask) ||
> -                                    task_on_cpu(rq, task) ||
> -                                    !dl_task(task) ||
> -                                    is_migration_disabled(task) ||
> -                                    !task_on_rq_queued(task))) {
> +                                    (task->dl.dl_throttled &&
> +                                      (task_rq(task) != rq ||
> +                                       task_on_cpu(rq, task) ||
> +                                       !dl_task(task)
> +                                       !task_on_rq_queued(task))) ||
> +                                    (!task->dl.dl_throttled &&
> +                                      task != pick_next_pushable_dl_task(rq)))) {
>                                 double_unlock_balance(rq, later_rq);
>                                 later_rq = NULL;
>                                 break;
>  
> Let me know your thoughts and I can send v2 patch accordingly.

So, it looks definitely more complicated (and fragile?), but I think I
still like it better. Maybe you could add a comment in the code
documenting the two different paths and the associated checks, so that we
don't forget. :)

What do others think?

Thanks!
Juri