Skip to contentSkip to conversation

x86, traps: Drop nmi_reason_lock until it is really needed

PATCH RESEND In review Cyrill Gorcunov 3 participants 8 messages Mar 2, 2011 x86

lore
Share & tools
lore

Cyrill Gorcunov

author
#0 lore raw

PATCH RESEND 2/2 x86, traps: Drop nmi_reason_lock until it is really needed

At moment we have only BSP apic configured to listen for external NMIs. So there is no reason for additional spinlock since only BSP will receive them.

Though we still have UV chips which do enable external NMIs on all cpus, but since an approach to allow retrieving NMI reason on BSP only was working pretty fine before -- I assume it still remains valid.

Also it's worth to mention that an initial idea of all this NMI code-path changes was to make BSP hot-unpluggable but until all other parts of kernel is prepared for it (which might consume quite a time to implement) I believe we should not lock/unlock for nothing.

Signed-off-by Cyrill Gorcunov <[email protected]>
---
 arch/x86/kernel/traps.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)
arch/x86/kernel/traps.c +6 -10
Index: linux-2.6.git/arch/x86/kernel/traps.c
=====================================================================
@@ -84,11 +84,6 @@ EXPORT_SYMBOL_GPL(used_vectors);
 static int ignore_nmis;
 
 int unknown_nmi_panic;
-/*
- * Prevent NMI reason port (0x61) being accessed simultaneously, can
- * only be used in NMI handler.
- */
-static DEFINE_RAW_SPINLOCK(nmi_reason_lock);
 
 static inline void conditional_sti(struct pt_regs *regs)
 {

@@ -406,9 +401,12 @@ static notrace __kprobes void default_do
 	if (notify_die(DIE_NMI, "nmi", regs, 0, 2, SIGINT) == NOTIFY_STOP)
 		return;
 
-	/* Non-CPU-specific NMI: NMI sources can be processed on any CPU */
-	raw_spin_lock(&nmi_reason_lock);
-	reason = get_nmi_reason();
+	/*
+	 * Only BSP is configured to listen and handle external NMIs.
+	 * Note this implicitly orders a call to the get_nmi_reason.
+	 */
+	if (!smp_processor_id())
+		reason = get_nmi_reason();
 
 	if (reason & NMI_REASON_MASK) {
 		if (reason & NMI_REASON_SERR)

@@ -422,10 +420,8 @@ static notrace __kprobes void default_do
 		 */
 		reassert_nmi();
 #endif
-		raw_spin_unlock(&nmi_reason_lock);
 		return;
 	}
-	raw_spin_unlock(&nmi_reason_lock);
 
 	unknown_nmi_error(reason, regs);
 }
Signature · 1 line
    Cyrill
#1 lore raw
Quoted 8 lines · Cyrill Gorcunov

At moment we have only BSP apic configured to listen for external NMIs. So there is no reason for additional spinlock since only BSP will receive them.

Though we still have UV chips which do enable external NMIs on all cpus, but since an approach to allow retrieving NMI reason on BSP only was working pretty fine before -- I assume it still remains valid.

I'm not sure I get the point here: we might get NMIs on non-BSP on UV systems ... so we want to remove the spinlock?

If UV systems can get NMIs on any CPU then the lock is needed.

It might have worked before - but UV systems are rare and relatively new - plus the race window is small, so it might not have been triggered in practice.

Quoted 5 lines

Also it's worth to mention that an initial idea of all this NMI code-path changes was to make BSP hot-unpluggable but until all other parts of kernel is prepared for it (which might consume quite a time to implement) I believe we should not lock/unlock for nothing.

That would be another argument in favor of keeping the lock, right?

Thanks,

Ingo

author
#2 lore raw
Quoted 12 lines · Ingo Molnar
Quoted 8 lines · Cyrill Gorcunov

At moment we have only BSP apic configured to listen for external NMIs. So there is no reason for additional spinlock since only BSP will receive them.

Though we still have UV chips which do enable external NMIs on all cpus, but since an approach to allow retrieving NMI reason on BSP only was working pretty fine before -- I assume it still remains valid.

Quoted 8 lines

I'm not sure I get the point here: we might get NMIs on non-BSP on UV systems ... so we want to remove the spinlock?

If UV systems can get NMIs on any CPU then the lock is needed.

It might have worked before - but UV systems are rare and relatively new - plus the race window is small, so it might not have been triggered in practice.

Well, it is incomplete anyway. As far as I can tell even ordering such
NMIs with spinlock would not make situation better 'cause other cpu might obtain unknown nmi (ie two or more cpu's gets NMI then handing started on first found that it was say MCE error, handle it, unlock spinlock and then the second cpu gets this nmi (the reason for which was already handled by first cpu) and sees unknown NMI. So this lock might simply hiding a bug. Of course I might be missing something.

Quoted 8 lines
Quoted 6 lines

Also it's worth to mention that an initial idea of all this NMI code-path changes was to make BSP hot-unpluggable but until all other parts of kernel is prepared for it (which might consume quite a time to implement) I believe we should not lock/unlock for nothing.

That would be another argument in favor of keeping the lock, right?

Yes, but I think this lock should be the last thing which is introduced, after all other parts of kernel are ready for bsp unplug.

Quoted 4 lines

Thanks,

Ingo

Signature · 1 line
    Cyrill
#3 lore raw
Quoted 22 lines · Cyrill Gorcunov
Quoted 12 lines · Ingo Molnar
Quoted 8 lines · Cyrill Gorcunov

At moment we have only BSP apic configured to listen for external NMIs. So there is no reason for additional spinlock since only BSP will receive them.

Though we still have UV chips which do enable external NMIs on all cpus, but since an approach to allow retrieving NMI reason on BSP only was working pretty fine before -- I assume it still remains valid.

