GHES NMI handler cleanup

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

Do that.

Quoted 5 lines · Borislav Petkov

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Quoted 1 line

Do that.

I think this should indeed be pushed forward. It fixes horrible spinlock contention on systems which are under NMI storm (such as when perf is active) unrelated to GHES.

Quoted 19 lines

Quoted 5 lines

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Quoted 1 line

Do that.

Quoted 3 lines

I think this should indeed be pushed forward. It fixes horrible spinlock contention on systems which are under NMI storm (such as when perf is active) unrelated to GHES.

Right, so I tested injecting an error without your patch and same behavior. So it all points at global sources AFAICT. It would be cool, though, if someone who knows the fw confirms unambiguously.

Three weeks have passed, therefore I find this an appropriate time for a friendly ping :)

Rafael? Naoya? Huang?

This fixes a contention spinlock problem in NMI observed on a real HW, so it would be really nice to have it fixed.

I think we should apply this.

Here's why: nothing in the ghes_notify_nmi() handler does CPU-specific accesses

Tony, objections?

Quoted 4 lines

I think we should apply this.

Here's why: nothing in the ghes_notify_nmi() handler does CPU-specific accesses

This looks to be true.

Quoted 1 line

Tony, objections?

No objections.

Quoted 5 lines · Borislav Petkov

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Quoted 2 lines

Do that.

I think this should indeed be pushed forward. It fixes horrible spinlock contention on systems which are under NMI storm (such as when perf is active) unrelated to GHES.

Thanks,

Signature · 7 lines

Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 12 lines · Jiri Kosina

Quoted 5 lines · Borislav Petkov

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Quoted 6 lines

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Hi,

I believe the answer to this question is no, they are not global but instead external. All external NMIs are routed to one cpu, normally cpu0. This spinlock was made global to handle the 'someday' case of hotplugging the bsp cpu (cpu0).

The other external NMIs (IO_CHECK and SERR) suffer from the same spinlock problem. I tried using an irq_workqueue to work around quirks there and PeterZ wasn't very fond of it (though he couldn't think of a better way to solve it).

This patch seems interesting but you might still run into the thundering herd problem with the global spinlock in arch/x86/kernel/nmi.c::default_do_nmi(). That functions grabs a global spinlock before processing the external NMI list (which GHES is a part of).

So I am assuming this patch solves the 'thundering herd' problem by minimizing all the useless writes the spinlock would do for each cpu that noticed it had no work to do?

In that case, I am in favor of this solution and would like to apply a similar solution to arch/x86/kernel/nmi.c, to see if that helps there too.

Cheers,
Don

Quoted 17 lines

Quoted 2 lines

Do that.

I think this should indeed be pushed forward. It fixes horrible spinlock contention on systems which are under NMI storm (such as when perf is active) unrelated to GHES.

Thanks,

Signature · 7 lines

Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Quoted 16 lines · Jiri Kosina

Quoted 5 lines · Borislav Petkov

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

Quoted 2 lines

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Hi,

I believe the answer to this question is no, they are not global but instead external. All external NMIs are routed to one cpu, normally cpu0.

This spinlock was made global to handle the 'someday' case of hotplugging the bsp cpu (cpu0).

The other external NMIs (IO_CHECK and SERR) suffer from the same spinlock problem. I tried using an irq_workqueue to work around quirks there and PeterZ wasn't very fond of it (though he couldn't think of a better way to solve it).

This patch seems interesting but you might still run into the thundering herd problem with the global spinlock in arch/x86/kernel/nmi.c::default_do_nmi(). That functions grabs a global spinlock before processing the external NMI list (which GHES is a part of).

So I am assuming this patch solves the 'thundering herd' problem by minimizing all the useless writes the spinlock would do for each cpu that noticed it had no work to do?

Quoted 22 lines · Don Zickus

Quoted 12 lines · Jiri Kosina

Quoted 5 lines · Borislav Petkov

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

