| Message ID | 1528851463-21140-21-git-send-email-ricardo.neri-calderon@linux.intel.com |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | Implement an HPET-based hardlockup detector | expand |
On Tue, 12 Jun 2018, Ricardo Neri wrote: > + /* There are no CPUs to monitor. */ > + if (!cpumask_weight(&hdata->monitored_mask)) > + return NMI_HANDLED; > + > inspect_for_hardlockups(regs); > > + /* > + * Target a new CPU. Keep trying until we find a monitored CPU. CPUs > + * are addded and removed to this mask at cpu_up() and cpu_down(), > + * respectively. Thus, the interrupt should be able to be moved to > + * the next monitored CPU. > + */ > + spin_lock(&hld_data->lock); Yuck. Taking a spinlock from NMI ... > + for_each_cpu_wrap(cpu, &hdata->monitored_mask, smp_processor_id() + 1) { > + if (!irq_set_affinity(hld_data->irq, cpumask_of(cpu))) > + break; ... and then calling into generic interrupt code which will take even more locks is completely broken. Guess what happens when the NMI hits a section where one of those locks is held? Then you need another watchdog to decode the lockup you just ran into. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 13, 2018 at 11:48:09AM +0200, Thomas Gleixner wrote: > On Tue, 12 Jun 2018, Ricardo Neri wrote: > > + /* There are no CPUs to monitor. */ > > + if (!cpumask_weight(&hdata->monitored_mask)) > > + return NMI_HANDLED; > > + > > inspect_for_hardlockups(regs); > > > > + /* > > + * Target a new CPU. Keep trying until we find a monitored CPU. CPUs > > + * are addded and removed to this mask at cpu_up() and cpu_down(), > > + * respectively. Thus, the interrupt should be able to be moved to > > + * the next monitored CPU. > > + */ > > + spin_lock(&hld_data->lock); > > Yuck. Taking a spinlock from NMI ... I am sorry. I will look into other options for locking. Do you think rcu_lock would help in this case? I need this locking because the CPUs being monitored changes as CPUs come online and offline. > > > + for_each_cpu_wrap(cpu, &hdata->monitored_mask, smp_processor_id() + 1) { > > + if (!irq_set_affinity(hld_data->irq, cpumask_of(cpu))) > > + break; > > ... and then calling into generic interrupt code which will take even more > locks is completely broken. I will into reworking how the destination of the interrupt is set. Thanks and BR, Ricardo -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 14 Jun 2018, Ricardo Neri wrote: > On Wed, Jun 13, 2018 at 11:48:09AM +0200, Thomas Gleixner wrote: > > On Tue, 12 Jun 2018, Ricardo Neri wrote: > > > + /* There are no CPUs to monitor. */ > > > + if (!cpumask_weight(&hdata->monitored_mask)) > > > + return NMI_HANDLED; > > > + > > > inspect_for_hardlockups(regs); > > > > > > + /* > > > + * Target a new CPU. Keep trying until we find a monitored CPU. CPUs > > > + * are addded and removed to this mask at cpu_up() and cpu_down(), > > > + * respectively. Thus, the interrupt should be able to be moved to > > > + * the next monitored CPU. > > > + */ > > > + spin_lock(&hld_data->lock); > > > > Yuck. Taking a spinlock from NMI ... > > I am sorry. I will look into other options for locking. Do you think rcu_lock > would help in this case? I need this locking because the CPUs being monitored > changes as CPUs come online and offline. Sure, but you _cannot_ take any locks in NMI context which are also taken in !NMI context. And RCU will not help either. How so? The NMI can hit exactly before the CPU bit is cleared and then the CPU goes down. So RCU _cannot_ protect anything. All you can do there is make sure that the TIMn_CONF is only ever accessed in !NMI code. Then you can stop the timer _before_ a CPU goes down and make sure that the eventually on the fly NMI is finished. After that you can fiddle with the CPU mask and restart the timer. Be aware that this is going to be more corner case handling that actual functionality. > > > + for_each_cpu_wrap(cpu, &hdata->monitored_mask, smp_processor_id() + 1) { > > > + if (!irq_set_affinity(hld_data->irq, cpumask_of(cpu))) > > > + break; > > > > ... and then calling into generic interrupt code which will take even more > > locks is completely broken. > > I will into reworking how the destination of the interrupt is set. You have to consider two cases: 1) !remapped mode: That's reasonably simple because you just have to deal with the HPET TIMERn_PROCMSG_ROUT register. But then you need to do this directly and not through any of the existing interrupt facilities. 2) remapped mode: That's way more complex as you _cannot_ ever do anything which touches the IOMMU and the related tables. So you'd need to reserve an IOMMU remapping entry for each CPU upfront, store the resulting value for the HPET TIMERn_PROCMSG_ROUT register in per cpu storage and just modify that one from NMI. Though there might be subtle side effects involved, which are related to the acknowledge part. You need to talk to the IOMMU wizards first. All in all, the idea itself is interesting, but the envisioned approach of round robin and no fast accessible NMI reason detection is going to create more problems than it solves. This all could have been avoided if Intel hadn't decided to reuse the APIC timer registers for the TSC deadline timer. If both would be available we'd have a CPU local fast accessible watchdog timer when TSC deadline is used for general timer purposes. But why am I complaining? I've resigned to the fact that timers are designed^Wcobbled together by janitors long ago. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Jun 15, 2018 at 12:29:06PM +0200, Thomas Gleixner wrote: > On Thu, 14 Jun 2018, Ricardo Neri wrote: > > On Wed, Jun 13, 2018 at 11:48:09AM +0200, Thomas Gleixner wrote: > > > On Tue, 12 Jun 2018, Ricardo Neri wrote: > > > > + /* There are no CPUs to monitor. */ > > > > + if (!cpumask_weight(&hdata->monitored_mask)) > > > > + return NMI_HANDLED; > > > > + > > > > inspect_for_hardlockups(regs); > > > > > > > > + /* > > > > + * Target a new CPU. Keep trying until we find a monitored CPU. CPUs > > > > + * are addded and removed to this mask at cpu_up() and cpu_down(), > > > > + * respectively. Thus, the interrupt should be able to be moved to > > > > + * the next monitored CPU. > > > > + */ > > > > + spin_lock(&hld_data->lock); > > > > > > Yuck. Taking a spinlock from NMI ... > > > > I am sorry. I will look into other options for locking. Do you think rcu_lock > > would help in this case? I need this locking because the CPUs being monitored > > changes as CPUs come online and offline. > > Sure, but you _cannot_ take any locks in NMI context which are also taken > in !NMI context. And RCU will not help either. How so? The NMI can hit > exactly before the CPU bit is cleared and then the CPU goes down. So RCU > _cannot_ protect anything. > > All you can do there is make sure that the TIMn_CONF is only ever accessed > in !NMI code. Then you can stop the timer _before_ a CPU goes down and make > sure that the eventually on the fly NMI is finished. After that you can > fiddle with the CPU mask and restart the timer. Be aware that this is going > to be more corner case handling that actual functionality. Thanks for the suggestion. It makes sense to stop the timer when updating the CPU mask. In this manner the timer will not cause any NMI. > > > > > + for_each_cpu_wrap(cpu, &hdata->monitored_mask, smp_processor_id() + 1) { > > > > + if (!irq_set_affinity(hld_data->irq, cpumask_of(cpu))) > > > > + break; > > > > > > ... and then calling into generic interrupt code which will take even more > > > locks is completely broken. > > > > I will into reworking how the destination of the interrupt is set. > > You have to consider two cases: > > 1) !remapped mode: > > That's reasonably simple because you just have to deal with the HPET > TIMERn_PROCMSG_ROUT register. But then you need to do this directly and > not through any of the existing interrupt facilities. Indeed, there is no need to use the generic interrupt faciities to set affinity; I am dealing with an NMI anyways. > > 2) remapped mode: > > That's way more complex as you _cannot_ ever do anything which touches > the IOMMU and the related tables. > > So you'd need to reserve an IOMMU remapping entry for each CPU upfront, > store the resulting value for the HPET TIMERn_PROCMSG_ROUT register in > per cpu storage and just modify that one from NMI. > > Though there might be subtle side effects involved, which are related to > the acknowledge part. You need to talk to the IOMMU wizards first. I see. I will look into the code and prototype something that makes sense for the IOMMU maintainers. > > All in all, the idea itself is interesting, but the envisioned approach of > round robin and no fast accessible NMI reason detection is going to create > more problems than it solves. I see it more clearly now. Thanks and BR, Ricardo -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 15 Jun 2018, Ricardo Neri wrote: > On Fri, Jun 15, 2018 at 12:29:06PM +0200, Thomas Gleixner wrote: > > You have to consider two cases: > > > > 1) !remapped mode: > > > > That's reasonably simple because you just have to deal with the HPET > > TIMERn_PROCMSG_ROUT register. But then you need to do this directly and > > not through any of the existing interrupt facilities. > > Indeed, there is no need to use the generic interrupt faciities to set affinity; > I am dealing with an NMI anyways. > > > > 2) remapped mode: > > > > That's way more complex as you _cannot_ ever do anything which touches > > the IOMMU and the related tables. > > > > So you'd need to reserve an IOMMU remapping entry for each CPU upfront, > > store the resulting value for the HPET TIMERn_PROCMSG_ROUT register in > > per cpu storage and just modify that one from NMI. > > > > Though there might be subtle side effects involved, which are related to > > the acknowledge part. You need to talk to the IOMMU wizards first. > > I see. I will look into the code and prototype something that makes sense for > the IOMMU maintainers. I'd recommend to talk to them _before_ you cobble something together. If we cannot reliably switch the affinity by directing the HPET NMI to a different IOMMU remapping entry then the whole scheme does not work at all. Thanks, tglx -- To unsubscribe from this list: send the line "unsubscribe sparclinux" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/kernel/watchdog_hld_hpet.c b/kernel/watchdog_hld_hpet.c index 857e051..c40acfd 100644 --- a/kernel/watchdog_hld_hpet.c +++ b/kernel/watchdog_hld_hpet.c @@ -10,6 +10,7 @@ #include <linux/nmi.h> #include <linux/hpet.h> #include <asm/hpet.h> +#include <asm/cpumask.h> #include <asm/irq_remapping.h> #undef pr_fmt @@ -199,8 +200,8 @@ static irqreturn_t hardlockup_detector_irq_handler(int irq, void *data) * @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. + * kick it. Move the interrupt to the next monitored CPU. The interrupt is + * always handled when if delivered via the Front-Side Bus. * * Returns: * @@ -211,7 +212,7 @@ static int hardlockup_detector_nmi_handler(unsigned int val, struct pt_regs *regs) { struct hpet_hld_data *hdata = hld_data; - unsigned int use_fsb; + unsigned int use_fsb, cpu; /* * If FSB delivery mode is used, the timer interrupt is programmed as @@ -222,8 +223,27 @@ static int hardlockup_detector_nmi_handler(unsigned int val, if (!use_fsb && !is_hpet_wdt_interrupt(hdata)) return NMI_DONE; + /* There are no CPUs to monitor. */ + if (!cpumask_weight(&hdata->monitored_mask)) + return NMI_HANDLED; + inspect_for_hardlockups(regs); + /* + * Target a new CPU. Keep trying until we find a monitored CPU. CPUs + * are addded and removed to this mask at cpu_up() and cpu_down(), + * respectively. Thus, the interrupt should be able to be moved to + * the next monitored CPU. + */ + spin_lock(&hld_data->lock); + for_each_cpu_wrap(cpu, &hdata->monitored_mask, smp_processor_id() + 1) { + if (!irq_set_affinity(hld_data->irq, cpumask_of(cpu))) + break; + pr_err("Could not assign interrupt to CPU %d. Trying with next present CPU.\n", + cpu); + } + spin_unlock(&hld_data->lock); + if (!(hdata->flags & HPET_DEV_PERI_CAP)) kick_timer(hdata); @@ -336,7 +356,7 @@ static int setup_hpet_irq(struct hpet_hld_data *hdata) * Request an interrupt to activate the irq in all the needed domains. */ ret = request_irq(hwirq, hardlockup_detector_irq_handler, - IRQF_TIMER | IRQF_DELIVER_AS_NMI, + IRQF_TIMER | IRQF_DELIVER_AS_NMI | IRQF_NOBALANCING, "hpet_hld", hdata); if (ret) unregister_nmi_handler(NMI_LOCAL, "hpet_hld");
In order to detect hardlockups in all the monitored CPUs, move the interrupt to the next monitored CPU when handling the NMI interrupt; wrap around when reaching the highest CPU in the mask. This rotation is achieved by setting the affinity mask to only contain the next CPU to monitor. In order to prevent our interrupt to be reassigned to another CPU, flag it as IRQF_NONBALANCING. The cpumask monitored_mask keeps track of the CPUs that the watchdog should monitor. This structure is updated when the NMI watchdog is enabled or disabled in a specific CPU. As this mask can change concurrently as CPUs are put online or offline and the watchdog is disabled or enabled, a lock is required to protect the monitored_mask. Cc: Ashok Raj <ashok.raj@intel.com> Cc: Andi Kleen <andi.kleen@intel.com> Cc: Tony Luck <tony.luck@intel.com> Cc: Borislav Petkov <bp@suse.de> Cc: Jacob Pan <jacob.jun.pan@intel.com> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com> Cc: Don Zickus <dzickus@redhat.com> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Michael Ellerman <mpe@ellerman.id.au> Cc: Frederic Weisbecker <frederic@kernel.org> Cc: Alexei Starovoitov <ast@kernel.org> Cc: Babu Moger <babu.moger@oracle.com> Cc: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> Cc: Masami Hiramatsu <mhiramat@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Philippe Ombredanne <pombredanne@nexb.com> Cc: Colin Ian King <colin.king@canonical.com> Cc: Byungchul Park <byungchul.park@lge.com> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> Cc: "Luis R. Rodriguez" <mcgrof@kernel.org> Cc: Waiman Long <longman@redhat.com> Cc: Josh Poimboeuf <jpoimboe@redhat.com> Cc: Randy Dunlap <rdunlap@infradead.org> Cc: Davidlohr Bueso <dave@stgolabs.net> Cc: Christoffer Dall <cdall@linaro.org> Cc: Marc Zyngier <marc.zyngier@arm.com> Cc: Kai-Heng Feng <kai.heng.feng@canonical.com> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> Cc: David Rientjes <rientjes@google.com> Cc: "Ravi V. Shankar" <ravi.v.shankar@intel.com> Cc: x86@kernel.org Cc: iommu@lists.linux-foundation.org Signed-off-by: Ricardo Neri <ricardo.neri-calderon@linux.intel.com> --- kernel/watchdog_hld_hpet.c | 28 ++++++++++++++++++++++++---- 1 file changed, 24 insertions(+), 4 deletions(-)