Messages in this thread | | | Date | Thu, 26 Apr 2018 22:24:10 +0200 (CEST) | From | Thomas Gleixner <> | 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@linutronix.de> > Signed-off-by: Nicholas Piggin <npiggin@gmail.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
| |