linux-kernel - Re: [PATCH] kernel/irq/proc: /proc/interrupts irq-off latency reduction

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]

Date:   Thu, 26 Apr 2018 22:24:10 +0200 (CEST)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Nicholas Piggin <npiggin@...il.com>
cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kernel/irq/proc: /proc/interrupts irq-off latency
 reduction

On Thu, 5 Apr 2018, Nicholas Piggin wrote:
> Reading /proc/interrupts shows up as a latency source of several
> hundred microseconds on a 2 socket 176 thread system, because it
> iterates twice over all CPUs under an irq lock pulling in remote
> cachelines, doing lots of seq_printf, etc.
> 
> On a 16 socket system with higher latencies to remote cachelines,
> latencies around 10ms would be seen here. irqbalanced periodically
> reads this file, and the latency spikes are noticed on smaller
> systems. Also the file is unprivileged, so it's important to reduce
> this latency.
> 
> Avoid holding the lock while iterating over CPUs to get their stats.
> The desc should be protected with irq_lock_sparse().

s/should/_IS_/

> 
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Signed-off-by: Nicholas Piggin <npiggin@...il.com>
> ---
>  kernel/irq/proc.c | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/kernel/irq/proc.c b/kernel/irq/proc.c
> index e8f374971e37..bbc4004b74bc 100644
> --- a/kernel/irq/proc.c
> +++ b/kernel/irq/proc.c
> @@ -544,17 +544,22 @@ int show_interrupts(struct seq_file *p, void *v)
>  	if (!desc)
>  		goto outsparse;
>  
> -	raw_spin_lock_irqsave(&desc->lock, flags);
>  	for_each_online_cpu(j)
>  		any_count |= kstat_irqs_cpu(i, j);
> +
>  	action = desc->action;

This is a bad idea. The action can vanish between here and the code further
below which derefences it.

You need to take desc->request_mutex for protection. That serializes
against concurrent request/free_irq().

> -	if ((!action || irq_desc_is_chained(desc)) && !any_count)
> -		goto out;
> +	if (!any_count) {
> +		raw_spin_lock_irqsave(&desc->lock, flags);
> +		if (!action || irq_desc_is_chained(desc))
> +			goto out;
> +		raw_spin_unlock_irqrestore(&desc->lock, flags);
> +	}
>  
>  	seq_printf(p, "%*d: ", prec, i);
>  	for_each_online_cpu(j)
>  		seq_printf(p, "%10u ", kstat_irqs_cpu(i, j));
>  
> +	raw_spin_lock_irqsave(&desc->lock, flags);
>  	if (desc->irq_data.chip) {
>  		if (desc->irq_data.chip->irq_print_chip)
>  			desc->irq_data.chip->irq_print_chip(&desc->irq_data, p);

And if you hold request_mutex you can drop the spinlock right before
printing desc->name.

Thanks,

	tglx