I originally wasn't 100% sure whether GHES sources are global (i.e. if it really doesn't matter which CPU is reading the registers), but looking at the code more it actually seems that this is really the right thing to do.

Quoted 6 lines

Rationale: ghes_ioremap_pfn_nmi() always ioremaps() (exclusively) the page with the registers, performs apei_read() (which is ghes-source specific, but not CPU-specific) and unmaps the page again.

There is nothing that would make this CPU-specific. Adding Huang Ying (the original author of the code) to confirm this. Huang?

Hi,

Quoted 2 lines

I believe the answer to this question is no, they are not global but instead external. All external NMIs are routed to one cpu, normally cpu0.

Actually, the things we're talking about here are not global NMIs but global error sources. I.e., the GHES sources which use an NMI to report errors with and which we noodle over in ghes_notify_nmi() twice:

	list_for_each_entry_rcu(ghes, &ghes_nmi, list) {

		...

Quoted 8 lines

This spinlock was made global to handle the 'someday' case of hotplugging the bsp cpu (cpu0).

The other external NMIs (IO_CHECK and SERR) suffer from the same spinlock problem. I tried using an irq_workqueue to work around quirks there and PeterZ wasn't very fond of it (though he couldn't think of a better way to solve it).

Quoted 4 lines

This patch seems interesting but you might still run into the thundering herd problem with the global spinlock in arch/x86/kernel/nmi.c::default_do_nmi(). That functions grabs a global spinlock before processing the external NMI list (which GHES is a part of).

nmi_reason_lock? And our handler is registered with register_nmi_handler() and so we iterate over it in nmi_handle(). It'd be interesting to know what NMI reason we get for those GHES NMIs so that we know whether nmi_handle() is called under the lock or not...

Quoted 4 lines

So I am assuming this patch solves the 'thundering herd' problem by minimizing all the useless writes the spinlock would do for each cpu that noticed it had no work to do?

Not that spinlock. It used to take another one in ghes_notify_nmi(). Removing it solved the issue. There were even boxes which would do:

[   24.332560] INFO: NMI handler (ghes_notify_nmi) took too long to run: 3.265 msecs
[   24.332567] INFO: NMI handler (ghes_notify_nmi) took too long to run: 5.946 msecs
[   24.332568] INFO: NMI handler (ghes_notify_nmi) took too long to run: 5.948 msecs

just like that during boot.

It was basically a lot of purely useless lock grabbing for no reason whatsoever.

Your solution seems much simpler. :-)

I followed up in another email stating I mis-spoke. I forgot this still uses the NMI_LOCAL shared NMI. So every perf NMI, will also call the GHES handler to make sure NMIs did not piggy back each other. So I don't believe

the NMI reason lock is called a majority of the time (except when the NMI is swallowed, but that is under heavy perf load...).

We both agree the mechanics of the spinlock are overkill here and cause much cache contention. Simplifying it to just 'reads' and return removes most of the problem.

Quoted 1 line · Don Zickus

Your solution seems much simpler. :-)

... and I love simpler :-)

Quoted 3 lines

I followed up in another email stating I mis-spoke. I forgot this still uses the NMI_LOCAL shared NMI. So every perf NMI, will also call the GHES handler to make sure NMIs did not piggy back each other. So I don't believe

And this is something we should really fix - perf and RAS should not have anything to do with each other. But I don't know the NMI code to even have an idea how. I don't even know whether we can differentiate NMIs, hell, I can't imagine the hardware giving us a different NMI reason through get_nmi_reason(). Maybe that byte returned from NMI_REASON_PORT is too small and hangs on too much legacy crap to even be usable. Questions over questions...

Quoted 3 lines

the NMI reason lock is called a majority of the time (except when the NMI is swallowed, but that is under heavy perf load...).

..

Quoted 3 lines

We both agree the mechanics of the spinlock are overkill here and cause much cache contention. Simplifying it to just 'reads' and return removes most of the problem.

Right.

Signature · 5 lines

Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

