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

On Thu, Jul 09, 2026 at 05:33:48AM +0000, [email protected] wrote:
> 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.

Both false positives. See below.

> --
> 
> --- 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.

This was triggered because the current sched_ext/for-7.3 is missing the
for-7.2-fixes merge.

As mentioned in the patch notes: "Based on sched_ext/for-7.3 (daf8e166ba59d)
with sched_ext/for-7.2-fixes (cfe950d79f524) assumed merged".

> 
> [ ... ]
> 
> > @@ -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?

False positive due to the reason above.

Thanks,
-Andrea

> 
> >  		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