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