RAS doesn't go through the legacy ports (ie get_nmi_reason()). Instead it triggers the external NMI through a different bit (ioapic I think).

The nmi code has no idea what io_remap'ed address apei is using to map its error handling register that GHES uses. Unlike the legacy port which is always port 0x61.

So, with NMI being basically a shared interrupt, with no ability to discern who sent the interrupt (and even worse no ability to know how _many_ were sent as the NMI is edge triggered instead of level triggered). As a result we rely on the NMI handlers to talk to their address space/registers to determine if they were they source of the interrupt.

Anyway, any ideas or thoughts for improvement are always welcomed. :-)

From: Borislav Petkov [mailto:[email protected]]
Sent: Friday, March 27, 2015 5:23 PM

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

Do that.

Signed-off-by Jiri Kosina <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 drivers/acpi/apei/ghes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/acpi/apei/ghes.c +3 -3

index 94a44bad5576..2bfd53cbfe80 100644
@@ -729,10 +729,10 @@ static struct llist_head ghes_estatus_llist;
 static struct irq_work ghes_proc_irq_work;
 
 /*
- * NMI may be triggered on any CPU, so ghes_nmi_lock is used for
- * mutual exclusion.
+ * NMI may be triggered on any CPU, so ghes_in_nmi is used for
+ * having only one concurrent reader.
  */
-static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
+static atomic_t ghes_in_nmi = ATOMIC_INIT(0);
 
 static LIST_HEAD(ghes_nmi);
 

drivers/acpi/apei/ghes.c +1 -2

 	list_for_each_entry_rcu(ghes, &ghes_nmi, list) {
 		if (ghes_read_estatus(ghes, 1)) {
 			ghes_clear_estatus(ghes);

@@ -863,7 +865,7 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 #ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG
 	irq_work_queue(&ghes_proc_irq_work);
 #endif
-	raw_spin_unlock(&ghes_nmi_lock);
+	atomic_dec(&ghes_in_nmi);
 	return ret;
 }
 
--

2.3.3

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 8 lines

drivers/acpi/apei/ghes.c +3 -1

@@ -840,7 +840,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 	struct ghes *ghes;
 	int sev, ret = NMI_DONE;
 
-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

Just a simple question.
Why not just using cmpxchg here instead of atomic_add_unless so that no atomic_dec will be needed.

From: Borislav Petkov [mailto:[email protected]]
Sent: Monday, April 27, 2015 4:47 PM

Quoted 11 lines · Lv

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 8 lines

drivers/acpi/apei/ghes.c +3 -1

@@ -840,7 +840,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 	struct ghes *ghes;
 	int sev, ret = NMI_DONE;
 
-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

Just a simple question.
Why not just using cmpxchg here instead of atomic_add_unless so that no atomic_dec will be needed.

What do you think atomic_add_unless ends up doing:

#APP
# 177 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:

	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37056, MEM[(volatile u32 *)&ghes_in_nmi]

# 0 "" 2
#NO_APP

And you need to atomic_dec() so that another reader can enter, i.e. how the exclusion primitive works.

Or did you have something else in mind?

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

From: Zheng, Lv
Sent: Tuesday, April 28, 2015 8:44 AM

Hi,

Quoted 29 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Monday, April 27, 2015 4:47 PM

Quoted 11 lines · Lv

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 8 lines

drivers/acpi/apei/ghes.c +3 -1

@@ -840,7 +840,9 @@ static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs)
 	struct ghes *ghes;
 	int sev, ret = NMI_DONE;
 
-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

Just a simple question.
Why not just using cmpxchg here instead of atomic_add_unless so that no atomic_dec will be needed.

What do you think atomic_add_unless ends up doing:

#APP
# 177 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:

	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37056, MEM[(volatile u32 *)&ghes_in_nmi]

# 0 "" 2
#NO_APP

Quoted 4 lines

And you need to atomic_dec() so that another reader can enter, i.e. how the exclusion primitive works.

Or did you have something else in mind?