Quoted 8 lines

I'm not sure I get the point here: we might get NMIs on non-BSP on UV systems ... so we want to remove the spinlock?

If UV systems can get NMIs on any CPU then the lock is needed.

It might have worked before - but UV systems are rare and relatively new - plus the race window is small, so it might not have been triggered in practice.

Quoted 6 lines

Well, it is incomplete anyway. As far as I can tell even ordering such NMIs with spinlock would not make situation better 'cause other cpu might obtain unknown nmi (ie two or more cpu's gets NMI then handing started on first found that it was say MCE error, handle it, unlock spinlock and then the second cpu gets this nmi (the reason for which was already handled by first cpu) and sees unknown NMI. So this lock might simply hiding a bug.

Well, the lock serializes the read-out of the 'NMI reason' port, the handling of whatever known reason and then the reassertion of the NMI (on 32-bit).

EDAC has a callback in pci_serr_error() - and this lock serializes that. So we cannot just remove a lock like that, if there's any chance of parallel execution on multiple CPUs.

Thanks,

Ingo

author
#4 lore raw

On 03/02/2011 07:03 PM, Ingo Molnar wrote:
...

Quoted 11 lines

Well, the lock serializes the read-out of the 'NMI reason' port, the handling of whatever known reason and then the reassertion of the NMI (on 32-bit).

EDAC has a callback in pci_serr_error() - and this lock serializes that. So we cannot just remove a lock like that, if there's any chance of parallel execution on multiple CPUs.

Thanks,

Ingo

OK, probably we need some UV person CC'ed (not sure whom) just to explain the reason for such nmi-listening model. Meanwhile -- lets drop my patch.

Signature · 1 line
    Cyrill
#5 lore raw
Quoted 10 lines · Cyrill Gorcunov

On 03/02/2011 07:03 PM, Ingo Molnar wrote:
...

Quoted 8 lines

Well, the lock serializes the read-out of the 'NMI reason' port, the handling of whatever known reason and then the reassertion of the NMI (on 32-bit).

EDAC has a callback in pci_serr_error() - and this lock serializes that. So we cannot just remove a lock like that, if there's any chance of parallel execution on multiple CPUs.

Quoted 6 lines
Quoted 3 lines

Thanks,

Ingo

OK, probably we need some UV person CC'ed (not sure whom) just to explain the reason for such nmi-listening model. Meanwhile -- lets drop my patch.

It's for debugging reasons. When their huge machine deadlocks, they wanted an easy mechanism to dump the cpu stacks. That mechanism was an nmi button. The problem was the button would only dump the first cpu. By opening up the other cpus to accept external nmis, they could dump all the cpus.

Now this spinlock doesn't affect them, because they registered an nmi handler to catch it and dump their stack (I modified the code to use DIE_NMIUNKNOWN instead of DIE_NMI to avoid conflict with the nmi_watchdog). But I don't know what the affect is, if that spinlock is not there (I sent a private email to SGI inquiring, their guy wasn't around this week).

Personally I am indifferent to this patch. I don't have any problems with the code the way it is now, but can understand what you mean having stuff lying around as 'dead code'. I had thought Intel would have pushed more patches upstream to remove the BSP lock-in by now.

Cheers,
Don

author
#6 lore raw
Quoted 15 lines · Don Zickus
Quoted 14 lines · Cyrill Gorcunov

On 03/02/2011 07:03 PM, Ingo Molnar wrote:
...

Quoted 11 lines

Well, the lock serializes the read-out of the 'NMI reason' port, the handling of whatever known reason and then the reassertion of the NMI (on 32-bit).

EDAC has a callback in pci_serr_error() - and this lock serializes that. So we cannot just remove a lock like that, if there's any chance of parallel execution on multiple CPUs.

Thanks,

Ingo

Quoted 8 lines
Quoted 2 lines

OK, probably we need some UV person CC'ed (not sure whom) just to explain the reason for such nmi-listening model. Meanwhile -- lets drop my patch.

It's for debugging reasons. When their huge machine deadlocks, they wanted an easy mechanism to dump the cpu stacks. That mechanism was an nmi button. The problem was the button would only dump the first cpu. By opening up the other cpus to accept external nmis, they could dump all the cpus.

Yeah, thanks Don, just noted that (actually the former commit log
/78c06176466cbd1b3f0f67709d3023c40dbebcbd/ didn't mention that x86 masks only LVT1).

Quoted 7 lines

Now this spinlock doesn't affect them, because they registered an nmi handler to catch it and dump their stack (I modified the code to use DIE_NMIUNKNOWN instead of DIE_NMI to avoid conflict with the nmi_watchdog). But I don't know what the affect is, if that spinlock is not there (I sent a private email to SGI inquiring, their guy wasn't around this week).

Don, do you know -- was new nmi-watchdog system tested with UV machine
somewhere?

Quoted 9 lines

Personally I am indifferent to this patch. I don't have any problems with the code the way it is now, but can understand what you mean having stuff lying around as 'dead code'. I had thought Intel would have pushed more patches upstream to remove the BSP lock-in by now.

Cheers,
Don

Signature · 1 line
    Cyrill
#7 lore raw
Quoted 10 lines · Cyrill Gorcunov
Quoted 7 lines

Now this spinlock doesn't affect them, because they registered an nmi handler to catch it and dump their stack (I modified the code to use DIE_NMIUNKNOWN instead of DIE_NMI to avoid conflict with the nmi_watchdog). But I don't know what the affect is, if that spinlock is not there (I sent a private email to SGI inquiring, their guy wasn't around this week).

Don, do you know -- was new nmi-watchdog system tested with UV machine
somewhere?

They are testing it now, I am working through the issues with them.

Cheers,
Don