linux-kernel - Re: [PATCH v2] genirq: add support for warning on long-running IRQ handlers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <44675637-8783-4731-b29b-02dd3e504d98@nokia.com>
Date: Wed, 23 Jul 2025 20:39:14 +0200
From: Wladislav Wiebe <wladislav.wiebe@...ia.com>
To: Thomas Gleixner <tglx@...utronix.de>, corbet@....net
Cc: akpm@...ux-foundation.org, paulmck@...nel.org, rostedt@...dmis.org,
 Neeraj.Upadhyay@....com, david@...hat.com, bp@...en8.de, arnd@...db.de,
 fvdl@...gle.com, linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
 peterz@...radead.org
Subject: Re: [PATCH v2] genirq: add support for warning on long-running IRQ
 handlers


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@nokia.com/
- W.W.