My mistake.
I mean cmpxchg() and xchg() (or atomic_cmpxchg() and atomic_xchg()) pair here, so nothing can be reduced.

But IMO, atomic_add_unless() is implemented via cmpxchg on many architectures.
And it might be better to use it directly here which is a bit faster as you actually only need one value switch here.

Thanks and best regards
-Lv

Quoted 7 lines

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Quoted 14 lines

Quoted 14 lines

#APP
# 177 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:

	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37056, MEM[(volatile u32 *)&ghes_in_nmi]

# 0 "" 2
#NO_APP

And you need to atomic_dec() so that another reader can enter, i.e. how the exclusion primitive works.

Quoted 4 lines

Quoted 1 line

Or did you have something else in mind?

My mistake.
I mean cmpxchg() and xchg() (or atomic_cmpxchg() and atomic_xchg()) pair here, so nothing can be reduced.

Let me correct, it should be atomic_cmpxchg() and atomic_set() here as you only need to switch between 0 and 1. Sorry for the noise.

From: Borislav Petkov [mailto:[email protected]]
Sent: Friday, March 27, 2015 5:23 PM
Subject: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

From: Jiri Kosina <[email protected]>

Since GHES sources are global, we theoretically need only a single CPU reading them per NMI instead of a thundering herd of CPUs waiting on a spinlock in NMI context for no reason at all.

Do that.

Signed-off-by Jiri Kosina <[email protected]>

Signed-off-by Borislav Petkov <[email protected]>

---
 drivers/acpi/apei/ghes.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

drivers/acpi/apei/ghes.c +3 -3

index 94a44bad5576..2bfd53cbfe80 100644
@@ -729,10 +729,10 @@ static struct llist_head ghes_estatus_llist;
 static struct irq_work ghes_proc_irq_work;
 
 /*
- * NMI may be triggered on any CPU, so ghes_nmi_lock is used for
- * mutual exclusion.
+ * NMI may be triggered on any CPU, so ghes_in_nmi is used for
+ * having only one concurrent reader.
  */
-static DEFINE_RAW_SPINLOCK(ghes_nmi_lock);
+static atomic_t ghes_in_nmi = ATOMIC_INIT(0);
 
 static LIST_HEAD(ghes_nmi);
 

drivers/acpi/apei/ghes.c +0 -1

 	return ret;
 }
 
--

2.3.3

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 4 lines

drivers/acpi/apei/ghes.c +3 -1

-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

if (atomic_cmpxchg(&ghes_in_nmi, 0, 1))

	return ret;

From: Borislav Petkov [mailto:[email protected]]
Sent: Tuesday, April 28, 2015 9:59 PM
Subject: Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

Quoted 7 lines · Lv

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 4 lines

drivers/acpi/apei/ghes.c +3 -1

-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

if (atomic_cmpxchg(&ghes_in_nmi, 0, 1))

	return ret;

Ok, now I understand what you mean.

We absolutely want to use atomic_add_unless() because we get to save us the expensive

LOCK; CMPXCHG

if the value was already 1. Which is exactly what this patch is trying to avoid - a thundering herd of cores CMPXCHGing a global variable.

I.e.,

	movl	ghes_in_nmi(%rip), %ecx	# MEM[(const int *)&ghes_in_nmi], c
	cmpl	$1, %ecx	#, c
	je	.L311	#,				<--- exit here if ghes_in_nmi == 1.
	leal	1(%rcx), %edx	#, D.37163
	movl	%ecx, %eax	# c, c

#APP
# 177 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:

	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37163, MEM[(volatile u32 *)&ghes_in_nmi]

# 0 "" 2
#NO_APP

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

From: Zheng, Lv
Sent: Wednesday, April 29, 2015 8:25 AM
Subject: RE: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

Hi,

Quoted 18 lines

From: Borislav Petkov [mailto:[email protected]]
Sent: Tuesday, April 28, 2015 9:59 PM
Subject: Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

Quoted 7 lines · Lv

