Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] entry: irqentry_exit_to_kernel_mode_after_preempt() breaks preemptirqsoff - [Medium] softirq: irqentry_hardirq_tail_exit() repeats wakeups with wake_flags = 0 - [Medium] entry: irqentry_hardirq_tail_exit() wakes softirq threads after preempt check --
commit d2af631b0f352ba4a12e0eb8fa80f5ff66476c74 Author: Karl Mehltretter <[email protected]> entry: Classify ordinary hardirq return-tail work as hardirq context This commit stages a fresh hardirq-count attempt and restores lockdep classification across traceable return-tail work on return from ordinary hardirqs. It adds irqentry_hardirq_tail_enter() and irqentry_hardirq_tail_exit() around the post-preemption return tail so return-tail callbacks run as hardirq context. > diff --git a/include/linux/irq-entry-common.h > b/include/linux/irq-entry-common.h > index 6f649f6613312..f51aafc1b49fd 100644 > --- a/include/linux/irq-entry-common.h > +++ b/include/linux/irq-entry-common.h > @@ -383,6 +383,32 @@ irqentry_hardirq_count_state_set(irqentry_state_t *state, > IRQENTRY_HARDIRQ_APPLIED : IRQENTRY_HARDIRQ_REJECTED; > } > > +/** > + * irqentry_hardirq_tail_enter - Stage ordinary-hardirq return-tail state > + * @state: Pointer to state returned by the matching irqentry_hardirq_enter() > + * > + * Make a fresh staged hardirq-count attempt and establish the lockdep > + * classification which traceable return-tail work needs. Accounting, RCU and > + * tick handling belong to the completed interrupt and must not be repeated > + * here. > + */ > +static __always_inline void > +irqentry_hardirq_tail_enter(irqentry_state_t *state) > +{ > + struct irqentry_hardirq_cookie cookie; > + > + if (!state->hardirq) > + return; > + > + cookie = irqentry_hardirq_count_start(); > + lockdep_hardirq_enter(); > + > + instrumentation_begin(); > + irqentry_hardirq_count_finish(cookie); > + instrumentation_end(); > + irqentry_hardirq_count_state_set(state, cookie); > +} [ ... ] > @@ -543,6 +574,31 @@ static __always_inline void > irqentry_hrtimer_rearm_deferred(void) > static __always_inline void > irqentry_exit_to_kernel_mode_after_preempt(struct pt_regs *regs, > irqentry_state_t state) > { > + struct ct_irq_exit_state ct_state = { }; > + > + if (state.hardirq) { > + if (!regs_irqs_disabled(regs)) { > + instrumentation_begin(); > + hrtimer_rearm_deferred(); > + /* Tell the tracer that the return will enable IRQs. */ > + trace_hardirqs_on_prepare(); > + lockdep_hardirqs_on_prepare_irqtail(); > + instrumentation_end(); > + } > + > + if (state.exit_rcu) > + ct_state = ct_irq_exit_prepare(); > + > + irqentry_hardirq_tail_exit(irqentry_hardirq_count_state(state)); [Severity: High] Does ordering trace_hardirqs_on_prepare() before irqentry_hardirq_tail_exit() cause a regression for the preemptirqsoff tracer when returning to preemptible, IRQ-enabled kernel mode? In irqentry_exit_to_kernel_mode(), irqentry_hardirq_tail_enter() stages HARDIRQ_OFFSET in preempt_count() and calls: irqentry_hardirq_count_finish() preempt_latency_start(HARDIRQ_OFFSET) trace_preempt_off() Next, when !regs_irqs_disabled(regs), irqentry_exit_to_kernel_mode_after_preempt() calls trace_hardirqs_on_prepare() while HARDIRQ_OFFSET is still set in preempt_count(). In kernel/trace/trace_irqsoff.c: void tracer_hardirqs_on(unsigned long a0, unsigned long a1) { if (!preempt_trace(preempt_count()) && irq_trace()) stop_critical_timing(a0, a1); } When the preemptirqsoff tracer is active (both CONFIG_IRQSOFF_TRACER and CONFIG_PREEMPT_TRACER enabled, so trace_type has both TRACER_IRQS_OFF and TRACER_PREEMPT_OFF), preempt_trace(preempt_count()) evaluates to true because preempt_count() contains HARDIRQ_OFFSET. As a result, tracer_hardirqs_on() skips stop_critical_timing(), while trace_hardirqs_on_prepare() still clears tracing_irq_cpu. Immediately after that, irqentry_hardirq_tail_exit() calls: irqentry_hardirq_exit_prepare() irqentry_hardirq_count_sub_prepare() preempt_latency_stop(HARDIRQ_OFFSET) trace_preempt_on() tracer_preempt_on() In kernel/trace/trace_irqsoff.c: void tracer_preempt_on(unsigned long a0, unsigned long a1) { if (preempt_trace(preempt_count()) && !irq_trace()) stop_critical_timing(a0, a1); } Because hardware interrupts remain disabled at the CPU until the exception return instruction, irqs_disabled() is still true, so irq_trace() evaluates to true and tracer_preempt_on() also skips stop_critical_timing(). Finally, irqentry_hardirq_exit_finish() removes HARDIRQ_OFFSET via __preempt_count_sub(HARDIRQ_OFFSET). Won't this leave per_cpu(tracing_cpu, cpu) stuck active after returning to preemptible, IRQ-enabled kernel execution, as well as emitting extra preempt_disable, irq_enable, and preempt_enable trace events in inverted order? > diff --git a/kernel/softirq.c b/kernel/softirq.c > index 169c559832501..85b167db37c29 100644 > --- a/kernel/softirq.c > +++ b/kernel/softirq.c > @@ -837,6 +837,27 @@ irq_exit_wake_late_softirqs(unsigned int wake_flags) > irq_exit_wake_ksoftirqd(); > } > > +/** > + * irqentry_hardirq_tail_exit - Close work and remove return-tail state > + * @cookie: Staged hardirq-count cookie from irqentry_hardirq_tail_enter() > + * > + * Run every instrumentable teardown operation, including the final worker > + * closure, before conditionally removing any applied raw hardirq count and > + * ending lockdep nesting. > + */ > +void noinstr > +irqentry_hardirq_tail_exit(struct irqentry_hardirq_cookie cookie) > +{ > + bool do_sub; > + > + instrumentation_begin(); > + do_sub = irqentry_hardirq_exit_prepare(cookie); > + irq_exit_wake_late_softirqs(0); > + instrumentation_end(); > + > + irqentry_hardirq_exit_finish(do_sub); > +} [Severity: Medium] Can passing a hardcoded 0 to irq_exit_wake_late_softirqs(0) in irqentry_hardirq_tail_exit() cause redundant wake_up_process() calls for ksoftirqd and ktimerd? During the earlier main hardirq exit in irq_exit_rcu_preempt_counted_prepare(), __irq_exit_rcu(true) records IRQ_EXIT_WAKE_KSOFTIRQD and IRQ_EXIT_WAKE_KTIMERD in wake_flags so irq_exit_wake_late_softirqs(wake_flags) avoids waking those threads a second time. However, local_softirq_pending() or local_timers_pending_force_th() remains non-zero until the woken thread actually runs on the CPU and clears the pending bits. If irqentry_exit_to_kernel_mode_preempt() does not switch to the worker thread (for example, on any interrupt from an RCU-idle context where state.exit_rcu is true, when the interrupted kernel context has preemption disabled, when regs_irqs_disabled(regs) is true, or when current has higher priority), irqentry_hardirq_tail_exit() runs with those pending bits still set: irq_exit_wake_late_softirqs(0) if (!ksoftirqd_wake_requested) ksoftirqd_wake_requested = irq_exit_wake_ksoftirqd(); if (!ktimerd_wake_requested) ktimerd_wake_requested = irq_exit_wake_timersd(); Because wake_flags is 0, won't this call wakeup_softirqd() or wake_timersd() a second time even when no return-tail callback raised new work, and do so after trace_hardirqs_on_prepare(), ct_irq_exit_prepare(), and irqentry_hardirq_exit_prepare() have already completed? [Severity: Medium] What happens if a return-tail callback raises softirq or forced-thread timer work while returning to a preemptible kernel context? In irqentry_exit_to_kernel_mode(): instrumentation_begin(); irqentry_exit_to_kernel_mode_preempt(regs, state); instrumentation_end(); irqentry_hardirq_tail_enter(&state); irqentry_exit_to_kernel_mode_after_preempt(regs, state); the preemption check in irqentry_exit_to_kernel_mode_preempt() runs before the hardirq return tail is entered. If a callback inside the return tail (such as during hrtimer_rearm_deferred(), trace_hardirqs_on_prepare(), lockdep_hardirqs_on_prepare_irqtail(), or irqentry_hardirq_exit_prepare()) raises softirq or timer work, irq_exit_wake_late_softirqs(0) in irqentry_hardirq_tail_exit() wakes ksoftirqd or ktimerd while HARDIRQ_OFFSET is still set in preempt_count(), setting need_resched() on current. Immediately afterwards, irqentry_hardirq_exit_finish() drops HARDIRQ_OFFSET via __preempt_count_sub(HARDIRQ_OFFSET) without checking for rescheduling. Could this return to a preemptible kernel task with need_resched() set and miss preempting for the newly woken worker thread (including a higher-priority SCHED_FIFO ktimerd/ksoftirqd under PREEMPT_RT or force_irqthreads)? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=10
