Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] rv: rq->nr_pinned leak on kunit_skip() under guard(migrate)() in irqstate tests - [Medium] rv: broken nested IRQ filter in irqstate_inline_hardirq_entry() and exit - [Medium] rv: missing local_bh_blocked() check and early wait in hardirq exit tests --
commit 7fb79c597a519150ad675f18bceb7d2417080c8b Author: Karl Mehltretter <[email protected]> rv: Test IRQ-exit softirq provenance and handoff This commit extends the irqstate KUnit test suite to cover inline softirq processing during hardirq exit, hardirq provenance tracking, and monitor synchronization. It also adds tests verifying late work handoff to ksoftirqd and ktimerd at the softirq and hardirq exit boundaries. > diff --git a/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c > b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c > index e3928d10b2825..31e827cdba1c6 100644 > --- a/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c > +++ b/kernel/trace/rv/monitors/irqstate/irqstate_kunit.c [ ... ] > @@ -893,6 +984,39 @@ static bool irqstate_empty_worker_matches(struct > irqstate_empty_worker_ctx *empt > return !empty_ctx->real_worker || softirq_kunit_empty_worker_active(); > } > > +static void irqstate_inline_hardirq_entry(void *data, unsigned int depth) > +{ > + struct irqstate_inline_bh_ctx *inline_ctx = data; > + > + /* Inline softirq processing enables IRQs; ignore a nested real IRQ. */ > + if (!READ_ONCE(inline_ctx->armed) || current != inline_ctx->owner || > + depth != 1) > + return; > + inline_ctx->hardirq_entries++; > + if (inline_ctx->event_index != 0) > + inline_ctx->bad_order = true; > + inline_ctx->event_index++; > + if (hardirq_count() != HARDIRQ_OFFSET || in_serving_softirq() || > + !irqs_disabled()) > + inline_ctx->bad_boundary_context = true; > +} > + > +static void irqstate_inline_hardirq_exit(void *data, unsigned int depth) > +{ > + struct irqstate_inline_bh_ctx *inline_ctx = data; > + > + if (!READ_ONCE(inline_ctx->armed) || current != inline_ctx->owner || > + depth != 1) > + return; > + inline_ctx->hardirq_exits++; > + if (inline_ctx->event_index != 3) > + inline_ctx->bad_order = true; > + inline_ctx->event_index++; > + if (hardirq_count() != HARDIRQ_OFFSET || in_serving_softirq() || > + !irqs_disabled()) > + inline_ctx->bad_boundary_context = true; > +} [Severity: Medium] Will checking depth != 1 in irqstate_inline_hardirq_entry() and irqstate_inline_hardirq_exit() actually filter out a real hardware interrupt that arrives while inline softirq processing runs with interrupts enabled? In the rv_test_irqstate_irq_exit_inline() -> irq_exit_rcu() -> __do_softirq() -> handle_softirqs() path, softirq_handle_begin() replaces HARDIRQ_OFFSET with SOFTIRQ_OFFSET before local_irq_enable() enables interrupts: kernel/softirq.c:softirq_handle_begin() { __preempt_count_sub(HARDIRQ_OFFSET - SOFTIRQ_OFFSET); lockdep_softirqs_off(_RET_IP_); WARN_ON_ONCE(irq_count() != SOFTIRQ_OFFSET); } Because HARDIRQ_OFFSET has been cleared, a real hardware interrupt arriving during the IRQ-enabled softirq window enters irq_enter_rcu() with hardirq_count() equal to HARDIRQ_OFFSET (so depth is 1, while in_serving_softirq() is non-zero), and exits irq_exit_rcu() with depth equal to 1 as well. As a result, depth != 1 is false for that interrupt in both irqstate_inline_hardirq_entry() and irqstate_inline_hardirq_exit(), causing both probes to increment hardirq_entries, hardirq_exits, and event_index and to set bad_order and bad_boundary_context. Should both callbacks also ignore events when in_serving_softirq() is true? [ ... ] > +/* Work first raised at the final hardirq boundary must reach ksoftirqd. */ > +static void rv_test_irqstate_hardirq_exit_handoff(struct kunit *test) > +{ > + struct irqstate_hardirq_exit_ctx *exit_ctx; > + struct rv_kunit_ctx *ctx = test->priv; > + unsigned long flags; > + int ret; > + > + prepare_test(test, &rv_irqstate_ops.mon); > + guard(migrate)(); > + exit_ctx = kunit_kzalloc(test, sizeof(*exit_ctx), GFP_KERNEL); > + KUNIT_ASSERT_NOT_NULL(test, exit_ctx); > + tasklet_setup(&exit_ctx->tasklet, irqstate_hardirq_exit_tasklet); > + exit_ctx->owner = current; > + exit_ctx->owner_cpu = raw_smp_processor_id(); > + exit_ctx->softirqd = this_cpu_ksoftirqd(); > + KUNIT_ASSERT_NOT_NULL(test, exit_ctx->softirqd); > + if (local_softirq_pending() || > + !wait_task_inactive(exit_ctx->softirqd, TASK_INTERRUPTIBLE)) { > + kunit_skip(test, "Requires an idle local ksoftirqd"); > + return; > + } [Severity: High] Does calling kunit_skip() or failing a KUNIT_ASSERT_*() while guard(migrate)() is active leak the per-CPU rq->nr_pinned counter? guard(migrate)() calls migrate_disable(), which increments rq->nr_pinned (this_rq_pinned()) and relies on the C cleanup attribute to invoke migrate_enable() at scope exit. However, kunit_skip() and failing KUNIT_ASSERT_*() macros call kunit_try_catch_throw(): lib/kunit/try-catch.c:kunit_try_catch_throw() { try_catch->try_result = -EFAULT; kthread_exit(0); } Because kunit_try_catch_throw() is noreturn and exits the KUnit runner kthread directly via kthread_exit(0) -> do_exit(0) without unwinding C stack frames, the guard(migrate)() cleanup never runs and do_exit() does not call migrate_enable(). That leaves rq->nr_pinned permanently elevated on the CPU, which makes rq_has_pinned_tasks(rq) stay true and causes subsequent CPU hotplug offline operations to hang in balance_hotplug_wait(). The same pattern occurs in rv_test_irqstate_hardirq_exit_timer_handoff() below, and when force_irqthreads() is enabled on a !CONFIG_PREEMPT_RT kernel to run rv_test_irqstate_hardirq_exit_timer_handoff(), the following test rv_test_irqstate_irq_exit_inline() diverts its softirq to ksoftirqd (ran_inline is false) and deterministically hits kunit_skip() under guard(migrate)(). > + > + ret = register_trace_hardirq_context_exit( > + irqstate_hardirq_exit_raise, exit_ctx); > + KUNIT_ASSERT_EQ(test, ret, 0); > + exit_ctx->attached = true; > + KUNIT_ASSERT_EQ(test, 0, > + kunit_add_action_or_reset(test, > + irqstate_hardirq_exit_cleanup, > + exit_ctx)); > + attach_irqstate_test_probes(); > + KUNIT_ASSERT_EQ(test, 0, > + kunit_add_action_or_reset(test, > detach_irqstate_test_probes, > + NULL)); > + > + local_irq_save(flags); > + if (local_softirq_pending() || task_is_running(exit_ctx->softirqd)) { > + local_irq_restore(flags); > + kunit_skip(test, "Local ksoftirqd stopped being idle"); > + return; > + } [Severity: Medium] Could rv_test_irqstate_hardirq_exit_handoff() and rv_test_irqstate_hardirq_exit_timer_handoff() fail or spuriously skip due to missing a local_bh_blocked() check on CONFIG_PREEMPT_RT and waiting for worker inactivity before registering tracepoints? In __irq_exit_rcu(), waking ksoftirqd for softirq work raised during trace_hardirq_context_exit() is gated on should_wake_ksoftirqd(): kernel/softirq.c:__irq_exit_rcu() { if (irq_count() == HARDIRQ_OFFSET && unlikely(local_softirq_pending()) && should_wake_ksoftirqd()) { wakeup_softirqd(); ksoftirqd_wake_requested = true; } } On CONFIG_PREEMPT_RT, should_wake_ksoftirqd() checks !this_cpu_read(softirq_ctrl.cnt) (!local_bh_blocked()). Because BH-disabled sections on PREEMPT_RT are preemptible and can block on RT locks (TASK_RTLOCK_WAIT), another task on the local CPU (or ksoftirqd itself after clearing local_softirq_pending() and blocking in TASK_RTLOCK_WAIT, where task_is_running() is false) can hold softirq_ctrl.cnt non-zero while local_softirq_pending() is 0 and task_is_running(softirqd) is false. In that case __irq_exit_rcu() skips wakeup_softirqd() and the assertion on softirqd_already_runnable fails. Additionally, unlike rv_test_irqstate_irq_exit_inline(), both handoff tests call wait_task_inactive() before register_trace_hardirq_context_exit(), register_trace_sched_waking(), and attach_irqstate_test_probes(), which can sleep on mutexes and queue RCU work that wakes ksoftirqd or ktimerd after the wait has already finished. Would it be better to attach the probes before waiting for the workers to go inactive, and also check local_bh_blocked()? > + WRITE_ONCE(exit_ctx->armed, true); > + irq_enter_rcu(); > + irq_exit_rcu(); > + exit_ctx->tasklet_pending = > + !!(local_softirq_pending() & BIT(TASKLET_SOFTIRQ)); > + exit_ctx->softirqd_already_runnable = > + !wake_up_process(exit_ctx->softirqd); > + WRITE_ONCE(exit_ctx->armed, false); > + local_irq_restore(flags); [ ... ] -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=9
