Re: [PATCH sched_ext/for-7.2-fixes] sched_ext: Preserve rq tracking across local DSQ dispatch

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Medium] Unhandled lockdep assertion failure in scx_dsq_move() due to stale scx_locked_rq_state tracking when moving tasks across remote rqs.
--

--- Patch [1]: [PATCH sched_ext/for-7.2-fixes] sched_ext: Preserve rq tracking across local DSQ dispatch ---
commit bf8a7ba58cd085582b43e1290a1b826afac5f411
Author: Andrea Righi <[email protected]>

sched_ext: Preserve rq tracking across local DSQ dispatch
    
This commit fixes a lockdep assertion in dispatch_to_local_dsq() that
occurs when nested operations restore a recorded rq that is no longer
locked. It clears the tracked rq before switching locks and restores
it only after reacquiring the original rq.

> diff --git a/kernel/sched/ext/ext.c b/kernel/sched/ext/ext.c
> index 691d53fe0f648..aa79dedba8aeb 100644
> --- a/kernel/sched/ext/ext.c
> +++ b/kernel/sched/ext/ext.c

[ ... ]

> @@ -2588,6 +2589,16 @@ static void dispatch_to_local_dsq(struct scx_sched *sch, struct rq *rq,
>  		return;
>  	}
>  
> +	/*
> +	 * This may be called from an SCX op with @rq recorded as the currently
> +	 * locked rq. Clear the tracking while switching rq locks so nested
> +	 * callbacks don't restore an rq which isn't locked anymore.
> +	 */
> +	if (tracked_rq) {
> +		WARN_ON_ONCE(tracked_rq != rq);
> +		update_locked_rq(NULL);
> +	}
> +
>  	/*
>  	 * @p is on a possibly remote @src_rq which we need to lock to move the

[Severity: Medium]
This isn't a bug introduced by this patch, but does this same lockdep
assertion failure happen in scx_dsq_move()?

Looking at scx_dsq_move() in kernel/sched/ext/ext.c, when moving a task
from a remote rq with in_balance set to true, it drops the this_rq lock:

        if (in_balance) {
                if (this_rq != src_rq) {
                        raw_spin_rq_unlock(this_rq);
                        raw_spin_rq_lock(src_rq);
                }
        } else {

Unlike the fix in dispatch_to_local_dsq() provided here, scx_dsq_move()
does not clear the tracked scx_locked_rq_state before dropping the lock.

If move_task_between_dsqs() then triggers nested callbacks like
SCX_CALL_OP_TASK(sch, dequeue, ...), won't it save the stale this_rq state,
and then trigger the same lockdep assertion in update_locked_rq() when
it tries to restore it?

> @@ -2640,6 +2651,8 @@ static void dispatch_to_local_dsq(struct scx_sched *sch, struct rq *rq,
>  		raw_spin_rq_unlock(locked_rq);
>  		raw_spin_rq_lock(rq);
>  	}
> +	if (tracked_rq)
> +		update_locked_rq(tracked_rq);
>  }
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1