kernel/sched/ext.c | 7 +++++++ 1 file changed, 7 insertions(+)
__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();
}
/*
---
base-commit: 155a3c003e555a7300d156a5252c004c392ec6b0
change-id: 20250716-scx_warning-5143cf17f806
Best regards,
--
Breno Leitao <leitao@debian.org>On Wed, 16 Jul 2025 05:46:15 -0700
Breno Leitao <leitao@debian.org> 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);
<blink>
If an rq lock is expected to be held, there had better be no preemption
enabled. How is this OK?
-- Steve
> + /*
> + * __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();
> }
>
> /*
>
> ---
> base-commit: 155a3c003e555a7300d156a5252c004c392ec6b0
> change-id: 20250716-scx_warning-5143cf17f806
>
> Best regards,
> --
> Breno Leitao <leitao@debian.org>
On Wed, Jul 16, 2025 at 08:54:47AM -0400, Steven Rostedt wrote:
> On Wed, 16 Jul 2025 05:46:15 -0700
> Breno Leitao <leitao@debian.org> 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);
>
> <blink>
>
> If an rq lock is expected to be held, there had better be no preemption
> enabled. How is this OK?
The rq=NULL case; but from the usage I've seen that also happens with
rq lock held.
Specifically I think the check ought to be:
if (rq)
lockdep_assert_rq_held(rq)
else
lockdep_assert_rq_held(__this_cpu_read(locked_rq));
On Wed, Jul 16, 2025 at 03:06:52PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 16, 2025 at 08:54:47AM -0400, Steven Rostedt wrote:
> > On Wed, 16 Jul 2025 05:46:15 -0700
> > Breno Leitao <leitao@debian.org> 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);
> >
> > <blink>
> >
> > If an rq lock is expected to be held, there had better be no preemption
> > enabled. How is this OK?
>
> The rq=NULL case; but from the usage I've seen that also happens with
> rq lock held.
>
> Specifically I think the check ought to be:
>
> if (rq)
> lockdep_assert_rq_held(rq)
> else
> lockdep_assert_rq_held(__this_cpu_read(locked_rq));
Hm... but if the same CPU invokes two "unlocked" callbacks in a row,
locked_rq would be NULL during the second call and we would check rq_held
against NULL.
-Andrea
On Wed, Jul 16, 2025 at 03:20:33PM +0200, Andrea Righi wrote:
> On Wed, Jul 16, 2025 at 03:06:52PM +0200, Peter Zijlstra wrote:
> > On Wed, Jul 16, 2025 at 08:54:47AM -0400, Steven Rostedt wrote:
> > > On Wed, 16 Jul 2025 05:46:15 -0700
> > > Breno Leitao <leitao@debian.org> 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);
> > >
> > > <blink>
> > >
> > > If an rq lock is expected to be held, there had better be no preemption
> > > enabled. How is this OK?
> >
> > The rq=NULL case; but from the usage I've seen that also happens with
> > rq lock held.
> >
> > Specifically I think the check ought to be:
> >
> > if (rq)
> > lockdep_assert_rq_held(rq)
> > else
> > lockdep_assert_rq_held(__this_cpu_read(locked_rq));
>
> Hm... but if the same CPU invokes two "unlocked" callbacks in a row,
> locked_rq would be NULL during the second call and we would check rq_held
> against NULL.
Current usage in SCX_CALL_OP*() seems to not generate this pattern. It
is always rq,NULL in order.
Ooh, there are a few SCX_CALL_OP*() instances where rq:=NULL, which
messes this up.
Changing them to:
if (rq)
update_locked_rq(rq);
...
if (rq)
update_locked_rq(NULL);
might help.
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.
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
On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > 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. Right; but doing superfluous NULL stores seems pointless. So better to avoid the store entirely, rather than making it more expensive and no less pointless, right?
On Wed, Jul 16, 2025 at 03:36:31PM +0200, Peter Zijlstra wrote:
> On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote:
>
> > 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.
>
> Right; but doing superfluous NULL stores seems pointless. So better to
> avoid the store entirely, rather than making it more expensive and no
> less pointless, right?
Right, we can definitely avoid rewriting NULL.
The following should do the trick.
Breno, can you give it a try?
Thanks,
-Andrea
kernel/sched/ext.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c
index e231450768897..c76d6c9e497b4 100644
--- a/kernel/sched/ext.c
+++ b/kernel/sched/ext.c
@@ -1290,12 +1290,15 @@ static inline void update_locked_rq(struct rq *rq)
*/
if (rq)
lockdep_assert_rq_held(rq);
+
+ WARN_ON_ONCE(preemptible());
__this_cpu_write(scx_locked_rq_state, rq);
}
#define SCX_CALL_OP(sch, mask, op, rq, args...) \
do { \
- update_locked_rq(rq); \
+ if (rq) \
+ update_locked_rq(rq); \
if (mask) { \
scx_kf_allow(mask); \
(sch)->ops.op(args); \
@@ -1303,14 +1306,16 @@ do { \
} else { \
(sch)->ops.op(args); \
} \
- update_locked_rq(NULL); \
+ if (rq) \
+ update_locked_rq(NULL); \
} while (0)
#define SCX_CALL_OP_RET(sch, mask, op, rq, args...) \
({ \
__typeof__((sch)->ops.op(args)) __ret; \
\
- update_locked_rq(rq); \
+ if (rq) \
+ update_locked_rq(rq); \
if (mask) { \
scx_kf_allow(mask); \
__ret = (sch)->ops.op(args); \
@@ -1318,7 +1323,8 @@ do { \
} else { \
__ret = (sch)->ops.op(args); \
} \
- update_locked_rq(NULL); \
+ if (rq) \
+ update_locked_rq(NULL); \
__ret; \
})
On Wed, Jul 16, 2025 at 04:26:23PM +0200, Andrea Righi wrote: > On Wed, Jul 16, 2025 at 03:36:31PM +0200, Peter Zijlstra wrote: > > On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > > > > > 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. > > > > Right; but doing superfluous NULL stores seems pointless. So better to > > avoid the store entirely, rather than making it more expensive and no > > less pointless, right? > > Right, we can definitely avoid rewriting NULL. > The following should do the trick. > > Breno, can you give it a try? Sure thing. I've tested it and I don't see the warning on my side. Would you like to me post the patch, probably removing the WARN_ONCE() as raised by peterz? Thanks --breno
On Wed, Jul 16, 2025 at 09:08:36AM -0700, Breno Leitao wrote: > On Wed, Jul 16, 2025 at 04:26:23PM +0200, Andrea Righi wrote: > > On Wed, Jul 16, 2025 at 03:36:31PM +0200, Peter Zijlstra wrote: > > > On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > > > > > > > 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. > > > > > > Right; but doing superfluous NULL stores seems pointless. So better to > > > avoid the store entirely, rather than making it more expensive and no > > > less pointless, right? > > > > Right, we can definitely avoid rewriting NULL. > > The following should do the trick. > > > > Breno, can you give it a try? > > Sure thing. I've tested it and I don't see the warning on my side. > > Would you like to me post the patch, probably removing the WARN_ONCE() > as raised by peterz? Sure, go ahead. Yes, let's remove the WARN_ON_ONCE(). And thanks Peter for all the suggestions! -Andrea
On Wed, Jul 16, 2025 at 04:26:23PM +0200, Andrea Righi wrote: > On Wed, Jul 16, 2025 at 03:36:31PM +0200, Peter Zijlstra wrote: > > On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > > > > > 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. > > > > Right; but doing superfluous NULL stores seems pointless. So better to > > avoid the store entirely, rather than making it more expensive and no > > less pointless, right? > > Right, we can definitely avoid rewriting NULL. > The following should do the trick. > > Breno, can you give it a try? > > Thanks, > -Andrea > > kernel/sched/ext.c | 14 ++++++++++---- > 1 file changed, 10 insertions(+), 4 deletions(-) > > diff --git a/kernel/sched/ext.c b/kernel/sched/ext.c > index e231450768897..c76d6c9e497b4 100644 > --- a/kernel/sched/ext.c > +++ b/kernel/sched/ext.c > @@ -1290,12 +1290,15 @@ static inline void update_locked_rq(struct rq *rq) > */ > if (rq) > lockdep_assert_rq_held(rq); > + > + WARN_ON_ONCE(preemptible()); > __this_cpu_write(scx_locked_rq_state, rq); > } __this_cpu_*() implies that check, which is how we got here ;-)
Hello Peter, On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > On Wed, Jul 16, 2025 at 02:51:28PM +0200, Peter Zijlstra wrote: > > Also, I don't seem to have this scx_layered This is a rust scheduler that could be found at https://github.com/sched-ext/scx/tree/main/scheds/rust/scx_layered Thanks for the review, --breno
On Wed, Jul 16, 2025 at 06:20:02AM -0700, Breno Leitao wrote: > Hello Peter, > > On Wed, Jul 16, 2025 at 03:15:12PM +0200, Andrea Righi wrote: > > On Wed, Jul 16, 2025 at 02:51:28PM +0200, Peter Zijlstra wrote: > > > Also, I don't seem to have this scx_layered > > This is a rust scheduler that could be found at > https://github.com/sched-ext/scx/tree/main/scheds/rust/scx_layered Well, that just means the Changelog is entirely useless. Please always refer to in-tree code when describing a problem.
© 2016 - 2025 Red Hat, Inc.