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
