Re: [PATCH v2] softirq: Preserve interrupt context during IRQ exit

[Sebastian Andrzej Siewior]

On 2026-09-19 09:51:05 [+0200], Karl Mehltretter wrote:
> On 2026-09-17 17:21:50 [+0200], Sebastian Andrzej Siewior wrote:
> > The breakage is limited to KCSAN & friends within the window during
> > transition to softirq and out. There is nothing else? Well, the timer
> > wake looks wrong in trace, noted.
> 
> I know of nothing that is broken today. It is more than instrumentation
> though. Code that reads the context from preempt_count sees the
> interrupted task in that window. Besides ftrace, KCSAN, KMSAN, KCOV and
> the printk caller id I found:
> 
>  - can_spin_trylock() and local_trylock() on RT refuse hard interrupt
>    context. A trylock on top of a task that is blocked on a lock
>    confuses the PI code. In that window they do not refuse. BPF attached
>    to sched_waking or sched_wakeup reaches them through kmalloc_nolock().
> 
>  - oops_end() and make_task_dead() test in_interrupt(). Today an oops in
>    that window is treated like an oops in task context and kills the
>    interrupted task. With HARDIRQ_OFFSET set it panics with "Fatal
>    exception in interrupt", like an oops in the handler itself.
> 
>  - rcu_read_unlock_special() and raise_softirq_irqoff(). See the end of
>    this mail.
> 
> I'll list these in the changelog. I will also say what the patch does
> not cover. tick_irq_exit() and the other deferred rearm sites still run
> after HARDIRQ_OFFSET is removed.

The "important" part is this fixing something that is broken today or is
it just avoiding fallout. We don't have any memory allocations/ locking
in the mentioned window as far as I know. That would fix things, just
avoid fallout.
The wake-up in that window does record wrong flags in the recorded trace
but I am unsure if this mandates a fix-me-backport for instance.

> The value is the same. Without the casts gcc warns:
> 
>   warning: overflow in conversion from 'long unsigned int' to 'int'
>   changes value from '18446744073692774656' to '-16776960' [-Woverflow]
> 
> -Woverflow is on by default. I'll swap the operands:

Is this some gcc-17 thing? I don't remember that I saw it and I did test
that.

> > that is quite some WARN_ON_ONCE. We would like to see just
> > HARDIRQ_OFFSET at the end. Or SOFTIRQ_OFFSET before the end. One should
> > be enough or the math is wrong.
> 
> softirq_handle_end() will have one. It checks irq_count() ==
> HARDIRQ_OFFSET after the addition. In softirq_handle_begin() I will move
> lockdep_softirqs_off() before the assertion. Then the lockdep softirq
> state is consistent if the assertion fires and printk runs.

Right. I mean you have one state and this what you want test for. I
don't think it make sense to test before and after arithmetics.
I am just not sure if those warnings should be hidden behind
CONFIG_DEBUG_PREEMPT similar as preempt_count_add() does it. Maybe it is
not hot-enough-path to worry about it.

> > Why is this preempt_count() instead irq_count. Why is there
> > IRQ_EXIT_TIMERS? It is almost as the first check except now we would
> > like to ignore the additional softirq_count().
> 
> Yes, that is the intent. irq_count() would skip the wakeup when the
> interrupt hit a BH disabled or softirq serving section. The old test
> did not skip it, and nothing else handles pending_timer_softirq.

I am slightly unsure but I think we want the wakeup of the timer thread
even if we are in a bh-disabled section. If the current task is a
SCHED_OTHER then the wake-up preempt it. If the thread is already woken
then the wake-up will do nothing.

> I'll drop the macro and the raw preempt_count() and use
> 
> 	!in_nmi() && hardirq_count() == HARDIRQ_OFFSET
> 
> This is the old test, evaluated before HARDIRQ_OFFSET is removed. It
> reads like the first check without softirq_count(). I'll add a comment
> that says why softirq_count() is left out.
> 
> I also want to change the order in your code. With HARDIRQ_OFFSET set,
> raise_softirq_irqoff() does not wake ksoftirqd. rcu_read_unlock_special()
> raises RCU_SOFTIRQ instead of setting NEED_RESCHED. Both assume that

It depends if RCU uses softirq _and_ BH was disabled. So I don't see
what is wrong with that.
Anyway, one step at a time with some reasoning why.

> interrupt exit handles pending softirqs. In v2 that is not true for a
> softirq raised inside wake_timersd(), because the wakeup comes after the
> pending check. The timer thread would handle it, because run_ktimerd()
> handles all vectors. I do not want to rely on that, but wake the timer
> thread first:
> 
> 	if (IS_ENABLED(CONFIG_IRQ_FORCED_THREADING) && force_irqthreads() &&
> 	    local_timers_pending_force_th() &&
> 	    !in_nmi() && hardirq_count() == HARDIRQ_OFFSET)
> 		wake_timersd();
> 
> 	if (irq_count() == HARDIRQ_OFFSET && local_softirq_pending()) {
> 		hrtimer_rearm_deferred();
> 		invoke_softirq();
> 	}
> 
> 	preempt_count_sub(HARDIRQ_OFFSET);
> 	tick_irq_exit();
> 
> Today the wakeup already runs before the rearm when no softirq is
> pending. Does the old order have a reason that I do not see? Then I
> keep it and document that the timer thread handles such a softirq.

This only matters for the threadirq case. Here invoke_softirq() will
only wake ksoftirqd and wake_timersd() will only wake the ktimers
thread. There will be no new softirqs added to the mask. This currently
is an ugly catch-all for both sides. Ideally only the softirqs raised by
task X should be handled by task X but the first one will do everything.

> Karl

Sebastian