Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [Medium] entry: irqentry_hardirq_enter() breaks preemptirqsoff critical timing start - [Medium] softirq: invoke_softirq() drops ksoftirqd wake flag after inline handoff --
commit 50eb58fd97f0b74442817ebd3d3092c57116862f Author: Karl Mehltretter <[email protected]> entry: Add the pre-counted ordinary-hardirq entry/exit contract This commit adds staged hardirq entry and retained exit helpers so that ordinary hardirqs can install HARDIRQ_OFFSET and lockdep hardirq state before entry tracing, and keep them installed until all traceable exit callbacks have completed. > diff --git a/kernel/entry/common.c b/kernel/entry/common.c > index 2f5229132466c..9d207c37c5707 100644 > --- a/kernel/entry/common.c > +++ b/kernel/entry/common.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > > #include <linux/futex.h> > +#include <linux/hardirq.h> > #include <linux/highmem.h> > #include <linux/irq-entry-common.h> > #include <linux/jump_label.h> > @@ -120,6 +121,42 @@ noinstr irqentry_state_t irqentry_enter(struct pt_regs > *regs) > return irqentry_enter_from_kernel_mode(regs); > } > > +/** > + * irqentry_hardirq_enter - Establish state for ordinary hardirq entry > + * @regs: Pointer to the interrupted register state > + * > + * Attempt a staged raw hardirq-count addition before anything instrumentable > + * can run and record whether it was applied. Then establish IRQ-disabled and > + * hardirq lockdep state before context tracking or irqflag tracing can > invoke > + * callbacks. The caller must have distinguished an ordinary hardirq from > + * NMI-like entry and must finish the staged attempt with > + * irq_enter_rcu_preempt_counted(). > + * > + * Returns: An opaque object for the matching irqentry exit operation. > + */ > +noinstr irqentry_state_t irqentry_hardirq_enter(struct pt_regs *regs) > +{ > + struct irqentry_hardirq_cookie cookie; > + irqentry_state_t ret; > + > + cookie = irqentry_hardirq_count_start(); > + > + if (user_mode(regs)) { > + ret = (irqentry_state_t) { > + .exit_rcu = false, > + }; > + irqentry_hardirq_count_state_set(&ret, cookie); > + > + __enter_from_user_mode(regs, true); > + rseq_note_user_irq_entry(); > + return ret; > + } > + > + ret = __irqentry_enter_from_kernel_mode(regs, true); > + irqentry_hardirq_count_state_set(&ret, cookie); > + return ret; > +} [Severity: Medium] Does staging HARDIRQ_OFFSET in irqentry_hardirq_enter() before calling __enter_from_user_mode() or __irqentry_enter_from_kernel_mode() prevent the preemptirqsoff latency tracer from starting critical timing when an ordinary hardirq interrupts user mode or preemptible kernel mode? When trace_type has both TRACER_IRQS_OFF and TRACER_PREEMPT_OFF enabled and an interrupt arrives while preempt_count() is 0: 1. irqentry_hardirq_enter() calls irqentry_hardirq_count_start(), which adds HARDIRQ_OFFSET to preempt_count() while hardware interrupts are disabled. 2. __enter_from_user_mode() or __irqentry_enter_from_kernel_mode() then calls trace_hardirqs_off_finish() -> tracer_hardirqs_off(): kernel/trace/trace_irqsoff.c:tracer_hardirqs_off() { if (!preempt_trace(preempt_count()) && irq_trace()) start_critical_timing(a0, a1); } Because HARDIRQ_OFFSET is already present in preempt_count(), preempt_trace(preempt_count()) evaluates to true, so tracer_hardirqs_off() assumes preemption was already disabled before IRQs were turned off and skips start_critical_timing(). 3. Later, irq_enter_rcu_preempt_counted() calls irqentry_hardirq_count_finish() -> preempt_latency_start(HARDIRQ_OFFSET) -> trace_preempt_off() -> tracer_preempt_off(): kernel/trace/trace_irqsoff.c:tracer_preempt_off() { if (preempt_trace(preempt_count()) && !irq_trace()) start_critical_timing(a0, a1); } Because hardware interrupts are disabled, irq_trace() evaluates to true, so tracer_preempt_off() assumes tracer_hardirqs_off() already started critical timing and also skips start_critical_timing(). As a result, neither callback starts critical timing for the hardirq and any subsequent softirq exit processing. > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 7fc2200392cdf..169c559832501 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c [ ... ] > @@ -525,12 +529,12 @@ static inline void ksoftirqd_run_end(void) > local_irq_enable(); > } > > -static inline bool should_wake_ksoftirqd(void) > +static __always_inline bool should_wake_ksoftirqd(void) > { > return true; > } > > -static inline void invoke_softirq(void) > +static inline bool invoke_softirq(void) > { > if (!force_irqthreads() || !__this_cpu_read(ksoftirqd)) { > #ifdef CONFIG_HAVE_IRQ_EXIT_ON_IRQ_STACK > @@ -548,9 +552,11 @@ static inline void invoke_softirq(void) > */ > do_softirq_own_stack(); > #endif > - } else { > - wakeup_softirqd(); > + return false; > } > + > + wakeup_softirqd(); > + return true; > } [Severity: Medium] When invoke_softirq() runs __do_softirq() or do_softirq_own_stack() on !CONFIG_PREEMPT_RT, can returning false unconditionally cause irq_exit_wake_late_softirqs() to repeat a wakeup_softirqd() call that was already performed by handle_softirqs()? Inside handle_softirqs(), if softirqs are re-raised during inline draining and the loop stops because need_resched() is set, the time limit is reached, or max_restart is exhausted, handle_softirqs() wakes ksoftirqd itself while leaving the remaining bits set in local_softirq_pending(): kernel/softirq.c:handle_softirqs() { ... pending = local_softirq_pending(); if (pending) { if (time_before(jiffies, end) && !need_resched() && --max_restart) goto restart; wakeup_softirqd(); } ... } Because invoke_softirq() still returns false after __do_softirq() or do_softirq_own_stack(), __irq_exit_rcu() does not set IRQ_EXIT_WAKE_KSOFTIRQD in wake_flags. When irq_exit_rcu_preempt_counted_prepare() then runs: wake_flags = __irq_exit_rcu(true); do_sub = irqentry_hardirq_exit_prepare(cookie); irq_exit_wake_late_softirqs(wake_flags); irq_exit_wake_late_softirqs() sees ksoftirqd_wake_requested as false while local_softirq_pending() is still non-zero from the earlier handoff, causing irq_exit_wake_ksoftirqd() to call wakeup_softirqd() a second time after irqentry_hardirq_exit_prepare() has already emitted trace_preempt_on(). -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=5
