* [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption @ 2025-07-16 12:46 Breno Leitao 2025-07-16 12:51 ` Peter Zijlstra 2025-07-16 12:54 ` Steven Rostedt 0 siblings, 2 replies; 14+ messages in thread From: Breno Leitao @ 2025-07-16 12:46 UTC (permalink / raw) To: Tejun Heo, David Vernet, Andrea Righi, Changwoo Min, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider Cc: sched-ext, linux-kernel, kernel-team, jake, Breno Leitao __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> ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 12:46 [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption Breno Leitao @ 2025-07-16 12:51 ` Peter Zijlstra 2025-07-16 13:15 ` Andrea Righi 2025-07-16 12:54 ` Steven Rostedt 1 sibling, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 12:51 UTC (permalink / raw) To: Breno Leitao Cc: Tejun Heo, David Vernet, Andrea Righi, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 12:51 ` Peter Zijlstra @ 2025-07-16 13:15 ` Andrea Righi 2025-07-16 13:20 ` Breno Leitao 2025-07-16 13:36 ` Peter Zijlstra 0 siblings, 2 replies; 14+ messages in thread From: Andrea Righi @ 2025-07-16 13:15 UTC (permalink / raw) To: Peter Zijlstra Cc: Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:15 ` Andrea Righi @ 2025-07-16 13:20 ` Breno Leitao 2025-07-16 13:40 ` Peter Zijlstra 2025-07-16 13:36 ` Peter Zijlstra 1 sibling, 1 reply; 14+ messages in thread From: Breno Leitao @ 2025-07-16 13:20 UTC (permalink / raw) To: Andrea Righi, peterz Cc: Peter Zijlstra, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:20 ` Breno Leitao @ 2025-07-16 13:40 ` Peter Zijlstra 0 siblings, 0 replies; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 13:40 UTC (permalink / raw) To: Breno Leitao Cc: Andrea Righi, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:15 ` Andrea Righi 2025-07-16 13:20 ` Breno Leitao @ 2025-07-16 13:36 ` Peter Zijlstra 2025-07-16 14:26 ` Andrea Righi 1 sibling, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 13:36 UTC (permalink / raw) To: Andrea Righi Cc: Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:36 ` Peter Zijlstra @ 2025-07-16 14:26 ` Andrea Righi 2025-07-16 15:49 ` Peter Zijlstra 2025-07-16 16:08 ` Breno Leitao 0 siblings, 2 replies; 14+ messages in thread From: Andrea Righi @ 2025-07-16 14:26 UTC (permalink / raw) To: Peter Zijlstra Cc: Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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; \ }) ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 14:26 ` Andrea Righi @ 2025-07-16 15:49 ` Peter Zijlstra 2025-07-16 16:08 ` Breno Leitao 1 sibling, 0 replies; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 15:49 UTC (permalink / raw) To: Andrea Righi Cc: Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ;-) ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 14:26 ` Andrea Righi 2025-07-16 15:49 ` Peter Zijlstra @ 2025-07-16 16:08 ` Breno Leitao 2025-07-16 16:13 ` Andrea Righi 1 sibling, 1 reply; 14+ messages in thread From: Breno Leitao @ 2025-07-16 16:08 UTC (permalink / raw) To: Andrea Righi Cc: Peter Zijlstra, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 16:08 ` Breno Leitao @ 2025-07-16 16:13 ` Andrea Righi 0 siblings, 0 replies; 14+ messages in thread From: Andrea Righi @ 2025-07-16 16:13 UTC (permalink / raw) To: Breno Leitao Cc: Peter Zijlstra, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 12:46 [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption Breno Leitao 2025-07-16 12:51 ` Peter Zijlstra @ 2025-07-16 12:54 ` Steven Rostedt 2025-07-16 13:06 ` Peter Zijlstra 1 sibling, 1 reply; 14+ messages in thread From: Steven Rostedt @ 2025-07-16 12:54 UTC (permalink / raw) To: Breno Leitao Cc: Tejun Heo, David Vernet, Andrea Righi, Changwoo Min, Ingo Molnar, Peter Zijlstra, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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> ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 12:54 ` Steven Rostedt @ 2025-07-16 13:06 ` Peter Zijlstra 2025-07-16 13:20 ` Andrea Righi 0 siblings, 1 reply; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 13:06 UTC (permalink / raw) To: Steven Rostedt Cc: Breno Leitao, Tejun Heo, David Vernet, Andrea Righi, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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)); ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:06 ` Peter Zijlstra @ 2025-07-16 13:20 ` Andrea Righi 2025-07-16 13:33 ` Peter Zijlstra 0 siblings, 1 reply; 14+ messages in thread From: Andrea Righi @ 2025-07-16 13:20 UTC (permalink / raw) To: Peter Zijlstra Cc: Steven Rostedt, Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption 2025-07-16 13:20 ` Andrea Righi @ 2025-07-16 13:33 ` Peter Zijlstra 0 siblings, 0 replies; 14+ messages in thread From: Peter Zijlstra @ 2025-07-16 13:33 UTC (permalink / raw) To: Andrea Righi Cc: Steven Rostedt, Breno Leitao, Tejun Heo, David Vernet, Changwoo Min, Ingo Molnar, Juri Lelli, Vincent Guittot, Dietmar Eggemann, Ben Segall, Mel Gorman, Valentin Schneider, sched-ext, linux-kernel, kernel-team, jake 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2025-07-16 16:13 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-07-16 12:46 [PATCH] sched/ext: Suppress warning in __this_cpu_write() by disabling preemption Breno Leitao 2025-07-16 12:51 ` Peter Zijlstra 2025-07-16 13:15 ` Andrea Righi 2025-07-16 13:20 ` Breno Leitao 2025-07-16 13:40 ` Peter Zijlstra 2025-07-16 13:36 ` Peter Zijlstra 2025-07-16 14:26 ` Andrea Righi 2025-07-16 15:49 ` Peter Zijlstra 2025-07-16 16:08 ` Breno Leitao 2025-07-16 16:13 ` Andrea Righi 2025-07-16 12:54 ` Steven Rostedt 2025-07-16 13:06 ` Peter Zijlstra 2025-07-16 13:20 ` Andrea Righi 2025-07-16 13:33 ` Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).