Re: [PATCH sched_ext/for-7.3] sched_ext: Keep rq tracking accurate in the consume path

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The `scx_locked_rq_state` tracking will be left permanently stale on the successful task migration path, directly contradicting the commit message's claim.
- [Low] The patch introduces calls to an undefined function or macro `switch_rq_lock()`, causing a compilation error.
--

--- Patch [1]: [PATCH sched_ext/for-7.3] sched_ext: Keep rq tracking accurate in the consume path ---
commit 0f65257c6efec221d0687e563233c82e3c63e9c8
Author: Andrea Righi <[email protected]>

sched_ext: Keep rq tracking accurate in the consume path

This commit attempts to fix a stale BPF runqueue lock tracking issue in
consume_remote_task() by introducing a new switch_rq_lock() helper. It
aims to ensure that scx_locked_rq() correctly follows the lock transitions
between this_rq and src_rq during remote task consumption.

Link: https://lore.kernel.org/all/[email protected]

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

[ ... ]

> @@ -2386,20 +2387,22 @@ static bool task_can_run_on_remote_rq(struct scx_sched *sch,
>   * On return, @dsq is unlocked and @src_rq is locked. Returns %true if @p is
>   * still valid. %false if lost to dequeue.
>   */
> -static bool unlink_dsq_and_lock_src_rq(struct task_struct *p,
> -				       struct scx_dispatch_q *dsq,
> -				       struct rq *src_rq)
> +static bool unlink_dsq_and_switch_rq_lock(struct task_struct *p,
> +					  struct scx_dispatch_q *dsq,
> +					  struct rq *locked_rq,
> +					  struct rq *src_rq)
>  {
>  	s32 cpu = raw_smp_processor_id();
>  
>  	lockdep_assert_held(&dsq->lock);
> +	lockdep_assert_rq_held(locked_rq);
>  
>  	WARN_ON_ONCE(p->scx.holding_cpu >= 0);
>  	task_unlink_from_dsq(p, dsq);
>  	p->scx.holding_cpu = cpu;
>  
>  	raw_spin_unlock(&dsq->lock);
> -	raw_spin_rq_lock(src_rq);
> +	switch_rq_lock(locked_rq, src_rq);

[Severity: Low]
Are we missing the definition of switch_rq_lock() in this patch?

Compiling this throws an implicit declaration error since switch_rq_lock()
doesn't seem to exist.

[ ... ]

> @@ -2410,14 +2413,11 @@ static bool consume_remote_task(struct rq *this_rq,
>  				struct task_struct *p, u64 enq_flags,
>  				struct scx_dispatch_q *dsq, struct rq *src_rq)
>  {
> -	raw_spin_rq_unlock(this_rq);
> -
> -	if (unlink_dsq_and_lock_src_rq(p, dsq, src_rq)) {
> +	if (unlink_dsq_and_switch_rq_lock(p, dsq, this_rq, src_rq)) {
>  		move_remote_task_to_local_dsq(p, enq_flags, src_rq, this_rq);

[Severity: High]
Does this leave the lock tracking state permanently stale on the success path?

In move_remote_task_to_local_dsq(), the physical lock is swapped from src_rq
to dst_rq (which is this_rq) using raw spinlocks:

kernel/sched/ext/ext.c:move_remote_task_to_local_dsq()
    ...
    raw_spin_rq_unlock(src_rq);
    raw_spin_rq_lock(dst_rq);
    ...

If unlink_dsq_and_switch_rq_lock() updates the tracking state to src_rq,
but move_remote_task_to_local_dsq() doesn't update it back to this_rq, won't
scx_locked_rq() falsely claim src_rq is still held when returning to the BPF
program?

>  		return true;
>  	} else {
> -		raw_spin_rq_unlock(src_rq);
> -		raw_spin_rq_lock(this_rq);
> +		switch_rq_lock(src_rq, this_rq);
>  		return false;
>  	}
>  }

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