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: <877c05w798.ffs@tglx>
Date: Fri, 18 Jul 2025 22:53:23 +0200
From: Thomas Gleixner <tglx@...utronix.de>
To: Wladislav Wiebe <wladislav.wiebe@...ia.com>, 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,
 wladislav.wiebe@...ia.com, peterz@...radead.org
Subject: Re: [PATCH v2] genirq: add support for warning on long-running IRQ
 handlers

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