drivers/acpi/apei/ghes.c static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) · line 845

Quoted 4 lines

drivers/acpi/apei/ghes.c +3 -1

-	raw_spin_lock(&ghes_nmi_lock);
+	if (!atomic_add_unless(&ghes_in_nmi, 1, 1))
+		return ret;
+

if (atomic_cmpxchg(&ghes_in_nmi, 0, 1))

	return ret;

Ok, now I understand what you mean.

We absolutely want to use atomic_add_unless() because we get to save us the expensive

Quoted 4 lines

LOCK; CMPXCHG

if the value was already 1. Which is exactly what this patch is trying to avoid - a thundering herd of cores CMPXCHGing a global variable.

IMO, on most architectures, the "cmp" part should work just like what you've done with "if". And on some architectures, if the "xchg" doesn't happen, the "cmp" part even won't cause a pipe line hazard.

Thanks and best regards
-Lv

Quoted 25 lines

I.e.,

	movl	ghes_in_nmi(%rip), %ecx	# MEM[(const int *)&ghes_in_nmi], c
	cmpl	$1, %ecx	#, c
	je	.L311	#,				<--- exit here if ghes_in_nmi == 1.
	leal	1(%rcx), %edx	#, D.37163
	movl	%ecx, %eax	# c, c

#APP
# 177 "./arch/x86/include/asm/atomic.h" 1
.pushsection .smp_locks,"a"
.balign 4
.long 671f - .
.popsection
671:

	lock; cmpxchgl %edx,ghes_in_nmi(%rip)	# D.37163, MEM[(volatile u32 *)&ghes_in_nmi]

# 0 "" 2
#NO_APP

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Quoted 10 lines

Quoted 7 lines

We absolutely want to use atomic_add_unless() because we get to save us the expensive

LOCK; CMPXCHG

if the value was already 1. Which is exactly what this patch is trying to avoid - a thundering herd of cores CMPXCHGing a global variable.

IMO, on most architectures, the "cmp" part should work just like what you've done with "if". And on some architectures, if the "xchg" doesn't happen, the "cmp" part even won't cause a pipe line hazard.

If you man the LOCK prefix, I understand now.

From: Borislav Petkov [mailto:[email protected]]
Sent: Wednesday, April 29, 2015 4:14 PM
Subject: Re: [RFC PATCH 5/5] GHES: Make NMI handler have a single reader

Quoted 10 lines · Lv

Quoted 10 lines

Quoted 7 lines

We absolutely want to use atomic_add_unless() because we get to save us the expensive

LOCK; CMPXCHG

if the value was already 1. Which is exactly what this patch is trying to avoid - a thundering herd of cores CMPXCHGing a global variable.

IMO, on most architectures, the "cmp" part should work just like what you've done with "if". And on some architectures, if the "xchg" doesn't happen, the "cmp" part even won't cause a pipe line hazard.

Even if CMPXCHG is being split into several microops, they all still need to flow down the pipe and require resources and tracking. And you only know at retire time what the CMP result is and can "discard" the XCHG part. Provided the uarch is smart enough to do that.

This is probably why CMPXCHG needs 5,6,7,10,22,... cycles depending on uarch and vendor, if I can trust Agner Fog's tables. And I bet those numbers are best-case only and in real-life they probably tend to fall out even worse.

CMP needs only 1. On almost every uarch and vendor. And even that cycle probably gets hidden with a good branch predictor.

Quoted 2 lines

If you man the LOCK prefix, I understand now.

And that makes several times worse: 22, 40, 80, ... cycles.

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--

Are there any such data around the SC and LL (MIPS)?

From: Borislav Petkov [mailto:[email protected]]
Sent: Thursday, April 30, 2015 4:49 PM

Quoted 1 line · Lv

Are there any such data around the SC and LL (MIPS)?

What, you can't search the internet yourself?

--
Regards/Gruss,
Boris.

ECO tip #101: Trim your mails when you reply.
--