On 18/07/2025 22:53, Thomas Gleixner wrote: > On Mon, Jul 14 2025 at 10:41, Wladislav Wiebe wrote: >> This patch adds a mechanism to detect and warn about long-running IRQ > # git grep 'This patch' Documentation/process/ > > Also please read: > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#changelog > >> +static int __init irqhandler_duration_check_setup(char *arg) >> +{ >> + unsigned long val; >> + int ret; >> + >> + if (!arg) >> + return 0; >> + >> + ret = kstrtoul(arg, 0, &val); >> + if (ret) >> + return ret; >> + >> + if (val > 0) { >> + irqhandler_duration_threshold_us = val; >> + static_branch_enable(&irqhandler_duration_check_enabled); >> + } else { >> + pr_err("Invalid irqhandler.duration_warn_us setting (%lu)\n", val); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> +early_param("irqhandler.duration_warn_us", irqhandler_duration_check_setup); > Why early_param? Nothing cares about this during early boot. > >> +static inline void irqhandler_duration_check(u64 ts_start, unsigned int irq, >> + struct irqaction *action) >> +{ >> + u64 delta_us = (local_clock() - ts_start) >> 10; > Lacks a comment that this is an intentional approximation. > >> + if (unlikely(delta_us > irqhandler_duration_threshold_us)) { >> + pr_warn_ratelimited("[CPU%d] long duration on IRQ[%u:%ps], took: %llu us\n", >> + smp_processor_id(), irq, action->handler, delta_us); > Please align the arguments in the second line properly. > > https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#line-breaks > >> + } >> +} >> + >> irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) >> { >> irqreturn_t retval = IRQ_NONE; >> @@ -146,6 +184,7 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) >> >> for_each_action_of_desc(desc, action) { >> irqreturn_t res; >> + u64 ts_start; > This wants to be in the if() branch where it is actually used. > >> /* >> * If this IRQ would be threaded under force_irqthreads, mark it so. >> @@ -155,7 +194,14 @@ irqreturn_t __handle_irq_event_percpu(struct irq_desc *desc) >> lockdep_hardirq_threaded(); >> >> trace_irq_handler_entry(irq, action); >> - res = action->handler(irq, action->dev_id); >> + >> + if (static_branch_unlikely(&irqhandler_duration_check_enabled)) { >> + ts_start = local_clock(); >> + res = action->handler(irq, action->dev_id); >> + irqhandler_duration_check(ts_start, irq, action); >> + } else >> + res = action->handler(irq, action->dev_id); >> + > Even if not required by C, the else clause wants brackets too for > symmetry. > > if (foo) > bar(); > else > baz(); > > parses perfectly fine. > > if (foo) { > do_stuff(); > bar(); > } else > baz(); > > is asymmetrical and disturbs the reading flow, which is pattern > based. The extra brackets just make it easier to parse: > > if (foo) { > do_stuff(); > bar(); > } else { > baz(); > } > > See? > > Thanks, > > tglx Thanks for further comments, I've addressed them in v3: https://lore.kernel.org/lkml/20250723182836.1177-1-wladislav.wiebe@xxxxxxxxx/ - W.W.
