| Message ID | 20201205014340.148235-2-jsnitsel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm_tis: Detect interrupt storms | expand |
On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: > To try and detect potential interrupt storms that > have been occurring with tpm_tis devices it was suggested > to use kstat_irqs() to get the number of interrupts. > Since tpm_tis can be built as a module it needs kstat_irqs > exported. I think you should also have a paragraph explicitly stating that i915_pmu.c contains a duplicate of kstat_irqs() because it is not exported as of today. It adds a lot more weight to this given that there is already existing mainline usage (kind of). > > Reported-by: kernel test robot <lkp@intel.com> I'm not sure if this makes much sense. > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Jarkko Sakkinen <jarkko@kernel.org> > Cc: Jason Gunthorpe <jgg@ziepe.ca> > Cc: Peter Huewe <peterhuewe@gmx.de> > Cc: James Bottomley <James.Bottomley@HansenPartnership.com> > Cc: Matthew Garrett <mjg59@google.com> > Cc: Hans de Goede <hdegoede@redhat.com> > Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> /Jarkko
On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: >> To try and detect potential interrupt storms that >> have been occurring with tpm_tis devices it was suggested >> to use kstat_irqs() to get the number of interrupts. >> Since tpm_tis can be built as a module it needs kstat_irqs >> exported. > > I think you should also have a paragraph explicitly stating that > i915_pmu.c contains a duplicate of kstat_irqs() because it is not > exported as of today. It adds a lot more weight to this given that > there is already existing mainline usage (kind of). It's abusage and just the fact that it exists is not an argument by itself. Thanks, tglx
On Sun, 2020-12-06 at 17:40 +0100, Thomas Gleixner wrote: > On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: > > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: > > > To try and detect potential interrupt storms that > > > have been occurring with tpm_tis devices it was suggested > > > to use kstat_irqs() to get the number of interrupts. > > > Since tpm_tis can be built as a module it needs kstat_irqs > > > exported. > > > > I think you should also have a paragraph explicitly stating that > > i915_pmu.c contains a duplicate of kstat_irqs() because it is not > > exported as of today. It adds a lot more weight to this given that > > there is already existing mainline usage (kind of). > > It's abusage and just the fact that it exists is not an argument by > itself. What we want is a count of the interrupts to see if we're having an interrupt storm from the TPM device (some seem to be wired to fire the interrupt even when there's no event to warrant it). Since kstat_irqs_user() does the correct RCU locking, should we be using that instead? James
Jerry, On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote: The proper prefix is 'genirq:' git log kernel/irq/irqdesc.c would have told you. > To try and detect potential interrupt storms that > have been occurring with tpm_tis devices it was suggested > to use kstat_irqs() to get the number of interrupts. > Since tpm_tis can be built as a module it needs kstat_irqs > exported. I'm not really enthused about exporting this without making it at least safe. Using it from an interrupt handler is obviously safe vs. concurrent removal, but the next driver writer who thinks this is cool is going to get it wrong for sure. Though I still have to figure out what the advantage of invoking a function which needs to do a radix tree lookup over a device local counter is just to keep track of this. I'll reply on the TPM part of this as well. Thanks, tglx
On Sun, Dec 06 2020 at 09:40, James Bottomley wrote: > On Sun, 2020-12-06 at 17:40 +0100, Thomas Gleixner wrote: >> On Sat, Dec 05 2020 at 12:39, Jarkko Sakkinen wrote: >> > On Fri, Dec 04, 2020 at 06:43:37PM -0700, Jerry Snitselaar wrote: >> > > To try and detect potential interrupt storms that >> > > have been occurring with tpm_tis devices it was suggested >> > > to use kstat_irqs() to get the number of interrupts. >> > > Since tpm_tis can be built as a module it needs kstat_irqs >> > > exported. >> > >> > I think you should also have a paragraph explicitly stating that >> > i915_pmu.c contains a duplicate of kstat_irqs() because it is not >> > exported as of today. It adds a lot more weight to this given that >> > there is already existing mainline usage (kind of). >> >> It's abusage and just the fact that it exists is not an argument by >> itself. > > What we want is a count of the interrupts to see if we're having an > interrupt storm from the TPM device (some seem to be wired to fire the > interrupt even when there's no event to warrant it). Since > kstat_irqs_user() does the correct RCU locking, should we be using that > instead? If we need to export it, yes. But I still have to understand the value. See my other reply. Thanks, tglx
Thomas Gleixner @ 2020-12-06 10:54 MST: > Jerry, > > On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote: > > The proper prefix is 'genirq:' git log kernel/irq/irqdesc.c would have > told you. > >> To try and detect potential interrupt storms that >> have been occurring with tpm_tis devices it was suggested >> to use kstat_irqs() to get the number of interrupts. >> Since tpm_tis can be built as a module it needs kstat_irqs >> exported. > > I'm not really enthused about exporting this without making it at least > safe. Using it from an interrupt handler is obviously safe vs. concurrent > removal, but the next driver writer who thinks this is cool is going to > get it wrong for sure. > > Though I still have to figure out what the advantage of invoking a > function which needs to do a radix tree lookup over a device local > counter is just to keep track of this. > > I'll reply on the TPM part of this as well. > > Thanks, > > tglx I can rework it to use a device local counter.
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h index 5745491303e0..fff88c1f1ac6 100644 --- a/include/linux/irqdesc.h +++ b/include/linux/irqdesc.h @@ -153,6 +153,7 @@ static inline void generic_handle_irq_desc(struct irq_desc *desc) } int generic_handle_irq(unsigned int irq); +unsigned int kstat_irqs(unsigned int irq); #ifdef CONFIG_HANDLE_DOMAIN_IRQ /* diff --git a/kernel/irq/irqdesc.c b/kernel/irq/irqdesc.c index 1a7723604399..12398ef1796b 100644 --- a/kernel/irq/irqdesc.c +++ b/kernel/irq/irqdesc.c @@ -1000,6 +1000,7 @@ unsigned int kstat_irqs(unsigned int irq) sum += *per_cpu_ptr(desc->kstat_irqs, cpu); return sum; } +EXPORT_SYMBOL_GPL(kstat_irqs); /** * kstat_irqs_usr - Get the statistics for an interrupt
To try and detect potential interrupt storms that have been occurring with tpm_tis devices it was suggested to use kstat_irqs() to get the number of interrupts. Since tpm_tis can be built as a module it needs kstat_irqs exported. Reported-by: kernel test robot <lkp@intel.com> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: Jarkko Sakkinen <jarkko@kernel.org> Cc: Jason Gunthorpe <jgg@ziepe.ca> Cc: Peter Huewe <peterhuewe@gmx.de> Cc: James Bottomley <James.Bottomley@HansenPartnership.com> Cc: Matthew Garrett <mjg59@google.com> Cc: Hans de Goede <hdegoede@redhat.com> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com> --- include/linux/irqdesc.h | 1 + kernel/irq/irqdesc.c | 1 + 2 files changed, 2 insertions(+)