include/linux/spinlock.h | 26 ++++++++++++++------------ kernel/sched/fair.c | 12 ++++++------ 2 files changed, 20 insertions(+), 18 deletions(-)
While the guards are properly nested, not all wrapped code is nice, as already
highlighted by that fair.c hunk.
Syzbot found another instance of this pattern in posix_timer_delete(), which
does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
Combined with this patch, that goes sideways most spectacular.
Undo this change, until we've developed stronger tools / debug for such issues.
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
include/linux/spinlock.h | 26 ++++++++++++++------------
kernel/sched/fair.c | 12 ++++++------
2 files changed, 20 insertions(+), 18 deletions(-)
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -572,12 +572,12 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_
#define class_raw_spinlock_nested_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_nested, _T)
DEFINE_LOCK_GUARD_1(raw_spinlock_irq, raw_spinlock_t,
- raw_spin_lock_irq_disable(_T->lock),
- raw_spin_unlock_irq_enable(_T->lock))
+ raw_spin_lock_irq(_T->lock),
+ raw_spin_unlock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, _T)
-DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq_disable(_T->lock))
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, _T)
@@ -592,13 +592,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_
#define class_raw_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_bh_try, _T)
DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
- raw_spin_lock_irq_disable(_T->lock),
- raw_spin_unlock_irq_enable(_T->lock))
+ raw_spin_lock_irqsave(_T->lock, _T->flags),
+ raw_spin_unlock_irqrestore(_T->lock, _T->flags),
+ unsigned long flags)
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, _T)
DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try,
- raw_spin_trylock_irq_disable(_T->lock))
+ raw_spin_trylock_irqsave(_T->lock, _T->flags))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, _T)
@@ -617,13 +618,13 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_try,
#define class_spinlock_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_try, _T)
DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
- spin_lock_irq_disable(_T->lock),
- spin_unlock_irq_enable(_T->lock))
+ spin_lock_irq(_T->lock),
+ spin_unlock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq, _T)
DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try,
- spin_trylock_irq_disable(_T->lock))
+ spin_trylock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq_try, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq_try, _T)
@@ -639,13 +640,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_bh_t
#define class_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_bh_try, _T)
DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t,
- spin_lock_irq_disable(_T->lock),
- spin_unlock_irq_enable(_T->lock))
+ spin_lock_irqsave(_T->lock, _T->flags),
+ spin_unlock_irqrestore(_T->lock, _T->flags),
+ unsigned long flags)
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave, _T)
DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
- spin_trylock_irq_disable(_T->lock))
+ spin_trylock_irqsave(_T->lock, _T->flags))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, _T)
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7253,7 +7253,7 @@ static bool distribute_cfs_runtime(struc
* period the timer is deactivated until scheduling resumes; cfs_b->idle is
* used to track this state.
*/
-static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun)
+static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
__must_hold(&cfs_b->lock)
{
int throttled;
@@ -7288,10 +7288,10 @@ static int do_sched_cfs_period_timer(str
* This check is repeated as we release cfs_b->lock while we unthrottle.
*/
while (throttled && cfs_b->runtime > 0) {
- raw_spin_unlock_irq_enable(&cfs_b->lock);
+ raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
throttled = distribute_cfs_runtime(cfs_b);
- raw_spin_lock_irq_disable(&cfs_b->lock);
+ raw_spin_lock_irqsave(&cfs_b->lock, flags);
}
/*
@@ -7399,7 +7399,7 @@ static __always_inline void return_cfs_r
static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
/* confirm we're still not at a refresh boundary */
- scoped_guard(raw_spinlock_irq, &cfs_b->lock) {
+ scoped_guard(raw_spinlock_irqsave, &cfs_b->lock) {
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
cfs_b->slack_started = false;
@@ -7484,14 +7484,14 @@ static enum hrtimer_restart sched_cfs_pe
int idle = 0;
int count = 0;
- guard(raw_spinlock_irq)(&cfs_b->lock);
+ CLASS(raw_spinlock_irqsave, cfsb_guard)(&cfs_b->lock);
for (;;) {
overrun = hrtimer_forward_now(timer, cfs_b->period);
if (!overrun)
break;
- idle = do_sched_cfs_period_timer(cfs_b, overrun);
+ idle = do_sched_cfs_period_timer(cfs_b, overrun, cfsb_guard.flags);
if (++count > 3) {
u64 new, old = ktime_to_ns(cfs_b->period);On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
>
> While the guards are properly nested, not all wrapped code is nice, as already
> highlighted by that fair.c hunk.
>
> Syzbot found another instance of this pattern in posix_timer_delete(), which
> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> Combined with this patch, that goes sideways most spectacular.
>
> Undo this change, until we've developed stronger tools / debug for such issues.
>
Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
__acquires() different contexts, we may be able to catch these issues at
compile time. I will explore a bit on this.
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Acked-by: Boqun Feng <boqun@kernel.org>
Regards,
Boqun
> ---
> include/linux/spinlock.h | 26 ++++++++++++++------------
> kernel/sched/fair.c | 12 ++++++------
> 2 files changed, 20 insertions(+), 18 deletions(-)
>
> --- a/include/linux/spinlock.h
> +++ b/include/linux/spinlock.h
> @@ -572,12 +572,12 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_
> #define class_raw_spinlock_nested_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_nested, _T)
>
> DEFINE_LOCK_GUARD_1(raw_spinlock_irq, raw_spinlock_t,
> - raw_spin_lock_irq_disable(_T->lock),
> - raw_spin_unlock_irq_enable(_T->lock))
> + raw_spin_lock_irq(_T->lock),
> + raw_spin_unlock_irq(_T->lock))
> DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
> #define class_raw_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, _T)
>
> -DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq_disable(_T->lock))
> +DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock))
> DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
> #define class_raw_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, _T)
>
> @@ -592,13 +592,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_
> #define class_raw_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_bh_try, _T)
>
> DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
> - raw_spin_lock_irq_disable(_T->lock),
> - raw_spin_unlock_irq_enable(_T->lock))
> + raw_spin_lock_irqsave(_T->lock, _T->flags),
> + raw_spin_unlock_irqrestore(_T->lock, _T->flags),
> + unsigned long flags)
> DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
> #define class_raw_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, _T)
>
> DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try,
> - raw_spin_trylock_irq_disable(_T->lock))
> + raw_spin_trylock_irqsave(_T->lock, _T->flags))
> DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
> #define class_raw_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, _T)
>
> @@ -617,13 +618,13 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_try,
> #define class_spinlock_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_try, _T)
>
> DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
> - spin_lock_irq_disable(_T->lock),
> - spin_unlock_irq_enable(_T->lock))
> + spin_lock_irq(_T->lock),
> + spin_unlock_irq(_T->lock))
> DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq, __acquires(_T), __releases(*(spinlock_t **)_T))
> #define class_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq, _T)
>
> DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try,
> - spin_trylock_irq_disable(_T->lock))
> + spin_trylock_irq(_T->lock))
> DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq_try, __acquires(_T), __releases(*(spinlock_t **)_T))
> #define class_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq_try, _T)
>
> @@ -639,13 +640,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_bh_t
> #define class_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_bh_try, _T)
>
> DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t,
> - spin_lock_irq_disable(_T->lock),
> - spin_unlock_irq_enable(_T->lock))
> + spin_lock_irqsave(_T->lock, _T->flags),
> + spin_unlock_irqrestore(_T->lock, _T->flags),
> + unsigned long flags)
> DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave, __acquires(_T), __releases(*(spinlock_t **)_T))
> #define class_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave, _T)
>
> DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
> - spin_trylock_irq_disable(_T->lock))
> + spin_trylock_irqsave(_T->lock, _T->flags))
> DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, __acquires(_T), __releases(*(spinlock_t **)_T))
> #define class_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, _T)
>
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -7253,7 +7253,7 @@ static bool distribute_cfs_runtime(struc
> * period the timer is deactivated until scheduling resumes; cfs_b->idle is
> * used to track this state.
> */
> -static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun)
> +static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
> __must_hold(&cfs_b->lock)
> {
> int throttled;
> @@ -7288,10 +7288,10 @@ static int do_sched_cfs_period_timer(str
> * This check is repeated as we release cfs_b->lock while we unthrottle.
> */
> while (throttled && cfs_b->runtime > 0) {
> - raw_spin_unlock_irq_enable(&cfs_b->lock);
> + raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
> /* we can't nest cfs_b->lock while distributing bandwidth */
> throttled = distribute_cfs_runtime(cfs_b);
> - raw_spin_lock_irq_disable(&cfs_b->lock);
> + raw_spin_lock_irqsave(&cfs_b->lock, flags);
> }
>
> /*
> @@ -7399,7 +7399,7 @@ static __always_inline void return_cfs_r
> static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
> {
> /* confirm we're still not at a refresh boundary */
> - scoped_guard(raw_spinlock_irq, &cfs_b->lock) {
> + scoped_guard(raw_spinlock_irqsave, &cfs_b->lock) {
> u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
>
> cfs_b->slack_started = false;
> @@ -7484,14 +7484,14 @@ static enum hrtimer_restart sched_cfs_pe
> int idle = 0;
> int count = 0;
>
> - guard(raw_spinlock_irq)(&cfs_b->lock);
> + CLASS(raw_spinlock_irqsave, cfsb_guard)(&cfs_b->lock);
>
> for (;;) {
> overrun = hrtimer_forward_now(timer, cfs_b->period);
> if (!overrun)
> break;
>
> - idle = do_sched_cfs_period_timer(cfs_b, overrun);
> + idle = do_sched_cfs_period_timer(cfs_b, overrun, cfsb_guard.flags);
>
> if (++count > 3) {
> u64 new, old = ktime_to_ns(cfs_b->period);
On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
>>
>> While the guards are properly nested, not all wrapped code is nice, as already
>> highlighted by that fair.c hunk.
>>
>> Syzbot found another instance of this pattern in posix_timer_delete(), which
>> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
>> Combined with this patch, that goes sideways most spectacular.
>>
>> Undo this change, until we've developed stronger tools / debug for such issues.
>>
>
> Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
> __acquires() different contexts, we may be able to catch these issues at
> compile time. I will explore a bit on this.
No.
Just do a wholesale conversion of all functions which affect the CPU
interrupt disabled state directly (local_irq_*) and indirectly (locking
functions etc.)
Anything else is just a whack a mole game.
TBH, I do not understand why you thought that you can get away with this
lazy approach especially after you discovered the same nasty problem in
do_sched_cfs_period_timer(). The resolution of that got buried in
1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
without even being mentioned.
When I was discussing the non-sensical syzbot messages earlier today
with Peter it immediately occurred to me that this undocumented change in
do_sched_cfs_period_timer() is not the only pattern which causes this to
go belly up. It took me five seconds to find the posix timer one.
TBH, my hope really was that the RUST people take the only valid
engineering principle "Correctness first" serious, but sadly they seem
to be the same lazy sods than everyone else who want to push their
agenda through no matter what.
Thanks,
tglx
On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
> On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
> >>
> >> While the guards are properly nested, not all wrapped code is nice, as already
> >> highlighted by that fair.c hunk.
> >>
> >> Syzbot found another instance of this pattern in posix_timer_delete(), which
> >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> >> Combined with this patch, that goes sideways most spectacular.
> >>
> >> Undo this change, until we've developed stronger tools / debug for such issues.
> >>
> >
> > Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
> > __acquires() different contexts, we may be able to catch these issues at
> > compile time. I will explore a bit on this.
>
> No.
>
> Just do a wholesale conversion of all functions which affect the CPU
> interrupt disabled state directly (local_irq_*) and indirectly (locking
> functions etc.)
>
> Anything else is just a whack a mole game.
>
Alright. But I'm afraid that's just another type of whack-a-mole games.
As I mentioned here [1], we are a few unpaired local_irq_disable() +
local_irq_enable(), we can spend time to clean them up, but no guarantee
people will not introduce more, plus we have code that does
spin_lock_irqsave(); spin_unlock_irq(); spin_lock_irq();
spin_unlock_irqrestore(); and expect it works. A more reasonable
approach to me is introducing the new API and fixing the problematic
usage one-by-one and then when we are certain about only a few cases
left, we do a flag day change.
Trying to do it (new API and whole conversion) in one go is easier
said than done. Of course I might miss something subtle here, looking
forwards to your suggestion.
> TBH, I do not understand why you thought that you can get away with this
> lazy approach especially after you discovered the same nasty problem in
> do_sched_cfs_period_timer(). The resolution of that got buried in
>
> 1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
>
> without even being mentioned.
>
I have this in the commit log:
[boqun: Adjust the user-side changes in do_sched_cfs_*_timer() provided
by Peter and Lyude]
but sure, I should have done a better job mentioning it.
A bit more context of switching the guard implementation: I wanted to
have some test/usage coverage other than Rust for the new API, and since
the guard() API is relatively new, so I thought people will not use it
"creatively" (but obviously I was wrong). Hence I add the conversation
for the guard APIs only. It is not a lazy approach IMO, but rather a way
to test how the new API works. Of course, a bug is a bug, I don't have
any excuse on that.
> When I was discussing the non-sensical syzbot messages earlier today
> with Peter it immediately occurred to me that this undocumented change in
> do_sched_cfs_period_timer() is not the only pattern which causes this to
> go belly up. It took me five seconds to find the posix timer one.
>
> TBH, my hope really was that the RUST people take the only valid
> engineering principle "Correctness first" serious, but sadly they seem
> to be the same lazy sods than everyone else who want to push their
> agenda through no matter what.
>
There seems some misunderstandings here. The only "lazy" part is we
defer the whole conversion because of the problems I mentioned above,
and that is because Correctness is valued.
[1]: https://lore.kernel.org/rust-for-linux/aPHlySQJQpDmgHAm@tardis.local/
Regards,
Boqun
> Thanks,
>
> tglx
On Tue, Aug 25 2026 at 16:28, Boqun Feng wrote:
> On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
>> On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
>> > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
>> >>
>> >> While the guards are properly nested, not all wrapped code is nice, as already
>> >> highlighted by that fair.c hunk.
>> >>
>> >> Syzbot found another instance of this pattern in posix_timer_delete(), which
>> >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
>> >> Combined with this patch, that goes sideways most spectacular.
>> >>
>> >> Undo this change, until we've developed stronger tools / debug for such issues.
>> >>
>> >
>> > Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
>> > __acquires() different contexts, we may be able to catch these issues at
>> > compile time. I will explore a bit on this.
>>
>> No.
>>
>> Just do a wholesale conversion of all functions which affect the CPU
>> interrupt disabled state directly (local_irq_*) and indirectly (locking
>> functions etc.)
>>
>> Anything else is just a whack a mole game.
>>
>
> Alright. But I'm afraid that's just another type of whack-a-mole
> games.
I don't think so.
> As I mentioned here [1], we are a few unpaired local_irq_disable() +
> local_irq_enable(), we can spend time to clean them up, but no guarantee
> people will not introduce more, plus we have code that does
> spin_lock_irqsave(); spin_unlock_irq(); spin_lock_irq();
> spin_unlock_irqrestore(); and expect it works.
It actually works and there are reasons why this needs to work in
certain cases. It needs some support with a different set of helper
functions for sure.
I played around with changing local_irq_disab/enable/save/restore almost
two decades ago when cli/sti was expensive, so we could do a lazy
disable approach. It went nowhere because it turned out to be too
complex to handle the interrupts which hit a lazy disabled region later,
but the principle itself worked.
I dealt with the above example by doing:
oldcnt = irq_save() return cnt++;
irq_restore(oldcnt) cnt = oldcnt;
irq_disable() cnt=1;
irq_enable() cnt=0;
See below.
> A more reasonable approach to me is introducing the new API and fixing
> the problematic usage one-by-one and then when we are certain about
> only a few cases left, we do a flag day change.
You already did a flag day change which causes problems, no?
The main problem is that you cover only half of it and there are
completely correct cases where this simply blows up in your face:
local_irq_disable(); // does not affect CNT
....
guard(raw_spinlock)(&l1); // does not affect CNT
foo()
guard(raw_spinlock_irqsave)(&l2); // observes CNT = 0
so the unlocking of &l2 will enable interrupts prematurely.
That's a very common scheme in interrupt handling. Functions which know
they are always invoked with interrupts disabled use raw_spinlock()
while others which can be invoked from different contexts use the
irqsave() variant.
Also the lack of rwlock support is a red flag. Again completely valid
code:
read_lock_irq()
...
guard(spinlock_irqsave)();
Same issue as above.
There are more subtle problems lurking around the corner.
> Trying to do it (new API and whole conversion) in one go is easier
> said than done. Of course I might miss something subtle here, looking
> forwards to your suggestion.
I did not say it's easy and I did not say that you have to do both in
one go, which is impossible.
You have to do it in stages, which means you put the infrastructure in
place first and then once that is settled you build the new API on top
if required at all. Building a new API first and hoping that it works
out without actually addressing the underlying issues first is just a
recipe for disaster.
You really want to start at the places which deal with the actual
interrupt flags of the CPU and that's definitely not locking. That's
only a couple of functions plus a few related helpers:
raw_local_irq_disable()
raw_local_irq_enable()
raw_local_irq_save()
raw_local_irq_restore()
If you actually look at the usage of the 'flags' argument of
raw_local_irq_save() and raw_local_irq_restore() then you'll notice that
it's a completely opaque cookie. Validating that there is no user which
is actually interested in seeing the real flags should be trivial
enough. A quick skim of x86 revealed exactly zero places, but I might
have missed one of course.
So you can get away with:
raw_local_irq_save(flags)
{
flags = count;
if (!count)
arch_local_irq_disable();
count++;
}
raw_local_irq_restore(flags)
{
if (!(count = flags))
arch_local_irq_enable();
}
raw_local_irq_disable()
{
arch_local_irq_disable();
count = 1;
}
raw_local_irq_enable()
{
count = 0;
arch_local_irq_enable();
}
To make this work you need to deal with the obvious race conditions
between modifying the counter and modifying the CPU flag, which is
relevant for all hardware initiated context changes (syscalls,
interrupts, exceptions, NMI).
In enter_from_user_mode() is trivial. All you need to add is an
unconditional
count = 1;
because interrupts are enabled when a task runs in user space. On entry
to the kernel (syscall, interrupt, exception, NMI) the CPU disables
interrupts so you have to reflect that in the software counter.
exit_to_user_mode() requires then obviously:
count = 0;
irqentry_enter_from_kernel_mode() is a bit more tricky because count and
the actual interrupt flags state in the CPU can be out of sync as you
can see in all four related functions above. But that's easy enough to
cure:
irqentry_state_t ret = {
.exit_rcu = false,
};
ret.irqdisable_cnt = count;
count = 1;
Setting it to 1 is the correct thing to do as this is fresh context and
it's safe for exception handlers which conditionally enable interrupts
because they explicitly rely on checking regs->eflags to figure out
whether the interrupted context had interrupts enabled.
That also makes this horrible hack in __irq_exit_rcu() go away because
the state is fully consistent.
In irqentry_exit_to_kernel_mode_after_preempt()
count = state.irqdisable_cnt;
In irqentry_nmi_enter() and irqentry_nmi_exit() you need exactly the
same.
With that you have a fully consistent and working system. Not what you
are aiming for in the very end, but a first step to cover the existing
code base fully without nasty to debug surprises.
Now you need to handle the oddball cases which nest an interrupt
enable/disable pair into a irqsave/restore region like the one in the
scheduler and the other in posix timers.
First of all, most of these places can be found by code analysis. When I
saw the one in the scheduler I whipped up a trivial coccinelle script
which found the one in posix timers immediately.
Then you can obviously add debug variants of those functions which are
conditional by an explicit config switch and emit warnings which are
easy enough to distinguish so that automated testing failures do not
result in a "paper over the problem" frenzy.
For dealing with those cases you want something like this:
raw_local_irq_enable_nested()
{
cur = count;
count = 0;
arch_local_irq_enable();
return cur;
}
raw_local_irq_disable_nested(oldcnt)
{
arch_local_irq_disable();
count = oldcnt;
}
Once all this headache is gone, you can modify the underlying machinery
without touching any other code at all and make the debug code a real
(lockdep) warning which has to be treated like any other splat.
See?
Thanks,
tglx
On Thu, Aug 27 2026 at 10:30, Thomas Gleixner wrote:
> On Tue, Aug 25 2026 at 16:28, Boqun Feng wrote:
> So you can get away with:
>
> raw_local_irq_save(flags)
> {
> flags = count;
> if (!count)
> arch_local_irq_disable();
> count++;
> }
>
> raw_local_irq_restore(flags)
> {
> if (!(count = flags))
> arch_local_irq_enable();
> }
>
> raw_local_irq_disable()
> {
> arch_local_irq_disable();
> count = 1;
> }
>
> raw_local_irq_enable()
> {
> count = 0;
> arch_local_irq_enable();
> }
Actually it can be done way simpler because the nasty case of
scoped_guard(lock_irqsave, lock) {
unlock_irq(lock);
lock_irq(lock);
}
is only valid for a single lock guard, because if it's nested then the
outer lock would lose the interrupt disabled protection. Anything else
would be a bug on its own and would have long ago blown up in our face.
So we can completely ignore flags.
raw_local_irq_save(flags)
{
if (!count)
arch_local_irq_disable();
count++;
}
raw_local_irq_restore(flags)
{
if (!--count)
arch_local_irq_enable();
}
raw_local_irq_disable()
{
arch_local_irq_disable();
count++;
}
raw_local_irq_enable()
{
count--;
arch_local_irq_enable();
}
with a copious amount of debug machinery to catch any oddballs.
Also note that this never uses irqsave/restore because historically that
has been way slower than CLI/STI.
A decade+ ago this used to be up to 30%, but micro architectures
optimized for it. Still on a SKL it's ~14% and on a Zen3 ~8% slower.
Thanks,
tglx
On Thu, Aug 27, 2026 at 10:29:10PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 10:30, Thomas Gleixner wrote:
> > On Tue, Aug 25 2026 at 16:28, Boqun Feng wrote:
> > So you can get away with:
> >
> > raw_local_irq_save(flags)
> > {
> > flags = count;
> > if (!count)
> > arch_local_irq_disable();
> > count++;
> > }
> >
> > raw_local_irq_restore(flags)
> > {
> > if (!(count = flags))
> > arch_local_irq_enable();
> > }
> >
> > raw_local_irq_disable()
> > {
> > arch_local_irq_disable();
> > count = 1;
> > }
> >
> > raw_local_irq_enable()
> > {
> > count = 0;
> > arch_local_irq_enable();
> > }
>
> Actually it can be done way simpler because the nasty case of
>
> scoped_guard(lock_irqsave, lock) {
> unlock_irq(lock);
> lock_irq(lock);
> }
>
> is only valid for a single lock guard, because if it's nested then the
> outer lock would lose the interrupt disabled protection. Anything else
> would be a bug on its own and would have long ago blown up in our face.
>
Right.
> So we can completely ignore flags.
>
> raw_local_irq_save(flags)
> {
> if (!count)
> arch_local_irq_disable();
> count++;
> }
>
> raw_local_irq_restore(flags)
> {
> if (!--count)
> arch_local_irq_enable();
> }
>
These are just local_interrupt_{disable,enable}() (replacing
arch_local_irq_save() with arch_local_irq_disable()) :)
> raw_local_irq_disable()
> {
> arch_local_irq_disable();
> count++;
> }
>
> raw_local_irq_enable()
> {
> count--;
> arch_local_irq_enable();
> }
>
> with a copious amount of debug machinery to catch any oddballs.
>
Ok, so brainstorm on the oddballs:
# 1: double disable
local_irq_disable();
local_irq_disable();
local_irq_enable();
# 2: double enable
local_irq_disable();
local_irq_enable();
local_irq_enable();
I think these mean we should probably do count = 1 and count = 0 in
irq_{enable,disable}() than count++ and count--?
# 3: only restore once
local_irq_save(flag1);
local_irq_save(flag2);
local_irq_restore(flag1);
# 4: keep restoring
local_irq_save(flag1);
local_irq_restore(flag1);
local_irq_restore(flag1);
these are a bit tricky, I guess we could only fix the users? But we
should not postpone the infrastructure because of these?
> Also note that this never uses irqsave/restore because historically that
> has been way slower than CLI/STI.
>
> A decade+ ago this used to be up to 30%, but micro architectures
> optimized for it. Still on a SKL it's ~14% and on a Zen3 ~8% slower.
>
Yes, if we go to the level to unify all irq disabling with counter
tracking then I think using arch_local_irq_disable() is possible and
makes a lot of senses.
Regards,
Boqun
> Thanks,
>
> tglx
>
On Thu, 27 Aug 2026 14:33:05 -0700
Boqun Feng <boqun@kernel.org> wrote:
....
> Yes, if we go to the level to unify all irq disabling with counter
> tracking then I think using arch_local_irq_disable() is possible and
> makes a lot of senses.
Where are you thinking of keeping the counter?
On non-x86 accessing it may be expensive.
The best bet is probably in 'current'.
Doesn't that make this valid?
int c = current->irq_disable_count;
if (c) {
current->irq_disable_count = c + 1;
return c;
}
disable_irq(); // asm("cli")
interrupt_disable_barrier(); // ISTR arm needs this
current->irq_disable_count = 1;
return 0;
}
The task can be preempted in the middle - but that doesn't matter.
If spin_lock_irqsave() returns current->irq_disable_count then
any existing code that does lock chaining works unaltered.
David
>
> Regards,
> Boqun
>
> > Thanks,
> >
> > tglx
> >
>
On Fri, Aug 28, 2026 at 09:22:05AM +0100, David Laight wrote:
> On Thu, 27 Aug 2026 14:33:05 -0700
> Boqun Feng <boqun@kernel.org> wrote:
>
> ....
> > Yes, if we go to the level to unify all irq disabling with counter
> > tracking then I think using arch_local_irq_disable() is possible and
> > makes a lot of senses.
>
> Where are you thinking of keeping the counter?
> On non-x86 accessing it may be expensive.
> The best bet is probably in 'current'.
>
The counter remains in preempt_count() as we already did for
local_interrupt_disable()?
> Doesn't that make this valid?
> int c = current->irq_disable_count;
> if (c) {
> current->irq_disable_count = c + 1;
> return c;
> }
> disable_irq(); // asm("cli")
> interrupt_disable_barrier(); // ISTR arm needs this
> current->irq_disable_count = 1;
> return 0;
> }
>
> The task can be preempted in the middle - but that doesn't matter.
>
> If spin_lock_irqsave() returns current->irq_disable_count then
> any existing code that does lock chaining works unaltered.
>
yes, but someone could be creative and do:
spin_lock_irqsave(l1, flag1);
spin_lock_irqsave(l2, flag2);
spin_unlock(l2);
spin_unlock_irqrestore(l1, flag1);
basically, perfectly nesting and paired critical sections always work,
it's the unknown oddballs that we need to worry about.
Regards,
Boqun
> David
>
> >
> > Regards,
> > Boqun
> >
> > > Thanks,
> > >
> > > tglx
> > >
> >
>
On Thu, Aug 27, 2026 at 02:33:05PM -0700, Boqun Feng wrote:
> Ok, so brainstorm on the oddballs:
>
> # 1: double disable
>
> local_irq_disable();
> local_irq_disable();
> local_irq_enable();
>
> # 2: double enable
>
> local_irq_disable();
> local_irq_enable();
> local_irq_enable();
>
> I think these mean we should probably do count = 1 and count = 0 in
> irq_{enable,disable}() than count++ and count--?
Could yeah, but ideally we'd take this opportunity to finally get rid of
them. We've ran into them a number of times, and I'm sure get fixed up a
bunch at some point, but never made the push to clean them out.
As is, lockdep only counts the redundant ones. They're a stat nobody
ever looks at.
> # 3: only restore once
>
> local_irq_save(flag1);
> local_irq_save(flag2);
> local_irq_restore(flag1);
>
> # 4: keep restoring
>
> local_irq_save(flag1);
> local_irq_restore(flag1);
> local_irq_restore(flag1);
>
> these are a bit tricky, I guess we could only fix the users? But we
> should not postpone the infrastructure because of these?
Yeah, so I do have a solution for that, but it is too horrible to write
in this small margin and all that :-) Thomas will kick my ass.
Best we simply detect and clean up.
On Thu, Aug 27, 2026 at 10:30:50AM +0200, Thomas Gleixner wrote:
[...]
> >> > Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
> >> > __acquires() different contexts, we may be able to catch these issues at
> >> > compile time. I will explore a bit on this.
> >>
> >> No.
> >>
> >> Just do a wholesale conversion of all functions which affect the CPU
> >> interrupt disabled state directly (local_irq_*) and indirectly (locking
> >> functions etc.)
> >>
> >> Anything else is just a whack a mole game.
> >>
> >
> > Alright. But I'm afraid that's just another type of whack-a-mole
> > games.
>
> I don't think so.
>
> > As I mentioned here [1], we are a few unpaired local_irq_disable() +
> > local_irq_enable(), we can spend time to clean them up, but no guarantee
> > people will not introduce more, plus we have code that does
> > spin_lock_irqsave(); spin_unlock_irq(); spin_lock_irq();
> > spin_unlock_irqrestore(); and expect it works.
>
> It actually works and there are reasons why this needs to work in
> certain cases. It needs some support with a different set of helper
> functions for sure.
>
> I played around with changing local_irq_disab/enable/save/restore almost
> two decades ago when cli/sti was expensive, so we could do a lazy
> disable approach. It went nowhere because it turned out to be too
> complex to handle the interrupts which hit a lazy disabled region later,
> but the principle itself worked.
>
> I dealt with the above example by doing:
>
> oldcnt = irq_save() return cnt++;
> irq_restore(oldcnt) cnt = oldcnt;
> irq_disable() cnt=1;
> irq_enable() cnt=0;
>
> See below.
>
> > A more reasonable approach to me is introducing the new API and fixing
> > the problematic usage one-by-one and then when we are certain about
> > only a few cases left, we do a flag day change.
>
> You already did a flag day change which causes problems, no?
>
(I will reply a few things here, and will read through your suggestions
below, and reply them latter.)
Right, but that was an attempt to see if we could switch to
scoped_guard() implementation to the new infrastructure (see below) in
this stage. Clearly we cannot because I overlooked cases like
posix_timer_delete(), but itself is not trying to introduce the new API.
> The main problem is that you cover only half of it and there are
> completely correct cases where this simply blows up in your face:
>
> local_irq_disable(); // does not affect CNT
> ....
> guard(raw_spinlock)(&l1); // does not affect CNT
> foo()
> guard(raw_spinlock_irqsave)(&l2); // observes CNT = 0
>
> so the unlocking of &l2 will enable interrupts prematurely.
>
If we are talking the switch in this patch, then no, the unlocking
of &l2 will NOT enable interrupts prematurely. The above code expands as
the following (using pseudo code to describe how
raw_spin_lock_irq_disable(), raw_spin_lock_irq_enable(),
local_interrupt_disable(), and local_interrupt_enable() work)
local_irq_disable(); // does not affect CNT
....
guard(raw_spinlock)(&l1); // does not affect CNT
foo()
guard(raw_spinlock_irqsave)(&l2):
raw_spin_lock_irq_disable():
local_interrupt_disable():
CNT++;
this_cpu(state) = local_irq_save(); // record the current state
raw_spin_lock(&l2);
...
raw_spin_lock_irq_enable():
raw_spin_unlock(&l2);
local_interrupt_enable():
CNT--;
if (CNT == 0)
local_irq_restore(this_cpu(state)); // recover the previous state
So local_interrupt_disable() and local_interrupt_enable() only recover
to the previous state, as a result it'll not enable interrupt
prematurely here. In other words, the following code works:
local_irq_disable();
local_interrupt_disable();
local_interrupt_enable(); // interrupt is not re-enabled here
// similar to how preempt_disable() does
// in a nested preemption disable
// critical section.
local_irq_enable();
These functions are the infrastructure thing you talk about below:
(they are introduced in commit e901c1510e24 ("irq,spin_lock: Add counted
interrupt disabling/enabling"))
* local_interrupt_disable()
* local_interrupt_enable()
* raw_spin_lock_irq_disable()
* raw_spin_lock_irq_enable()
They currently work when nested in local_irq_disable() or
local_irq_save() because of the per-CPU irq state tracking when CNT
reaches 0->1 or 1->0.
> That's a very common scheme in interrupt handling. Functions which know
> they are always invoked with interrupts disabled use raw_spinlock()
> while others which can be invoked from different contexts use the
> irqsave() variant.
>
> Also the lack of rwlock support is a red flag. Again completely valid
We could use local_interrupt_disable() and local_interrupt_enable() to
implement a new API for rwlock when the support is needed in the future.
> code:
>
> read_lock_irq()
> ...
> guard(spinlock_irqsave)();
>
> Same issue as above.
>
Similar as above, no issue in this case.
> There are more subtle problems lurking around the corner.
>
> > Trying to do it (new API and whole conversion) in one go is easier
> > said than done. Of course I might miss something subtle here, looking
> > forwards to your suggestion.
>
> I did not say it's easy and I did not say that you have to do both in
> one go, which is impossible.
>
> You have to do it in stages, which means you put the infrastructure in
> place first and then once that is settled you build the new API on top
> if required at all. Building a new API first and hoping that it works
> out without actually addressing the underlying issues first is just a
> recipe for disaster.
>
I agree and that is actually what I did here: adding the infrastructure
local_interrupt_disable() and local_interrupt_enable() and gradually
using that infrastructure to support building new API (or existing API).
`
The part that went wrong for this particular patch was I was missing the
usage similar to posix_timer_delete() cases where users want to drop the
lock under scoped_guard context, I have a proposal in another reply,
and I think that might be better way, but of course the infrastructure
can support without it, we just need to postpone the implementation
switch of scoped_guard() until it's ready.
All I'm trying to say here is I'm doing this slow and steady :)
[I will take a deep look for the following later, I feel I need to reply
above in case I or the patch confused you somehow]
Regards,
Boqun
> interrupt flags of the CPU and that's definitely not locking. That's
> only a couple of functions plus a few related helpers:
>
> raw_local_irq_disable()
> raw_local_irq_enable()
> raw_local_irq_save()
> raw_local_irq_restore()
>
> If you actually look at the usage of the 'flags' argument of
> raw_local_irq_save() and raw_local_irq_restore() then you'll notice that
> it's a completely opaque cookie. Validating that there is no user which
> is actually interested in seeing the real flags should be trivial
> enough. A quick skim of x86 revealed exactly zero places, but I might
> have missed one of course.
>
> So you can get away with:
>
> raw_local_irq_save(flags)
> {
> flags = count;
> if (!count)
> arch_local_irq_disable();
> count++;
> }
>
> raw_local_irq_restore(flags)
> {
> if (!(count = flags))
> arch_local_irq_enable();
> }
>
> raw_local_irq_disable()
> {
> arch_local_irq_disable();
> count = 1;
> }
>
> raw_local_irq_enable()
> {
> count = 0;
> arch_local_irq_enable();
> }
>
> To make this work you need to deal with the obvious race conditions
> between modifying the counter and modifying the CPU flag, which is
> relevant for all hardware initiated context changes (syscalls,
> interrupts, exceptions, NMI).
>
> In enter_from_user_mode() is trivial. All you need to add is an
> unconditional
>
> count = 1;
>
> because interrupts are enabled when a task runs in user space. On entry
> to the kernel (syscall, interrupt, exception, NMI) the CPU disables
> interrupts so you have to reflect that in the software counter.
>
> exit_to_user_mode() requires then obviously:
>
> count = 0;
>
> irqentry_enter_from_kernel_mode() is a bit more tricky because count and
> the actual interrupt flags state in the CPU can be out of sync as you
> can see in all four related functions above. But that's easy enough to
> cure:
>
> irqentry_state_t ret = {
> .exit_rcu = false,
> };
>
> ret.irqdisable_cnt = count;
> count = 1;
>
> Setting it to 1 is the correct thing to do as this is fresh context and
> it's safe for exception handlers which conditionally enable interrupts
> because they explicitly rely on checking regs->eflags to figure out
> whether the interrupted context had interrupts enabled.
>
> That also makes this horrible hack in __irq_exit_rcu() go away because
> the state is fully consistent.
>
> In irqentry_exit_to_kernel_mode_after_preempt()
>
> count = state.irqdisable_cnt;
>
> In irqentry_nmi_enter() and irqentry_nmi_exit() you need exactly the
> same.
>
> With that you have a fully consistent and working system. Not what you
> are aiming for in the very end, but a first step to cover the existing
> code base fully without nasty to debug surprises.
>
> Now you need to handle the oddball cases which nest an interrupt
> enable/disable pair into a irqsave/restore region like the one in the
> scheduler and the other in posix timers.
>
> First of all, most of these places can be found by code analysis. When I
> saw the one in the scheduler I whipped up a trivial coccinelle script
> which found the one in posix timers immediately.
>
> Then you can obviously add debug variants of those functions which are
> conditional by an explicit config switch and emit warnings which are
> easy enough to distinguish so that automated testing failures do not
> result in a "paper over the problem" frenzy.
>
> For dealing with those cases you want something like this:
>
> raw_local_irq_enable_nested()
> {
> cur = count;
> count = 0;
> arch_local_irq_enable();
> return cur;
> }
>
> raw_local_irq_disable_nested(oldcnt)
> {
> arch_local_irq_disable();
> count = oldcnt;
> }
>
> Once all this headache is gone, you can modify the underlying machinery
> without touching any other code at all and make the debug code a real
> (lockdep) warning which has to be treated like any other splat.
>
> See?
>
> Thanks,
>
> tglx
On Thu, Aug 27 2026 at 06:14, Boqun Feng wrote:
> On Thu, Aug 27, 2026 at 10:30:50AM +0200, Thomas Gleixner wrote:
>> The main problem is that you cover only half of it and there are
>> completely correct cases where this simply blows up in your face:
>>
>> local_irq_disable(); // does not affect CNT
>> ....
>> guard(raw_spinlock)(&l1); // does not affect CNT
>> foo()
>> guard(raw_spinlock_irqsave)(&l2); // observes CNT = 0
>>
>> so the unlocking of &l2 will enable interrupts prematurely.
>>
>
> If we are talking the switch in this patch, then no, the unlocking
> of &l2 will NOT enable interrupts prematurely. The above code expands as
> the following (using pseudo code to describe how
> raw_spin_lock_irq_disable(), raw_spin_lock_irq_enable(),
> local_interrupt_disable(), and local_interrupt_enable() work)
>
> local_irq_disable(); // does not affect CNT
> ....
> guard(raw_spinlock)(&l1); // does not affect CNT
> foo()
> guard(raw_spinlock_irqsave)(&l2):
> raw_spin_lock_irq_disable():
> local_interrupt_disable():
> CNT++;
> this_cpu(state) = local_irq_save(); // record the current state
> raw_spin_lock(&l2);
> ...
> raw_spin_lock_irq_enable():
> raw_spin_unlock(&l2);
> local_interrupt_enable():
> CNT--;
> if (CNT == 0)
> local_irq_restore(this_cpu(state)); // recover the previous state
>
> So local_interrupt_disable() and local_interrupt_enable() only recover
> to the previous state, as a result it'll not enable interrupt
> prematurely here. In other words, the following code works:
>
> local_irq_disable();
> local_interrupt_disable();
> local_interrupt_enable(); // interrupt is not re-enabled here
> // similar to how preempt_disable() does
> // in a nested preemption disable
> // critical section.
> local_irq_enable();
Fair enough. I misread that part.
But my main observation that the counter is inconsistent still stands
and I think that's a fundamental flaw because there is no way that code
can rely on that counter until everything has been converted over and
the interrupt/exception/nmi/syscall entry/exit code has been fixed up.
Just let me look at local_interrupt_disable() and __irq_exit_rcu()
again.
local_interrupt_disable()
new_count = hardirq_disable_enter();
/* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */
if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
_local_interrupt_disable();
This is absolutely not ok. Why?
The counter is incremented _before_ interrupts are actually
disabled. Now in __irq_exit_rcu():
if (!in_interrupt() && !hardirq_disable_count() &&
local_softirq_pending()) {
which prevents soft interrupt handling in a completely legitimate
situation. As a consequence _nothing_ will handle the pending soft
interrupt until:
- an interrupt coming in which observes consistent state
- a local_bh_enable() processes them
That explains that recently quite a few more spurious 'local soft irq
pending' printk's have been observed by people as there is no guarantee
that either one of those events happens _before_ a CPU reaches
idle. Even in the non-idle case deferring this to the next 'by chance'
handling is fundamentally broken. This needs to be removed ASAP.
But coming back to the problem underneath. The ordering in
local_interrupt_disable() is simply wrong. You need to disable first and
then update the counter. Reverse order for enable() obviously update
counter and enable, which you got right.
And to make stuff work correctly you need the fixups I pointed out in my
previous reply to the various entry/exit functions. It's exactly the
same problem as we handle in interrupt/exception/nmi entry/exit code
vs. RCU, lockdep, tracing etc.
So if disable() does:
if (!count)
arch_local_irq_disable();
count++;
then an interrupt hitting before arch_local_irq_disable() will always
observe the correct state. After that it can't be delivered.
Now with exceptions that's a different story because they can hit after
local_irq_disable() and before the count is incremented.
I thought some more about the state handling there and I think we can
avoid irq_state_t completely:
irqentry_enter_from_kernel_mode()
count++;
...
irqentry_exit_to_kernel_mode_after_preempt()
...
count--;
For a regular interrupt which hit _before_ disable() managed to disable
it at the CPU level, this will go from 0 -> 1 and on return from 1 -> 0.
For an exception which hits between disabling and incrementing the
counter this will go from 0 -> 1 as well, but there is nothing which can
be done about that and exception handlers need to consult regs->eflags
to figure out the state of the context they interrupted. If the
exception hits afterwards then it will set the correct state. But it
does not matter in that case because everything there needs to do
irqsave() so interrupts can't be enabled accidentaly. The only exception
to that rule is the conditional enable:
if (regs->eflags & X86_EFLAGS_IF)
local_irq_enable();
And for that to work correctly you want overall consistent counter
state. Otherwise your counter is just a random number generator.
With that fixed the disable race becomes:
disable()
if (!count)
-> Interrupt before interrupts are disabled in the CPU.
irqentry_enter_from_kernel_mode()
count++; // Correct state because the CPU disabled interrupts
...
__irq_exit_rcu()
if (!in_interrupt() && local_softirq_pending()) {
handle_softirqs()
...
local_irq_enable(); -> Count goes to 0
...
guard(spinlock_irq)(&lock)
local_interrupt_disable()
// Observes count == 0
if (!count)
arch_local_irq_disable();
...
irqentry_exit_to_kernel_mode_after_preempt()
...
count--;
And yes, this only works correctly when _all_ state is consistent. You
can't get it to work properly with half of it without creating hard to
debug problems.
Thanks,
tglx
On Thu, Aug 27, 2026 at 05:43:26PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 06:14, Boqun Feng wrote:
> > On Thu, Aug 27, 2026 at 10:30:50AM +0200, Thomas Gleixner wrote:
> >> The main problem is that you cover only half of it and there are
> >> completely correct cases where this simply blows up in your face:
> >>
> >> local_irq_disable(); // does not affect CNT
> >> ....
> >> guard(raw_spinlock)(&l1); // does not affect CNT
> >> foo()
> >> guard(raw_spinlock_irqsave)(&l2); // observes CNT = 0
> >>
> >> so the unlocking of &l2 will enable interrupts prematurely.
> >>
> >
> > If we are talking the switch in this patch, then no, the unlocking
> > of &l2 will NOT enable interrupts prematurely. The above code expands as
> > the following (using pseudo code to describe how
> > raw_spin_lock_irq_disable(), raw_spin_lock_irq_enable(),
> > local_interrupt_disable(), and local_interrupt_enable() work)
> >
> > local_irq_disable(); // does not affect CNT
> > ....
> > guard(raw_spinlock)(&l1); // does not affect CNT
> > foo()
> > guard(raw_spinlock_irqsave)(&l2):
> > raw_spin_lock_irq_disable():
> > local_interrupt_disable():
> > CNT++;
> > this_cpu(state) = local_irq_save(); // record the current state
> > raw_spin_lock(&l2);
> > ...
> > raw_spin_lock_irq_enable():
> > raw_spin_unlock(&l2);
> > local_interrupt_enable():
> > CNT--;
> > if (CNT == 0)
> > local_irq_restore(this_cpu(state)); // recover the previous state
> >
> > So local_interrupt_disable() and local_interrupt_enable() only recover
> > to the previous state, as a result it'll not enable interrupt
> > prematurely here. In other words, the following code works:
> >
> > local_irq_disable();
> > local_interrupt_disable();
> > local_interrupt_enable(); // interrupt is not re-enabled here
> > // similar to how preempt_disable() does
> > // in a nested preemption disable
> > // critical section.
> > local_irq_enable();
>
> Fair enough. I misread that part.
>
> But my main observation that the counter is inconsistent still stands
> and I think that's a fundamental flaw because there is no way that code
> can rely on that counter until everything has been converted over and
> the interrupt/exception/nmi/syscall entry/exit code has been fixed up.
>
> Just let me look at local_interrupt_disable() and __irq_exit_rcu()
> again.
>
> local_interrupt_disable()
> new_count = hardirq_disable_enter();
>
> /* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */
>
> if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
> _local_interrupt_disable();
>
> This is absolutely not ok. Why?
>
> The counter is incremented _before_ interrupts are actually
> disabled. Now in __irq_exit_rcu():
>
> if (!in_interrupt() && !hardirq_disable_count() &&
> local_softirq_pending()) {
>
> which prevents soft interrupt handling in a completely legitimate
> situation. As a consequence _nothing_ will handle the pending soft
> interrupt until:
>
> - an interrupt coming in which observes consistent state
>
> - a local_bh_enable() processes them
>
> That explains that recently quite a few more spurious 'local soft irq
> pending' printk's have been observed by people as there is no guarantee
> that either one of those events happens _before_ a CPU reaches
> idle. Even in the non-idle case deferring this to the next 'by chance'
> handling is fundamentally broken. This needs to be removed ASAP.
>
> But coming back to the problem underneath. The ordering in
> local_interrupt_disable() is simply wrong. You need to disable first and
> then update the counter. Reverse order for enable() obviously update
> counter and enable, which you got right.
>
Noted, the reason that I used the current order is to optimize
local_interrupt_disable() from re-disabling interrupt every time:
https://lore.kernel.org/rust-for-linux/87a5eu7gvw.ffs@tglx/
but looks like we cannot do it without the fixups you mention below.
For now I will reverse the order and remove the additional checking in
softirq to fix the softirq pending issue.
Regards,
Boqun
> And to make stuff work correctly you need the fixups I pointed out in my
> previous reply to the various entry/exit functions. It's exactly the
> same problem as we handle in interrupt/exception/nmi entry/exit code
> vs. RCU, lockdep, tracing etc.
>
> So if disable() does:
>
> if (!count)
> arch_local_irq_disable();
> count++;
>
> then an interrupt hitting before arch_local_irq_disable() will always
> observe the correct state. After that it can't be delivered.
>
[...]
On Thu, Aug 27 2026 at 09:52, Boqun Feng wrote:
> On Thu, Aug 27, 2026 at 05:43:26PM +0200, Thomas Gleixner wrote:
>> But coming back to the problem underneath. The ordering in
>> local_interrupt_disable() is simply wrong. You need to disable first and
>> then update the counter. Reverse order for enable() obviously update
>> counter and enable, which you got right.
>>
>
> Noted, the reason that I used the current order is to optimize
> local_interrupt_disable() from re-disabling interrupt every time:
>
> https://lore.kernel.org/rust-for-linux/87a5eu7gvw.ffs@tglx/
Yes. I gave you the wrong order, but I expected you to actually think it
through and not blindly copy it. :)
> but looks like we cannot do it without the fixups you mention below.
But that does not mean it can't be done. Checking for 0 first and
incrementing after the actual disable is still achieving the same result
of touching the CPU only once, no?
> For now I will reverse the order and remove the additional checking in
> softirq to fix the softirq pending issue.
That "fixes" another nasty bug which was latent for weeks and people
could not get a handle on it because it was absolutely not
reproducible. Given all that I'm absolutely not convinced that there
isn't another pile of latent surprises lurking.
Aside of that I'm worried about having this new counter exposed in the
current state of affairs. Nothing prevents arbitrary code from using
hardirq_disable_count(), which is definitely faster than
irqs_disabled(), but returns a random value depending on context. That's
just another recipe for latent and hard to debug disasters to happen as
you already demonstrated in __irq_exit_rcu().
It's not the end of the world to bite the bullet and undo the whole
pile, except for the then unused expansion of preempt count, go back to
the drawing board and come up with a consistent and better overall
solution.
I know that hurts, I've been there myself more than once. But at the end
I was always happy that we decided to rip it out instead of trying to
debug and duct tape it to death.
A inconsistent and fragile facility is worse than having none.
Thanks,
tglx
On Thu, Aug 27, 2026 at 08:15:44PM +0200, Thomas Gleixner wrote: > On Thu, Aug 27 2026 at 09:52, Boqun Feng wrote: > > On Thu, Aug 27, 2026 at 05:43:26PM +0200, Thomas Gleixner wrote: > >> But coming back to the problem underneath. The ordering in > >> local_interrupt_disable() is simply wrong. You need to disable first and > >> then update the counter. Reverse order for enable() obviously update > >> counter and enable, which you got right. > >> > > > > Noted, the reason that I used the current order is to optimize > > local_interrupt_disable() from re-disabling interrupt every time: > > > > https://lore.kernel.org/rust-for-linux/87a5eu7gvw.ffs@tglx/ > > Yes. I gave you the wrong order, but I expected you to actually think it > through and not blindly copy it. :) > No, not blaming you :) I was just providing a bit more context. I did think through a few parts to make it work, but TBH I lack of the sensitivity for the impact that no interrupt happen on one CPU for a while, so I didn't think this part very seriously. And I just liked the idea we could skip disabling IRQ if possible. > > but looks like we cannot do it without the fixups you mention below. > > But that does not mean it can't be done. Checking for 0 first and > incrementing after the actual disable is still achieving the same result > of touching the CPU only once, no? > Yeah, that should work. But I need to think a bit hard on this. > > For now I will reverse the order and remove the additional checking in > > softirq to fix the softirq pending issue. > > That "fixes" another nasty bug which was latent for weeks and people > could not get a handle on it because it was absolutely not > reproducible. Given all that I'm absolutely not convinced that there > isn't another pile of latent surprises lurking. > > Aside of that I'm worried about having this new counter exposed in the > current state of affairs. Nothing prevents arbitrary code from using > hardirq_disable_count(), which is definitely faster than > irqs_disabled(), but returns a random value depending on context. That's Random how? Are you saying in the current (wrong) order? Because after reversing the order, hardirq_disable_count() != 0 means the interrupt has been disabled, no? But I checked, actually with the reverse order, we don't need hardirq_disable_count(), so we can remove it entirely. Will send a follow up patch on this. > just another recipe for latent and hard to debug disasters to happen as > you already demonstrated in __irq_exit_rcu(). > > It's not the end of the world to bite the bullet and undo the whole > pile, except for the then unused expansion of preempt count, go back to > the drawing board and come up with a consistent and better overall > solution. > > I know that hurts, I've been there myself more than once. But at the end > I was always happy that we decided to rip it out instead of trying to > debug and duct tape it to death. > > A inconsistent and fragile facility is worse than having none. > To be honest, it doesn't hurt myself if we have to redo the work, I would always like to do it correct. So I don't mind doing that. But it might hurt others who want to develop real drivers with Rust because no SpinLockIrq for them until the redo finishes. That's the major reason that I would like to keep local_interrupt_disable() and spin_lock_irq_disable(). (I also feel like with the order fix and hardirq_disable_count() remove, the design is robust enough to exist and evolve, but I may miss something subtle?) Alternatively, we can move the current API to be Rust use only (we can make the implementation in Rust even, if we maintain the state and counter in Rust) in this way, there is only a limit set interactions from the new things with the existing kernel, and Rust can always make the guard work properly. But honestly, it'll be just duplicating what we already have here to the Rust side. So it's not my own desire that I want to keep the current things in tree, it's more that I also look at this from a different angle, and it make some sense engineer-wise: the semantics of local_interrupt_disable() is so easy and straightforward that I feel it's unfair to block the potential user especially when the users can guarantee the correct usages with the type system. Anyway, that's just my two cents. Regards, Boqun > Thanks, > > tglx > > > > > > >
On Thu, Aug 27 2026 at 12:41, Boqun Feng wrote:
> On Thu, Aug 27, 2026 at 08:15:44PM +0200, Thomas Gleixner wrote:
>> > For now I will reverse the order and remove the additional checking in
>> > softirq to fix the softirq pending issue.
>>
>> That "fixes" another nasty bug which was latent for weeks and people
>> could not get a handle on it because it was absolutely not
>> reproducible. Given all that I'm absolutely not convinced that there
>> isn't another pile of latent surprises lurking.
>>
>> Aside of that I'm worried about having this new counter exposed in the
>> current state of affairs. Nothing prevents arbitrary code from using
>> hardirq_disable_count(), which is definitely faster than
>> irqs_disabled(), but returns a random value depending on context. That's
>
> Random how? Are you saying in the current (wrong) order? Because after
> reversing the order, hardirq_disable_count() != 0 means the interrupt
> has been disabled, no?
>
> But I checked, actually with the reverse order, we don't need
> hardirq_disable_count(), so we can remove it entirely. Will send a
> follow up patch on this.
The point is that the counter is only valid when used within the limits
of the current coverage. Other than that it is not:
spin_lock_irq() // or any other non-covered mechanism
// observes 0
cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
That's inconsistent and therefore it is a random number, no?
You have no way to prevent that this happens and if it does it becomes a
nightmare to debug for everyone. Guess who got the bug reports about
preemption counter issues and local softirq pending messages in his
inbox and dealt with them.
There is a world outside of your safe rust zone and that needs to be
safe too. This half finished attempt to make Rust work is absolutely
not and I have zero interrest to deal with the fallout.
It's not safe and no extra hacks will make it safe. Which means it is
not ready. So the only sensible thing is to revert everything which
touches that section of preempt_count() and provides interfaces.
As this annoyed me, I rumaged through my poison cabinet and found the
old patches again. They obviously don't apply anymore but I found the
hints which corners need some care. With the generic entry code that
also got way simpler.
So I sat down and reverted
1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")
and then hacked it up just to see how far I get before vanishing to bed.
Three hours later it surprisingly booted right away into a full distro
kernel and survived kernel builds and a few test cases. :)
Obviously I did not do any serious testing on it, but I wanted to share
it as a starting point and food for thoughts.
Yes, it needs to be enabled per architecture as the preempt counter
initialization is architecture specific and it requires generic entry
code. But those are not uncommon prerequisites and an incentive for
architecture people to get their act together.
But it is fully consistent and the fully refcounted thing can be
built on top of it. If you look carefuly you'll notice that
__raw_local_irq_disable/enable() are just optimized versions of
__raw_local_irq_save/restore() as they don't have the conditionals, so
they can be unified completely at least for debug builds or in general
when it turns out that the overhead is neglible.
There is a wide range of optimizations possible with that especially by
combining preempt/interrupt modifications into one operation and
rescheduling without changing the preemption counter in the first
place. Which is what I hinted to in the mail you linked earlier. I'm so
tempted to hack that up tomorrow once my brain is less fried than now
and after I exposed it to some serious testing.
Thanks,
tglx
---
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -318,6 +318,7 @@ config X86
select PCI_DOMAINS if PCI
select PCI_LOCKLESS_CONFIG if PCI
select PERF_EVENTS
+ select PREEMPT_COUNT_IRQFLAGS
select RTC_LIB
select RTC_MC146818_LIB
select SPARSE_IRQ
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -61,8 +61,8 @@ static __always_inline void preempt_coun
*/
#define init_task_preempt_count(p) do { } while (0)
-#define init_idle_preempt_count(p, cpu) do { \
- per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
+#define init_idle_preempt_count(p, cpu) do { \
+ per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED | HARDIRQ_DISABLE_OFFSET; \
} while (0)
/*
--- a/include/linux/irq-entry-common.h
+++ b/include/linux/irq-entry-common.h
@@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_
*/
static __always_inline void enter_from_user_mode(struct pt_regs *regs)
{
+ __preempt_count_inc_hardirqs_disable();
arch_enter_from_user_mode(regs);
lockdep_hardirqs_off(CALLER_ADDR0);
@@ -275,6 +276,7 @@ static __always_inline void exit_to_user
user_enter_irqoff();
arch_exit_to_user_mode();
lockdep_hardirqs_on(CALLER_ADDR0);
+ __preempt_count_dec_hardirqs_disable();
}
/**
@@ -385,6 +387,8 @@ static __always_inline irqentry_state_t
.exit_rcu = false,
};
+ __preempt_count_inc_hardirqs_disable();
+
/*
* If this entry hit the idle task invoke ct_irq_enter() whether
* RCU is watching or not.
@@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preem
instrumentation_end();
ct_irq_exit();
lockdep_hardirqs_on(CALLER_ADDR0);
+ __preempt_count_dec_hardirqs_disable();
return;
}
@@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preem
if (state.exit_rcu)
ct_irq_exit();
}
+ __preempt_count_dec_hardirqs_disable();
}
/**
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -13,6 +13,7 @@
#define _LINUX_TRACE_IRQFLAGS_H
#include <linux/irqflags_types.h>
+#include <linux/preempt.h>
#include <linux/typecheck.h>
#include <linux/cleanup.h>
#include <asm/irqflags.h>
@@ -165,31 +166,124 @@ extern void warn_bogus_irq_restore(void)
/*
* Wrap the arch provided IRQ routines to provide appropriate checks.
*/
-#define raw_local_irq_disable() arch_local_irq_disable()
-#define raw_local_irq_enable() arch_local_irq_enable()
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+static __always_inline void raw_local_irq_disable(void)
+{
+ arch_local_irq_disable();
+ preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+ preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+ unsigned long cnt = preempt_count();
+
+ if (!(cnt & HARDIRQ_DISABLE_MASK))
+ arch_local_irq_disable();
+ preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+
+ // Probably not even needed unless something feeds 'flags' into
+ // irqs_disabled_flags()
+ return cnt;
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long cnt)
+{
+ if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+ return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
+{
+ return !!cnt;
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+ return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+ preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+ arch_safe_halt();
+}
+
+#else
+
+static __always_inline void raw_local_irq_disable(void)
+{
+ arch_local_irq_disable();
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+ return arch_local_irq_save();
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long flags)
+{
+ arch_local_irq_restore(flags);
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+ return arch_local_save_flags();
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long flags)
+{
+ return arch_irqs_disabled_flags(flags);
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+ return arch_irqs_disabled();
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+ arch_safe_halt();
+}
+
+#endif
+
#define raw_local_irq_save(flags) \
do { \
typecheck(unsigned long, flags); \
- flags = arch_local_irq_save(); \
+ flags = __raw_local_irq_save(); \
} while (0)
#define raw_local_irq_restore(flags) \
do { \
typecheck(unsigned long, flags); \
raw_check_bogus_irq_restore(); \
- arch_local_irq_restore(flags); \
+ __raw_local_irq_restore(flags); \
} while (0)
#define raw_local_save_flags(flags) \
do { \
typecheck(unsigned long, flags); \
- flags = arch_local_save_flags(); \
+ flags = __raw_local_save_flags(); \
} while (0)
#define raw_irqs_disabled_flags(flags) \
({ \
typecheck(unsigned long, flags); \
- arch_irqs_disabled_flags(flags); \
+ __raw_irqs_disabled_flags(flags); \
})
-#define raw_irqs_disabled() (arch_irqs_disabled())
-#define raw_safe_halt() arch_safe_halt()
/*
* The local_irq_*() APIs are equal to the raw_local_irq*()
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -54,31 +54,31 @@
* NMI_MASK: 0xf0000000
* (PREEMPT_NEED_RESCHED is in a different word)
*/
-#define PREEMPT_BITS 8
-#define SOFTIRQ_BITS 8
+#define PREEMPT_BITS 8
+#define SOFTIRQ_BITS 8
#define HARDIRQ_DISABLE_BITS 8
-#define HARDIRQ_BITS 4
-#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
+#define HARDIRQ_BITS 4
+#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
-#define PREEMPT_SHIFT 0
-#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
+#define PREEMPT_SHIFT 0
+#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_DISABLE_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
-#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
-#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
+#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
-#define __IRQ_MASK(x) ((1UL << (x))-1)
+#define __IRQ_MASK(x) ((1UL << (x))-1)
-#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
-#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
+#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
+#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_DISABLE_MASK (__IRQ_MASK(HARDIRQ_DISABLE_BITS) << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
-#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
+#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
-#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
-#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
+#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
+#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_DISABLE_OFFSET (1UL << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
-#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
+#define NMI_OFFSET (1UL << NMI_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
@@ -90,7 +90,11 @@
*
* Reset by start_kernel()->sched_init()->init_idle()->init_idle_preempt_count().
*/
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+#define INIT_PREEMPT_COUNT (PREEMPT_OFFSET + HARDIRQ_DISABLE_OFFSET)
+#else
#define INIT_PREEMPT_COUNT PREEMPT_OFFSET
+#endif
/*
* Initial preempt_count value; reflects the preempt_count schedule invariant
@@ -322,6 +326,21 @@ do { \
#endif /* CONFIG_PREEMPT_COUNT */
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+static __always_inline void __preempt_count_inc_hardirqs_disable(void)
+{
+ __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void __preempt_count_dec_hardirqs_disable(void)
+{
+ __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+}
+#else
+static __always_inline void __preempt_count_inc_hardirqs_disable(void) { }
+static __always_inline void __preempt_count_dec_hardirqs_disable(void) { }
+#endif
+
#ifdef MODULE
/*
* Modules have no business playing preemption tricks.
--- a/kernel/Kconfig.preempt
+++ b/kernel/Kconfig.preempt
@@ -152,6 +152,9 @@ config PREEMPT_DYNAMIC
Interesting if you want the same pre-built kernel should be used for
both Server and Desktop workloads.
+config PREEMPT_COUNT_IRQFLAGS
+ bool
+
config SCHED_CORE
bool "Core Scheduling for SMT"
depends on SCHED_SMT
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_en
{
irqentry_state_t irq_state;
+ __preempt_count_inc_hardirqs_disable();
irq_state.lockdep = lockdep_hardirqs_enabled();
__nmi_enter();
@@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt
if (irq_state.lockdep)
lockdep_hardirqs_on(CALLER_ADDR0);
__nmi_exit();
+ __preempt_count_dec_hardirqs_disable();
}
On Fri, Aug 28 2026 at 00:52, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 12:41, Boqun Feng wrote:
> So I sat down and reverted
>
> 1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
> e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")
>
> and then hacked it up just to see how far I get before vanishing to bed.
>
> Three hours later it surprisingly booted right away into a full distro
> kernel and survived kernel builds and a few test cases. :)
/FACMEPALM
Yesterday night I was really surprised but too tired to think about it.
When I came around today to look at it again I was more than embarrassed
to figure out that the KVM script rebuilt the wrong branch over and
over. So the build numbers kept increasing...
Brown paperbag time ...
Of course the real thing did _NOT_ boot at all, so I sat down and
figured out what's going wrong and added a pile of debug to it, which is
sadly non-existing in this magic local_interrupt_dis/enable() code.
The overall fallout is moderate. Some of it are actual (but harmless)
bugs and the rest are the oddball cases we talked about before.
It builds and boots now for real, but of course your mileage will vary
depending on hardware and .config. Combo patch on top of the reverts is
below.
The whole pile can be retrieved from git via:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irqflags
I have some thoughts about how to deal with the overall disaster, but
that has to wait until my brain is truly awake again...
Thanks,
tglx
---
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index 15fd9ec5ecac..c7229a7cfa7c 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -133,6 +133,7 @@ config X86
select ARCH_USES_CFI_TRAPS if X86_64 && CFI
select ARCH_SUPPORTS_LTO_CLANG
select ARCH_SUPPORTS_LTO_CLANG_THIN
+ select ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
select ARCH_SUPPORTS_RT
select ARCH_USE_BUILTIN_BSWAP
select ARCH_USE_CMPXCHG_LOCKREF
diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
index dea60d66d976..34bdf24b939b 100644
--- a/arch/x86/include/asm/hardirq.h
+++ b/arch/x86/include/asm/hardirq.h
@@ -113,4 +113,6 @@ static __always_inline bool kvm_get_cpu_l1tf_flush_l1d(void)
static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
#endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
+#define __ARCH_IRQ_EXIT_IRQS_DISABLED 1
+
#endif /* _ASM_X86_HARDIRQ_H */
diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
index fafb6f8cdac3..8b4d4cbae52e 100644
--- a/arch/x86/include/asm/preempt.h
+++ b/arch/x86/include/asm/preempt.h
@@ -61,10 +61,20 @@ static __always_inline void preempt_count_set(unsigned long pc)
*/
#define init_task_preempt_count(p) do { } while (0)
-#define init_idle_preempt_count(p, cpu) do { \
- per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+
+#define init_idle_preempt_count(p, cpu) do { \
+ per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED | HARDIRQ_DISABLE_OFFSET; \
} while (0)
+#else
+
+#define init_idle_preempt_count(p, cpu) do { \
+ per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
+} while (0)
+
+#endif
+
/*
* We fold the NEED_RESCHED bit into the preempt count such that
* preempt_enable() can decrement and test for needing to reschedule with a
diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
index 6b0a5861ccb8..0b5a05cb543f 100644
--- a/arch/x86/kernel/kvm.c
+++ b/arch/x86/kernel/kvm.c
@@ -256,9 +256,9 @@ noinstr u32 kvm_read_and_reset_apf_flags(void)
{
u32 flags = 0;
- if (__this_cpu_read(async_pf_enabled)) {
- flags = __this_cpu_read(apf_reason.flags);
- __this_cpu_write(apf_reason.flags, 0);
+ if (raw_cpu_read(async_pf_enabled)) {
+ flags = raw_cpu_read(apf_reason.flags);
+ raw_cpu_write(apf_reason.flags, 0);
}
return flags;
diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
index 346c438ac880..b0be24a386f3 100644
--- a/arch/x86/kernel/process.c
+++ b/arch/x86/kernel/process.c
@@ -824,7 +824,7 @@ void __noreturn stop_this_cpu(void *dummy)
struct cpuinfo_x86 *c = this_cpu_ptr(&cpu_info);
unsigned int cpu = smp_processor_id();
- local_irq_disable();
+ raw_force_local_irq_disable();
/*
* Remove this CPU from the online mask and disable it
diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 0fed6d0d7e32..40104170be24 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c
@@ -98,7 +98,7 @@ static int __init set_efi_reboot(const struct dmi_system_id *d)
void __noreturn machine_real_restart(unsigned int type)
{
- local_irq_disable();
+ raw_force_local_irq_disable();
/*
* Write zero to CMOS register number 0x0f, which the BIOS POST
@@ -535,7 +535,7 @@ static inline void nmi_shootdown_cpus_on_restart(void);
#if IS_ENABLED(CONFIG_KVM_X86)
static void emergency_reboot_disable_virtualization(void)
{
- local_irq_disable();
+ raw_force_local_irq_disable();
/*
* Disable virtualization on all CPUs before rebooting to avoid hanging
@@ -699,7 +699,7 @@ void native_machine_shutdown(void)
* not receive the per-cpu timer interrupt which may trigger
* scheduler's load balance.
*/
- local_irq_disable();
+ raw_force_local_irq_disable();
stop_other_cpus();
#endif
@@ -823,7 +823,8 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
*/
if (cpu == crashing_cpu)
return NMI_HANDLED;
- local_irq_disable();
+
+ raw_force_local_irq_disable();
if (shootdown_callback)
shootdown_callback(cpu, regs);
@@ -865,7 +866,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
{
unsigned long msecs;
- local_irq_disable();
+ raw_force_local_irq_disable();
/*
* Avoid certain doom if a shootdown already occurred; re-registering
diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
index aa88370ce739..3164eab7cecd 100644
--- a/arch/x86/mm/fault.c
+++ b/arch/x86/mm/fault.c
@@ -1486,7 +1486,8 @@ handle_page_fault(struct pt_regs *regs, unsigned long error_code,
* page fault handling might have reenabled interrupts,
* make sure to disable them again.
*/
- local_irq_disable();
+ if (!irqs_disabled())
+ local_irq_disable();
}
DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
index 132a9df98471..f13e88fb7a78 100644
--- a/drivers/acpi/sleep.c
+++ b/drivers/acpi/sleep.c
@@ -1095,7 +1095,7 @@ static int acpi_power_off(struct sys_off_data *data)
{
/* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
pr_debug("%s called\n", __func__);
- local_irq_disable();
+ raw_force_local_irq_disable();
acpi_enter_sleep_state(ACPI_STATE_S5);
return NOTIFY_DONE;
}
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 3bf969ad8fe0..b54afbd6e073 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -594,13 +594,14 @@ struct softirq_action
asmlinkage void do_softirq(void);
asmlinkage void __do_softirq(void);
+void do_softirq_irqsoff(void);
#ifdef CONFIG_PREEMPT_RT
extern void do_softirq_post_smp_call_flush(unsigned int was_pending);
#else
static inline void do_softirq_post_smp_call_flush(unsigned int unused)
{
- do_softirq();
+ do_softirq_irqsoff();
}
#endif
diff --git a/include/linux/irq-entry-common.h b/include/linux/irq-entry-common.h
index 0bb6c03481fa..ba5210fab31f 100644
--- a/include/linux/irq-entry-common.h
+++ b/include/linux/irq-entry-common.h
@@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_eqs(void) { return false; }
*/
static __always_inline void enter_from_user_mode(struct pt_regs *regs)
{
+ __preempt_count_inc_hardirqs_disable();
arch_enter_from_user_mode(regs);
lockdep_hardirqs_off(CALLER_ADDR0);
@@ -275,6 +276,7 @@ static __always_inline void exit_to_user_mode(void)
user_enter_irqoff();
arch_exit_to_user_mode();
lockdep_hardirqs_on(CALLER_ADDR0);
+ __preempt_count_dec_hardirqs_disable();
}
/**
@@ -385,6 +387,8 @@ static __always_inline irqentry_state_t irqentry_enter_from_kernel_mode(struct p
.exit_rcu = false,
};
+ __preempt_count_inc_hardirqs_disable();
+
/*
* If this entry hit the idle task invoke ct_irq_enter() whether
* RCU is watching or not.
@@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, irqentry_state_
instrumentation_end();
ct_irq_exit();
lockdep_hardirqs_on(CALLER_ADDR0);
+ __preempt_count_dec_hardirqs_disable();
return;
}
@@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, irqentry_state_
if (state.exit_rcu)
ct_irq_exit();
}
+ __preempt_count_dec_hardirqs_disable();
}
/**
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index 57b074e0cfbb..dd55786768d1 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -13,6 +13,7 @@
#define _LINUX_TRACE_IRQFLAGS_H
#include <linux/irqflags_types.h>
+#include <linux/preempt.h>
#include <linux/typecheck.h>
#include <linux/cleanup.h>
#include <asm/irqflags.h>
@@ -163,33 +164,149 @@ extern void warn_bogus_irq_restore(void);
#endif
/*
- * Wrap the arch provided IRQ routines to provide appropriate checks.
+ * Wrap the architecture specific routines to provide appropriate checks.
*/
-#define raw_local_irq_disable() arch_local_irq_disable()
-#define raw_local_irq_enable() arch_local_irq_enable()
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+
+// FIXME: Convert this into a proper debug mechanism
+#define debug_assert(c) \
+do { \
+ WARN_ON(!(c)); \
+} while (0)
+
+static __always_inline void raw_local_irq_disable(void)
+{
+ debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == 0);
+ arch_local_irq_disable();
+ __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void raw_force_local_irq_disable(void)
+{
+ arch_local_irq_disable();
+ __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+ debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET);
+ __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+ unsigned int cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
+
+ debug_assert(cnt != HARDIRQ_DISABLE_MASK);
+
+ if (!cnt)
+ arch_local_irq_disable();
+ __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+
+ return cnt;
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long cnt)
+{
+ debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
+
+ if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+ return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
+{
+ return !!cnt;
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+ return preempt_count() & HARDIRQ_DISABLE_MASK;
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+ debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET);
+ __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+ arch_safe_halt();
+}
+
+#else
+
+static __always_inline void raw_local_irq_disable(void)
+{
+ arch_local_irq_disable();
+}
+
+static __always_inline void raw_force_local_irq_disable(void)
+{
+ arch_local_irq_disable();
+}
+
+static __always_inline void raw_local_irq_enable(void)
+{
+ arch_local_irq_enable();
+}
+
+static __always_inline unsigned long __raw_local_irq_save(void)
+{
+ return arch_local_irq_save();
+}
+
+static __always_inline void __raw_local_irq_restore(unsigned long flags)
+{
+ arch_local_irq_restore(flags);
+}
+
+static __always_inline unsigned long __raw_local_save_flags(void)
+{
+ return arch_local_save_flags();
+}
+
+static __always_inline bool __raw_irqs_disabled_flags(unsigned long flags)
+{
+ return arch_irqs_disabled_flags(flags);
+}
+
+static __always_inline bool raw_irqs_disabled(void)
+{
+ return arch_irqs_disabled();
+}
+
+static __always_inline void raw_safe_halt(void)
+{
+ arch_safe_halt();
+}
+
+#endif
+
#define raw_local_irq_save(flags) \
do { \
typecheck(unsigned long, flags); \
- flags = arch_local_irq_save(); \
+ flags = __raw_local_irq_save(); \
} while (0)
#define raw_local_irq_restore(flags) \
do { \
typecheck(unsigned long, flags); \
raw_check_bogus_irq_restore(); \
- arch_local_irq_restore(flags); \
+ __raw_local_irq_restore(flags); \
} while (0)
#define raw_local_save_flags(flags) \
do { \
typecheck(unsigned long, flags); \
- flags = arch_local_save_flags(); \
+ flags = __raw_local_save_flags(); \
} while (0)
#define raw_irqs_disabled_flags(flags) \
({ \
typecheck(unsigned long, flags); \
- arch_irqs_disabled_flags(flags); \
+ __raw_irqs_disabled_flags(flags); \
})
-#define raw_irqs_disabled() (arch_irqs_disabled())
-#define raw_safe_halt() arch_safe_halt()
/*
* The local_irq_*() APIs are equal to the raw_local_irq*()
diff --git a/include/linux/preempt.h b/include/linux/preempt.h
index 2e689de7b29a..c953cdfb3cc2 100644
--- a/include/linux/preempt.h
+++ b/include/linux/preempt.h
@@ -54,31 +54,31 @@
* NMI_MASK: 0xf0000000
* (PREEMPT_NEED_RESCHED is in a different word)
*/
-#define PREEMPT_BITS 8
-#define SOFTIRQ_BITS 8
+#define PREEMPT_BITS 8
+#define SOFTIRQ_BITS 8
#define HARDIRQ_DISABLE_BITS 8
-#define HARDIRQ_BITS 4
-#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
+#define HARDIRQ_BITS 4
+#define NMI_BITS (1 + 3 * IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
-#define PREEMPT_SHIFT 0
-#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
+#define PREEMPT_SHIFT 0
+#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
#define HARDIRQ_DISABLE_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
-#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
-#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
+#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
+#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
-#define __IRQ_MASK(x) ((1UL << (x))-1)
+#define __IRQ_MASK(x) ((1UL << (x))-1)
-#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
-#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
+#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
+#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
#define HARDIRQ_DISABLE_MASK (__IRQ_MASK(HARDIRQ_DISABLE_BITS) << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
-#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
+#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
+#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
-#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
-#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
+#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
+#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
#define HARDIRQ_DISABLE_OFFSET (1UL << HARDIRQ_DISABLE_SHIFT)
-#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
-#define NMI_OFFSET (1UL << NMI_SHIFT)
+#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
+#define NMI_OFFSET (1UL << NMI_SHIFT)
#define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
@@ -90,18 +90,29 @@
*
* Reset by start_kernel()->sched_init()->init_idle()->init_idle_preempt_count().
*/
+
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+
+#define INIT_PREEMPT_COUNT (PREEMPT_OFFSET + HARDIRQ_DISABLE_OFFSET)
+#define SCHED_PREEMPT_COUNT (2 * PREEMPT_DISABLE_OFFSET + HARDIRQ_DISABLE_OFFSET)
+
+#else
+
#define INIT_PREEMPT_COUNT PREEMPT_OFFSET
+#define SCHED_PREEMPT_COUNT (2 * PREEMPT_DISABLE_OFFSET)
+
+#endif
/*
* Initial preempt_count value; reflects the preempt_count schedule invariant
* which states that during context switches:
*
- * preempt_count() == 2*PREEMPT_DISABLE_OFFSET
+ * preempt_count() == SCHED_PREEMPT_COUNT
*
- * Note: PREEMPT_DISABLE_OFFSET is 0 for !PREEMPT_COUNT kernels.
+ * Note: SCHED_PREEMPT_COUNT is 0 for !PREEMPT_COUNT kernels.
* Note: See finish_task_switch().
*/
-#define FORK_PREEMPT_COUNT (2*PREEMPT_DISABLE_OFFSET + PREEMPT_ENABLED)
+#define FORK_PREEMPT_COUNT (SCHED_PREEMPT_COUNT + PREEMPT_ENABLED)
/* preempt_count() and related functions, depends on PREEMPT_NEED_RESCHED */
#include <asm/preempt.h>
@@ -168,6 +179,16 @@ static __always_inline unsigned char interrupt_context_level(void)
#define in_softirq() (softirq_count())
#define in_interrupt() (irq_count())
+/*
+ * Check whether a fault happened in an atomic context. Depending on
+ * CONFIG_PREEMPT_COUNT and CONFIG_PREEMPTION this check might be useless.
+ */
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+# define fault_in_atomic() (preempt_count() != HARDIRQ_DISABLE_OFFSET)
+#else
+# define fault_in_atomic() in_atomic()
+#endif
+
/*
* The preempt_count offset after preempt_disable();
*/
@@ -322,6 +343,21 @@ do { \
#endif /* CONFIG_PREEMPT_COUNT */
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
+static __always_inline void __preempt_count_inc_hardirqs_disable(void)
+{
+ __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
+}
+
+static __always_inline void __preempt_count_dec_hardirqs_disable(void)
+{
+ __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
+}
+#else
+static __always_inline void __preempt_count_inc_hardirqs_disable(void) { }
+static __always_inline void __preempt_count_dec_hardirqs_disable(void) { }
+#endif
+
#ifdef MODULE
/*
* Modules have no business playing preemption tricks.
diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
index eddbbb65ccc4..086de4c18575 100644
--- a/include/linux/uaccess.h
+++ b/include/linux/uaccess.h
@@ -296,9 +296,9 @@ static inline bool pagefault_disabled(void)
* stick to pagefault_disabled().
* Please NEVER use preempt_disable() to disable the fault handler. With
* !CONFIG_PREEMPT_COUNT, this is like a NOP. So the handler won't be disabled.
- * in_atomic() will report different values based on !CONFIG_PREEMPT_COUNT.
+ * fault_in_atomic() will report different values based on !CONFIG_PREEMPT_COUNT.
*/
-#define faulthandler_disabled() (pagefault_disabled() || in_atomic())
+#define faulthandler_disabled() (pagefault_disabled() || fault_in_atomic())
DEFINE_LOCK_GUARD_0(pagefault, pagefault_disable(), pagefault_enable())
diff --git a/init/main.c b/init/main.c
index 2613d3f9b3ce..fa84ce260b04 100644
--- a/init/main.c
+++ b/init/main.c
@@ -991,7 +991,6 @@ void start_kernel(void)
cgroup_init_early();
- local_irq_disable();
early_boot_irqs_disabled = true;
/*
diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
index f294dad43bd7..c44607990219 100644
--- a/kernel/Kconfig.preempt
+++ b/kernel/Kconfig.preempt
@@ -152,6 +152,15 @@ config PREEMPT_DYNAMIC
Interesting if you want the same pre-built kernel should be used for
both Server and Desktop workloads.
+config ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
+ bool
+
+config PREEMPT_COUNT_IRQFLAGS
+ bool "Enable reference counted interrupt disable/enable mechanisms"
+ depends on ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
+ help
+ FIXME: Add some useful blurb
+
config SCHED_CORE
bool "Core Scheduling for SMT"
depends on SCHED_SMT
diff --git a/kernel/entry/common.c b/kernel/entry/common.c
index e3d381fd3d25..3c93ce7f86f6 100644
--- a/kernel/entry/common.c
+++ b/kernel/entry/common.c
@@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs)
{
irqentry_state_t irq_state;
+ __preempt_count_inc_hardirqs_disable();
irq_state.lockdep = lockdep_hardirqs_enabled();
__nmi_enter();
@@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt_regs *regs, irqentry_state_t irq_state)
if (irq_state.lockdep)
lockdep_hardirqs_on(CALLER_ADDR0);
__nmi_exit();
+ __preempt_count_dec_hardirqs_disable();
}
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index f78275192036..b6d14feaaf56 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -5343,7 +5343,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
*
* Also, see FORK_PREEMPT_COUNT.
*/
- if (WARN_ONCE(preempt_count() != 2*PREEMPT_DISABLE_OFFSET,
+ if (WARN_ONCE(preempt_count() != SCHED_PREEMPT_COUNT,
"corrupted preempt_count: %s/%d/0x%x\n",
current->comm, current->pid, preempt_count()))
preempt_count_set(FORK_PREEMPT_COUNT);
diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
index eb73b65ce6c4..7132320035eb 100644
--- a/kernel/sched/idle.c
+++ b/kernel/sched/idle.c
@@ -380,7 +380,8 @@ static void do_idle(void)
* RCU relies on this call to be done outside of an RCU read-side
* critical section.
*/
- flush_smp_call_function_queue();
+ scoped_guard(irq)
+ flush_smp_call_function_queue();
schedule_idle();
if (unlikely(klp_patch_pending(current)))
diff --git a/kernel/smp.c b/kernel/smp.c
index b696bcc60c08..d51e4bc8da1b 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -665,19 +665,17 @@ static void __flush_smp_call_function_queue(bool warn_cpu_offline)
void flush_smp_call_function_queue(void)
{
unsigned int was_pending;
- unsigned long flags;
if (llist_empty(this_cpu_ptr(&call_single_queue)))
return;
- local_irq_save(flags);
+ lockdep_assert_irqs_disabled();
+
/* Get the already pending soft interrupts for RT enabled kernels */
was_pending = local_softirq_pending();
__flush_smp_call_function_queue(true);
if (local_softirq_pending())
do_softirq_post_smp_call_flush(was_pending);
-
- local_irq_restore(flags);
}
static int __smp_call_function_single(int cpu, smp_call_func_t func,
diff --git a/kernel/softirq.c b/kernel/softirq.c
index e1a773e3eb4e..33f82d03ca2b 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -455,7 +455,10 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
* Run softirq if any pending. And do it in its own stack
* as we may be calling this deep in a task call stack already.
*/
- do_softirq();
+ if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS))
+ do_softirq_irqsoff();
+ else
+ do_softirq();
}
preempt_count_dec();
@@ -517,20 +520,21 @@ static inline void invoke_softirq(void)
asmlinkage __visible void do_softirq(void)
{
- __u32 pending;
- unsigned long flags;
-
if (in_interrupt())
return;
- local_irq_save(flags);
+ guard(irqsave)();
+ if (local_softirq_pending())
+ do_softirq_own_stack();
+}
- pending = local_softirq_pending();
+void do_softirq_irqsoff(void)
+{
+ if (in_interrupt())
+ return;
- if (pending)
+ if (local_softirq_pending())
do_softirq_own_stack();
-
- local_irq_restore(flags);
}
#endif /* !CONFIG_PREEMPT_RT */
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 530d61257b9a..977dd8928934 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -2068,7 +2068,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
lockdep_hrtimer_exit(expires_in_hardirq);
trace_hrtimer_expire_exit(timer);
- raw_spin_lock_irq(&cpu_base->lock);
+ raw_spin_lock_irqsave(&cpu_base->lock, flags);
/*
* Note: We clear the running state after enqueue_hrtimer and
On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
[...]
> +static __always_inline unsigned long __raw_local_irq_save(void)
> +{
> + unsigned int cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
> +
> + debug_assert(cnt != HARDIRQ_DISABLE_MASK);
> +
> + if (!cnt)
> + arch_local_irq_disable();
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +
> + return cnt;
> +}
> +
> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
> +
> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> + arch_local_irq_enable();
> +}
And while we are at, we can just introduce a
raw_local_irq_restore_auto() (definitely needs a better name), which
doesn't need a cnt:
static __always_inline void __raw_local_irq_restore_auto(void)
{
debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
arch_local_irq_enable();
}
And we can slowly convert irq_restore() users to use it?
Regards,
Boqun
> +
> +static __always_inline unsigned long __raw_local_save_flags(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
> +{
> + return !!cnt;
> +}
> +
> +static __always_inline bool raw_irqs_disabled(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline void raw_safe_halt(void)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET);
> + __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> + arch_safe_halt();
> +}
> +
> +#else
[..]
On Sat, Aug 29 2026 at 16:37, Boqun Feng wrote:
> On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
>> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
>> +{
>> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
>> +
>> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
>> + arch_local_irq_enable();
>> +}
>
> And while we are at, we can just introduce a
> raw_local_irq_restore_auto() (definitely needs a better name), which
> doesn't need a cnt:
>
> static __always_inline void __raw_local_irq_restore_auto(void)
> {
> debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
>
> if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> arch_local_irq_enable();
> }
Yes, we can add something like this once we got the design and the debug
infrastructure in place. For now the count is helpful to debug stuff and
we want to have something equivalent at least for lockdep builds.
Thanks,
tglx
On Sat, Aug 29, 2026 at 04:37:37PM -0700, Boqun Feng wrote:
> On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
> [...]
> > +static __always_inline unsigned long __raw_local_irq_save(void)
> > +{
> > + unsigned int cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
> > +
> > + debug_assert(cnt != HARDIRQ_DISABLE_MASK);
> > +
> > + if (!cnt)
> > + arch_local_irq_disable();
> > + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> > +
> > + return cnt;
> > +}
> > +
> > +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
> > +{
> > + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
> > +
> > + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> > + arch_local_irq_enable();
> > +}
>
> And while we are at, we can just introduce a
> raw_local_irq_restore_auto() (definitely needs a better name), which
> doesn't need a cnt:
>
> static __always_inline void __raw_local_irq_restore_auto(void)
> {
> debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
>
> if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> arch_local_irq_enable();
> }
>
> And we can slowly convert irq_restore() users to use it?
>
I decided to use local_irq_resume(), not sure whether it's a good name
either...
If we are OK to not fully revert on commit e901c1510e24 ("irq,spin_lock:
Add counted interrupt disabling/enabling"), I think the following will
resolve the 0day built errors on your irqflags branch (I rebased onto
tip/locking/urgent with rest of your series on irqflags branch). A few
things to notice:
* local_interrupt_*() can obviously be removed at all, this patch is
just for the idea, so I didn't do that. Similarly I didn't introduce
a spin_lock_irqresume().
* I haven't found a way that we can do a local_irq_resume() when
!CONFIG_PREEMPT_COUNT_IRQFLAGS, and if we cannot, it's going to make
part of Rust code depends on CONFIG_PREEMPT_COUNT_IRQFLAGS=y
* Further cleanups on hardirq_disable_*() are needed.
Thoughts?
Regards,
Boqun
------------------------------>8
Subject: [PATCH] interrupt: Add {raw_}local_irq_resume()
With the new PREEMPT_COUNT_IRQFLAGS design,
local_interrupt_{dis,en}able() can be implemented by the same machinery
in local_irq_{save,restore}(). Add a new API local_irq_resume() to skip
the need of passing a previous flags/count to local_irq_restore(). Map
local_interrupt_{dis,en}able() to local_irq{save,resume}().
Signed-off-by: Boqun Feng <boqun@kernel.org>
---
include/linux/interrupt_rc.h | 79 ----------------------------
include/linux/irqflags.h | 39 ++++++++++++++
include/linux/spinlock.h | 1 -
kernel/irq/refcount_interrupt_test.c | 2 +-
kernel/softirq.c | 15 ------
5 files changed, 40 insertions(+), 96 deletions(-)
delete mode 100644 include/linux/interrupt_rc.h
diff --git a/include/linux/interrupt_rc.h b/include/linux/interrupt_rc.h
deleted file mode 100644
index e68e1bedba66..000000000000
--- a/include/linux/interrupt_rc.h
+++ /dev/null
@@ -1,79 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __LINUX_INTERRUPT_RC_H
-#define __LINUX_INTERRUPT_RC_H
-
-/*
- * include/linux/interrupt_rc.h - refcounted local processor interrupt
- * management.
- *
- * Since the implementation of this API currently depends on
- * local_irq_save()/local_irq_restore(), we split this into its own header to
- * make it easier to include without hitting circular header dependencies.
- */
-
-#include <linux/irqflags.h>
-#include <linux/preempt.h>
-#include <linux/processor.h>
-#include <linux/smp.h>
-
-#ifndef MODULE
-/* Per-CPU interrupt disabling state for local_interrupt_{disable,enable}(). */
-DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
-
-static __always_inline void __local_interrupt_save_state(unsigned long flags)
-{
- raw_cpu_write(local_interrupt_disable_state, flags);
-}
-
-static __always_inline void __local_interrupt_enable(void)
-{
- unsigned long flags = raw_cpu_read(local_interrupt_disable_state);
-
- local_irq_restore(flags);
-}
-
-#ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
-static __always_inline void _local_interrupt_save_state(unsigned long flags)
-{
- __local_interrupt_save_state(flags);
-}
-
-static __always_inline void _local_interrupt_enable(void)
-{
- __local_interrupt_enable();
-}
-#else
-extern void _local_interrupt_save_state(unsigned long flags);
-extern void _local_interrupt_enable(void);
-#endif
-
-#else /* !MODULE */
-extern void _local_interrupt_save_state(unsigned long flags);
-extern void _local_interrupt_enable(void);
-#endif /* !MODULE */
-
-static inline void local_interrupt_disable(void)
-{
- int new_count;
- unsigned long flags;
-
- WARN_ON_ONCE(in_nmi());
-
- local_irq_save(flags);
- new_count = hardirq_disable_enter();
-
- if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
- _local_interrupt_save_state(flags);
-}
-
-static inline void local_interrupt_enable(void)
-{
- int new_count;
-
- new_count = hardirq_disable_exit();
-
- if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
- _local_interrupt_enable();
-}
-
-#endif /* !__LINUX_INTERRUPT_RC_H */
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index dd55786768d1..6d4e5f1c73a8 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -215,6 +215,15 @@ static __always_inline void __raw_local_irq_restore(unsigned long cnt)
arch_local_irq_enable();
}
+/* Same as raw_local_irq_restore() but don't need user to pass a count. */
+static __always_inline void raw_local_irq_resume(void)
+{
+ debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
+
+ if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
+ arch_local_irq_enable();
+}
+
static __always_inline unsigned long __raw_local_save_flags(void)
{
return preempt_count() & HARDIRQ_DISABLE_MASK;
@@ -264,6 +273,15 @@ static __always_inline void __raw_local_irq_restore(unsigned long flags)
arch_local_irq_restore(flags);
}
+static __always_inline void raw_local_irq_resume(void)
+{
+ /*
+ * local_irq_resume() is not supported when
+ * CONFIG_PREEMPT_COUNT_IRQFLAGS = n
+ */
+ BUG();
+}
+
static __always_inline unsigned long __raw_local_save_flags(void)
{
return arch_local_save_flags();
@@ -342,6 +360,14 @@ static __always_inline void raw_safe_halt(void)
raw_local_irq_restore(flags); \
} while (0)
+#define local_irq_resume() \
+ do { \
+ if ((preempt_count() & HARDIRQ_DISABLE_MASK) == \
+ HARDIRQ_DISABLE_OFFSET) \
+ trace_hardirqs_on(); \
+ raw_local_irq_resume(); \
+ } while (0)
+
#define safe_halt() \
do { \
trace_hardirqs_on(); \
@@ -355,10 +381,23 @@ static __always_inline void raw_safe_halt(void)
#define local_irq_disable() do { raw_local_irq_disable(); } while (0)
#define local_irq_save(flags) do { raw_local_irq_save(flags); } while (0)
#define local_irq_restore(flags) do { raw_local_irq_restore(flags); } while (0)
+#define local_irq_resume() do { raw_local_irq_resume(); } while (0)
#define safe_halt() do { raw_safe_halt(); } while (0)
#endif /* CONFIG_TRACE_IRQFLAGS */
+static __always_inline void local_interrupt_disable(void)
+{
+ unsigned long flags;
+
+ local_irq_save(flags);
+}
+
+static __always_inline void local_interrupt_enable(void)
+{
+ local_irq_resume();
+}
+
#define local_save_flags(flags) raw_local_save_flags(flags)
/*
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 3d405cc4c121..c619502501e2 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -57,7 +57,6 @@
#include <linux/linkage.h>
#include <linux/compiler.h>
#include <linux/irqflags.h>
-#include <linux/interrupt_rc.h>
#include <linux/thread_info.h>
#include <linux/stringify.h>
#include <linux/bottom_half.h>
diff --git a/kernel/irq/refcount_interrupt_test.c b/kernel/irq/refcount_interrupt_test.c
index ca904dba24b9..cbd3b5b9cfa3 100644
--- a/kernel/irq/refcount_interrupt_test.c
+++ b/kernel/irq/refcount_interrupt_test.c
@@ -4,7 +4,7 @@
*/
#include <kunit/test.h>
-#include <linux/interrupt_rc.h>
+#include <linux/irqflags.h>
#define TEST_IRQ_ON() KUNIT_EXPECT_FALSE(test, irqs_disabled())
#define TEST_IRQ_OFF() KUNIT_EXPECT_TRUE(test, irqs_disabled())
diff --git a/kernel/softirq.c b/kernel/softirq.c
index d124ae6fd1e4..47fb46f9d6f6 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -9,7 +9,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
#include <linux/export.h>
#include <linux/kernel_stat.h>
#include <linux/interrupt.h>
@@ -89,20 +88,6 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabled);
EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
#endif
-DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
-
-void _local_interrupt_save_state(unsigned long flags)
-{
- __local_interrupt_save_state(flags);
-}
-EXPORT_SYMBOL(_local_interrupt_save_state);
-
-void _local_interrupt_enable(void)
-{
- __local_interrupt_enable();
-}
-EXPORT_SYMBOL(_local_interrupt_enable);
-
#ifndef CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS
/*
* Any 32bit architecture that still cares about performance should
--
2.50.1 (Apple Git-155)
On Sun, Aug 30 2026 at 08:18, Boqun Feng wrote:
> On Sat, Aug 29, 2026 at 04:37:37PM -0700, Boqun Feng wrote:
>> And while we are at, we can just introduce a
>> raw_local_irq_restore_auto() (definitely needs a better name), which
>> doesn't need a cnt:
>>
>> static __always_inline void __raw_local_irq_restore_auto(void)
>> {
>> debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
>>
>> if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
>> arch_local_irq_enable();
>> }
Can we just make the thing work in the first place?
>> And we can slowly convert irq_restore() users to use it?
Somewhere down the road.
> I decided to use local_irq_resume(), not sure whether it's a good name
> either...
>
> If we are OK to not fully revert on commit e901c1510e24 ("irq,spin_lock:
> Add counted interrupt disabling/enabling"), I think the following will
> resolve the 0day built errors on your irqflags branch (I rebased onto
> tip/locking/urgent with rest of your series on irqflags branch). A few
> things to notice:
The zero day failure is not due to that. It's because I reverted that
refcounted muck as well in my git tree.
I just rebased the series on top of tip locking/urgent, which only has
the irq,spinlock revert and your patches on top.
> * I haven't found a way that we can do a local_irq_resume() when
> !CONFIG_PREEMPT_COUNT_IRQFLAGS, and if we cannot, it's going to make
> part of Rust code depends on CONFIG_PREEMPT_COUNT_IRQFLAGS=y
That's a good thing as it might make people actually get their act
together. And you can work around that in interrupt_rc.h itself.
Can you please stop bouncing around like a rubber ball and take your
time?
First of all I want to move interrupt_rc.h and the whole spinlock muck
into rust/helpers/ now. Why?
Simply because it is Rust only and we don't want to expose any of this
stuff to random driver writers. See attached patch.
I've rebased my devel branch on top of tip/locking/urgent and applied
that patch so the robots can have their field day.
The actual PREEMPT_COUNT_IRQFLAGS thing will be 7.4 material obviously
and as this is confined to Rust then it's trivial enough to work around
it locally without exposing more stuff. See tiny delta patch below.
So the only side effect of that is that the Rust implementation will not
be fully integrated into the preempt count magic, but it should just
work, no?
And when an architecture supports the real thing then it gets all the
benefits with bells and whistels.
I really want to get the PREEMPT_COUNT_IRQFLAGS design right first and
then we can think about simplifications and cleanups and remove the
whole Rust magic once all architectures which support Rust play along.
Thanks,
tglx
---
rust/helpers/interrupt_rc.h | 6 ++++++
1 file changed, 6 insertions(+)
--- a/rust/helpers/interrupt_rc.h
+++ b/rust/helpers/interrupt_rc.h
@@ -38,8 +38,14 @@ extern void _local_interrupt_save_state(
extern void _local_interrupt_enable(void);
#endif
+#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
#define hardirq_disable_enter() __preempt_count_add_return(HARDIRQ_DISABLE_OFFSET)
#define hardirq_disable_exit() __preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET)
+#else
+DECLARE_PER_CPU(unsigned int, local_interrupt_cnt);
+#define hardirq_disable_enter() __this_cpu_add_return(local_interrupt_count, HARDIRQ_DISABLE_OFFSET)
+#define hardirq_disable_exit() __this_cpu_sub_return(local_interrupt_count, HARDIRQ_DISABLE_OFFSET)
+#endif
static inline void local_interrupt_disable(void)
{
On Sun, Aug 30, 2026 at 09:57:30PM +0200, Thomas Gleixner wrote:
[...]
> +
> +#ifdef CONFIG_RT
0 day probably will find it, but FWIW this one should be
CONFIG_PREEMPT_RT.
Regards,
Boqun
> +static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
> + __acquires(lock)
> +{
> + rt_spin_lock(lock);
> +}
> +
> +static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
> + __releases(lock)
> +{
> + rt_spin_unlock(lock);
> +}
> +
> +static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
> + __cond_acquires(true, lock)
> +{
> + return rt_spin_trylock(lock);
> +}
> +
> +#else /* CONFIG_RT */
> +
[...]
On Sun, Aug 30 2026 at 14:42, Boqun Feng wrote: > On Sun, Aug 30, 2026 at 09:57:30PM +0200, Thomas Gleixner wrote: > [...] >> + >> +#ifdef CONFIG_RT > > 0 day probably will find it, but FWIW this one should be > CONFIG_PREEMPT_RT. Indeed
On Sun, Aug 30, 2026 at 09:57:30PM +0200, Thomas Gleixner wrote:
> On Sun, Aug 30 2026 at 08:18, Boqun Feng wrote:
> > On Sat, Aug 29, 2026 at 04:37:37PM -0700, Boqun Feng wrote:
> >> And while we are at, we can just introduce a
> >> raw_local_irq_restore_auto() (definitely needs a better name), which
> >> doesn't need a cnt:
> >>
> >> static __always_inline void __raw_local_irq_restore_auto(void)
> >> {
> >> debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK));
> >>
> >> if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> >> arch_local_irq_enable();
> >> }
>
> Can we just make the thing work in the first place?
>
Yes, for sure.
> >> And we can slowly convert irq_restore() users to use it?
>
> Somewhere down the road.
>
> > I decided to use local_irq_resume(), not sure whether it's a good name
> > either...
> >
> > If we are OK to not fully revert on commit e901c1510e24 ("irq,spin_lock:
> > Add counted interrupt disabling/enabling"), I think the following will
> > resolve the 0day built errors on your irqflags branch (I rebased onto
> > tip/locking/urgent with rest of your series on irqflags branch). A few
> > things to notice:
>
> The zero day failure is not due to that. It's because I reverted that
> refcounted muck as well in my git tree.
>
Right, that's why I said "if we are OK to not fully revert commit
e901c1510e24 ...", but your movestuff.patch should works as well.
> I just rebased the series on top of tip locking/urgent, which only has
> the irq,spinlock revert and your patches on top.
>
> > * I haven't found a way that we can do a local_irq_resume() when
> > !CONFIG_PREEMPT_COUNT_IRQFLAGS, and if we cannot, it's going to make
> > part of Rust code depends on CONFIG_PREEMPT_COUNT_IRQFLAGS=y
>
> That's a good thing as it might make people actually get their act
> together. And you can work around that in interrupt_rc.h itself.
>
> Can you please stop bouncing around like a rubber ball and take your
> time?
>
Apologies. I was just trying to help (and explore myself) the
integration of PREEMPT_COUNT_IRQFLAGS with local_interrupt_disable(),
it's merely some food for thoughts...
> First of all I want to move interrupt_rc.h and the whole spinlock muck
> into rust/helpers/ now. Why?
>
> Simply because it is Rust only and we don't want to expose any of this
> stuff to random driver writers. See attached patch.
>
Sure. I will give the movestuff.patch some test.
> I've rebased my devel branch on top of tip/locking/urgent and applied
> that patch so the robots can have their field day.
>
> The actual PREEMPT_COUNT_IRQFLAGS thing will be 7.4 material obviously
> and as this is confined to Rust then it's trivial enough to work around
> it locally without exposing more stuff. See tiny delta patch below.
>
> So the only side effect of that is that the Rust implementation will not
> be fully integrated into the preempt count magic, but it should just
> work, no?
>
Right, that works. It's similar to the "alternatively" approach I
mentioned here [1].
> And when an architecture supports the real thing then it gets all the
> benefits with bells and whistels.
>
> I really want to get the PREEMPT_COUNT_IRQFLAGS design right first and
> then we can think about simplifications and cleanups and remove the
> whole Rust magic once all architectures which support Rust play along.
>
Yeah, that's a better plan.
[1]: https://lore.kernel.org/lkml/apCS3T63WUxHd1GH@tardis.local/
Regards,
Boqun
> Thanks,
>
> tglx
>
> ---
[...]
On Sun, Aug 30 2026 at 14:23, Boqun Feng wrote:
> On Sun, Aug 30, 2026 at 09:57:30PM +0200, Thomas Gleixner wrote:
>> The actual PREEMPT_COUNT_IRQFLAGS thing will be 7.4 material obviously
>> and as this is confined to Rust then it's trivial enough to work around
>> it locally without exposing more stuff. See tiny delta patch below.
>>
>> So the only side effect of that is that the Rust implementation will not
>> be fully integrated into the preempt count magic, but it should just
>> work, no?
>>
>
> Right, that works. It's similar to the "alternatively" approach I
> mentioned here [1].
But thinking more about it. It actually just works with the preempt
count bits independent of PREEMPT_COUNT_IRQFLAGS.
For PREEMPT_COUNT_IRQFLAGS=n, Rust is the only one using it.
For PREEMPT_COUNT_IRQFLAGS=y, the Rust part integrates with the core
implementation. And once all Rust supporting architectures are enabling
PREEMPT_COUNT_IRQFLAGS the Rust extra magic goes away.
No?
Thanks,
tglx
On Mon, Aug 31, 2026 at 12:02:50PM +0200, Thomas Gleixner wrote:
> On Sun, Aug 30 2026 at 14:23, Boqun Feng wrote:
> > On Sun, Aug 30, 2026 at 09:57:30PM +0200, Thomas Gleixner wrote:
> >> The actual PREEMPT_COUNT_IRQFLAGS thing will be 7.4 material obviously
> >> and as this is confined to Rust then it's trivial enough to work around
> >> it locally without exposing more stuff. See tiny delta patch below.
> >>
> >> So the only side effect of that is that the Rust implementation will not
> >> be fully integrated into the preempt count magic, but it should just
> >> work, no?
> >>
> >
> > Right, that works. It's similar to the "alternatively" approach I
> > mentioned here [1].
>
> But thinking more about it. It actually just works with the preempt
> count bits independent of PREEMPT_COUNT_IRQFLAGS.
>
> For PREEMPT_COUNT_IRQFLAGS=n, Rust is the only one using it.
>
> For PREEMPT_COUNT_IRQFLAGS=y, the Rust part integrates with the core
> implementation. And once all Rust supporting architectures are enabling
> PREEMPT_COUNT_IRQFLAGS the Rust extra magic goes away.
>
> No?
>
Right, that's why I thought fully revert on commit e901c1510e24 might
not be needed.
To me, the only "magic" part when PREEMPT_COUNT_IRQFLAGS=n are 1)
preempt_count() is inconsistent on HARDIRQ_DISABLE_MASK about interrupt
disabling as you pointed out earlier and 2)
local_interrupt_{en,dis}able() has to save the current irq disabling
state/flag to work with local_irq_*(). PREEMPT_COUNT_IRQFLAGS=y resolves
both of them.
And if PREEMPT_COUNT_IRQFLAGS=y is the future (i.e. it'll be always y),
then we will likely have local_interrupt_{en,dis}able() (or a different
name) as a general API for everyone. So the API (and its semantics) is
not Rust-specific considering the future direction. Hence previously I
said that we can move them to Rust only but seems a bit unnecessary to
me.
Hope this makes sense.
Regards,
Boqun
> Thanks,
>
> tglx
>
>
> On Mon, Aug 31, 2026 at 12:02:50PM +0200, Thomas Gleixner wrote:
> Right, that's why I thought fully revert on commit e901c1510e24 might
> not be needed.
It's gone already and as I told you before the reason is that the issues
were not restricted to the ordering parts v.s. count/hardware and the
fallout in the softirq code. The whole issue with nested unlock/lock
inside a guard are not solved by reordering local_interrupt_disable().
Not to talk about the lack of proper debug features for it.
> And if PREEMPT_COUNT_IRQFLAGS=y is the future (i.e. it'll be always y),
> then we will likely have local_interrupt_{en,dis}able() (or a different
> name) as a general API for everyone. So the API (and its semantics) is
> not Rust-specific considering the future direction. Hence previously I
> said that we can move them to Rust only but seems a bit unnecessary to
> me.
No. We need a proper strategy to pull that off and not exposing the
functionality and the name right now outside of Rust makes that way
simpler. Changing Rust is one thing, chasing down a pile of random use
cases which crept in _before_ the design and strategy is settled is a
completely different story.
Thanks,
tglx
On Tue, Sep 01, 2026 at 03:43:47PM +0200, Thomas Gleixner wrote:
> > On Mon, Aug 31, 2026 at 12:02:50PM +0200, Thomas Gleixner wrote:
> > Right, that's why I thought fully revert on commit e901c1510e24 might
> > not be needed.
>
> It's gone already and as I told you before the reason is that the issues
> were not restricted to the ordering parts v.s. count/hardware and the
> fallout in the softirq code. The whole issue with nested unlock/lock
> inside a guard are not solved by reordering local_interrupt_disable().
> Not to talk about the lack of proper debug features for it.
>
> > And if PREEMPT_COUNT_IRQFLAGS=y is the future (i.e. it'll be always y),
> > then we will likely have local_interrupt_{en,dis}able() (or a different
> > name) as a general API for everyone. So the API (and its semantics) is
> > not Rust-specific considering the future direction. Hence previously I
> > said that we can move them to Rust only but seems a bit unnecessary to
> > me.
>
> No. We need a proper strategy to pull that off and not exposing the
> functionality and the name right now outside of Rust makes that way
> simpler. Changing Rust is one thing, chasing down a pile of random use
> cases which crept in _before_ the design and strategy is settled is a
> completely different story.
>
Fair enough.
Not trying to keep interrupt_{en,dis}able() from moving, but after some
thoughts, I think I figured out a few debugs we can add for
PREEMPT_COUNT_IRQFLAGS=n case, things we want to avoid:
* interrupt_disable(); irq_disable(); irq_enable(); interrupt_enable();
* interrupt_disable(); irq_enable(); irq_disable(); interrupt_enable();
* irq_save(flags); interrupt_disable(); irq_restore(flags); interrupt_enable();
on top of your current work, we can do the following when
PREEMPT_COUNT_IRQFLAGS=n. It'll help find a few random use cases that
break. (even when interrupt_disable() are Rust-only, Rust code can still
call a function which does irq_enable(); irq_disable(); while in a
interrupt_disable() critical section, so it makes sense to catch them).
Thoughts?
Regards,
Boqun
---------------------->8
diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
index dd55786768d1..a41d188dceef 100644
--- a/include/linux/irqflags.h
+++ b/include/linux/irqflags.h
@@ -241,6 +241,14 @@ static __always_inline void raw_safe_halt(void)
static __always_inline void raw_local_irq_disable(void)
{
+ /*
+ * Assuming local_irq_{en,dis}able() always paired, then
+ * local_irq_disable() should not be used inside an
+ * local_interrupt_disable() critical section. Because the paired
+ * local_irq_enable() would enable the interrupt inside a
+ * local_interrupt_disable() critical section.
+ */
+ debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK));
arch_local_irq_disable();
}
@@ -251,6 +259,12 @@ static __always_inline void raw_force_local_irq_disable(void)
static __always_inline void raw_local_irq_enable(void)
{
+ /*
+ * local_irq_enable() should not be called inside a
+ * local_interrupt_disable() critical section, but it would enable the
+ * interrupt unexpectedly.
+ */
+ debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK));
arch_local_irq_enable();
}
@@ -261,6 +275,12 @@ static __always_inline unsigned long __raw_local_irq_save(void)
static __always_inline void __raw_local_irq_restore(unsigned long flags)
{
+ /*
+ * local_irq_restore() should not enable interrupt unexpectedly inside
+ * a local_interrupt_disable() critical section
+ */
+ debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK) ||
+ arch_irqs_disabled_flags(flags));
arch_local_irq_restore(flags);
}
On Tue, Sep 01 2026 at 08:13, Boqun Feng wrote:
> On Tue, Sep 01, 2026 at 03:43:47PM +0200, Thomas Gleixner wrote:
> static __always_inline void raw_local_irq_disable(void)
> {
> + /*
> + * Assuming local_irq_{en,dis}able() always paired, then
> + * local_irq_disable() should not be used inside an
> + * local_interrupt_disable() critical section. Because the paired
> + * local_irq_enable() would enable the interrupt inside a
> + * local_interrupt_disable() critical section.
> + */
> + debug_assert(!(preempt_count() & HARDIRQ_DISABLE_MASK));
> arch_local_irq_disable();
The problem with pure debug_assert()s is that the damage is already
done. I learned that the hard way when I was chasing the last issue in
the #UD handler of x86 that this starts to recurse up to the point where
the system falls apart.
The modified version of this debug stuff in my devel branch does actual
fixups to prevent the subsequent damage. It's a hack and I did not come
around yet to make it actually less horrible.
After reverting the spinlock conversion and a lengthy discussion it's the
best to confine the reference counted interrupt disable/enable mechanism to
Rust which is the only user.
This should become the new norm, but that needs more thoughts and cleaning
up the confined usage in Rust at some point is way simpler than chasing
random places which adopt it in the meanwhile.
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
include/linux/interrupt_rc.h | 82 -------------------------
include/linux/spinlock.h | 23 -------
include/linux/spinlock_api_smp.h | 41 ------------
include/linux/spinlock_api_up.h | 15 ----
include/linux/spinlock_rt.h | 18 -----
kernel/irq/refcount_interrupt_test.c | 2
kernel/locking/spinlock.c | 31 ---------
kernel/softirq.c | 15 ----
rust/helpers/interrupt.c | 21 ++++++
rust/helpers/interrupt_rc.h | 68 ++++++++++++++++++++
rust/helpers/spinlock.c | 39 +++++++++++
rust/helpers/spinlock.h | 114 +++++++++++++++++++++++++++++++++++
12 files changed, 241 insertions(+), 228 deletions(-)
--- a/include/linux/interrupt_rc.h
+++ /dev/null
@@ -1,82 +0,0 @@
-/* SPDX-License-Identifier: GPL-2.0 */
-#ifndef __LINUX_INTERRUPT_RC_H
-#define __LINUX_INTERRUPT_RC_H
-
-/*
- * include/linux/interrupt_rc.h - refcounted local processor interrupt
- * management.
- *
- * Since the implementation of this API currently depends on
- * local_irq_save()/local_irq_restore(), we split this into its own header to
- * make it easier to include without hitting circular header dependencies.
- */
-
-#include <linux/irqflags.h>
-#include <linux/preempt.h>
-#include <linux/processor.h>
-#include <linux/smp.h>
-
-#ifndef MODULE
-/* Per-CPU interrupt disabling state for local_interrupt_{disable,enable}(). */
-DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
-
-static __always_inline void __local_interrupt_disable(void)
-{
- unsigned long flags;
-
- local_irq_save(flags);
- raw_cpu_write(local_interrupt_disable_state, flags);
-}
-
-static __always_inline void __local_interrupt_enable(void)
-{
- unsigned long flags = raw_cpu_read(local_interrupt_disable_state);
-
- local_irq_restore(flags);
-}
-
-#ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
-static __always_inline void _local_interrupt_disable(void)
-{
- __local_interrupt_disable();
-}
-
-static __always_inline void _local_interrupt_enable(void)
-{
- __local_interrupt_enable();
-}
-#else
-extern void _local_interrupt_disable(void);
-extern void _local_interrupt_enable(void);
-#endif
-
-#else /* !MODULE */
-extern void _local_interrupt_disable(void);
-extern void _local_interrupt_enable(void);
-#endif /* !MODULE */
-
-static inline void local_interrupt_disable(void)
-{
- int new_count;
-
- WARN_ON_ONCE(in_nmi());
-
- new_count = hardirq_disable_enter();
-
- /* Interrupts can happen here, but it's OK, see __irq_exit_rcu(). */
-
- if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
- _local_interrupt_disable();
-}
-
-static inline void local_interrupt_enable(void)
-{
- int new_count;
-
- new_count = hardirq_disable_exit();
-
- if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
- _local_interrupt_enable();
-}
-
-#endif /* !__LINUX_INTERRUPT_RC_H */
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -57,7 +57,6 @@
#include <linux/linkage.h>
#include <linux/compiler.h>
#include <linux/irqflags.h>
-#include <linux/interrupt_rc.h>
#include <linux/thread_info.h>
#include <linux/stringify.h>
#include <linux/bottom_half.h>
@@ -274,11 +273,9 @@ static inline void do_raw_spin_unlock(ra
#endif
#define raw_spin_lock_irq(lock) _raw_spin_lock_irq(lock)
-#define raw_spin_lock_irq_disable(lock) _raw_spin_lock_irq_disable(lock)
#define raw_spin_lock_bh(lock) _raw_spin_lock_bh(lock)
#define raw_spin_unlock(lock) _raw_spin_unlock(lock)
#define raw_spin_unlock_irq(lock) _raw_spin_unlock_irq(lock)
-#define raw_spin_unlock_irq_enable(lock) _raw_spin_unlock_irq_enable(lock)
#define raw_spin_unlock_irqrestore(lock, flags) \
do { \
@@ -293,8 +290,6 @@ static inline void do_raw_spin_unlock(ra
#define raw_spin_trylock_irqsave(lock, flags) _raw_spin_trylock_irqsave(lock, &(flags))
-#define raw_spin_trylock_irq_disable(lock) _raw_spin_trylock_irq_disable(lock)
-
#ifndef CONFIG_PREEMPT_RT
/* Include rwlock functions for !RT */
#include <linux/rwlock.h>
@@ -377,12 +372,6 @@ static __always_inline void spin_lock_ir
raw_spin_lock_irq(&lock->rlock);
}
-static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
- __acquires(lock) __no_context_analysis
-{
- raw_spin_lock_irq_disable(&lock->rlock);
-}
-
#define spin_lock_irqsave(lock, flags) \
do { \
raw_spin_lock_irqsave(spinlock_check(lock), flags); \
@@ -413,12 +402,6 @@ static __always_inline void spin_unlock_
raw_spin_unlock_irq(&lock->rlock);
}
-static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
- __releases(lock) __no_context_analysis
-{
- raw_spin_unlock_irq_enable(&lock->rlock);
-}
-
static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
__releases(lock) __no_context_analysis
{
@@ -444,12 +427,6 @@ static __always_inline bool _spin_tryloc
}
#define spin_trylock_irqsave(lock, flags) _spin_trylock_irqsave(lock, &(flags))
-static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
- __cond_acquires(true, lock) __no_context_analysis
-{
- return raw_spin_trylock_irq_disable(&lock->rlock);
-}
-
/**
* spin_is_locked() - Check whether a spinlock is locked.
* @lock: Pointer to the spinlock.
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -28,8 +28,6 @@ void __lockfunc
void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock) __acquires(lock);
void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock)
__acquires(lock);
-void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
- __acquires(lock);
unsigned long __lockfunc _raw_spin_lock_irqsave(raw_spinlock_t *lock)
__acquires(lock);
@@ -41,7 +39,6 @@ int __lockfunc _raw_spin_trylock_bh(raw_
void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock) __releases(lock);
void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock) __releases(lock);
void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock) __releases(lock);
-void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock) __releases(lock);
void __lockfunc
_raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
__releases(lock);
@@ -58,11 +55,6 @@ void __lockfunc
#define _raw_spin_lock_irq(lock) __raw_spin_lock_irq(lock)
#endif
-/* Use the same config as spin_lock_irq() temporarily. */
-#ifdef CONFIG_INLINE_SPIN_LOCK_IRQ
-#define _raw_spin_lock_irq_disable(lock) __raw_spin_lock_irq_disable(lock)
-#endif
-
#ifdef CONFIG_INLINE_SPIN_LOCK_IRQSAVE
#define _raw_spin_lock_irqsave(lock) __raw_spin_lock_irqsave(lock)
#endif
@@ -87,11 +79,6 @@ void __lockfunc
#define _raw_spin_unlock_irq(lock) __raw_spin_unlock_irq(lock)
#endif
-/* Use the same config as spin_unlock_irq() temporarily. */
-#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
-#define _raw_spin_unlock_irq_enable(lock) __raw_spin_unlock_irq_enable(lock)
-#endif
-
#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE
#define _raw_spin_unlock_irqrestore(lock, flags) __raw_spin_unlock_irqrestore(lock, flags)
#endif
@@ -118,16 +105,6 @@ static __always_inline bool _raw_spin_tr
return false;
}
-static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- local_interrupt_disable();
- if (_raw_spin_trylock(lock))
- return true;
- local_interrupt_enable();
- return false;
-}
-
static __always_inline bool _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags)
__cond_acquires(true, lock)
{
@@ -166,15 +143,6 @@ static inline void __raw_spin_lock_irq(r
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
}
-static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
- __acquires(lock) __no_context_analysis
-{
- local_interrupt_disable();
- preempt_disable();
- spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
- LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
-}
-
static inline void __raw_spin_lock_bh(raw_spinlock_t *lock)
__acquires(lock) __no_context_analysis
{
@@ -220,15 +188,6 @@ static inline void __raw_spin_unlock_irq
preempt_enable();
}
-static inline void __raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
- __releases(lock)
-{
- spin_release(&lock->dep_map, _RET_IP_);
- do_raw_spin_unlock(lock);
- local_interrupt_enable();
- preempt_enable();
-}
-
static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock)
__releases(lock)
{
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -42,9 +42,6 @@
#define __LOCK_IRQSAVE(lock, flags, ...) \
do { local_irq_save(flags); __LOCK(lock, ##__VA_ARGS__); } while (0)
-#define __LOCK_IRQ_DISABLE(lock, ...) \
- do { local_interrupt_disable(); __LOCK(lock, ##__VA_ARGS__); } while (0)
-
#define ___UNLOCK_(lock) \
do { __release(lock); (void)(lock); } while (0)
@@ -64,9 +61,6 @@
#define __UNLOCK_IRQRESTORE(lock, flags, ...) \
do { local_irq_restore(flags); __UNLOCK(lock, ##__VA_ARGS__); } while (0)
-#define __UNLOCK_IRQ_ENABLE(lock, ...) \
- do { __UNLOCK(lock, ##__VA_ARGS__); local_interrupt_enable(); } while (0)
-
#define _raw_spin_lock(lock) __LOCK(lock)
#define _raw_spin_lock_nested(lock, subclass) __LOCK(lock)
#define _raw_read_lock(lock) __LOCK(lock, shared)
@@ -76,7 +70,6 @@
#define _raw_read_lock_bh(lock) __LOCK_BH(lock, shared)
#define _raw_write_lock_bh(lock) __LOCK_BH(lock)
#define _raw_spin_lock_irq(lock) __LOCK_IRQ(lock)
-#define _raw_spin_lock_irq_disable(lock) __LOCK_IRQ_DISABLE(lock)
#define _raw_read_lock_irq(lock) __LOCK_IRQ(lock, shared)
#define _raw_write_lock_irq(lock) __LOCK_IRQ(lock)
#define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
@@ -104,13 +97,6 @@ static __always_inline int _raw_spin_try
return 1;
}
-static __always_inline int _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- __LOCK_IRQ_DISABLE(lock);
- return 1;
-}
-
static __always_inline int _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags)
__cond_acquires(true, lock)
{
@@ -146,7 +132,6 @@ static __always_inline int _raw_write_tr
#define _raw_write_unlock_bh(lock) __UNLOCK_BH(lock)
#define _raw_read_unlock_bh(lock) __UNLOCK_BH(lock, shared)
#define _raw_spin_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _raw_spin_unlock_irq_enable(lock) __UNLOCK_IRQ_ENABLE(lock)
#define _raw_read_unlock_irq(lock) __UNLOCK_IRQ(lock, shared)
#define _raw_write_unlock_irq(lock) __UNLOCK_IRQ(lock)
#define _raw_spin_unlock_irqrestore(lock, flags) \
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -96,12 +96,6 @@ static __always_inline void spin_lock_ir
rt_spin_lock(lock);
}
-static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
- __acquires(lock)
-{
- rt_spin_lock(lock);
-}
-
#define spin_lock_irqsave(lock, flags) \
do { \
typecheck(unsigned long, flags); \
@@ -128,12 +122,6 @@ static __always_inline void spin_unlock_
rt_spin_unlock(lock);
}
-static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
- __releases(lock)
-{
- rt_spin_unlock(lock);
-}
-
static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,
unsigned long flags)
__releases(lock)
@@ -143,12 +131,6 @@ static __always_inline void spin_unlock_
#define spin_trylock(lock) rt_spin_trylock(lock)
-static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- return rt_spin_trylock(lock);
-}
-
#define spin_trylock_bh(lock) rt_spin_trylock_bh(lock)
#define spin_trylock_irq(lock) rt_spin_trylock(lock)
--- a/kernel/irq/refcount_interrupt_test.c
+++ b/kernel/irq/refcount_interrupt_test.c
@@ -4,7 +4,7 @@
*/
#include <kunit/test.h>
-#include <linux/interrupt_rc.h>
+#include <../../rust/helpers/interrupt_rc.h>
#define TEST_IRQ_ON() KUNIT_EXPECT_FALSE(test, irqs_disabled())
#define TEST_IRQ_OFF() KUNIT_EXPECT_TRUE(test, irqs_disabled())
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -129,21 +129,6 @@ static void __lockfunc __raw_##op##_lock
*/
BUILD_LOCK_OPS(spin, raw_spinlock, __acquires);
-/* No rwlock_t variants for now, so just build this function by hand */
-static void __lockfunc __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
-{
- for (;;) {
- preempt_disable();
- local_interrupt_disable();
- if (likely(do_raw_spin_trylock(lock)))
- break;
- local_interrupt_enable();
- preempt_enable();
-
- arch_spin_relax(&lock->raw_lock);
- }
-}
-
#ifndef CONFIG_PREEMPT_RT
BUILD_LOCK_OPS(read, rwlock, __acquires_shared);
BUILD_LOCK_OPS(write, rwlock, __acquires);
@@ -191,14 +176,6 @@ noinline void __lockfunc _raw_spin_lock_
EXPORT_SYMBOL(_raw_spin_lock_irq);
#endif
-#ifndef CONFIG_INLINE_SPIN_LOCK_IRQ
-noinline void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
-{
- __raw_spin_lock_irq_disable(lock);
-}
-EXPORT_SYMBOL_GPL(_raw_spin_lock_irq_disable);
-#endif
-
#ifndef CONFIG_INLINE_SPIN_LOCK_BH
noinline void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock)
{
@@ -231,14 +208,6 @@ noinline void __lockfunc _raw_spin_unloc
EXPORT_SYMBOL(_raw_spin_unlock_irq);
#endif
-#ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ
-noinline void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
-{
- __raw_spin_unlock_irq_enable(lock);
-}
-EXPORT_SYMBOL_GPL(_raw_spin_unlock_irq_enable);
-#endif
-
#ifndef CONFIG_INLINE_SPIN_UNLOCK_BH
noinline void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock)
{
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -9,7 +9,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
#include <linux/export.h>
#include <linux/kernel_stat.h>
#include <linux/interrupt.h>
@@ -89,20 +88,6 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabl
EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
#endif
-DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
-
-void _local_interrupt_disable(void)
-{
- __local_interrupt_disable();
-}
-EXPORT_SYMBOL(_local_interrupt_disable);
-
-void _local_interrupt_enable(void)
-{
- __local_interrupt_enable();
-}
-EXPORT_SYMBOL(_local_interrupt_enable);
-
#ifndef CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS
/*
* Any 32bit architecture that still cares about performance should
--- a/rust/helpers/interrupt.c
+++ b/rust/helpers/interrupt.c
@@ -1,6 +1,25 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/spinlock.h>
+#include <linux/export.h>
+#include <linux/percpu.h>
+
+#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
+#include "interrupt_rc.h"
+#include "spinlock.h"
+
+DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
+
+void _local_interrupt_save_state(unsigned long flags)
+{
+ __local_interrupt_save_state(flags);
+}
+EXPORT_SYMBOL(_local_interrupt_save_state);
+
+void _local_interrupt_enable(void)
+{
+ __local_interrupt_enable();
+}
+EXPORT_SYMBOL(_local_interrupt_enable);
__rust_helper void rust_helper_local_interrupt_disable(void)
{
--- /dev/null
+++ b/rust/helpers/interrupt_rc.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __RUST_HELPERS_INTERRUPT_RC_H
+#define __RUST_HELPERS_INTERRUPT_RC_H
+/*
+ * refcounted local processor interrupt management.
+ */
+#include <linux/irqflags.h>
+#include <linux/percpu.h>
+#include <linux/preempt.h>
+
+/* Per-CPU interrupt disabling state for local_interrupt_{disable,enable}(). */
+DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
+
+static __always_inline void __local_interrupt_save_state(unsigned long flags)
+{
+ raw_cpu_write(local_interrupt_disable_state, flags);
+}
+
+static __always_inline void __local_interrupt_enable(void)
+{
+ unsigned long flags = raw_cpu_read(local_interrupt_disable_state);
+
+ local_irq_restore(flags);
+}
+
+#ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
+static __always_inline void _local_interrupt_save_state(unsigned long flags)
+{
+ __local_interrupt_save_state(flags);
+}
+
+static __always_inline void _local_interrupt_enable(void)
+{
+ __local_interrupt_enable();
+}
+#else
+extern void _local_interrupt_save_state(unsigned long flags);
+extern void _local_interrupt_enable(void);
+#endif
+
+#define hardirq_disable_enter() __preempt_count_add_return(HARDIRQ_DISABLE_OFFSET)
+#define hardirq_disable_exit() __preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET)
+
+static inline void local_interrupt_disable(void)
+{
+ int new_count;
+ unsigned long flags;
+
+ WARN_ON_ONCE(in_nmi());
+
+ local_irq_save(flags);
+ new_count = hardirq_disable_enter();
+
+ if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
+ _local_interrupt_save_state(flags);
+}
+
+static inline void local_interrupt_enable(void)
+{
+ int new_count;
+
+ new_count = hardirq_disable_exit();
+
+ if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
+ _local_interrupt_enable();
+}
+
+#endif /* !__RUST_HELPERS_INTERRUPT_RC_H */
--- a/rust/helpers/spinlock.c
+++ b/rust/helpers/spinlock.c
@@ -1,6 +1,43 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/spinlock.h>
+#include <linux/export.h>
+#include "spinlock.h"
+
+#if !defined(CONFIG_GENERIC_LOCKBREAK) || defined(CONFIG_DEBUG_LOCK_ALLOC)
+/* The __lock_function inlines are taken from "spinlock.h" */
+#else
+
+/* No rwlock_t variants for now, so just build this function by hand */
+static void __lockfunc __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+{
+ for (;;) {
+ preempt_disable();
+ local_interrupt_disable();
+ if (likely(do_raw_spin_trylock(lock)))
+ break;
+ local_interrupt_enable();
+ preempt_enable();
+
+ arch_spin_relax(&lock->raw_lock);
+ }
+}
+#endif
+
+#ifndef CONFIG_INLINE_SPIN_LOCK_IRQ
+noinline void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+{
+ __raw_spin_lock_irq_disable(lock);
+}
+EXPORT_SYMBOL_GPL(_raw_spin_lock_irq_disable);
+#endif
+
+#ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ
+noinline void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
+{
+ __raw_spin_unlock_irq_enable(lock);
+}
+EXPORT_SYMBOL_GPL(_raw_spin_unlock_irq_enable);
+#endif
__rust_helper void rust_helper___spin_lock_init(spinlock_t *lock,
const char *name,
--- /dev/null
+++ b/rust/helpers/spinlock.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __RUST_HELPERS_SPINLOCK_H
+#define __RUST_HELPERS_SPINLOCK_H
+
+#include <linux/spinlock.h>
+#include "interrupt_rc.h"
+
+#ifdef CONFIG_SMP
+void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock) __acquires(lock);
+void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock) __releases(lock);
+
+/* Use the same config as spin_lock_irq() temporarily. */
+#ifdef CONFIG_INLINE_SPIN_LOCK_IRQ
+#define _raw_spin_lock_irq_disable(lock) __raw_spin_lock_irq_disable(lock)
+#endif
+
+/* Use the same config as spin_unlock_irq() temporarily. */
+#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
+#define _raw_spin_unlock_irq_enable(lock) __raw_spin_unlock_irq_enable(lock)
+#endif
+
+static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ local_interrupt_disable();
+ if (_raw_spin_trylock(lock))
+ return true;
+ local_interrupt_enable();
+ return false;
+}
+
+static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+ __acquires(lock) __no_context_analysis
+{
+ local_interrupt_disable();
+ preempt_disable();
+ spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+ LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
+}
+
+static inline void __raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
+ __releases(lock)
+{
+ spin_release(&lock->dep_map, _RET_IP_);
+ do_raw_spin_unlock(lock);
+ local_interrupt_enable();
+ preempt_enable();
+}
+
+#else /* CONFIG_SMP */
+
+#define __LOCK_IRQ_DISABLE(lock, ...) \
+ do { local_interrupt_disable(); __LOCK(lock, ##__VA_ARGS__); } while (0)
+#define __UNLOCK_IRQ_ENABLE(lock, ...) \
+ do { __UNLOCK(lock, ##__VA_ARGS__); local_interrupt_enable(); } while (0)
+
+#define _raw_spin_lock_irq_disable(lock) __LOCK_IRQ_DISABLE(lock)
+#define _raw_spin_unlock_irq_enable(lock) __UNLOCK_IRQ_ENABLE(lock)
+
+static __always_inline int _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ __LOCK_IRQ_DISABLE(lock);
+ return 1;
+}
+
+#endif /* CONFIG_SMP */
+
+#define raw_spin_lock_irq_disable(lock) _raw_spin_lock_irq_disable(lock)
+#define raw_spin_unlock_irq_enable(lock) _raw_spin_unlock_irq_enable(lock)
+#define raw_spin_trylock_irq_disable(lock) _raw_spin_trylock_irq_disable(lock)
+
+#ifdef CONFIG_PREEMPT_RT
+static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
+ __acquires(lock)
+{
+ rt_spin_lock(lock);
+}
+
+static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
+ __releases(lock)
+{
+ rt_spin_unlock(lock);
+}
+
+static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ return rt_spin_trylock(lock);
+}
+
+#else /* CONFIG_PREEMPT_RT */
+
+static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
+ __acquires(lock) __no_context_analysis
+{
+ raw_spin_lock_irq_disable(&lock->rlock);
+}
+
+static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
+ __releases(lock) __no_context_analysis
+{
+ raw_spin_unlock_irq_enable(&lock->rlock);
+}
+
+static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
+ __cond_acquires(true, lock) __no_context_analysis
+{
+ return raw_spin_trylock_irq_disable(&lock->rlock);
+}
+
+#endif /* !CONFIG_PREEMPT_RT */
+
+#endif /* __RUST_HELPERS_SPINLOCK_H */After reverting the spinlock conversion and a lengthy discussion it's the
best to confine the reference counted interrupt disable/enable mechanism to
Rust which is the only user.
This should become the new norm, but that needs more thoughts and cleaning
up the confined usage in Rust at some point is way simpler than chasing
random places which adopt it in the meanwhile.
Signed-off-by: Thomas Gleixner <tglx@kernel.org>
---
Resend because I fatfingered the Subject line ... Sorry for the noise in
case you got the original busted one.
Applies against tip locking/urgent
---
include/linux/spinlock.h | 23 -------
include/linux/spinlock_api_smp.h | 41 ------------
include/linux/spinlock_api_up.h | 15 ----
include/linux/spinlock_rt.h | 18 -----
kernel/irq/refcount_interrupt_test.c | 2
kernel/locking/spinlock.c | 31 ---------
kernel/softirq.c | 15 ----
rust/helpers/interrupt.c | 21 ++++++
rust/helpers/interrupt_rc.h | 68 ++++++++++++++++++++
rust/helpers/spinlock.c | 39 +++++++++++
rust/helpers/spinlock.h | 114 +++++++++++++++++++++++++++++++++++
11 files changed, 241 insertions(+), 146 deletions(-)
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -57,7 +57,6 @@
#include <linux/linkage.h>
#include <linux/compiler.h>
#include <linux/irqflags.h>
-#include <linux/interrupt_rc.h>
#include <linux/thread_info.h>
#include <linux/stringify.h>
#include <linux/bottom_half.h>
@@ -274,11 +273,9 @@ static inline void do_raw_spin_unlock(ra
#endif
#define raw_spin_lock_irq(lock) _raw_spin_lock_irq(lock)
-#define raw_spin_lock_irq_disable(lock) _raw_spin_lock_irq_disable(lock)
#define raw_spin_lock_bh(lock) _raw_spin_lock_bh(lock)
#define raw_spin_unlock(lock) _raw_spin_unlock(lock)
#define raw_spin_unlock_irq(lock) _raw_spin_unlock_irq(lock)
-#define raw_spin_unlock_irq_enable(lock) _raw_spin_unlock_irq_enable(lock)
#define raw_spin_unlock_irqrestore(lock, flags) \
do { \
@@ -293,8 +290,6 @@ static inline void do_raw_spin_unlock(ra
#define raw_spin_trylock_irqsave(lock, flags) _raw_spin_trylock_irqsave(lock, &(flags))
-#define raw_spin_trylock_irq_disable(lock) _raw_spin_trylock_irq_disable(lock)
-
#ifndef CONFIG_PREEMPT_RT
/* Include rwlock functions for !RT */
#include <linux/rwlock.h>
@@ -377,12 +372,6 @@ static __always_inline void spin_lock_ir
raw_spin_lock_irq(&lock->rlock);
}
-static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
- __acquires(lock) __no_context_analysis
-{
- raw_spin_lock_irq_disable(&lock->rlock);
-}
-
#define spin_lock_irqsave(lock, flags) \
do { \
raw_spin_lock_irqsave(spinlock_check(lock), flags); \
@@ -413,12 +402,6 @@ static __always_inline void spin_unlock_
raw_spin_unlock_irq(&lock->rlock);
}
-static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
- __releases(lock) __no_context_analysis
-{
- raw_spin_unlock_irq_enable(&lock->rlock);
-}
-
static __always_inline void spin_unlock_irqrestore(spinlock_t *lock, unsigned long flags)
__releases(lock) __no_context_analysis
{
@@ -444,12 +427,6 @@ static __always_inline bool _spin_tryloc
}
#define spin_trylock_irqsave(lock, flags) _spin_trylock_irqsave(lock, &(flags))
-static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
- __cond_acquires(true, lock) __no_context_analysis
-{
- return raw_spin_trylock_irq_disable(&lock->rlock);
-}
-
/**
* spin_is_locked() - Check whether a spinlock is locked.
* @lock: Pointer to the spinlock.
--- a/include/linux/spinlock_api_smp.h
+++ b/include/linux/spinlock_api_smp.h
@@ -28,8 +28,6 @@ void __lockfunc
void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock) __acquires(lock);
void __lockfunc _raw_spin_lock_irq(raw_spinlock_t *lock)
__acquires(lock);
-void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
- __acquires(lock);
unsigned long __lockfunc _raw_spin_lock_irqsave(raw_spinlock_t *lock)
__acquires(lock);
@@ -41,7 +39,6 @@ int __lockfunc _raw_spin_trylock_bh(raw_
void __lockfunc _raw_spin_unlock(raw_spinlock_t *lock) __releases(lock);
void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock) __releases(lock);
void __lockfunc _raw_spin_unlock_irq(raw_spinlock_t *lock) __releases(lock);
-void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock) __releases(lock);
void __lockfunc
_raw_spin_unlock_irqrestore(raw_spinlock_t *lock, unsigned long flags)
__releases(lock);
@@ -58,11 +55,6 @@ void __lockfunc
#define _raw_spin_lock_irq(lock) __raw_spin_lock_irq(lock)
#endif
-/* Use the same config as spin_lock_irq() temporarily. */
-#ifdef CONFIG_INLINE_SPIN_LOCK_IRQ
-#define _raw_spin_lock_irq_disable(lock) __raw_spin_lock_irq_disable(lock)
-#endif
-
#ifdef CONFIG_INLINE_SPIN_LOCK_IRQSAVE
#define _raw_spin_lock_irqsave(lock) __raw_spin_lock_irqsave(lock)
#endif
@@ -87,11 +79,6 @@ void __lockfunc
#define _raw_spin_unlock_irq(lock) __raw_spin_unlock_irq(lock)
#endif
-/* Use the same config as spin_unlock_irq() temporarily. */
-#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
-#define _raw_spin_unlock_irq_enable(lock) __raw_spin_unlock_irq_enable(lock)
-#endif
-
#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQRESTORE
#define _raw_spin_unlock_irqrestore(lock, flags) __raw_spin_unlock_irqrestore(lock, flags)
#endif
@@ -118,16 +105,6 @@ static __always_inline bool _raw_spin_tr
return false;
}
-static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- local_interrupt_disable();
- if (_raw_spin_trylock(lock))
- return true;
- local_interrupt_enable();
- return false;
-}
-
static __always_inline bool _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags)
__cond_acquires(true, lock)
{
@@ -166,15 +143,6 @@ static inline void __raw_spin_lock_irq(r
LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
}
-static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
- __acquires(lock) __no_context_analysis
-{
- local_interrupt_disable();
- preempt_disable();
- spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
- LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
-}
-
static inline void __raw_spin_lock_bh(raw_spinlock_t *lock)
__acquires(lock) __no_context_analysis
{
@@ -220,15 +188,6 @@ static inline void __raw_spin_unlock_irq
preempt_enable();
}
-static inline void __raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
- __releases(lock)
-{
- spin_release(&lock->dep_map, _RET_IP_);
- do_raw_spin_unlock(lock);
- local_interrupt_enable();
- preempt_enable();
-}
-
static inline void __raw_spin_unlock_bh(raw_spinlock_t *lock)
__releases(lock)
{
--- a/include/linux/spinlock_api_up.h
+++ b/include/linux/spinlock_api_up.h
@@ -42,9 +42,6 @@
#define __LOCK_IRQSAVE(lock, flags, ...) \
do { local_irq_save(flags); __LOCK(lock, ##__VA_ARGS__); } while (0)
-#define __LOCK_IRQ_DISABLE(lock, ...) \
- do { local_interrupt_disable(); __LOCK(lock, ##__VA_ARGS__); } while (0)
-
#define ___UNLOCK_(lock) \
do { __release(lock); (void)(lock); } while (0)
@@ -64,9 +61,6 @@
#define __UNLOCK_IRQRESTORE(lock, flags, ...) \
do { local_irq_restore(flags); __UNLOCK(lock, ##__VA_ARGS__); } while (0)
-#define __UNLOCK_IRQ_ENABLE(lock, ...) \
- do { __UNLOCK(lock, ##__VA_ARGS__); local_interrupt_enable(); } while (0)
-
#define _raw_spin_lock(lock) __LOCK(lock)
#define _raw_spin_lock_nested(lock, subclass) __LOCK(lock)
#define _raw_read_lock(lock) __LOCK(lock, shared)
@@ -76,7 +70,6 @@
#define _raw_read_lock_bh(lock) __LOCK_BH(lock, shared)
#define _raw_write_lock_bh(lock) __LOCK_BH(lock)
#define _raw_spin_lock_irq(lock) __LOCK_IRQ(lock)
-#define _raw_spin_lock_irq_disable(lock) __LOCK_IRQ_DISABLE(lock)
#define _raw_read_lock_irq(lock) __LOCK_IRQ(lock, shared)
#define _raw_write_lock_irq(lock) __LOCK_IRQ(lock)
#define _raw_spin_lock_irqsave(lock, flags) __LOCK_IRQSAVE(lock, flags)
@@ -104,13 +97,6 @@ static __always_inline int _raw_spin_try
return 1;
}
-static __always_inline int _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- __LOCK_IRQ_DISABLE(lock);
- return 1;
-}
-
static __always_inline int _raw_spin_trylock_irqsave(raw_spinlock_t *lock, unsigned long *flags)
__cond_acquires(true, lock)
{
@@ -146,7 +132,6 @@ static __always_inline int _raw_write_tr
#define _raw_write_unlock_bh(lock) __UNLOCK_BH(lock)
#define _raw_read_unlock_bh(lock) __UNLOCK_BH(lock, shared)
#define _raw_spin_unlock_irq(lock) __UNLOCK_IRQ(lock)
-#define _raw_spin_unlock_irq_enable(lock) __UNLOCK_IRQ_ENABLE(lock)
#define _raw_read_unlock_irq(lock) __UNLOCK_IRQ(lock, shared)
#define _raw_write_unlock_irq(lock) __UNLOCK_IRQ(lock)
#define _raw_spin_unlock_irqrestore(lock, flags) \
--- a/include/linux/spinlock_rt.h
+++ b/include/linux/spinlock_rt.h
@@ -96,12 +96,6 @@ static __always_inline void spin_lock_ir
rt_spin_lock(lock);
}
-static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
- __acquires(lock)
-{
- rt_spin_lock(lock);
-}
-
#define spin_lock_irqsave(lock, flags) \
do { \
typecheck(unsigned long, flags); \
@@ -128,12 +122,6 @@ static __always_inline void spin_unlock_
rt_spin_unlock(lock);
}
-static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
- __releases(lock)
-{
- rt_spin_unlock(lock);
-}
-
static __always_inline void spin_unlock_irqrestore(spinlock_t *lock,
unsigned long flags)
__releases(lock)
@@ -143,12 +131,6 @@ static __always_inline void spin_unlock_
#define spin_trylock(lock) rt_spin_trylock(lock)
-static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
- __cond_acquires(true, lock)
-{
- return rt_spin_trylock(lock);
-}
-
#define spin_trylock_bh(lock) rt_spin_trylock_bh(lock)
#define spin_trylock_irq(lock) rt_spin_trylock(lock)
--- a/kernel/irq/refcount_interrupt_test.c
+++ b/kernel/irq/refcount_interrupt_test.c
@@ -4,7 +4,7 @@
*/
#include <kunit/test.h>
-#include <linux/interrupt_rc.h>
+#include <../../rust/helpers/interrupt_rc.h>
#define TEST_IRQ_ON() KUNIT_EXPECT_FALSE(test, irqs_disabled())
#define TEST_IRQ_OFF() KUNIT_EXPECT_TRUE(test, irqs_disabled())
--- a/kernel/locking/spinlock.c
+++ b/kernel/locking/spinlock.c
@@ -129,21 +129,6 @@ static void __lockfunc __raw_##op##_lock
*/
BUILD_LOCK_OPS(spin, raw_spinlock, __acquires);
-/* No rwlock_t variants for now, so just build this function by hand */
-static void __lockfunc __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
-{
- for (;;) {
- preempt_disable();
- local_interrupt_disable();
- if (likely(do_raw_spin_trylock(lock)))
- break;
- local_interrupt_enable();
- preempt_enable();
-
- arch_spin_relax(&lock->raw_lock);
- }
-}
-
#ifndef CONFIG_PREEMPT_RT
BUILD_LOCK_OPS(read, rwlock, __acquires_shared);
BUILD_LOCK_OPS(write, rwlock, __acquires);
@@ -191,14 +176,6 @@ noinline void __lockfunc _raw_spin_lock_
EXPORT_SYMBOL(_raw_spin_lock_irq);
#endif
-#ifndef CONFIG_INLINE_SPIN_LOCK_IRQ
-noinline void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
-{
- __raw_spin_lock_irq_disable(lock);
-}
-EXPORT_SYMBOL_GPL(_raw_spin_lock_irq_disable);
-#endif
-
#ifndef CONFIG_INLINE_SPIN_LOCK_BH
noinline void __lockfunc _raw_spin_lock_bh(raw_spinlock_t *lock)
{
@@ -231,14 +208,6 @@ noinline void __lockfunc _raw_spin_unloc
EXPORT_SYMBOL(_raw_spin_unlock_irq);
#endif
-#ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ
-noinline void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
-{
- __raw_spin_unlock_irq_enable(lock);
-}
-EXPORT_SYMBOL_GPL(_raw_spin_unlock_irq_enable);
-#endif
-
#ifndef CONFIG_INLINE_SPIN_UNLOCK_BH
noinline void __lockfunc _raw_spin_unlock_bh(raw_spinlock_t *lock)
{
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -9,7 +9,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
-#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
#include <linux/export.h>
#include <linux/kernel_stat.h>
#include <linux/interrupt.h>
@@ -89,20 +88,6 @@ EXPORT_PER_CPU_SYMBOL_GPL(hardirqs_enabl
EXPORT_PER_CPU_SYMBOL_GPL(hardirq_context);
#endif
-DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
-
-void _local_interrupt_save_state(unsigned long flags)
-{
- __local_interrupt_save_state(flags);
-}
-EXPORT_SYMBOL(_local_interrupt_save_state);
-
-void _local_interrupt_enable(void)
-{
- __local_interrupt_enable();
-}
-EXPORT_SYMBOL(_local_interrupt_enable);
-
#ifndef CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS
/*
* Any 32bit architecture that still cares about performance should
--- a/rust/helpers/interrupt.c
+++ b/rust/helpers/interrupt.c
@@ -1,6 +1,25 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/spinlock.h>
+#include <linux/export.h>
+#include <linux/percpu.h>
+
+#define INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
+#include "interrupt_rc.h"
+#include "spinlock.h"
+
+DEFINE_PER_CPU(unsigned long, local_interrupt_disable_state);
+
+void _local_interrupt_save_state(unsigned long flags)
+{
+ __local_interrupt_save_state(flags);
+}
+EXPORT_SYMBOL(_local_interrupt_save_state);
+
+void _local_interrupt_enable(void)
+{
+ __local_interrupt_enable();
+}
+EXPORT_SYMBOL(_local_interrupt_enable);
__rust_helper void rust_helper_local_interrupt_disable(void)
{
--- /dev/null
+++ b/rust/helpers/interrupt_rc.h
@@ -0,0 +1,68 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __RUST_HELPERS_INTERRUPT_RC_H
+#define __RUST_HELPERS_INTERRUPT_RC_H
+/*
+ * refcounted local processor interrupt management.
+ */
+#include <linux/irqflags.h>
+#include <linux/percpu.h>
+#include <linux/preempt.h>
+
+/* Per-CPU interrupt disabling state for local_interrupt_{disable,enable}(). */
+DECLARE_PER_CPU(unsigned long, local_interrupt_disable_state);
+
+static __always_inline void __local_interrupt_save_state(unsigned long flags)
+{
+ raw_cpu_write(local_interrupt_disable_state, flags);
+}
+
+static __always_inline void __local_interrupt_enable(void)
+{
+ unsigned long flags = raw_cpu_read(local_interrupt_disable_state);
+
+ local_irq_restore(flags);
+}
+
+#ifndef INSTANTIATE_EXPORTED_INTERRUPT_DISABLE
+static __always_inline void _local_interrupt_save_state(unsigned long flags)
+{
+ __local_interrupt_save_state(flags);
+}
+
+static __always_inline void _local_interrupt_enable(void)
+{
+ __local_interrupt_enable();
+}
+#else
+extern void _local_interrupt_save_state(unsigned long flags);
+extern void _local_interrupt_enable(void);
+#endif
+
+#define hardirq_disable_enter() __preempt_count_add_return(HARDIRQ_DISABLE_OFFSET)
+#define hardirq_disable_exit() __preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET)
+
+static inline void local_interrupt_disable(void)
+{
+ int new_count;
+ unsigned long flags;
+
+ WARN_ON_ONCE(in_nmi());
+
+ local_irq_save(flags);
+ new_count = hardirq_disable_enter();
+
+ if ((new_count & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET)
+ _local_interrupt_save_state(flags);
+}
+
+static inline void local_interrupt_enable(void)
+{
+ int new_count;
+
+ new_count = hardirq_disable_exit();
+
+ if ((new_count & HARDIRQ_DISABLE_MASK) == 0)
+ _local_interrupt_enable();
+}
+
+#endif /* !__RUST_HELPERS_INTERRUPT_RC_H */
--- a/rust/helpers/spinlock.c
+++ b/rust/helpers/spinlock.c
@@ -1,6 +1,43 @@
// SPDX-License-Identifier: GPL-2.0
-#include <linux/spinlock.h>
+#include <linux/export.h>
+#include "spinlock.h"
+
+#if !defined(CONFIG_GENERIC_LOCKBREAK) || defined(CONFIG_DEBUG_LOCK_ALLOC)
+/* The __lock_function inlines are taken from "spinlock.h" */
+#else
+
+/* No rwlock_t variants for now, so just build this function by hand */
+static void __lockfunc __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+{
+ for (;;) {
+ preempt_disable();
+ local_interrupt_disable();
+ if (likely(do_raw_spin_trylock(lock)))
+ break;
+ local_interrupt_enable();
+ preempt_enable();
+
+ arch_spin_relax(&lock->raw_lock);
+ }
+}
+#endif
+
+#ifndef CONFIG_INLINE_SPIN_LOCK_IRQ
+noinline void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+{
+ __raw_spin_lock_irq_disable(lock);
+}
+EXPORT_SYMBOL_GPL(_raw_spin_lock_irq_disable);
+#endif
+
+#ifndef CONFIG_INLINE_SPIN_UNLOCK_IRQ
+noinline void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
+{
+ __raw_spin_unlock_irq_enable(lock);
+}
+EXPORT_SYMBOL_GPL(_raw_spin_unlock_irq_enable);
+#endif
__rust_helper void rust_helper___spin_lock_init(spinlock_t *lock,
const char *name,
--- /dev/null
+++ b/rust/helpers/spinlock.h
@@ -0,0 +1,114 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef __RUST_HELPERS_SPINLOCK_H
+#define __RUST_HELPERS_SPINLOCK_H
+
+#include <linux/spinlock.h>
+#include "interrupt_rc.h"
+
+#ifdef CONFIG_SMP
+void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock) __acquires(lock);
+void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock) __releases(lock);
+
+/* Use the same config as spin_lock_irq() temporarily. */
+#ifdef CONFIG_INLINE_SPIN_LOCK_IRQ
+#define _raw_spin_lock_irq_disable(lock) __raw_spin_lock_irq_disable(lock)
+#endif
+
+/* Use the same config as spin_unlock_irq() temporarily. */
+#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
+#define _raw_spin_unlock_irq_enable(lock) __raw_spin_unlock_irq_enable(lock)
+#endif
+
+static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ local_interrupt_disable();
+ if (_raw_spin_trylock(lock))
+ return true;
+ local_interrupt_enable();
+ return false;
+}
+
+static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
+ __acquires(lock) __no_context_analysis
+{
+ local_interrupt_disable();
+ preempt_disable();
+ spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
+ LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
+}
+
+static inline void __raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
+ __releases(lock)
+{
+ spin_release(&lock->dep_map, _RET_IP_);
+ do_raw_spin_unlock(lock);
+ local_interrupt_enable();
+ preempt_enable();
+}
+
+#else /* CONFIG_SMP */
+
+#define __LOCK_IRQ_DISABLE(lock, ...) \
+ do { local_interrupt_disable(); __LOCK(lock, ##__VA_ARGS__); } while (0)
+#define __UNLOCK_IRQ_ENABLE(lock, ...) \
+ do { __UNLOCK(lock, ##__VA_ARGS__); local_interrupt_enable(); } while (0)
+
+#define _raw_spin_lock_irq_disable(lock) __LOCK_IRQ_DISABLE(lock)
+#define _raw_spin_unlock_irq_enable(lock) __UNLOCK_IRQ_ENABLE(lock)
+
+static __always_inline int _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ __LOCK_IRQ_DISABLE(lock);
+ return 1;
+}
+
+#endif /* CONFIG_SMP */
+
+#define raw_spin_lock_irq_disable(lock) _raw_spin_lock_irq_disable(lock)
+#define raw_spin_unlock_irq_enable(lock) _raw_spin_unlock_irq_enable(lock)
+#define raw_spin_trylock_irq_disable(lock) _raw_spin_trylock_irq_disable(lock)
+
+#ifdef CONFIG_PREEMPT_RT
+static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
+ __acquires(lock)
+{
+ rt_spin_lock(lock);
+}
+
+static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
+ __releases(lock)
+{
+ rt_spin_unlock(lock);
+}
+
+static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
+ __cond_acquires(true, lock)
+{
+ return rt_spin_trylock(lock);
+}
+
+#else /* CONFIG_PREEMPT_RT */
+
+static __always_inline void spin_lock_irq_disable(spinlock_t *lock)
+ __acquires(lock) __no_context_analysis
+{
+ raw_spin_lock_irq_disable(&lock->rlock);
+}
+
+static __always_inline void spin_unlock_irq_enable(spinlock_t *lock)
+ __releases(lock) __no_context_analysis
+{
+ raw_spin_unlock_irq_enable(&lock->rlock);
+}
+
+static __always_inline int spin_trylock_irq_disable(spinlock_t *lock)
+ __cond_acquires(true, lock) __no_context_analysis
+{
+ return raw_spin_trylock_irq_disable(&lock->rlock);
+}
+
+#endif /* !CONFIG_PREEMPT_RT */
+
+#endif /* __RUST_HELPERS_SPINLOCK_H */[Cc Miguel, Lyude, Alice, Gary]
On Fri, Sep 04, 2026 at 03:26:40PM +0200, Thomas Gleixner wrote:
> After reverting the spinlock conversion and a lengthy discussion it's the
> best to confine the reference counted interrupt disable/enable mechanism to
> Rust which is the only user.
>
> This should become the new norm, but that needs more thoughts and cleaning
> up the confined usage in Rust at some point is way simpler than chasing
> random places which adopt it in the meanwhile.
>
Thank you for doing this! I think the subject should be:
irq: Move local_interrupt_{dis,en}able() into Rust
to be accurate about the name of the functions.
> Signed-off-by: Thomas Gleixner <tglx@kernel.org>
> ---
> Resend because I fatfingered the Subject line ... Sorry for the noise in
> case you got the original busted one.
>
> Applies against tip locking/urgent
> ---
> include/linux/spinlock.h | 23 -------
> include/linux/spinlock_api_smp.h | 41 ------------
> include/linux/spinlock_api_up.h | 15 ----
> include/linux/spinlock_rt.h | 18 -----
Seems we are missing a deletion of include/linux/interrupt_rc.h here.
> kernel/irq/refcount_interrupt_test.c | 2
> kernel/locking/spinlock.c | 31 ---------
> kernel/softirq.c | 15 ----
> rust/helpers/interrupt.c | 21 ++++++
> rust/helpers/interrupt_rc.h | 68 ++++++++++++++++++++
> rust/helpers/spinlock.c | 39 +++++++++++
> rust/helpers/spinlock.h | 114 +++++++++++++++++++++++++++++++++++
> 11 files changed, 241 insertions(+), 146 deletions(-)
>
[...]
> --- /dev/null
> +++ b/rust/helpers/spinlock.h
> @@ -0,0 +1,114 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef __RUST_HELPERS_SPINLOCK_H
> +#define __RUST_HELPERS_SPINLOCK_H
> +
> +#include <linux/spinlock.h>
> +#include "interrupt_rc.h"
> +
> +#ifdef CONFIG_SMP
> +void __lockfunc _raw_spin_lock_irq_disable(raw_spinlock_t *lock) __acquires(lock);
> +void __lockfunc _raw_spin_unlock_irq_enable(raw_spinlock_t *lock) __releases(lock);
> +
> +/* Use the same config as spin_lock_irq() temporarily. */
> +#ifdef CONFIG_INLINE_SPIN_LOCK_IRQ
> +#define _raw_spin_lock_irq_disable(lock) __raw_spin_lock_irq_disable(lock)
> +#endif
> +
> +/* Use the same config as spin_unlock_irq() temporarily. */
> +#ifdef CONFIG_INLINE_SPIN_UNLOCK_IRQ
> +#define _raw_spin_unlock_irq_enable(lock) __raw_spin_unlock_irq_enable(lock)
> +#endif
> +
> +static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
> + __cond_acquires(true, lock)
> +{
> + local_interrupt_disable();
> + if (_raw_spin_trylock(lock))
> + return true;
> + local_interrupt_enable();
> + return false;
> +}
> +
> +static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
> + __acquires(lock) __no_context_analysis
I think we need to put the
#if !defined(CONFIG_GENERIC_LOCKBREAK) || defined(CONFIG_DEBUG_LOCK_ALLOC)
#endif
around this. Because in the #else branch of rust/helpers/spinlock.c we
have an out-of-line definition of the same function.
Regards,
Boqun
> +{
> + local_interrupt_disable();
> + preempt_disable();
> + spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
> + LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
> +}
> +
> +static inline void __raw_spin_unlock_irq_enable(raw_spinlock_t *lock)
> + __releases(lock)
> +{
> + spin_release(&lock->dep_map, _RET_IP_);
> + do_raw_spin_unlock(lock);
> + local_interrupt_enable();
> + preempt_enable();
> +}
> +
> +#else /* CONFIG_SMP */
> +
> +#define __LOCK_IRQ_DISABLE(lock, ...) \
> + do { local_interrupt_disable(); __LOCK(lock, ##__VA_ARGS__); } while (0)
> +#define __UNLOCK_IRQ_ENABLE(lock, ...) \
> + do { __UNLOCK(lock, ##__VA_ARGS__); local_interrupt_enable(); } while (0)
> +
> +#define _raw_spin_lock_irq_disable(lock) __LOCK_IRQ_DISABLE(lock)
> +#define _raw_spin_unlock_irq_enable(lock) __UNLOCK_IRQ_ENABLE(lock)
> +
> +static __always_inline int _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
> + __cond_acquires(true, lock)
> +{
> + __LOCK_IRQ_DISABLE(lock);
> + return 1;
> +}
> +
[...]
On Fri, Sep 04 2026 at 08:25, Boqun Feng wrote:
> [Cc Miguel, Lyude, Alice, Gary]
>
> On Fri, Sep 04, 2026 at 03:26:40PM +0200, Thomas Gleixner wrote:
>> After reverting the spinlock conversion and a lengthy discussion it's the
>> best to confine the reference counted interrupt disable/enable mechanism to
>> Rust which is the only user.
>>
>> This should become the new norm, but that needs more thoughts and cleaning
>> up the confined usage in Rust at some point is way simpler than chasing
>> random places which adopt it in the meanwhile.
>>
>
> Thank you for doing this! I think the subject should be:
>
> irq: Move local_interrupt_{dis,en}able() into Rust
>
> to be accurate about the name of the functions.
It actually also fails to mention the spinlock part :(
>> include/linux/spinlock.h | 23 -------
>> include/linux/spinlock_api_smp.h | 41 ------------
>> include/linux/spinlock_api_up.h | 15 ----
>> include/linux/spinlock_rt.h | 18 -----
>
> Seems we are missing a deletion of include/linux/interrupt_rc.h here.
Weird. I'm sure I deleted it, but ...
>> +static __always_inline bool _raw_spin_trylock_irq_disable(raw_spinlock_t *lock)
>> + __cond_acquires(true, lock)
>> +{
>> + local_interrupt_disable();
>> + if (_raw_spin_trylock(lock))
>> + return true;
>> + local_interrupt_enable();
>> + return false;
>> +}
>> +
>> +static inline void __raw_spin_lock_irq_disable(raw_spinlock_t *lock)
>> + __acquires(lock) __no_context_analysis
>
> I think we need to put the
>
> #if !defined(CONFIG_GENERIC_LOCKBREAK) || defined(CONFIG_DEBUG_LOCK_ALLOC)
> #endif
Duh yes. Missed that completely.
> around this. Because in the #else branch of rust/helpers/spinlock.c we
> have an out-of-line definition of the same function.
Right.
Thanks,
tglx
On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 346c438ac880..b0be24a386f3 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -824,7 +824,7 @@ void __noreturn stop_this_cpu(void *dummy)
> struct cpuinfo_x86 *c = this_cpu_ptr(&cpu_info);
> unsigned int cpu = smp_processor_id();
>
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Remove this CPU from the online mask and disable it
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 0fed6d0d7e32..40104170be24 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -98,7 +98,7 @@ static int __init set_efi_reboot(const struct dmi_system_id *d)
>
> void __noreturn machine_real_restart(unsigned int type)
> {
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Write zero to CMOS register number 0x0f, which the BIOS POST
> @@ -535,7 +535,7 @@ static inline void nmi_shootdown_cpus_on_restart(void);
> #if IS_ENABLED(CONFIG_KVM_X86)
> static void emergency_reboot_disable_virtualization(void)
> {
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Disable virtualization on all CPUs before rebooting to avoid hanging
> @@ -699,7 +699,7 @@ void native_machine_shutdown(void)
> * not receive the per-cpu timer interrupt which may trigger
> * scheduler's load balance.
> */
> - local_irq_disable();
> + raw_force_local_irq_disable();
> stop_other_cpus();
> #endif
>
> @@ -823,7 +823,8 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
> */
> if (cpu == crashing_cpu)
> return NMI_HANDLED;
> - local_irq_disable();
> +
> + raw_force_local_irq_disable();
>
> if (shootdown_callback)
> shootdown_callback(cpu, regs);
> @@ -865,7 +866,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
> {
> unsigned long msecs;
>
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Avoid certain doom if a shootdown already occurred; re-registering
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 132a9df98471..f13e88fb7a78 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -1095,7 +1095,7 @@ static int acpi_power_off(struct sys_off_data *data)
> {
> /* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
> pr_debug("%s called\n", __func__);
> - local_irq_disable();
> + raw_force_local_irq_disable();
> acpi_enter_sleep_state(ACPI_STATE_S5);
> return NOTIFY_DONE;
> }
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 57b074e0cfbb..dd55786768d1 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> +static __always_inline void raw_force_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
So this thing is on all sorts of don't care, we're going down paths. It
needs to ensure IRQs really are off, and preempt_count has at least one
DISABLE_OFFSET on.
*However* if something like acpi_power_off() were to 'fail' to enter S5
and continue on with the notifier, things are now unbalanced. Probably
not a problem, since the next handler will likely do
raw_force_load_irq_disable() again.
At the least this wants a comment I suppose.
> diff --git a/init/main.c b/init/main.c
> index 2613d3f9b3ce..fa84ce260b04 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -991,7 +991,6 @@ void start_kernel(void)
>
> cgroup_init_early();
>
> - local_irq_disable();
> early_boot_irqs_disabled = true;
If we want to preserve the paranoia of having that statement in the
first place, it could be replaced with something like:
WARN_ON_ONCE(!irqs_disabled());
I suppose (lockdep isn't available yet).
On Sat, Aug 29 2026 at 10:05, Peter Zijlstra wrote:
> On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
>> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
>> index 57b074e0cfbb..dd55786768d1 100644
>> --- a/include/linux/irqflags.h
>> +++ b/include/linux/irqflags.h
>
>> +static __always_inline void raw_force_local_irq_disable(void)
>> +{
>> + arch_local_irq_disable();
>> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
>> +}
>
> So this thing is on all sorts of don't care, we're going down paths. It
> needs to ensure IRQs really are off, and preempt_count has at least one
> DISABLE_OFFSET on.
>
> *However* if something like acpi_power_off() were to 'fail' to enter S5
> and continue on with the notifier, things are now unbalanced. Probably
> not a problem, since the next handler will likely do
> raw_force_load_irq_disable() again.
>
> At the least this wants a comment I suppose.
Right. That stuff needs some eyeballs.
>> diff --git a/init/main.c b/init/main.c
>> index 2613d3f9b3ce..fa84ce260b04 100644
>> --- a/init/main.c
>> +++ b/init/main.c
>> @@ -991,7 +991,6 @@ void start_kernel(void)
>>
>> cgroup_init_early();
>>
>> - local_irq_disable();
>> early_boot_irqs_disabled = true;
>
> If we want to preserve the paranoia of having that statement in the
> first place, it could be replaced with something like:
>
> WARN_ON_ONCE(!irqs_disabled());
Right.
> I suppose (lockdep isn't available yet).
Good question.
On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
> On Fri, Aug 28 2026 at 00:52, Thomas Gleixner wrote:
> > On Thu, Aug 27 2026 at 12:41, Boqun Feng wrote:
> > So I sat down and reverted
> >
> > 1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
> > e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")
> >
> > and then hacked it up just to see how far I get before vanishing to bed.
> >
> > Three hours later it surprisingly booted right away into a full distro
> > kernel and survived kernel builds and a few test cases. :)
>
> /FACMEPALM
>
> Yesterday night I was really surprised but too tired to think about it.
>
> When I came around today to look at it again I was more than embarrassed
> to figure out that the KVM script rebuilt the wrong branch over and
> over. So the build numbers kept increasing...
>
> Brown paperbag time ...
>
> Of course the real thing did _NOT_ boot at all, so I sat down and
> figured out what's going wrong and added a pile of debug to it, which is
> sadly non-existing in this magic local_interrupt_dis/enable() code.
>
> The overall fallout is moderate. Some of it are actual (but harmless)
> bugs and the rest are the oddball cases we talked about before.
>
> It builds and boots now for real, but of course your mileage will vary
> depending on hardware and .config. Combo patch on top of the reverts is
> below.
>
> The whole pile can be retrieved from git via:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git irqflags
>
> I have some thoughts about how to deal with the overall disaster, but
> that has to wait until my brain is truly awake again...
>
> Thanks,
>
> tglx
> ---
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 15fd9ec5ecac..c7229a7cfa7c 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -133,6 +133,7 @@ config X86
> select ARCH_USES_CFI_TRAPS if X86_64 && CFI
> select ARCH_SUPPORTS_LTO_CLANG
> select ARCH_SUPPORTS_LTO_CLANG_THIN
> + select ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
> select ARCH_SUPPORTS_RT
> select ARCH_USE_BUILTIN_BSWAP
> select ARCH_USE_CMPXCHG_LOCKREF
> diff --git a/arch/x86/include/asm/hardirq.h b/arch/x86/include/asm/hardirq.h
> index dea60d66d976..34bdf24b939b 100644
> --- a/arch/x86/include/asm/hardirq.h
> +++ b/arch/x86/include/asm/hardirq.h
> @@ -113,4 +113,6 @@ static __always_inline bool kvm_get_cpu_l1tf_flush_l1d(void)
> static __always_inline void kvm_set_cpu_l1tf_flush_l1d(void) { }
> #endif /* IS_ENABLED(CONFIG_KVM_INTEL) */
>
> +#define __ARCH_IRQ_EXIT_IRQS_DISABLED 1
> +
> #endif /* _ASM_X86_HARDIRQ_H */
> diff --git a/arch/x86/include/asm/preempt.h b/arch/x86/include/asm/preempt.h
> index fafb6f8cdac3..8b4d4cbae52e 100644
> --- a/arch/x86/include/asm/preempt.h
> +++ b/arch/x86/include/asm/preempt.h
> @@ -61,10 +61,20 @@ static __always_inline void preempt_count_set(unsigned long pc)
> */
> #define init_task_preempt_count(p) do { } while (0)
>
> -#define init_idle_preempt_count(p, cpu) do { \
> - per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +
> +#define init_idle_preempt_count(p, cpu) do { \
> + per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED | HARDIRQ_DISABLE_OFFSET; \
> } while (0)
>
> +#else
> +
> +#define init_idle_preempt_count(p, cpu) do { \
> + per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
> +} while (0)
> +
> +#endif
> +
> /*
> * We fold the NEED_RESCHED bit into the preempt count such that
> * preempt_enable() can decrement and test for needing to reschedule with a
> diff --git a/arch/x86/kernel/kvm.c b/arch/x86/kernel/kvm.c
> index 6b0a5861ccb8..0b5a05cb543f 100644
> --- a/arch/x86/kernel/kvm.c
> +++ b/arch/x86/kernel/kvm.c
> @@ -256,9 +256,9 @@ noinstr u32 kvm_read_and_reset_apf_flags(void)
> {
> u32 flags = 0;
>
> - if (__this_cpu_read(async_pf_enabled)) {
> - flags = __this_cpu_read(apf_reason.flags);
> - __this_cpu_write(apf_reason.flags, 0);
> + if (raw_cpu_read(async_pf_enabled)) {
> + flags = raw_cpu_read(apf_reason.flags);
> + raw_cpu_write(apf_reason.flags, 0);
> }
>
> return flags;
> diff --git a/arch/x86/kernel/process.c b/arch/x86/kernel/process.c
> index 346c438ac880..b0be24a386f3 100644
> --- a/arch/x86/kernel/process.c
> +++ b/arch/x86/kernel/process.c
> @@ -824,7 +824,7 @@ void __noreturn stop_this_cpu(void *dummy)
> struct cpuinfo_x86 *c = this_cpu_ptr(&cpu_info);
> unsigned int cpu = smp_processor_id();
>
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Remove this CPU from the online mask and disable it
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 0fed6d0d7e32..40104170be24 100644
> --- a/arch/x86/kernel/reboot.c
> +++ b/arch/x86/kernel/reboot.c
> @@ -98,7 +98,7 @@ static int __init set_efi_reboot(const struct dmi_system_id *d)
>
> void __noreturn machine_real_restart(unsigned int type)
> {
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Write zero to CMOS register number 0x0f, which the BIOS POST
> @@ -535,7 +535,7 @@ static inline void nmi_shootdown_cpus_on_restart(void);
> #if IS_ENABLED(CONFIG_KVM_X86)
> static void emergency_reboot_disable_virtualization(void)
> {
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Disable virtualization on all CPUs before rebooting to avoid hanging
> @@ -699,7 +699,7 @@ void native_machine_shutdown(void)
> * not receive the per-cpu timer interrupt which may trigger
> * scheduler's load balance.
> */
> - local_irq_disable();
> + raw_force_local_irq_disable();
> stop_other_cpus();
> #endif
>
> @@ -823,7 +823,8 @@ static int crash_nmi_callback(unsigned int val, struct pt_regs *regs)
> */
> if (cpu == crashing_cpu)
> return NMI_HANDLED;
> - local_irq_disable();
> +
> + raw_force_local_irq_disable();
>
> if (shootdown_callback)
> shootdown_callback(cpu, regs);
> @@ -865,7 +866,7 @@ void nmi_shootdown_cpus(nmi_shootdown_cb callback)
> {
> unsigned long msecs;
>
> - local_irq_disable();
> + raw_force_local_irq_disable();
>
> /*
> * Avoid certain doom if a shootdown already occurred; re-registering
> diff --git a/arch/x86/mm/fault.c b/arch/x86/mm/fault.c
> index aa88370ce739..3164eab7cecd 100644
> --- a/arch/x86/mm/fault.c
> +++ b/arch/x86/mm/fault.c
> @@ -1486,7 +1486,8 @@ handle_page_fault(struct pt_regs *regs, unsigned long error_code,
> * page fault handling might have reenabled interrupts,
> * make sure to disable them again.
> */
> - local_irq_disable();
> + if (!irqs_disabled())
> + local_irq_disable();
> }
>
> DEFINE_IDTENTRY_RAW_ERRORCODE(exc_page_fault)
> diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c
> index 132a9df98471..f13e88fb7a78 100644
> --- a/drivers/acpi/sleep.c
> +++ b/drivers/acpi/sleep.c
> @@ -1095,7 +1095,7 @@ static int acpi_power_off(struct sys_off_data *data)
> {
> /* acpi_sleep_prepare(ACPI_STATE_S5) should have already been called */
> pr_debug("%s called\n", __func__);
> - local_irq_disable();
> + raw_force_local_irq_disable();
> acpi_enter_sleep_state(ACPI_STATE_S5);
> return NOTIFY_DONE;
> }
> diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
> index 3bf969ad8fe0..b54afbd6e073 100644
> --- a/include/linux/interrupt.h
> +++ b/include/linux/interrupt.h
> @@ -594,13 +594,14 @@ struct softirq_action
>
> asmlinkage void do_softirq(void);
> asmlinkage void __do_softirq(void);
> +void do_softirq_irqsoff(void);
>
> #ifdef CONFIG_PREEMPT_RT
> extern void do_softirq_post_smp_call_flush(unsigned int was_pending);
> #else
> static inline void do_softirq_post_smp_call_flush(unsigned int unused)
> {
> - do_softirq();
> + do_softirq_irqsoff();
> }
> #endif
>
> diff --git a/include/linux/irq-entry-common.h b/include/linux/irq-entry-common.h
> index 0bb6c03481fa..ba5210fab31f 100644
> --- a/include/linux/irq-entry-common.h
> +++ b/include/linux/irq-entry-common.h
> @@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_eqs(void) { return false; }
> */
> static __always_inline void enter_from_user_mode(struct pt_regs *regs)
> {
> + __preempt_count_inc_hardirqs_disable();
> arch_enter_from_user_mode(regs);
> lockdep_hardirqs_off(CALLER_ADDR0);
>
> @@ -275,6 +276,7 @@ static __always_inline void exit_to_user_mode(void)
> user_enter_irqoff();
> arch_exit_to_user_mode();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> @@ -385,6 +387,8 @@ static __always_inline irqentry_state_t irqentry_enter_from_kernel_mode(struct p
> .exit_rcu = false,
> };
>
> + __preempt_count_inc_hardirqs_disable();
> +
> /*
> * If this entry hit the idle task invoke ct_irq_enter() whether
> * RCU is watching or not.
> @@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, irqentry_state_
> instrumentation_end();
> ct_irq_exit();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> return;
> }
>
> @@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, irqentry_state_
> if (state.exit_rcu)
> ct_irq_exit();
> }
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> diff --git a/include/linux/irqflags.h b/include/linux/irqflags.h
> index 57b074e0cfbb..dd55786768d1 100644
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -13,6 +13,7 @@
> #define _LINUX_TRACE_IRQFLAGS_H
>
> #include <linux/irqflags_types.h>
> +#include <linux/preempt.h>
> #include <linux/typecheck.h>
> #include <linux/cleanup.h>
> #include <asm/irqflags.h>
> @@ -163,33 +164,149 @@ extern void warn_bogus_irq_restore(void);
> #endif
>
> /*
> - * Wrap the arch provided IRQ routines to provide appropriate checks.
> + * Wrap the architecture specific routines to provide appropriate checks.
> */
> -#define raw_local_irq_disable() arch_local_irq_disable()
> -#define raw_local_irq_enable() arch_local_irq_enable()
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +
> +// FIXME: Convert this into a proper debug mechanism
> +#define debug_assert(c) \
> +do { \
> + WARN_ON(!(c)); \
> +} while (0)
> +
> +static __always_inline void raw_local_irq_disable(void)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == 0);
> + arch_local_irq_disable();
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
> +
> +static __always_inline void raw_force_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
> +
> +static __always_inline void raw_local_irq_enable(void)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET);
> + __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> + arch_local_irq_enable();
> +}
> +
> +static __always_inline unsigned long __raw_local_irq_save(void)
> +{
> + unsigned int cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
> +
> + debug_assert(cnt != HARDIRQ_DISABLE_MASK);
> +
> + if (!cnt)
> + arch_local_irq_disable();
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +
> + return cnt;
> +}
> +
> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
> +
> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> + arch_local_irq_enable();
> +}
> +
So we can change the semantics of local_irq_disable(),
local_irq_enable(), local_irq_save() and local_irq_restore()? Nice!
local_irq_{en,dis}able() are no longer idempotent, and
local_irq_{save,restore}() have to pair with each other or a
local_irq_{en,dis}able(). This would make things much easier. And TBH, I
never think this is an option because there could be so much code not
obeying this (at least not on day 1). Now I see your point on getting
the design correct at the first place :D
Regards,
Boqun
> +static __always_inline unsigned long __raw_local_save_flags(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
> +{
> + return !!cnt;
> +}
> +
> +static __always_inline bool raw_irqs_disabled(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline void raw_safe_halt(void)
> +{
> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == HARDIRQ_DISABLE_OFFSET);
> + __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> + arch_safe_halt();
> +}
> +
> +#else
> +
> +static __always_inline void raw_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> +}
> +
> +static __always_inline void raw_force_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> +}
> +
> +static __always_inline void raw_local_irq_enable(void)
> +{
> + arch_local_irq_enable();
> +}
> +
> +static __always_inline unsigned long __raw_local_irq_save(void)
> +{
> + return arch_local_irq_save();
> +}
> +
> +static __always_inline void __raw_local_irq_restore(unsigned long flags)
> +{
> + arch_local_irq_restore(flags);
> +}
> +
> +static __always_inline unsigned long __raw_local_save_flags(void)
> +{
> + return arch_local_save_flags();
> +}
> +
> +static __always_inline bool __raw_irqs_disabled_flags(unsigned long flags)
> +{
> + return arch_irqs_disabled_flags(flags);
> +}
> +
> +static __always_inline bool raw_irqs_disabled(void)
> +{
> + return arch_irqs_disabled();
> +}
> +
> +static __always_inline void raw_safe_halt(void)
> +{
> + arch_safe_halt();
> +}
> +
> +#endif
> +
> #define raw_local_irq_save(flags) \
> do { \
> typecheck(unsigned long, flags); \
> - flags = arch_local_irq_save(); \
> + flags = __raw_local_irq_save(); \
> } while (0)
> #define raw_local_irq_restore(flags) \
> do { \
> typecheck(unsigned long, flags); \
> raw_check_bogus_irq_restore(); \
> - arch_local_irq_restore(flags); \
> + __raw_local_irq_restore(flags); \
> } while (0)
> #define raw_local_save_flags(flags) \
> do { \
> typecheck(unsigned long, flags); \
> - flags = arch_local_save_flags(); \
> + flags = __raw_local_save_flags(); \
> } while (0)
> #define raw_irqs_disabled_flags(flags) \
> ({ \
> typecheck(unsigned long, flags); \
> - arch_irqs_disabled_flags(flags); \
> + __raw_irqs_disabled_flags(flags); \
> })
> -#define raw_irqs_disabled() (arch_irqs_disabled())
> -#define raw_safe_halt() arch_safe_halt()
>
> /*
> * The local_irq_*() APIs are equal to the raw_local_irq*()
> diff --git a/include/linux/preempt.h b/include/linux/preempt.h
> index 2e689de7b29a..c953cdfb3cc2 100644
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -54,31 +54,31 @@
> * NMI_MASK: 0xf0000000
> * (PREEMPT_NEED_RESCHED is in a different word)
> */
> -#define PREEMPT_BITS 8
> -#define SOFTIRQ_BITS 8
> +#define PREEMPT_BITS 8
> +#define SOFTIRQ_BITS 8
> #define HARDIRQ_DISABLE_BITS 8
> -#define HARDIRQ_BITS 4
> -#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
> +#define HARDIRQ_BITS 4
> +#define NMI_BITS (1 + 3 * IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
>
> -#define PREEMPT_SHIFT 0
> -#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> +#define PREEMPT_SHIFT 0
> +#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> #define HARDIRQ_DISABLE_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
> -#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
> -#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
> +#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
>
> -#define __IRQ_MASK(x) ((1UL << (x))-1)
> +#define __IRQ_MASK(x) ((1UL << (x))-1)
>
> -#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> -#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> +#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> +#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_DISABLE_MASK (__IRQ_MASK(HARDIRQ_DISABLE_BITS) << HARDIRQ_DISABLE_SHIFT)
> -#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> -#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
> +#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> +#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
>
> -#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> -#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> +#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> +#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> #define HARDIRQ_DISABLE_OFFSET (1UL << HARDIRQ_DISABLE_SHIFT)
> -#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> -#define NMI_OFFSET (1UL << NMI_SHIFT)
> +#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> +#define NMI_OFFSET (1UL << NMI_SHIFT)
>
> #define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
>
> @@ -90,18 +90,29 @@
> *
> * Reset by start_kernel()->sched_init()->init_idle()->init_idle_preempt_count().
> */
> +
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +
> +#define INIT_PREEMPT_COUNT (PREEMPT_OFFSET + HARDIRQ_DISABLE_OFFSET)
> +#define SCHED_PREEMPT_COUNT (2 * PREEMPT_DISABLE_OFFSET + HARDIRQ_DISABLE_OFFSET)
> +
> +#else
> +
> #define INIT_PREEMPT_COUNT PREEMPT_OFFSET
> +#define SCHED_PREEMPT_COUNT (2 * PREEMPT_DISABLE_OFFSET)
> +
> +#endif
>
> /*
> * Initial preempt_count value; reflects the preempt_count schedule invariant
> * which states that during context switches:
> *
> - * preempt_count() == 2*PREEMPT_DISABLE_OFFSET
> + * preempt_count() == SCHED_PREEMPT_COUNT
> *
> - * Note: PREEMPT_DISABLE_OFFSET is 0 for !PREEMPT_COUNT kernels.
> + * Note: SCHED_PREEMPT_COUNT is 0 for !PREEMPT_COUNT kernels.
> * Note: See finish_task_switch().
> */
> -#define FORK_PREEMPT_COUNT (2*PREEMPT_DISABLE_OFFSET + PREEMPT_ENABLED)
> +#define FORK_PREEMPT_COUNT (SCHED_PREEMPT_COUNT + PREEMPT_ENABLED)
>
> /* preempt_count() and related functions, depends on PREEMPT_NEED_RESCHED */
> #include <asm/preempt.h>
> @@ -168,6 +179,16 @@ static __always_inline unsigned char interrupt_context_level(void)
> #define in_softirq() (softirq_count())
> #define in_interrupt() (irq_count())
>
> +/*
> + * Check whether a fault happened in an atomic context. Depending on
> + * CONFIG_PREEMPT_COUNT and CONFIG_PREEMPTION this check might be useless.
> + */
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +# define fault_in_atomic() (preempt_count() != HARDIRQ_DISABLE_OFFSET)
> +#else
> +# define fault_in_atomic() in_atomic()
> +#endif
> +
> /*
> * The preempt_count offset after preempt_disable();
> */
> @@ -322,6 +343,21 @@ do { \
>
> #endif /* CONFIG_PREEMPT_COUNT */
>
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +static __always_inline void __preempt_count_inc_hardirqs_disable(void)
> +{
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
> +
> +static __always_inline void __preempt_count_dec_hardirqs_disable(void)
> +{
> + __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> +}
> +#else
> +static __always_inline void __preempt_count_inc_hardirqs_disable(void) { }
> +static __always_inline void __preempt_count_dec_hardirqs_disable(void) { }
> +#endif
> +
> #ifdef MODULE
> /*
> * Modules have no business playing preemption tricks.
> diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h
> index eddbbb65ccc4..086de4c18575 100644
> --- a/include/linux/uaccess.h
> +++ b/include/linux/uaccess.h
> @@ -296,9 +296,9 @@ static inline bool pagefault_disabled(void)
> * stick to pagefault_disabled().
> * Please NEVER use preempt_disable() to disable the fault handler. With
> * !CONFIG_PREEMPT_COUNT, this is like a NOP. So the handler won't be disabled.
> - * in_atomic() will report different values based on !CONFIG_PREEMPT_COUNT.
> + * fault_in_atomic() will report different values based on !CONFIG_PREEMPT_COUNT.
> */
> -#define faulthandler_disabled() (pagefault_disabled() || in_atomic())
> +#define faulthandler_disabled() (pagefault_disabled() || fault_in_atomic())
>
> DEFINE_LOCK_GUARD_0(pagefault, pagefault_disable(), pagefault_enable())
>
> diff --git a/init/main.c b/init/main.c
> index 2613d3f9b3ce..fa84ce260b04 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -991,7 +991,6 @@ void start_kernel(void)
>
> cgroup_init_early();
>
> - local_irq_disable();
> early_boot_irqs_disabled = true;
>
> /*
> diff --git a/kernel/Kconfig.preempt b/kernel/Kconfig.preempt
> index f294dad43bd7..c44607990219 100644
> --- a/kernel/Kconfig.preempt
> +++ b/kernel/Kconfig.preempt
> @@ -152,6 +152,15 @@ config PREEMPT_DYNAMIC
> Interesting if you want the same pre-built kernel should be used for
> both Server and Desktop workloads.
>
> +config ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
> + bool
> +
> +config PREEMPT_COUNT_IRQFLAGS
> + bool "Enable reference counted interrupt disable/enable mechanisms"
> + depends on ARCH_SUPPORTS_PREEMPT_COUNT_IRQFLAGS
> + help
> + FIXME: Add some useful blurb
> +
> config SCHED_CORE
> bool "Core Scheduling for SMT"
> depends on SCHED_SMT
> diff --git a/kernel/entry/common.c b/kernel/entry/common.c
> index e3d381fd3d25..3c93ce7f86f6 100644
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_enter(struct pt_regs *regs)
> {
> irqentry_state_t irq_state;
>
> + __preempt_count_inc_hardirqs_disable();
> irq_state.lockdep = lockdep_hardirqs_enabled();
>
> __nmi_enter();
> @@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt_regs *regs, irqentry_state_t irq_state)
> if (irq_state.lockdep)
> lockdep_hardirqs_on(CALLER_ADDR0);
> __nmi_exit();
> + __preempt_count_dec_hardirqs_disable();
> }
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index f78275192036..b6d14feaaf56 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -5343,7 +5343,7 @@ static struct rq *finish_task_switch(struct task_struct *prev)
> *
> * Also, see FORK_PREEMPT_COUNT.
> */
> - if (WARN_ONCE(preempt_count() != 2*PREEMPT_DISABLE_OFFSET,
> + if (WARN_ONCE(preempt_count() != SCHED_PREEMPT_COUNT,
> "corrupted preempt_count: %s/%d/0x%x\n",
> current->comm, current->pid, preempt_count()))
> preempt_count_set(FORK_PREEMPT_COUNT);
> diff --git a/kernel/sched/idle.c b/kernel/sched/idle.c
> index eb73b65ce6c4..7132320035eb 100644
> --- a/kernel/sched/idle.c
> +++ b/kernel/sched/idle.c
> @@ -380,7 +380,8 @@ static void do_idle(void)
> * RCU relies on this call to be done outside of an RCU read-side
> * critical section.
> */
> - flush_smp_call_function_queue();
> + scoped_guard(irq)
> + flush_smp_call_function_queue();
> schedule_idle();
>
> if (unlikely(klp_patch_pending(current)))
> diff --git a/kernel/smp.c b/kernel/smp.c
> index b696bcc60c08..d51e4bc8da1b 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -665,19 +665,17 @@ static void __flush_smp_call_function_queue(bool warn_cpu_offline)
> void flush_smp_call_function_queue(void)
> {
> unsigned int was_pending;
> - unsigned long flags;
>
> if (llist_empty(this_cpu_ptr(&call_single_queue)))
> return;
>
> - local_irq_save(flags);
> + lockdep_assert_irqs_disabled();
> +
> /* Get the already pending soft interrupts for RT enabled kernels */
> was_pending = local_softirq_pending();
> __flush_smp_call_function_queue(true);
> if (local_softirq_pending())
> do_softirq_post_smp_call_flush(was_pending);
> -
> - local_irq_restore(flags);
> }
>
> static int __smp_call_function_single(int cpu, smp_call_func_t func,
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index e1a773e3eb4e..33f82d03ca2b 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -455,7 +455,10 @@ void __local_bh_enable_ip(unsigned long ip, unsigned int cnt)
> * Run softirq if any pending. And do it in its own stack
> * as we may be calling this deep in a task call stack already.
> */
> - do_softirq();
> + if (IS_ENABLED(CONFIG_TRACE_IRQFLAGS))
> + do_softirq_irqsoff();
> + else
> + do_softirq();
> }
>
> preempt_count_dec();
> @@ -517,20 +520,21 @@ static inline void invoke_softirq(void)
>
> asmlinkage __visible void do_softirq(void)
> {
> - __u32 pending;
> - unsigned long flags;
> -
> if (in_interrupt())
> return;
>
> - local_irq_save(flags);
> + guard(irqsave)();
> + if (local_softirq_pending())
> + do_softirq_own_stack();
> +}
>
> - pending = local_softirq_pending();
> +void do_softirq_irqsoff(void)
> +{
> + if (in_interrupt())
> + return;
>
> - if (pending)
> + if (local_softirq_pending())
> do_softirq_own_stack();
> -
> - local_irq_restore(flags);
> }
>
> #endif /* !CONFIG_PREEMPT_RT */
> diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
> index 530d61257b9a..977dd8928934 100644
> --- a/kernel/time/hrtimer.c
> +++ b/kernel/time/hrtimer.c
> @@ -2068,7 +2068,7 @@ static void __run_hrtimer(struct hrtimer_cpu_base *cpu_base, struct hrtimer_cloc
>
> lockdep_hrtimer_exit(expires_in_hardirq);
> trace_hrtimer_expire_exit(timer);
> - raw_spin_lock_irq(&cpu_base->lock);
> + raw_spin_lock_irqsave(&cpu_base->lock, flags);
>
> /*
> * Note: We clear the running state after enqueue_hrtimer and
>
>
>
On Fri, Aug 28 2026 at 17:45, Boqun Feng wrote:
> On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
>> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
>> +{
>> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
>> +
>> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
>> + arch_local_irq_enable();
>> +}
>> +
>
> So we can change the semantics of local_irq_disable(),
> local_irq_enable(), local_irq_save() and local_irq_restore()? Nice!
> local_irq_{en,dis}able() are no longer idempotent, and
> local_irq_{save,restore}() have to pair with each other or a
> local_irq_{en,dis}able(). This would make things much easier. And TBH, I
> never think this is an option because there could be so much code not
> obeying this (at least not on day 1). Now I see your point on getting
> the design correct at the first place :D
Compared to the insanities I had to handle almost 20 years ago when I
tried that this has become much easier because lockdep and RT made quite
some of the nastier lock/local_irq games go away.
There are probably a few other places in cpu idle drivers or in dark
half maintained driver implementations which might need some care, but I
expect the overall fallout to be managable. The debug mechanisms should
help to identify them quickly.
On Sat, Aug 29, 2026 at 10:44:28PM +0200, Thomas Gleixner wrote:
> On Fri, Aug 28 2026 at 17:45, Boqun Feng wrote:
> > On Sat, Aug 29, 2026 at 01:11:56AM +0200, Thomas Gleixner wrote:
> >> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
> >> +{
> >> + debug_assert((preempt_count() & HARDIRQ_DISABLE_MASK) == (cnt + HARDIRQ_DISABLE_OFFSET));
> >> +
> >> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> >> + arch_local_irq_enable();
> >> +}
> >> +
> >
> > So we can change the semantics of local_irq_disable(),
> > local_irq_enable(), local_irq_save() and local_irq_restore()? Nice!
> > local_irq_{en,dis}able() are no longer idempotent, and
> > local_irq_{save,restore}() have to pair with each other or a
> > local_irq_{en,dis}able(). This would make things much easier. And TBH, I
> > never think this is an option because there could be so much code not
> > obeying this (at least not on day 1). Now I see your point on getting
> > the design correct at the first place :D
>
> Compared to the insanities I had to handle almost 20 years ago when I
> tried that this has become much easier because lockdep and RT made quite
> some of the nastier lock/local_irq games go away.
>
> There are probably a few other places in cpu idle drivers or in dark
> half maintained driver implementations which might need some care, but I
> expect the overall fallout to be managable. The debug mechanisms should
> help to identify them quickly.
>
Yeah, that's something I was missing, I was too afraid to change API
semantics, but with reasonable debug mechanisms, at least users would
have a pointer to resolve the use case issues. Lesson learned.
I will fix the Rust's side compile error.
Regards,
Boqun
On Fri, Aug 28, 2026 at 12:52:26AM +0200, Thomas Gleixner wrote:
> --- a/include/linux/irq-entry-common.h
> +++ b/include/linux/irq-entry-common.h
> @@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_
> */
> static __always_inline void enter_from_user_mode(struct pt_regs *regs)
> {
> + __preempt_count_inc_hardirqs_disable();
> arch_enter_from_user_mode(regs);
> lockdep_hardirqs_off(CALLER_ADDR0);
>
> @@ -275,6 +276,7 @@ static __always_inline void exit_to_user
> user_enter_irqoff();
> arch_exit_to_user_mode();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> @@ -385,6 +387,8 @@ static __always_inline irqentry_state_t
> .exit_rcu = false,
> };
>
> + __preempt_count_inc_hardirqs_disable();
> +
> /*
> * If this entry hit the idle task invoke ct_irq_enter() whether
> * RCU is watching or not.
> @@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preem
> instrumentation_end();
> ct_irq_exit();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> return;
> }
>
> @@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preem
> if (state.exit_rcu)
> ct_irq_exit();
> }
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_en
> {
> irqentry_state_t irq_state;
>
> + __preempt_count_inc_hardirqs_disable();
> irq_state.lockdep = lockdep_hardirqs_enabled();
>
> __nmi_enter();
> @@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt
> if (irq_state.lockdep)
> lockdep_hardirqs_on(CALLER_ADDR0);
> __nmi_exit();
> + __preempt_count_dec_hardirqs_disable();
> }
So I was thinking about this, and can't we get away with not doing this?
That is, simply leave DISABLED_OFFSET set while in userspace?
We always exit to userspace with IRQs disabled, and every entry will
disable them anyway, so they match up, might as well make use of that,
no?
On Fri, Aug 28, 2026 at 12:52:26AM +0200, Thomas Gleixner wrote:
> On Thu, Aug 27 2026 at 12:41, Boqun Feng wrote:
> > On Thu, Aug 27, 2026 at 08:15:44PM +0200, Thomas Gleixner wrote:
> >> > For now I will reverse the order and remove the additional checking in
> >> > softirq to fix the softirq pending issue.
> >>
> >> That "fixes" another nasty bug which was latent for weeks and people
> >> could not get a handle on it because it was absolutely not
> >> reproducible. Given all that I'm absolutely not convinced that there
> >> isn't another pile of latent surprises lurking.
> >>
> >> Aside of that I'm worried about having this new counter exposed in the
> >> current state of affairs. Nothing prevents arbitrary code from using
> >> hardirq_disable_count(), which is definitely faster than
> >> irqs_disabled(), but returns a random value depending on context. That's
> >
> > Random how? Are you saying in the current (wrong) order? Because after
> > reversing the order, hardirq_disable_count() != 0 means the interrupt
> > has been disabled, no?
> >
> > But I checked, actually with the reverse order, we don't need
> > hardirq_disable_count(), so we can remove it entirely. Will send a
> > follow up patch on this.
>
> The point is that the counter is only valid when used within the limits
> of the current coverage. Other than that it is not:
>
> spin_lock_irq() // or any other non-covered mechanism
> // observes 0
> cnt = preempt_count() & HARDIRQ_DISABLE_MASK;
>
> That's inconsistent and therefore it is a random number, no?
>
> You have no way to prevent that this happens and if it does it becomes a
> nightmare to debug for everyone. Guess who got the bug reports about
> preemption counter issues and local softirq pending messages in his
> inbox and dealt with them.
>
> There is a world outside of your safe rust zone and that needs to be
> safe too. This half finished attempt to make Rust work is absolutely
> not and I have zero interrest to deal with the fallout.
>
> It's not safe and no extra hacks will make it safe. Which means it is
> not ready. So the only sensible thing is to revert everything which
> touches that section of preempt_count() and provides interfaces.
>
> As this annoyed me, I rumaged through my poison cabinet and found the
> old patches again. They obviously don't apply anymore but I found the
> hints which corners need some care. With the generic entry code that
> also got way simpler.
>
> So I sat down and reverted
>
> 1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
> e901c1510e24 ("irq,spin_lock: Add counted interrupt disabling/enabling")
>
> and then hacked it up just to see how far I get before vanishing to bed.
>
> Three hours later it surprisingly booted right away into a full distro
> kernel and survived kernel builds and a few test cases. :)
>
> Obviously I did not do any serious testing on it, but I wanted to share
> it as a starting point and food for thoughts.
>
My biggest concern is how you are going to handle the oddballs I
mentioned in another thread, especially when you need to fix the users.
Because this would lead to a all-or-nothing solution: unless we resolve
all the oddballs, we cannot enable this.
> Yes, it needs to be enabled per architecture as the preempt counter
> initialization is architecture specific and it requires generic entry
> code. But those are not uncommon prerequisites and an incentive for
> architecture people to get their act together.
>
> But it is fully consistent and the fully refcounted thing can be
> built on top of it. If you look carefuly you'll notice that
> __raw_local_irq_disable/enable() are just optimized versions of
> __raw_local_irq_save/restore() as they don't have the conditionals, so
> they can be unified completely at least for debug builds or in general
> when it turns out that the overhead is neglible.
>
> There is a wide range of optimizations possible with that especially by
> combining preempt/interrupt modifications into one operation and
> rescheduling without changing the preemption counter in the first
> place. Which is what I hinted to in the mail you linked earlier. I'm so
> tempted to hack that up tomorrow once my brain is less fried than now
> and after I exposed it to some serious testing.
>
If you did, looking forwards to it, I can help enable that for other
architectures if needed.
Regards,
Boqun
> Thanks,
>
> tglx
> ---
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -318,6 +318,7 @@ config X86
> select PCI_DOMAINS if PCI
> select PCI_LOCKLESS_CONFIG if PCI
> select PERF_EVENTS
> + select PREEMPT_COUNT_IRQFLAGS
> select RTC_LIB
> select RTC_MC146818_LIB
> select SPARSE_IRQ
> --- a/arch/x86/include/asm/preempt.h
> +++ b/arch/x86/include/asm/preempt.h
> @@ -61,8 +61,8 @@ static __always_inline void preempt_coun
> */
> #define init_task_preempt_count(p) do { } while (0)
>
> -#define init_idle_preempt_count(p, cpu) do { \
> - per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED; \
> +#define init_idle_preempt_count(p, cpu) do { \
> + per_cpu(__preempt_count, (cpu)) = PREEMPT_DISABLED | HARDIRQ_DISABLE_OFFSET; \
> } while (0)
>
> /*
> --- a/include/linux/irq-entry-common.h
> +++ b/include/linux/irq-entry-common.h
> @@ -97,6 +97,7 @@ static __always_inline bool arch_in_rcu_
> */
> static __always_inline void enter_from_user_mode(struct pt_regs *regs)
> {
> + __preempt_count_inc_hardirqs_disable();
> arch_enter_from_user_mode(regs);
> lockdep_hardirqs_off(CALLER_ADDR0);
>
> @@ -275,6 +276,7 @@ static __always_inline void exit_to_user
> user_enter_irqoff();
> arch_exit_to_user_mode();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> @@ -385,6 +387,8 @@ static __always_inline irqentry_state_t
> .exit_rcu = false,
> };
>
> + __preempt_count_inc_hardirqs_disable();
> +
> /*
> * If this entry hit the idle task invoke ct_irq_enter() whether
> * RCU is watching or not.
> @@ -498,6 +502,7 @@ irqentry_exit_to_kernel_mode_after_preem
> instrumentation_end();
> ct_irq_exit();
> lockdep_hardirqs_on(CALLER_ADDR0);
> + __preempt_count_dec_hardirqs_disable();
> return;
> }
>
> @@ -514,6 +519,7 @@ irqentry_exit_to_kernel_mode_after_preem
> if (state.exit_rcu)
> ct_irq_exit();
> }
> + __preempt_count_dec_hardirqs_disable();
> }
>
> /**
> --- a/include/linux/irqflags.h
> +++ b/include/linux/irqflags.h
> @@ -13,6 +13,7 @@
> #define _LINUX_TRACE_IRQFLAGS_H
>
> #include <linux/irqflags_types.h>
> +#include <linux/preempt.h>
> #include <linux/typecheck.h>
> #include <linux/cleanup.h>
> #include <asm/irqflags.h>
> @@ -165,31 +166,124 @@ extern void warn_bogus_irq_restore(void)
> /*
> * Wrap the arch provided IRQ routines to provide appropriate checks.
> */
> -#define raw_local_irq_disable() arch_local_irq_disable()
> -#define raw_local_irq_enable() arch_local_irq_enable()
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +static __always_inline void raw_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> + preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
> +
> +static __always_inline void raw_local_irq_enable(void)
> +{
> + preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> + arch_local_irq_enable();
> +}
> +
> +static __always_inline unsigned long __raw_local_irq_save(void)
> +{
> + unsigned long cnt = preempt_count();
> +
> + if (!(cnt & HARDIRQ_DISABLE_MASK))
> + arch_local_irq_disable();
> + preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +
> + // Probably not even needed unless something feeds 'flags' into
> + // irqs_disabled_flags()
> + return cnt;
> +}
> +
> +static __always_inline void __raw_local_irq_restore(unsigned long cnt)
> +{
> + if (!(__preempt_count_sub_return(HARDIRQ_DISABLE_OFFSET) & HARDIRQ_DISABLE_MASK))
> + arch_local_irq_enable();
> +}
> +
> +static __always_inline unsigned long __raw_local_save_flags(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline bool __raw_irqs_disabled_flags(unsigned long cnt)
> +{
> + return !!cnt;
> +}
> +
> +static __always_inline bool raw_irqs_disabled(void)
> +{
> + return preempt_count() & HARDIRQ_DISABLE_MASK;
> +}
> +
> +static __always_inline void raw_safe_halt(void)
> +{
> + preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> + arch_safe_halt();
> +}
> +
> +#else
> +
> +static __always_inline void raw_local_irq_disable(void)
> +{
> + arch_local_irq_disable();
> +}
> +
> +static __always_inline void raw_local_irq_enable(void)
> +{
> + arch_local_irq_enable();
> +}
> +
> +static __always_inline unsigned long __raw_local_irq_save(void)
> +{
> + return arch_local_irq_save();
> +}
> +
> +static __always_inline void __raw_local_irq_restore(unsigned long flags)
> +{
> + arch_local_irq_restore(flags);
> +}
> +
> +static __always_inline unsigned long __raw_local_save_flags(void)
> +{
> + return arch_local_save_flags();
> +}
> +
> +static __always_inline bool __raw_irqs_disabled_flags(unsigned long flags)
> +{
> + return arch_irqs_disabled_flags(flags);
> +}
> +
> +static __always_inline bool raw_irqs_disabled(void)
> +{
> + return arch_irqs_disabled();
> +}
> +
> +static __always_inline void raw_safe_halt(void)
> +{
> + arch_safe_halt();
> +}
> +
> +#endif
> +
> #define raw_local_irq_save(flags) \
> do { \
> typecheck(unsigned long, flags); \
> - flags = arch_local_irq_save(); \
> + flags = __raw_local_irq_save(); \
> } while (0)
> #define raw_local_irq_restore(flags) \
> do { \
> typecheck(unsigned long, flags); \
> raw_check_bogus_irq_restore(); \
> - arch_local_irq_restore(flags); \
> + __raw_local_irq_restore(flags); \
> } while (0)
> #define raw_local_save_flags(flags) \
> do { \
> typecheck(unsigned long, flags); \
> - flags = arch_local_save_flags(); \
> + flags = __raw_local_save_flags(); \
> } while (0)
> #define raw_irqs_disabled_flags(flags) \
> ({ \
> typecheck(unsigned long, flags); \
> - arch_irqs_disabled_flags(flags); \
> + __raw_irqs_disabled_flags(flags); \
> })
> -#define raw_irqs_disabled() (arch_irqs_disabled())
> -#define raw_safe_halt() arch_safe_halt()
>
> /*
> * The local_irq_*() APIs are equal to the raw_local_irq*()
> --- a/include/linux/preempt.h
> +++ b/include/linux/preempt.h
> @@ -54,31 +54,31 @@
> * NMI_MASK: 0xf0000000
> * (PREEMPT_NEED_RESCHED is in a different word)
> */
> -#define PREEMPT_BITS 8
> -#define SOFTIRQ_BITS 8
> +#define PREEMPT_BITS 8
> +#define SOFTIRQ_BITS 8
> #define HARDIRQ_DISABLE_BITS 8
> -#define HARDIRQ_BITS 4
> -#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
> +#define HARDIRQ_BITS 4
> +#define NMI_BITS (1 + 3*IS_ENABLED(CONFIG_HAS_SEPARATE_PREEMPT_RESCHED_BITS))
>
> -#define PREEMPT_SHIFT 0
> -#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> +#define PREEMPT_SHIFT 0
> +#define SOFTIRQ_SHIFT (PREEMPT_SHIFT + PREEMPT_BITS)
> #define HARDIRQ_DISABLE_SHIFT (SOFTIRQ_SHIFT + SOFTIRQ_BITS)
> -#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
> -#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
> +#define HARDIRQ_SHIFT (HARDIRQ_DISABLE_SHIFT + HARDIRQ_DISABLE_BITS)
> +#define NMI_SHIFT (HARDIRQ_SHIFT + HARDIRQ_BITS)
>
> -#define __IRQ_MASK(x) ((1UL << (x))-1)
> +#define __IRQ_MASK(x) ((1UL << (x))-1)
>
> -#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> -#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> +#define PREEMPT_MASK (__IRQ_MASK(PREEMPT_BITS) << PREEMPT_SHIFT)
> +#define SOFTIRQ_MASK (__IRQ_MASK(SOFTIRQ_BITS) << SOFTIRQ_SHIFT)
> #define HARDIRQ_DISABLE_MASK (__IRQ_MASK(HARDIRQ_DISABLE_BITS) << HARDIRQ_DISABLE_SHIFT)
> -#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> -#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
> +#define HARDIRQ_MASK (__IRQ_MASK(HARDIRQ_BITS) << HARDIRQ_SHIFT)
> +#define NMI_MASK (__IRQ_MASK(NMI_BITS) << NMI_SHIFT)
>
> -#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> -#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> +#define PREEMPT_OFFSET (1UL << PREEMPT_SHIFT)
> +#define SOFTIRQ_OFFSET (1UL << SOFTIRQ_SHIFT)
> #define HARDIRQ_DISABLE_OFFSET (1UL << HARDIRQ_DISABLE_SHIFT)
> -#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> -#define NMI_OFFSET (1UL << NMI_SHIFT)
> +#define HARDIRQ_OFFSET (1UL << HARDIRQ_SHIFT)
> +#define NMI_OFFSET (1UL << NMI_SHIFT)
>
> #define SOFTIRQ_DISABLE_OFFSET (2 * SOFTIRQ_OFFSET)
>
> @@ -90,7 +90,11 @@
> *
> * Reset by start_kernel()->sched_init()->init_idle()->init_idle_preempt_count().
> */
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +#define INIT_PREEMPT_COUNT (PREEMPT_OFFSET + HARDIRQ_DISABLE_OFFSET)
> +#else
> #define INIT_PREEMPT_COUNT PREEMPT_OFFSET
> +#endif
>
> /*
> * Initial preempt_count value; reflects the preempt_count schedule invariant
> @@ -322,6 +326,21 @@ do { \
>
> #endif /* CONFIG_PREEMPT_COUNT */
>
> +#ifdef CONFIG_PREEMPT_COUNT_IRQFLAGS
> +static __always_inline void __preempt_count_inc_hardirqs_disable(void)
> +{
> + __preempt_count_add(HARDIRQ_DISABLE_OFFSET);
> +}
> +
> +static __always_inline void __preempt_count_dec_hardirqs_disable(void)
> +{
> + __preempt_count_sub(HARDIRQ_DISABLE_OFFSET);
> +}
> +#else
> +static __always_inline void __preempt_count_inc_hardirqs_disable(void) { }
> +static __always_inline void __preempt_count_dec_hardirqs_disable(void) { }
> +#endif
> +
> #ifdef MODULE
> /*
> * Modules have no business playing preemption tricks.
> --- a/kernel/Kconfig.preempt
> +++ b/kernel/Kconfig.preempt
> @@ -152,6 +152,9 @@ config PREEMPT_DYNAMIC
> Interesting if you want the same pre-built kernel should be used for
> both Server and Desktop workloads.
>
> +config PREEMPT_COUNT_IRQFLAGS
> + bool
> +
> config SCHED_CORE
> bool "Core Scheduling for SMT"
> depends on SCHED_SMT
> --- a/kernel/entry/common.c
> +++ b/kernel/entry/common.c
> @@ -171,6 +171,7 @@ irqentry_state_t noinstr irqentry_nmi_en
> {
> irqentry_state_t irq_state;
>
> + __preempt_count_inc_hardirqs_disable();
> irq_state.lockdep = lockdep_hardirqs_enabled();
>
> __nmi_enter();
> @@ -202,4 +203,5 @@ void noinstr irqentry_nmi_exit(struct pt
> if (irq_state.lockdep)
> lockdep_hardirqs_on(CALLER_ADDR0);
> __nmi_exit();
> + __preempt_count_dec_hardirqs_disable();
> }
On Tue, Aug 25, 2026 at 04:28:59PM -0700, Boqun Feng wrote:
> On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
> > On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> > > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
> > >>
> > >> While the guards are properly nested, not all wrapped code is nice, as already
> > >> highlighted by that fair.c hunk.
> > >>
> > >> Syzbot found another instance of this pattern in posix_timer_delete(), which
> > >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> > >> Combined with this patch, that goes sideways most spectacular.
> > >>
I'm not saying the posix_timer_delete() implementation has any problem,
but TBH allowing spin_unlock_irq()+spin_lock_irq() inside
scoped_guard(spinlock_irq) is questionable design, and can result into
foot-gun code like:
scoped_guard(spinlock_irq) {
...
spin_unlock_irq();
if (cond)
return; // BOOM, double unlock
spin_lock_irq();
}
Sure, if handling carefully, it won't cause problem, but it undermines
the easy-to-use and less-err-prone features of scoped_guard().
Regards,
Boqun
> > >> Undo this change, until we've developed stronger tools / debug for such issues.
> > >>
> > >
> > > Mainly hand-waving, but if we make _irq(), irqsave(), _disable()
> > > __acquires() different contexts, we may be able to catch these issues at
> > > compile time. I will explore a bit on this.
[...]
On Tue, Aug 25, 2026 at 04:48:03PM -0700, Boqun Feng wrote:
> On Tue, Aug 25, 2026 at 04:28:59PM -0700, Boqun Feng wrote:
> > On Wed, Aug 26, 2026 at 12:59:25AM +0200, Thomas Gleixner wrote:
> > > On Mon, Aug 24 2026 at 18:33, Boqun Feng wrote:
> > > > On Mon, Aug 24, 2026 at 12:55:23PM +0200, Peter Zijlstra wrote:
> > > >>
> > > >> While the guards are properly nested, not all wrapped code is nice, as already
> > > >> highlighted by that fair.c hunk.
> > > >>
> > > >> Syzbot found another instance of this pattern in posix_timer_delete(), which
> > > >> does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
> > > >> Combined with this patch, that goes sideways most spectacular.
> > > >>
>
> I'm not saying the posix_timer_delete() implementation has any problem,
> but TBH allowing spin_unlock_irq()+spin_lock_irq() inside
> scoped_guard(spinlock_irq) is questionable design, and can result into
> foot-gun code like:
>
> scoped_guard(spinlock_irq) {
> ...
> spin_unlock_irq();
> if (cond)
> return; // BOOM, double unlock
> spin_lock_irq();
> }
>
> Sure, if handling carefully, it won't cause problem, but it undermines
> the easy-to-use and less-err-prone features of scoped_guard().
>
One idea is since each scoped_guard() creates a scope guard, the guard
should be used as token for the inner unlock (guard_drop()) and lock
(guard_retake()). So the above code become:
scoped_guard(spinlock_irq, ...) {
guard_drop(spinlock_irq, &scope);
// ^ scope is modified to know that no more unlock is
// needed.
if (cond)
return; // no double unlock
guard_retake(spinlock_irq, &scope, ...);
}
The following shows the idea (only compile test). Also applies to
lock_timer as well.
Regards,
Boqun
--------------------------------->8
diff --git a/include/linux/cleanup.h b/include/linux/cleanup.h
index b1b5698cbf1b..459c533e4cc8 100644
--- a/include/linux/cleanup.h
+++ b/include/linux/cleanup.h
@@ -249,6 +249,19 @@ const volatile void * __must_check_fn(const volatile void *val)
*/
#define retain_and_null_ptr(p) ((void)__get_and_null(p, NULL))
+/*
+ * Drops a scoped guard
+ */
+#define guard_drop(name, p) class_##name##_destructor(p)
+
+/*
+ * Re-takes a scoped guard
+ */
+#define guard_retake(name, p, ...) \
+do { \
+ class_##name##_retake(p, __VA_ARGS__); \
+} while (0)
+
/*
* DEFINE_CLASS(name, type, exit, init, init_args...):
* helper to define the destructor and constructor for a type.
@@ -492,7 +505,10 @@ typedef struct { \
static __always_inline void class_##_name##_destructor(class_##_name##_t *_T) \
__no_context_analysis \
{ \
- _unlock; \
+ if ((_T)->lock) { \
+ _unlock; \
+ (_T)->lock = NULL; \
+ } \
} \
\
__DEFINE_GUARD_LOCK_PTR(_name, &_T->lock)
@@ -505,6 +521,14 @@ class_##_name##_t class_##_name##_constructor(_type *l) \
class_##_name##_t _t = { .lock = l }, *_T = &_t; \
__VA_ARGS__; \
return _t; \
+} \
+static __always_inline __nonnull_args(2) \
+void class_##_name##_retake(class_##_name##_t *_T, _type *l) \
+ __no_context_analysis \
+{ \
+ BUG_ON((_T)->lock); \
+ (_T)->lock = l; \
+ __VA_ARGS__; \
}
#define __DEFINE_LOCK_GUARD_0(_name, ...) \
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 436ba794cc0b..b34e8292e989 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1026,7 +1026,8 @@ static inline void posix_timer_cleanup_ignored(struct k_itimer *tmr)
}
}
-static void posix_timer_delete(struct k_itimer *timer)
+static void posix_timer_delete(struct k_itimer *timer,
+ class_spinlock_irq_t *guard)
{
/*
* Invalidate the timer, remove it from the linked list and remove
@@ -1057,9 +1058,10 @@ static void posix_timer_delete(struct k_itimer *timer)
while (timer->kclock->timer_del(timer) == TIMER_RETRY) {
guard(rcu)();
- spin_unlock_irq(&timer->it_lock);
+
+ guard_drop(spinlock_irq, guard);
timer_wait_running(timer);
- spin_lock_irq(&timer->it_lock);
+ guard_retake(spinlock_irq, guard, &timer->it_lock);
}
}
@@ -1069,8 +1071,14 @@ SYSCALL_DEFINE1(timer_delete, timer_t, timer_id)
struct k_itimer *timer;
scoped_timer_get_or_fail(timer_id) {
+ // Needs a better to "cast" a guard of "lock_timer" to
+ // "spinlock_irq".
+ class_spinlock_irq_t guard = {
+ .lock = &scoped_timer->it_lock,
+ };
+
timer = scoped_timer;
- posix_timer_delete(timer);
+ posix_timer_delete(timer, &guard);
}
/* Remove it from the hash, which frees up the timer ID */
posix_timer_unhash_and_free(timer);
@@ -1101,7 +1109,7 @@ void exit_itimers(struct task_struct *tsk)
/* The timers are not longer accessible via tsk::signal */
hlist_for_each_entry_safe(timer, next, &timers, list) {
scoped_guard (spinlock_irq, &timer->it_lock)
- posix_timer_delete(timer);
+ posix_timer_delete(timer, &scope);
posix_timer_unhash_and_free(timer);
cond_resched();
}
The following commit has been merged into the locking/urgent branch of tip:
Commit-ID: 46094a7708b7945cb7eba9eb887e3ea9757440a7
Gitweb: https://git.kernel.org/tip/46094a7708b7945cb7eba9eb887e3ea9757440a7
Author: Peter Zijlstra <peterz@infradead.org>
AuthorDate: Mon, 24 Aug 2026 12:49:10 +02:00
Committer: Peter Zijlstra <peterz@infradead.org>
CommitterDate: Mon, 24 Aug 2026 12:58:54 +02:00
locking: Revert switching guards to _irq_{disable,enable}()
Revert commit 1b0866874833 ("locking: Switch to _irq_{disable,enable}()
variants in cleanup guards").
While the guards are properly nested, not all wrapped code is nice, as already
highlighted by that fair.c hunk.
Syzbot found another instance of this pattern in posix_timer_delete(), which
does spin_unlock_irq()+spin_lock_irq() inside scoped_guard(spinlock_irq).
Combined with this patch, that goes sideways most spectacular.
Undo this until we've developed stronger tools / debug for such issues.
Fixes: 1b0866874833 ("locking: Switch to _irq_{disable,enable}() variants in cleanup guards")
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
Link: https://patch.msgid.link/20260824105523.GA4121620%40noisy.programming.kicks-ass.net
---
include/linux/spinlock.h | 26 ++++++++++++++------------
kernel/sched/fair.c | 12 ++++++------
2 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h
index 799a8f7..3d405cc 100644
--- a/include/linux/spinlock.h
+++ b/include/linux/spinlock.h
@@ -572,12 +572,12 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_nested, __acquires(_T), __releases(*(raw
#define class_raw_spinlock_nested_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_nested, _T)
DEFINE_LOCK_GUARD_1(raw_spinlock_irq, raw_spinlock_t,
- raw_spin_lock_irq_disable(_T->lock),
- raw_spin_unlock_irq_enable(_T->lock))
+ raw_spin_lock_irq(_T->lock),
+ raw_spin_unlock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq, _T)
-DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq_disable(_T->lock))
+DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irq, _try, raw_spin_trylock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irq_try, _T)
@@ -592,13 +592,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_bh_try, __acquires(_T), __releases(*(raw
#define class_raw_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_bh_try, _T)
DEFINE_LOCK_GUARD_1(raw_spinlock_irqsave, raw_spinlock_t,
- raw_spin_lock_irq_disable(_T->lock),
- raw_spin_unlock_irq_enable(_T->lock))
+ raw_spin_lock_irqsave(_T->lock, _T->flags),
+ raw_spin_unlock_irqrestore(_T->lock, _T->flags),
+ unsigned long flags)
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave, _T)
DEFINE_LOCK_GUARD_1_COND(raw_spinlock_irqsave, _try,
- raw_spin_trylock_irq_disable(_T->lock))
+ raw_spin_trylock_irqsave(_T->lock, _T->flags))
DECLARE_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, __acquires(_T), __releases(*(raw_spinlock_t **)_T))
#define class_raw_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(raw_spinlock_irqsave_try, _T)
@@ -617,13 +618,13 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_try, __acquires(_T), __releases(*(spinlock_t
#define class_spinlock_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_try, _T)
DEFINE_LOCK_GUARD_1(spinlock_irq, spinlock_t,
- spin_lock_irq_disable(_T->lock),
- spin_unlock_irq_enable(_T->lock))
+ spin_lock_irq(_T->lock),
+ spin_unlock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irq_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq, _T)
DEFINE_LOCK_GUARD_1_COND(spinlock_irq, _try,
- spin_trylock_irq_disable(_T->lock))
+ spin_trylock_irq(_T->lock))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irq_try, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irq_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irq_try, _T)
@@ -639,13 +640,14 @@ DECLARE_LOCK_GUARD_1_ATTRS(spinlock_bh_try, __acquires(_T), __releases(*(spinloc
#define class_spinlock_bh_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_bh_try, _T)
DEFINE_LOCK_GUARD_1(spinlock_irqsave, spinlock_t,
- spin_lock_irq_disable(_T->lock),
- spin_unlock_irq_enable(_T->lock))
+ spin_lock_irqsave(_T->lock, _T->flags),
+ spin_unlock_irqrestore(_T->lock, _T->flags),
+ unsigned long flags)
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irqsave_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave, _T)
DEFINE_LOCK_GUARD_1_COND(spinlock_irqsave, _try,
- spin_trylock_irq_disable(_T->lock))
+ spin_trylock_irqsave(_T->lock, _T->flags))
DECLARE_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, __acquires(_T), __releases(*(spinlock_t **)_T))
#define class_spinlock_irqsave_try_constructor(_T) WITH_LOCK_GUARD_1_ATTRS(spinlock_irqsave_try, _T)
diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
index 6d881e5..8dff370 100644
--- a/kernel/sched/fair.c
+++ b/kernel/sched/fair.c
@@ -7253,7 +7253,7 @@ static bool distribute_cfs_runtime(struct cfs_bandwidth *cfs_b)
* period the timer is deactivated until scheduling resumes; cfs_b->idle is
* used to track this state.
*/
-static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun)
+static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun, unsigned long flags)
__must_hold(&cfs_b->lock)
{
int throttled;
@@ -7288,10 +7288,10 @@ static int do_sched_cfs_period_timer(struct cfs_bandwidth *cfs_b, int overrun)
* This check is repeated as we release cfs_b->lock while we unthrottle.
*/
while (throttled && cfs_b->runtime > 0) {
- raw_spin_unlock_irq_enable(&cfs_b->lock);
+ raw_spin_unlock_irqrestore(&cfs_b->lock, flags);
/* we can't nest cfs_b->lock while distributing bandwidth */
throttled = distribute_cfs_runtime(cfs_b);
- raw_spin_lock_irq_disable(&cfs_b->lock);
+ raw_spin_lock_irqsave(&cfs_b->lock, flags);
}
/*
@@ -7399,7 +7399,7 @@ static __always_inline void return_cfs_rq_runtime(struct cfs_rq *cfs_rq)
static void do_sched_cfs_slack_timer(struct cfs_bandwidth *cfs_b)
{
/* confirm we're still not at a refresh boundary */
- scoped_guard(raw_spinlock_irq, &cfs_b->lock) {
+ scoped_guard(raw_spinlock_irqsave, &cfs_b->lock) {
u64 runtime = 0, slice = sched_cfs_bandwidth_slice();
cfs_b->slack_started = false;
@@ -7484,14 +7484,14 @@ static enum hrtimer_restart sched_cfs_period_timer(struct hrtimer *timer)
int idle = 0;
int count = 0;
- guard(raw_spinlock_irq)(&cfs_b->lock);
+ CLASS(raw_spinlock_irqsave, cfsb_guard)(&cfs_b->lock);
for (;;) {
overrun = hrtimer_forward_now(timer, cfs_b->period);
if (!overrun)
break;
- idle = do_sched_cfs_period_timer(cfs_b, overrun);
+ idle = do_sched_cfs_period_timer(cfs_b, overrun, cfsb_guard.flags);
if (++count > 3) {
u64 new, old = ktime_to_ns(cfs_b->period);© 2016 - 2026 Red Hat, Inc.