Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Tue, Jun 19, 2018 at 05:25:09PM -0700, Randy Dunlap wrote: > On 06/19/2018 05:15 PM, Ricardo Neri wrote: > > On Sat, Jun 16, 2018 at 03:24:49PM +0200, Thomas Gleixner wrote: > >> On Fri, 15 Jun 2018, Ricardo Neri wrote: > >>> On Fri, Jun 15, 2018 at 11:19:09AM +0200, Thomas Gleixner wrote: > On Thu, 14 Jun 2018, Ricardo Neri wrote: > > Alternatively, there could be a counter that skips reading the HPET > > status > > register (and the detection of hardlockups) for every X NMIs. This would > > reduce the overall frequency of HPET register reads. > > Great plan. So if the watchdog is the only NMI (because perf is off) then > you delay the watchdog detection by that count. > >>> > >>> OK. This was a bad idea. Then, is it acceptable to have an read to an HPET > >>> register per NMI just to check in the status register if the HPET timer > >>> caused the NMI? > >> > >> The status register is useless in case of MSI. MSI is edge triggered > >> > >> The only register which gives you proper information is the counter > >> register itself. That adds an massive overhead to each NMI, because the > >> counter register access is synchronized to the HPET clock with hardware > >> magic. Plus on larger systems, the HPET access is cross node and even > >> slower. > > > > It starts to sound that the HPET is too slow to drive the hardlockup > > detector. > > > > Would it be possible to envision a variant of this implementation? In this > > variant, the HPET only targets a single CPU. The actual hardlockup detector > > is implemented by this single CPU sending interprocessor interrupts to the > > rest of the CPUs. > > > > In this manner only one CPU has to deal with the slowness of the HPET; the > > rest of the CPUs don't have to read or write any HPET registers. A sysfs > > entry could be added to configure which CPU will have to deal with the HPET > > timer. However, profiling could not be done accurately on such CPU. > > Please forgive my simple question: > > What happens when this one CPU is the one that locks up? I think that in this particular case this one CPU would check for hardlockups on itself when it receives the NMI from the HPET timer. It would also issue NMIs to the other monitored processors. Thanks and BR, Ricardo ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Tue, 19 Jun 2018, Ricardo Neri wrote: > On Sat, Jun 16, 2018 at 03:24:49PM +0200, Thomas Gleixner wrote: > > The status register is useless in case of MSI. MSI is edge triggered > > > > The only register which gives you proper information is the counter > > register itself. That adds an massive overhead to each NMI, because the > > counter register access is synchronized to the HPET clock with hardware > > magic. Plus on larger systems, the HPET access is cross node and even > > slower. > > It starts to sound that the HPET is too slow to drive the hardlockup detector. > > Would it be possible to envision a variant of this implementation? In this > variant, the HPET only targets a single CPU. The actual hardlockup detector > is implemented by this single CPU sending interprocessor interrupts to the > rest of the CPUs. And these IPIs must be NMIs which need to have a software based indicator that the watchdog needs to be checked, which is going to create yet another can of race conditions and in the worst case 'unknown NMI' splats. Not pretty either. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Fri, 15 Jun 2018, Ricardo Neri wrote: > On Fri, Jun 15, 2018 at 11:19:09AM +0200, Thomas Gleixner wrote: > > On Thu, 14 Jun 2018, Ricardo Neri wrote: > > > Alternatively, there could be a counter that skips reading the HPET status > > > register (and the detection of hardlockups) for every X NMIs. This would > > > reduce the overall frequency of HPET register reads. > > > > Great plan. So if the watchdog is the only NMI (because perf is off) then > > you delay the watchdog detection by that count. > > OK. This was a bad idea. Then, is it acceptable to have an read to an HPET > register per NMI just to check in the status register if the HPET timer > caused the NMI? The status register is useless in case of MSI. MSI is edge triggered The only register which gives you proper information is the counter register itself. That adds an massive overhead to each NMI, because the counter register access is synchronized to the HPET clock with hardware magic. Plus on larger systems, the HPET access is cross node and even slower. Thanks, tglx ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Fri, Jun 15, 2018 at 11:19:09AM +0200, Thomas Gleixner wrote:
> On Thu, 14 Jun 2018, Ricardo Neri wrote:
> > On Wed, Jun 13, 2018 at 11:40:00AM +0200, Thomas Gleixner wrote:
> > > On Tue, 12 Jun 2018, Ricardo Neri wrote:
> > > > @@ -183,6 +184,8 @@ static irqreturn_t
> > > > hardlockup_detector_irq_handler(int irq, void *data)
> > > > if (!(hdata->flags & HPET_DEV_PERI_CAP))
> > > > kick_timer(hdata);
> > > >
> > > > + pr_err("This interrupt should not have happened. Ensure
> > > > delivery mode is NMI.\n");
> > >
> > > Eeew.
> >
> > If you don't mind me asking. What is the problem with this error message?
>
> The problem is not the error message. The problem is the abuse of
> request_irq() and the fact that this irq handler function exists in the
> first place for something which is NMI based.
I wanted to add this handler in case the interrupt was not configured correctly
to be delivered as NMI (e.g., not supported by the hardware). I see your point.
Perhaps this is not needed. There is code in place to complain when an interrupt
that nobody was expecting happens.
>
> > > And in case that the HPET does not support periodic mode this reprogramms
> > > the timer on every NMI which means that while perf is running the watchdog
> > > will never ever detect anything.
> >
> > Yes. I see that this is wrong. With MSI interrupts, as far as I can
> > see, there is not a way to make sure that the HPET timer caused the NMI
> > perhaps the only option is to use an IO APIC interrupt and read the
> > interrupt status register.
> >
> > > Aside of that, reading TWO HPET registers for every NMI is insane. HPET
> > > access is horribly slow, so any high frequency perf monitoring will take a
> > > massive performance hit.
> >
> > If an IO APIC interrupt is used, only HPET register (the status register)
> > would need to be read for every NMI. Would that be more acceptable?
> > Otherwise,
> > there is no way to determine if the HPET cause the NMI.
>
> You need level trigger for the HPET status register to be useful at all
> because in edge mode the interrupt status bits read always 0.
Indeed.
>
> That means you have to fiddle with the IOAPIC acknowledge magic from NMI
> context. Brilliant idea. If the NMI hits in the middle of a regular
> io_apic_read() then the interrupted code will endup with the wrong index
> register. Not to talk about the fun which the affinity rotation from NMI
> context would bring.
>
> Do not even think about using IOAPIC and level for this.
OK. I will stay away of it and focus on MSI.
>
> > Alternatively, there could be a counter that skips reading the HPET status
> > register (and the detection of hardlockups) for every X NMIs. This would
> > reduce the overall frequency of HPET register reads.
>
> Great plan. So if the watchdog is the only NMI (because perf is off) then
> you delay the watchdog detection by that count.
OK. This was a bad idea. Then, is it acceptable to have an read to an HPET
register per NMI just to check in the status register if the HPET timer
caused the NMI?
Thanks and BR,
Ricardo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Thu, 14 Jun 2018, Ricardo Neri wrote:
> On Wed, Jun 13, 2018 at 11:40:00AM +0200, Thomas Gleixner wrote:
> > On Tue, 12 Jun 2018, Ricardo Neri wrote:
> > > @@ -183,6 +184,8 @@ static irqreturn_t
> > > hardlockup_detector_irq_handler(int irq, void *data)
> > > if (!(hdata->flags & HPET_DEV_PERI_CAP))
> > > kick_timer(hdata);
> > >
> > > + pr_err("This interrupt should not have happened. Ensure delivery mode
> > > is NMI.\n");
> >
> > Eeew.
>
> If you don't mind me asking. What is the problem with this error message?
The problem is not the error message. The problem is the abuse of
request_irq() and the fact that this irq handler function exists in the
first place for something which is NMI based.
> > And in case that the HPET does not support periodic mode this reprogramms
> > the timer on every NMI which means that while perf is running the watchdog
> > will never ever detect anything.
>
> Yes. I see that this is wrong. With MSI interrupts, as far as I can
> see, there is not a way to make sure that the HPET timer caused the NMI
> perhaps the only option is to use an IO APIC interrupt and read the
> interrupt status register.
>
> > Aside of that, reading TWO HPET registers for every NMI is insane. HPET
> > access is horribly slow, so any high frequency perf monitoring will take a
> > massive performance hit.
>
> If an IO APIC interrupt is used, only HPET register (the status register)
> would need to be read for every NMI. Would that be more acceptable? Otherwise,
> there is no way to determine if the HPET cause the NMI.
You need level trigger for the HPET status register to be useful at all
because in edge mode the interrupt status bits read always 0.
That means you have to fiddle with the IOAPIC acknowledge magic from NMI
context. Brilliant idea. If the NMI hits in the middle of a regular
io_apic_read() then the interrupted code will endup with the wrong index
register. Not to talk about the fun which the affinity rotation from NMI
context would bring.
Do not even think about using IOAPIC and level for this.
> Alternatively, there could be a counter that skips reading the HPET status
> register (and the detection of hardlockups) for every X NMIs. This would
> reduce the overall frequency of HPET register reads.
Great plan. So if the watchdog is the only NMI (because perf is off) then
you delay the watchdog detection by that count.
You neither can do a time based check, because time might be corrupted and
then you end up in lala land as well.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Wed, Jun 13, 2018 at 11:07:20AM +0200, Peter Zijlstra wrote:
> On Tue, Jun 12, 2018 at 05:57:37PM -0700, Ricardo Neri wrote:
>
> +static bool is_hpet_wdt_interrupt(struct hpet_hld_data *hdata)
> +{
> + unsigned long this_isr;
> + unsigned int lvl_trig;
> +
> + this_isr = hpet_readl(HPET_STATUS) & BIT(hdata->num);
> +
> + lvl_trig = hpet_readl(HPET_Tn_CFG(hdata->num)) & HPET_TN_LEVEL;
> +
> + if (lvl_trig && this_isr)
> + return true;
> +
> + return false;
> +}
>
> > +static int hardlockup_detector_nmi_handler(unsigned int val,
> > + struct pt_regs *regs)
> > +{
> > + struct hpet_hld_data *hdata = hld_data;
> > + unsigned int use_fsb;
> > +
> > + /*
> > +* If FSB delivery mode is used, the timer interrupt is programmed as
> > +* edge-triggered and there is no need to check the ISR register.
> > +*/
> > + use_fsb = hdata->flags & HPET_DEV_FSB_CAP;
>
> Please do explain.. That FSB thing basically means MSI. But there's only
> a single NMI vector. How do we know this NMI came from the HPET?
Indeed, I see now that this is wrong. There is no way to know. The only way
is to use an IO APIC interrupt and read the HPET status register.
>
> > +
> > + if (!use_fsb && !is_hpet_wdt_interrupt(hdata))
>
> So you add _2_ HPET reads for every single NMI that gets triggered...
> and IIRC HPET reads are _slloww_.
Since the trigger mode of the HPET timer is not expected to change,
perhaps is_hpet_wdt_interrupt() can only need the interrupt status
register. This would reduce the reads to one. Furthermore, the hardlockup
detector can skip an X number of NMIs and reduce further the frequency
of reads. Does this make sense?
>
> > + return NMI_DONE;
> > +
> > + inspect_for_hardlockups(regs);
> > +
> > + if (!(hdata->flags & HPET_DEV_PERI_CAP))
> > + kick_timer(hdata);
> > +
> > + /* Acknowledge interrupt if in level-triggered mode */
> > + if (!use_fsb)
> > + hpet_writel(BIT(hdata->num), HPET_STATUS);
> > +
> > + return NMI_HANDLED;
>
> So if I read this right, when in FSB/MSI mode, we'll basically _always_
> claim every single NMI as handled?
>
> That's broken.
Yes, this is not correct. I will drop the functionality to use
FSB/MSI mode.
Thanks and BR,
Ricardo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Wed, Jun 13, 2018 at 11:40:00AM +0200, Thomas Gleixner wrote:
> On Tue, 12 Jun 2018, Ricardo Neri wrote:
> > @@ -183,6 +184,8 @@ static irqreturn_t hardlockup_detector_irq_handler(int
> > irq, void *data)
> > if (!(hdata->flags & HPET_DEV_PERI_CAP))
> > kick_timer(hdata);
> >
> > + pr_err("This interrupt should not have happened. Ensure delivery mode
> > is NMI.\n");
>
> Eeew.
If you don't mind me asking. What is the problem with this error message?
>
> > /**
> > + * hardlockup_detector_nmi_handler() - NMI Interrupt handler
> > + * @val: Attribute associated with the NMI. Not used.
> > + * @regs: Register values as seen when the NMI was asserted
> > + *
> > + * When an NMI is issued, look for hardlockups. If the timer is not
> > periodic,
> > + * kick it. The interrupt is always handled when if delivered via the
> > + * Front-Side Bus.
> > + *
> > + * Returns:
> > + *
> > + * NMI_DONE if the HPET timer did not cause the interrupt. NMI_HANDLED
> > + * otherwise.
> > + */
> > +static int hardlockup_detector_nmi_handler(unsigned int val,
> > + struct pt_regs *regs)
> > +{
> > + struct hpet_hld_data *hdata = hld_data;
> > + unsigned int use_fsb;
> > +
> > + /*
> > +* If FSB delivery mode is used, the timer interrupt is programmed as
> > +* edge-triggered and there is no need to check the ISR register.
> > +*/
> > + use_fsb = hdata->flags & HPET_DEV_FSB_CAP;
> > +
> > + if (!use_fsb && !is_hpet_wdt_interrupt(hdata))
> > + return NMI_DONE;
>
> So for 'use_fsb == True' every single NMI will fall through into the
> watchdog code below.
>
> > + inspect_for_hardlockups(regs);
> > +
> > + if (!(hdata->flags & HPET_DEV_PERI_CAP))
> > + kick_timer(hdata);
>
> And in case that the HPET does not support periodic mode this reprogramms
> the timer on every NMI which means that while perf is running the watchdog
> will never ever detect anything.
Yes. I see that this is wrong. With MSI interrupts, as far as I can
see, there is not a way to make sure that the HPET timer caused the NMI
perhaps the only option is to use an IO APIC interrupt and read the
interrupt status register.
>
> Aside of that, reading TWO HPET registers for every NMI is insane. HPET
> access is horribly slow, so any high frequency perf monitoring will take a
> massive performance hit.
If an IO APIC interrupt is used, only HPET register (the status register)
would need to be read for every NMI. Would that be more acceptable? Otherwise,
there is no way to determine if the HPET cause the NMI.
Alternatively, there could be a counter that skips reading the HPET status
register (and the detection of hardlockups) for every X NMIs. This would
reduce the overall frequency of HPET register reads.
Is that more acceptable?
Thanks and BR,
Ricardo
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Tue, 12 Jun 2018, Ricardo Neri wrote:
> @@ -183,6 +184,8 @@ static irqreturn_t hardlockup_detector_irq_handler(int
> irq, void *data)
> if (!(hdata->flags & HPET_DEV_PERI_CAP))
> kick_timer(hdata);
>
> + pr_err("This interrupt should not have happened. Ensure delivery mode
> is NMI.\n");
Eeew.
> /**
> + * hardlockup_detector_nmi_handler() - NMI Interrupt handler
> + * @val: Attribute associated with the NMI. Not used.
> + * @regs:Register values as seen when the NMI was asserted
> + *
> + * When an NMI is issued, look for hardlockups. If the timer is not periodic,
> + * kick it. The interrupt is always handled when if delivered via the
> + * Front-Side Bus.
> + *
> + * Returns:
> + *
> + * NMI_DONE if the HPET timer did not cause the interrupt. NMI_HANDLED
> + * otherwise.
> + */
> +static int hardlockup_detector_nmi_handler(unsigned int val,
> +struct pt_regs *regs)
> +{
> + struct hpet_hld_data *hdata = hld_data;
> + unsigned int use_fsb;
> +
> + /*
> + * If FSB delivery mode is used, the timer interrupt is programmed as
> + * edge-triggered and there is no need to check the ISR register.
> + */
> + use_fsb = hdata->flags & HPET_DEV_FSB_CAP;
> +
> + if (!use_fsb && !is_hpet_wdt_interrupt(hdata))
> + return NMI_DONE;
So for 'use_fsb == True' every single NMI will fall through into the
watchdog code below.
> + inspect_for_hardlockups(regs);
> +
> + if (!(hdata->flags & HPET_DEV_PERI_CAP))
> + kick_timer(hdata);
And in case that the HPET does not support periodic mode this reprogramms
the timer on every NMI which means that while perf is running the watchdog
will never ever detect anything.
Aside of that, reading TWO HPET registers for every NMI is insane. HPET
access is horribly slow, so any high frequency perf monitoring will take a
massive performance hit.
Thanks,
tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 17/23] watchdog/hardlockup/hpet: Convert the timer's interrupt to NMI
On Tue, Jun 12, 2018 at 05:57:37PM -0700, Ricardo Neri wrote:
+static bool is_hpet_wdt_interrupt(struct hpet_hld_data *hdata)
+{
+ unsigned long this_isr;
+ unsigned int lvl_trig;
+
+ this_isr = hpet_readl(HPET_STATUS) & BIT(hdata->num);
+
+ lvl_trig = hpet_readl(HPET_Tn_CFG(hdata->num)) & HPET_TN_LEVEL;
+
+ if (lvl_trig && this_isr)
+ return true;
+
+ return false;
+}
> +static int hardlockup_detector_nmi_handler(unsigned int val,
> +struct pt_regs *regs)
> +{
> + struct hpet_hld_data *hdata = hld_data;
> + unsigned int use_fsb;
> +
> + /*
> + * If FSB delivery mode is used, the timer interrupt is programmed as
> + * edge-triggered and there is no need to check the ISR register.
> + */
> + use_fsb = hdata->flags & HPET_DEV_FSB_CAP;
Please do explain.. That FSB thing basically means MSI. But there's only
a single NMI vector. How do we know this NMI came from the HPET?
> +
> + if (!use_fsb && !is_hpet_wdt_interrupt(hdata))
So you add _2_ HPET reads for every single NMI that gets triggered...
and IIRC HPET reads are _slloww_.
> + return NMI_DONE;
> +
> + inspect_for_hardlockups(regs);
> +
> + if (!(hdata->flags & HPET_DEV_PERI_CAP))
> + kick_timer(hdata);
> +
> + /* Acknowledge interrupt if in level-triggered mode */
> + if (!use_fsb)
> + hpet_writel(BIT(hdata->num), HPET_STATUS);
> +
> + return NMI_HANDLED;
So if I read this right, when in FSB/MSI mode, we'll basically _always_
claim every single NMI as handled?
That's broken.
> +}
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
