| Message ID | 1562214115-14022-1-git-send-email-sthotton@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | genirq: update irq stats from NMI handlers | expand |
Hi Shijith, On 04/07/2019 05:22, Shijith Thotton wrote: > The NMI handlers handle_percpu_devid_fasteoi_nmi() and > handle_fasteoi_nmi() added by commit 2dcf1fbcad35 ("genirq: Provide NMI > handlers") do not update the interrupt counts. Due to that the NMI > interrupt count does not show up correctly in /proc/interrupts. > > Update the functions to fix this. With this change, we can see stats of > the perf NMI interrupts on arm64. > > Fixes: 2dcf1fbcad35 ("genirq: Provide NMI handlers") > > Signed-off-by: Shijith Thotton <sthotton@marvell.com> > --- > kernel/irq/chip.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c > index 29d6c7d070b4..88d1e054c6ea 100644 > --- a/kernel/irq/chip.c > +++ b/kernel/irq/chip.c > @@ -748,6 +748,8 @@ void handle_fasteoi_nmi(struct irq_desc *desc) > unsigned int irq = irq_desc_get_irq(desc); > irqreturn_t res; > > + kstat_incr_irqs_this_cpu(desc); > + This needs to be called with the desc lock taken, otherwise we're likely to corrupt desc->tot_count. But taking the desc lock is something we can't do in NMI context ( *spin_lock_irq*() won't prevent an NMI from happening). > trace_irq_handler_entry(irq, action); > /* > * NMIs cannot be shared, there is only one action. > @@ -962,6 +964,8 @@ void handle_percpu_devid_fasteoi_nmi(struct irq_desc *desc) > unsigned int irq = irq_desc_get_irq(desc); > irqreturn_t res; > > + __kstat_incr_irqs_this_cpu(desc); > + Looking at handle_percpu_irq(), I think this might be acceptable. But does it make sense to only have kstats for percpu NMIs? Cheers,
Hi Julien, On 7/4/19 12:13 AM, Julien Thierry wrote: > On 04/07/2019 05:22, Shijith Thotton wrote: >> The NMI handlers handle_percpu_devid_fasteoi_nmi() and >> handle_fasteoi_nmi() added by commit 2dcf1fbcad35 ("genirq: Provide NMI >> handlers") do not update the interrupt counts. Due to that the NMI >> interrupt count does not show up correctly in /proc/interrupts. >> >> Update the functions to fix this. With this change, we can see stats of >> the perf NMI interrupts on arm64. >> >> Fixes: 2dcf1fbcad35 ("genirq: Provide NMI handlers") >> >> Signed-off-by: Shijith Thotton <sthotton@marvell.com> >> --- >> kernel/irq/chip.c | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c >> index 29d6c7d070b4..88d1e054c6ea 100644 >> --- a/kernel/irq/chip.c >> +++ b/kernel/irq/chip.c >> @@ -748,6 +748,8 @@ void handle_fasteoi_nmi(struct irq_desc *desc) >> unsigned int irq = irq_desc_get_irq(desc); >> irqreturn_t res; >> >> + kstat_incr_irqs_this_cpu(desc); >> + > > This needs to be called with the desc lock taken, otherwise we're likely > to corrupt desc->tot_count. > But taking the desc lock is something we can't do in NMI context ( > *spin_lock_irq*() won't prevent an NMI from happening). > >> trace_irq_handler_entry(irq, action); >> /* >> * NMIs cannot be shared, there is only one action. >> @@ -962,6 +964,8 @@ void handle_percpu_devid_fasteoi_nmi(struct irq_desc *desc) >> unsigned int irq = irq_desc_get_irq(desc); >> irqreturn_t res; >> >> + __kstat_incr_irqs_this_cpu(desc); >> + > > Looking at handle_percpu_irq(), I think this might be acceptable. But > does it make sense to only have kstats for percpu NMIs? > It would be better to have stats for both. handle_fasteoi_nmi() can use __kstat_incr_irqs_this_cpu() if below change can be added to kstat_irqs_cpu(). diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index a92b33593b8d..9484e88dabc2 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -950,6 +950,11 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu) *per_cpu_ptr(desc->kstat_irqs, cpu) : 0; } +static bool irq_is_nmi(struct irq_desc *desc) +{ + return desc->istate & IRQS_NMI; +} + /** * kstat_irqs - Get the statistics for an interrupt * @irq: The interrupt number @@ -967,7 +972,8 @@ unsigned int kstat_irqs(unsigned int irq) if (!desc || !desc->kstat_irqs) return 0; if (!irq_settings_is_per_cpu_devid(desc) && - !irq_settings_is_per_cpu(desc)) + !irq_settings_is_per_cpu(desc) && + !irq_is_nmi(desc)) return desc->tot_count; for_each_possible_cpu(cpu) Thomas, Please suggest a better way if any. Thanks, Shijith
On Thu, 4 Jul 2019, Shijith Thotton wrote: > On 7/4/19 12:13 AM, Julien Thierry wrote: > > Looking at handle_percpu_irq(), I think this might be acceptable. But > > does it make sense to only have kstats for percpu NMIs? > > > > It would be better to have stats for both. > > handle_fasteoi_nmi() can use __kstat_incr_irqs_this_cpu() if below > change can be added to kstat_irqs_cpu(). > > diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c > index a92b33593b8d..9484e88dabc2 100644 > --- a/kernel/irq/irqdesc.c > +++ b/kernel/irq/irqdesc.c > @@ -950,6 +950,11 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu) > *per_cpu_ptr(desc->kstat_irqs, cpu) : 0; > } > > +static bool irq_is_nmi(struct irq_desc *desc) > +{ > + return desc->istate & IRQS_NMI; > +} > + > /** > * kstat_irqs - Get the statistics for an interrupt > * @irq: The interrupt number > @@ -967,7 +972,8 @@ unsigned int kstat_irqs(unsigned int irq) > if (!desc || !desc->kstat_irqs) > return 0; > if (!irq_settings_is_per_cpu_devid(desc) && > - !irq_settings_is_per_cpu(desc)) > + !irq_settings_is_per_cpu(desc) && > + !irq_is_nmi(desc)) > return desc->tot_count; > > for_each_possible_cpu(cpu) > > > Thomas, > Please suggest a better way if any. Looks good. Thanks, tglx
On 7/4/19 9:19 AM, Thomas Gleixner wrote: > On Thu, 4 Jul 2019, Shijith Thotton wrote: >> On 7/4/19 12:13 AM, Julien Thierry wrote: >>> Looking at handle_percpu_irq(), I think this might be acceptable. But >>> does it make sense to only have kstats for percpu NMIs? >>> >> >> It would be better to have stats for both. >> >> handle_fasteoi_nmi() can use __kstat_incr_irqs_this_cpu() if below >> change can be added to kstat_irqs_cpu(). >> >> diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c >> index a92b33593b8d..9484e88dabc2 100644 >> --- a/kernel/irq/irqdesc.c >> +++ b/kernel/irq/irqdesc.c >> @@ -950,6 +950,11 @@ unsigned int kstat_irqs_cpu(unsigned int irq, int cpu) >> *per_cpu_ptr(desc->kstat_irqs, cpu) : 0; >> } >> >> +static bool irq_is_nmi(struct irq_desc *desc) >> +{ >> + return desc->istate & IRQS_NMI; >> +} >> + >> /** >> * kstat_irqs - Get the statistics for an interrupt >> * @irq: The interrupt number >> @@ -967,7 +972,8 @@ unsigned int kstat_irqs(unsigned int irq) >> if (!desc || !desc->kstat_irqs) >> return 0; >> if (!irq_settings_is_per_cpu_devid(desc) && >> - !irq_settings_is_per_cpu(desc)) >> + !irq_settings_is_per_cpu(desc) && >> + !irq_is_nmi(desc)) >> return desc->tot_count; >> >> for_each_possible_cpu(cpu) >> >> >> Thomas, >> Please suggest a better way if any. > > Looks good. > Thanks Thomas. Will share v2 with the changes. Thanks, Shijith
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 29d6c7d070b4..88d1e054c6ea 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -748,6 +748,8 @@ void handle_fasteoi_nmi(struct irq_desc *desc) unsigned int irq = irq_desc_get_irq(desc); irqreturn_t res; + kstat_incr_irqs_this_cpu(desc); + trace_irq_handler_entry(irq, action); /* * NMIs cannot be shared, there is only one action. @@ -962,6 +964,8 @@ void handle_percpu_devid_fasteoi_nmi(struct irq_desc *desc) unsigned int irq = irq_desc_get_irq(desc); irqreturn_t res; + __kstat_incr_irqs_this_cpu(desc); + trace_irq_handler_entry(irq, action); res = action->handler(irq, raw_cpu_ptr(action->percpu_dev_id)); trace_irq_handler_exit(irq, action, res);
The NMI handlers handle_percpu_devid_fasteoi_nmi() and handle_fasteoi_nmi() added by commit 2dcf1fbcad35 ("genirq: Provide NMI handlers") do not update the interrupt counts. Due to that the NMI interrupt count does not show up correctly in /proc/interrupts. Update the functions to fix this. With this change, we can see stats of the perf NMI interrupts on arm64. Fixes: 2dcf1fbcad35 ("genirq: Provide NMI handlers") Signed-off-by: Shijith Thotton <sthotton@marvell.com> --- kernel/irq/chip.c | 4 ++++ 1 file changed, 4 insertions(+)