Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption

[Andrea Righi <arighi@nvidia.com>]

On Wed, Jul 16, 2025 at 02:51:28PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 16, 2025 at 05:46:15AM -0700, Breno Leitao wrote:
> > __this_cpu_write() emits a warning if used with preemption enabled.
> > 
> > Function update_locked_rq() might be called with preemption enabled,
> > which causes the following warning:
> > 
> > 	BUG: using __this_cpu_write() in preemptible [00000000] code: scx_layered_6-9/68770
> > 
> > Disable preemption around the __this_cpu_write() call in
> > update_locked_rq() to suppress the warning, without affecting behavior.
> > 
> > If preemption triggers a  jump to another CPU during the callback it's
> > fine, since we would track the rq state on the other CPU with its own
> > local variable.
> > 
> > Suggested-by: Andrea Righi <arighi@nvidia.com>
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > Fixes: 18853ba782bef ("sched_ext: Track currently locked rq")
> > Acked-by: Andrea Righi <arighi@nvidia.com>
> > ---
> >  kernel/sched/ext.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
> > index b498d867ba210..24fcbd7331f73 100644
> > --- a/kernel/sched/ext.c
> > +++ b/kernel/sched/ext.c
> > @@ -1258,7 +1258,14 @@ static inline void update_locked_rq(struct rq *rq)
> >  	 */
> >  	if (rq)
> >  		lockdep_assert_rq_held(rq);
> > +	/*
> > +	 * __this_cpu_write() emits a warning when used with preemption enabled.
> > +	 * While there's no functional issue if the callback runs on another
> > +	 * CPU, we disable preemption here solely to suppress that warning.
> > +	 */
> > +	preempt_disable();
> >  	__this_cpu_write(locked_rq, rq);
> > +	preempt_enable();
> >  }
> 
> This looks dodgy as heck. Why can't we do this update_locked_rq(NULL)
> call while still holding rq? Also, I don't seem to have this scx_layered
> thing.

Giving more context.

The idea is to track the scx callbacks that are invoked with a rq lock held
and, in those cases, store the locked rq. However, some callbacks may also
be invoked from an unlocked context, where no rq is locked and in this case
rq should be NULL.

In the latter case, it's acceptable for preemption to remain enabled, but
we still want to explicitly set locked_rq = NULL. If during the execution
of the callback we jump on another CPU, it'd still be in an unlocked state,
so it's locked_rq is still NULL.

Basically we would need preempt_disable/enable() only when rq == NULL to be
formally correct. And if rq != NULL we should probably trigger a warning,
as initially suggested by Breno.

How about something like this?

static inline void update_locked_rq(struct rq *rq)
{
	if (rq) {
		lockdep_assert_rq_held(rq);
		WARN_ON_ONCE(preemptible());
		__this_cpu_write(locked_rq, rq);
		return;
	}

	/*
	 * Certain callbacks may be invoked from an unlocked context, where
	 * no rq is held. In those cases, we still want to constently set
	 * locked_rq to NULL, disabling preemption.
	 */
	preempt_disable();
	__this_cpu_write(locked_rq, NULL);
	preempt_enable();
}

Thanks,
-Andrea