x86: avoid atomic operation in test_and_set_bit_lock if possible

Quoted 1 line

Quoted 1 line · Nikanth Karthikesan

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty
scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if it finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

arch/x86/include/asm/bitops.h +0 -13

index 903683b..26a42ff 100644
@@ -203,19 +203,6 @@ static inline int test_and_set_bit(int nr, volatile 
 unsigned long *addr)
 }
 
 /**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
-static __always_inline int
-test_and_set_bit_lock(int nr, volatile unsigned long *addr)
-{
-	return test_and_set_bit(nr, addr);
-}
-
-/**
  * __test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set

* @addr: Address to count from

arch/x86/include/asm/bitops.h +19 -0

@@ -339,6 +326,25 @@ static int test_bit(int nr, const volatile unsigned long 
 *addr);
 	 : variable_test_bit((nr), (addr)))
 
 /**
+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */
+static __always_inline int
+test_and_set_bit_lock(int nr, volatile unsigned long *addr)
+{
+#ifdef CONFIG_SMP
+	barrier();
+	if (test_bit(nr, addr))
+		return 1;
+#endif
+	return test_and_set_bit(nr, addr);
+}
+
+/**
  * __ffs - find first set bit in word
  * @word: The word to search

*

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if it finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

arch/x86/include/asm/bitops.h +7 -13

index 903683b..26a42ff 100644
@@ -203,19 +203,6 @@ static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 }
 
 /**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
-static __always_inline int
-test_and_set_bit_lock(int nr, volatile unsigned long *addr)
-{
-	return test_and_set_bit(nr, addr);
-}
-
-/**
  * __test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set
  * @addr: Address to count from

@@ -339,6 +326,25 @@ static int test_bit(int nr, const volatile unsigned long *addr);
 	 : variable_test_bit((nr), (addr)))
 
 /**
+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */

Quoted 7 lines · Nikanth Karthikesan

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if it finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

Quoted 13 lines

+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */

arch/x86/include/asm/bitops.h +6 -0

+static __always_inline int
+test_and_set_bit_lock(int nr, volatile unsigned long *addr)
+{
+#ifdef CONFIG_SMP
+	barrier();
+	if (test_bit(nr, addr))

arch/x86/include/asm/bitops.h static int test_bit(int nr, const volatile unsigned long *addr); · line 345

Quoted 4 lines

arch/x86/include/asm/bitops.h +4 -0

+		return 1;
+#endif
+	return test_and_set_bit(nr, addr);
+}

On modern x86 CPUs there's no "#LOCK signal" anymore - it's replaced by a M[O]ESI cache coherency bus. I'd expect modern x86 CPUs to be pretty fast when the cacheline is local and the bit is set already.

So you really need to back up your patch with actual hard numbers. Putting this code into user-space and using pthreads to loop on the same global variable and testing the before/after effect would be sufficient i think. You can use 'perf stat --repeat 10' kind of measurements to see whether there's any improvement larger than the noise of the measurement.

Quoted 1 line

Quoted 1 line · Borislav Petkov

Quoted 5 lines · Nikanth Karthikesan

Quoted 3 lines

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this

penalty

Quoted 2 lines

Quoted 2 lines

scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if

it

Quoted 3 lines

Quoted 3 lines

finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

[..]

Quoted 21 lines

Quoted 7 lines

+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */

arch/x86/include/asm/bitops.h static int test_bit(int nr, const volatile unsigned long *addr); · line 345

Quoted 10 lines

arch/x86/include/asm/bitops.h +10 -0

+static __always_inline int
+test_and_set_bit_lock(int nr, volatile unsigned long *addr)
+{
+#ifdef CONFIG_SMP
+	barrier();
+	if (test_bit(nr, addr))
+		return 1;
+#endif
+	return test_and_set_bit(nr, addr);
+}

On modern x86 CPUs there's no "#LOCK signal" anymore - it's replaced by a M[O]ESI cache coherency bus. I'd expect modern x86 CPUs to be pretty fast when the cacheline is local and the bit is set already.

Quoted 6 lines

So you really need to back up your patch with actual hard numbers. Putting this code into user-space and using pthreads to loop on the same global variable and testing the before/after effect would be sufficient i think. You can use 'perf stat --repeat 10' kind of measurements to see whether there's any improvement larger than the noise of the measurement.

and Ingo's question is right on the money - is this speedup noticeable or does it simply disappear in the noise?

Quoted 1 line

Quoted 1 line

Quoted 1 line · Borislav Petkov

(haven't seen Ingo's original reply, so responding here)

Quoted 7 lines · Ingo Molnar

Quoted 5 lines · Nikanth Karthikesan

Quoted 3 lines

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this

penalty

Quoted 30 lines

Quoted 2 lines

Quoted 2 lines

scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if

it

Quoted 3 lines

Quoted 3 lines

finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

[..]

Quoted 21 lines

Quoted 7 lines

+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */

arch/x86/include/asm/bitops.h static int test_bit(int nr, const volatile unsigned long *addr); · line 345

Quoted 10 lines

arch/x86/include/asm/bitops.h +10 -0

+static __always_inline int
+test_and_set_bit_lock(int nr, volatile unsigned long *addr)
+{
+#ifdef CONFIG_SMP
+	barrier();
+	if (test_bit(nr, addr))
+		return 1;
+#endif
+	return test_and_set_bit(nr, addr);
+}

On modern x86 CPUs there's no "#LOCK signal" anymore - it's replaced by a M[O]ESI cache coherency bus. I'd expect modern x86 CPUs to be pretty fast when the cacheline is local and the bit is set already.

Are you certain? Iirc the lock prefix implies minimally a read-for- ownership (if CPUs are really smart enough to optimize away the write - I wonder whether that would be correct at all when it comes to locked operations), which means a cacheline can still be bouncing heavily.

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 5 lines · Jan Beulich

Are you certain? Iirc the lock prefix implies minimally a read-for- ownership (if CPUs are really smart enough to optimize away the write - I wonder whether that would be correct at all when it comes to locked operations), which means a cacheline can still be bouncing heavily.

Yeah. On what workload was this?

Generally you use test_and_set_bit() if you expect it to be 'owned' by whoever calls it, and released by someone else.

It would be really useful to run perf top on an affected box and see which kernel function causes this. It might be better to add a test_bit() to the affected codepath - instead of bloating all test_and_set_bit() users.

Note that the patch can also cause overhead: the test_bit() can miss the cache, it will bring in the cacheline shared, and the subsequent test_and_set() call will then dirty the cacheline - so the CPU might miss again and has to wait for other CPUs to first flush this cacheline.

So we really need more details here.

Quoted 8 lines

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 5 lines · Jan Beulich

Are you certain? Iirc the lock prefix implies minimally a read-for- ownership (if CPUs are really smart enough to optimize away the write - I wonder whether that would be correct at all when it comes to locked operations), which means a cacheline can still be bouncing heavily.

Quoted 8 lines

Yeah. On what workload was this?

Generally you use test_and_set_bit() if you expect it to be 'owned' by whoever calls it, and released by someone else.

It would be really useful to run perf top on an affected box and see which kernel function causes this. It might be better to add a test_bit() to the affected codepath - instead of bloating all test_and_set_bit() users.

Indeed, I agree with you and Linus in this aspect.

Quoted 5 lines

Note that the patch can also cause overhead: the test_bit() can miss the cache, it will bring in the cacheline shared, and the subsequent test_and_set() call will then dirty the cacheline - so the CPU might miss again and has to wait for other CPUs to first flush this cacheline.

Quoted 1 line

So we really need more details here.

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

Quoted 26 lines · Jan Beulich

Quoted 10 lines

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 5 lines · Jan Beulich

Are you certain? Iirc the lock prefix implies minimally a read-for- ownership (if CPUs are really smart enough to optimize away the write - I wonder whether that would be correct at all when it comes to locked operations), which means a cacheline can still be bouncing heavily.

Yeah. On what workload was this?

Quoted 7 lines

Generally you use test_and_set_bit() if you expect it to be 'owned' by whoever calls it, and released by someone else.

It would be really useful to run perf top on an affected box and see which kernel function causes this. It might be better to add a test_bit() to the affected codepath - instead of bloating all test_and_set_bit() users.

Indeed, I agree with you and Linus in this aspect.

Quoted 6 lines

Note that the patch can also cause overhead: the test_bit() can miss the cache, it will bring in the cacheline shared, and the subsequent test_and_set() call will then dirty the cacheline - so the CPU might miss again and has to wait for other CPUs to first flush this cacheline.

Quoted 6 lines

Quoted 1 line

So we really need more details here.

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

The page lock flag is indeed one of those (rather rare) exceptions to typical object locking patterns. So in that particular case adding the PageLocked() test to trylock_page() would be the right approach to improving performance.

In the common case this change actively hurts for various reasons:

- can turn a cache miss into two cache misses
- adds an often unnecessary branch instruction
- adds often unnecessary bloat
- leaks a barrier

Quoted 8 lines

Quoted 1 line

Quoted 1 line · Ingo Molnar

Quoted 5 lines · Jan Beulich

Are you certain? Iirc the lock prefix implies minimally a read-for- ownership (if CPUs are really smart enough to optimize away the write - I wonder whether that would be correct at all when it comes to locked operations), which means a cacheline can still be bouncing heavily.

Quoted 8 lines

Yeah. On what workload was this?

Generally you use test_and_set_bit() if you expect it to be 'owned' by whoever calls it, and released by someone else.

It would be really useful to run perf top on an affected box and see which kernel function causes this. It might be better to add a test_bit() to the affected codepath - instead of bloating all test_and_set_bit() users.

Indeed, I agree with you and Linus in this aspect.

Quoted 6 lines

Note that the patch can also cause overhead: the test_bit() can miss the cache, it will bring in the cacheline shared, and the subsequent test_and_set() call will then dirty the cacheline - so the CPU might miss again and has to wait for other CPUs to first flush this cacheline.

So we really need more details here.

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

Jack - were there any similar measurements done on upstream code?

Jan

**** Quoting Jack Steiner <[email protected]> ****

The following tests were run on UVSW :

	768p Westmere
	 128 nodes

Boot times - greater than 2X reduction in boot time:

	2286s PTF #8
	1899s PTF #8
	 975s new algorithm
	 962s new algorithm

Boot messages referring to udev timeouts - eliminated:
(After the udevadm settle timeout, the events queue contains):

7174 PTF #8
9435 PTF #8
0 new algorithm
0 new algorithm

AIM7 results - no difference at low numbers of tasks. Improvements at high counts:

	Jobs/Min at 2000 users
		 5100 PTF #8
		17750 new algorithm

	Wallclock seconds to run test at 2000 users
		2250s PTF #8
	 	 650s new algorithm

	CPU Seconds at 2000 users
		1300000 PTF #8
		  14000 new algorithm

Test of large parallel app faulting for text.

	Text resident in page cache (10000 pages):
		REAL	USER		SYS
		22.830s	23m5.567s	 85m59.042s	PTF #8 run1
		26.267s	34m3.536s	104m20.035s	PTF #8 run2
		10.890s	19m27.305s	 39m50.949s	new algorithm run1
		10.860s	20m42.698s	 40m48.889s	new algorithm run2

	Text on Disk (1000 pages)
		REAL	USER		SYS
		31.658s	9m25.379s	71m11.967s	PTF #8
		24.348s	6m15.323s	45m27.578s	new algorithm

_________________________________________________________________________________
The following tests were run on UV48:

	    4 racks
	  256 sockets
	2452p westmere

Boot time:

	4562 sec PTF#8
	1965 sec new

MPI "helloworld" with 1024 ranks

	35 sec PTF #8
	22 sec new

Test of large parallel app faulting for text.

	Text resident in page cache (10000 pages):
		REAL	USER		SYS
		46.394s	141m19s		366m53s		PTF #8
		38.986s	137m36		264m52s		PTF #8
		 7.987s	 34m50s		 42m36s		new algorithm
		10.550s	 43m31s		 59m45s		new algorithm

AIM7 Results (this is the original AIM7 - not the recent opensource version)

	------------------------------
	Jobs/Min
	 TASKS      PTF #8         new
	     1       487.8       486.6
	    10      4405.8      4940.6
	   100     18570.5     18198.9
	  1000     17262.3     17167.1
	  2000      4879.3     18163.9
	  4000        **       18846.2
	------------------------------
	Real Seconds
	 TASKS      PTF #8         new
	     1        11.9        12.0
	    10        13.2        11.8
	   100        31.3        32.0
	  1000       337.2       339.0
	  2000      2385.6       640.8
	  4000        **        1235.3
	------------------------------
	CPU Seconds
	 TASKS      PTF #8         new
	     1         1.6         1.6
	    10        11.5        12.9
	   100       132.2       137.2
	  1000      4486.5      6586.3
	  2000   1758419.7     27845.7
	  4000        **       65619.5

           ** Timed out

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

Quoted 1 line

Quoted 1 line · Linus Torvalds

Quoted 5 lines · Jan Beulich

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

Ok. __lock_page() _definitely_ should do the test_bit() thing first, because it's normally called from lock_page() that has already tested the bit.

But it already seems to do that, so I'm wondering what your variant is.

I'm also a bit surprised that lock_page() is that hot (unless your _lock_page() variant is simply too broken and ends up spinning?). Maybe we have some path that takes the page lock unnecessarily? What's the load?

Quoted 5 lines · Jan Beulich

The problem was observed with __lock_page() (in a variant not upstream for reasons not known to me), and prefixing e.g. trylock_page() with an extra PageLocked() check yielded the below quoted improvements.

Ok. __lock_page() _definitely_ should do the test_bit() thing first, because it's normally called from lock_page() that has already tested the bit.

But it already seems to do that, so I'm wondering what your variant is.

I'm also a bit surprised that lock_page() is that hot (unless your _lock_page() variant is simply too broken and ends up spinning?). Maybe we have some path that takes the page lock unnecessarily? What's the load?

This cacheline bouncing was actually observed and measured on SGI UV systems, but I'm not certain we're permitted to publish that data. I'm copying the two SGI guys who had reported that issue (and the special case fix, which Nikanth simply generalized) to us, for them to decide.

Quoted 6 lines

This cacheline bouncing was actually observed and measured on SGI UV systems, but I'm not certain we're permitted to publish that data. I'm copying the two SGI guys who had reported that issue (and the special case fix, which Nikanth simply generalized) to us, for them to decide.

We frequently run into the cacheline bouncing issues. I don't have the data handy that you refer to, but feel free to publish it.

One good way to see cache bounces is to run a misses/accesses ratio profile:

   perf top -e cache-misses -e cache-references --count-filter 10

Note the two events: this runs a 'weighted' profile, you'll see (LLC) cache-misses of a function relative to cache-references it does, a misses/references ratio in essence.

Quoted 5 lines · Ingo Molnar

One good way to see cache bounces is to run a misses/accesses ratio profile:

   perf top -e cache-misses -e cache-references --count-filter 10

Quoted 3 lines

Note the two events: this runs a 'weighted' profile, you'll see (LLC) cache-misses of a function relative to cache-references it does, a misses/references ratio in essence.

If anyone does this on a Nehalem please only use 2.6.39-rc*+ which includes the offcore patches. Anything before that will get you complete bogus numbers.

In that case the second example i quoted can be used:

  perf top -e L1-dcache-store-misses -e L1-dcache-stores --count-filter 10

One good way to see cache bounces is to run a misses/accesses ratio profile:

   perf top -e cache-misses -e cache-references --count-filter 10

Quoted 4 lines · Ingo Molnar

One good way to see cache bounces is to run a misses/accesses ratio profile:

   perf top -e cache-misses -e cache-references --count-filter 10

Oh well , something must be broken...

"perf top" is working here, but if I use any "-e ...." argument, it fails :

# perf top -e cache-misses

Error: sys_perf_event_open() syscall returned with 2 (No such file or directory). /bin/dmesg may provide additional information.

Fatal: No CONFIG_PERF_EVENTS=y kernel support configured?

# grep PERF_EVENTS .config
CONFIG_HAVE_PERF_EVENTS=y
CONFIG_PERF_EVENTS=y
CONFIG_HAVE_PERF_EVENTS_NMI=y

Nothing in dmesg...

uname -a
Linux ed001 2.6.38-08165-g2712750-dirty #517 SMP Thu Mar 24 21:31:00 CET 2011 i686 i686 i386 GNU/Linux

Does it work better if you run it as root?

To easiest way to debug as user is to do:

echo -1 > /proc/sys/kernel/perf_event_paranoid

and to put this into /etc/sysctl.conf:

kernel.perf_event_paranoid = -1

[    0.015998] Performance Events: PEBS fmt0+, Core2 events, Broken BIOS detected, using software events only.
[    0.015998] [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 186 is 43003c)

I am pretty sure I used "-e ..." some time ago with same BIOS.

The "if anyone else uses the PMU throw in your toys and sulk" check was only recently added.

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),
but I think Linux should do better than just disabling PMU use when this happens.

However I suspect taking over SCI would cause endless problems and is very likely not a good idea.

Quoted 13 lines · Andi Kleen

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),
but I think Linux should do better than just disabling PMU use when this happens.

Quoted 2 lines

However I suspect taking over SCI would cause endless problems and is very likely not a good idea.

I tried many different changes in BIOS and all failed (the machine is damn slow at boot, this takes age).

I am stuck :(

From 14df27334ac47a5cec67fb2238d14499346acc38 Mon Sep 17 00:00:00 2001

Quoted 17 lines · Eric Dumazet

Quoted 15 lines · Andi Kleen

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),
but I think Linux should do better than just disabling PMU use when this happens.

However I suspect taking over SCI would cause endless problems and is very likely not a good idea.

Quoted 4 lines

I tried many different changes in BIOS and all failed (the machine is damn slow at boot, this takes age).

I am stuck :(

Could you please try the patch below?

Quoted 19 lines · Ingo Molnar

Quoted 17 lines · Eric Dumazet

Quoted 15 lines · Andi Kleen

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),
but I think Linux should do better than just disabling PMU use when this happens.

However I suspect taking over SCI would cause endless problems and is very likely not a good idea.

Quoted 6 lines

Quoted 4 lines

I tried many different changes in BIOS and all failed (the machine is damn slow at boot, this takes age).

I am stuck :(

Could you please try the patch below?

This obviously works, [...]

[...] but you probably need to make a full pass to make sure we dont have a MSR failure -this should return false in this case.

Quoted 2 lines

[...] but you probably need to make a full pass to make sure we dont have a MSR failure -this should return false in this case.

Wanted to keep this patch simple - we are not really hitting MSR failure cases in practice, and by getting the message the user is at least warned that *something* is amiss.

Wanted to keep this patch simple - we are not really hitting MSR failure cases in practice,

Quoted 3 lines · Ingo Molnar

Wanted to keep this patch simple - we are not really hitting MSR failure cases in practice,

Everything qemu/kvm hits those.

Quoted 14 lines · Eric Dumazet

Quoted 13 lines · Andi Kleen

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),
but I think Linux should do better than just disabling PMU use when this happens.

Quoted 7 lines

Quoted 2 lines

However I suspect taking over SCI would cause endless problems and is very likely not a good idea.

I tried many different changes in BIOS and all failed (the machine is damn slow at boot, this takes age).

I am stuck :(

Could you please try the patch below?

Thanks,

Ingo

------------------->
From 14df27334ac47a5cec67fb2238d14499346acc38 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <[email protected]>
Date: Fri, 25 Mar 2011 10:24:23 +0100
Subject: [PATCH] perf, x86: Complain louder about BIOSen corrupting CPU/PMU state and continue

Eric Dumazet reported that hardware PMU events do not work on his system, due to the BIOS corrupting PMU state:

Performance Events: PEBS fmt0+, Core2 events, Broken BIOS detected, using software events only. [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 186 is 43003c)

Linus suggested that we continue in the face of such BIOS-induced CPU state corruption:

   http://lkml.org/lkml/2011/3/24/608

Such BIOSes will have to be fixed - developers rely on a working and fully capable PMU and BIOS interfering with CPU state is simply not acceptable.

So this patch changes perf to continue when it detects such BIOS interaction, some hardware events may be unreliable due to the BIOS writing and re-writing them - there's not much the kernel can do about that.

Reported-by Eric Dumazet <[email protected]>

Suggested-by Linus Torvalds <[email protected]>

Cc Peter Zijlstra <[email protected]>

Cc Arnaldo Carvalho de Melo <[email protected]>

Cc Frederic Weisbecker <[email protected]>

Cc Mike Galbraith <[email protected]>

Cc Steven Rostedt <[email protected]>

LKML-Reference: <new-submission>

Signed-off-by Ingo Molnar <[email protected]>

---
 arch/x86/kernel/cpu/perf_event.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

arch/x86/kernel/cpu/perf_event.c +0 -0

index ec46eea..eb00677 100644
@@ -500,12 +500,17 @@ static bool check_hw_exists(void)
 	return true;
 

arch/x86/kernel/cpu/perf_event.c +1 -1

 msr_fail:
 	printk(KERN_CONT "Broken PMU hardware detected, using software events only.\n");
+
 	return false;
 }
 
--

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/

Could you please try the patch below?

Quoted 1 line

Could you please try the patch below?

This is very likely not a good idea. I heard some systems can hang if you overwrite their PMUs this way and you're unlucky enough :-(

I tried many different changes in BIOS and all failed (the machine is damn slow at boot, this takes age).

BIOS : Version: I27 Release Date: 09/30/2010

[    0.021849] CPU0: Intel(R) Xeon(R) CPU           E5640  @ 2.67GHz stepping 02
[    0.123904] Performance Events: PEBS fmt1+, Westmere events, Broken BIOS detected, using software events only.
[    0.124125] [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 38d is 330) 

Quoted 15 lines

On the BL460c G6/G7, one could perform the following steps:

To configure BIOS low-latency options using RBSU
1. Press F9 during POST to enter RBSU.
2. Press CTRL-A to open the menu.
3. Select Service Options.
4. Disable either or both of the following options:
o Processor Power and Utilization Monitoring
o Memory Pre-Failure Notification

By doing the above the BIOS should release the performance counter, and the kernel's "perf" should not encounter a conflict.

The document that you are looking for is here: (please see page 8 and later):
http://h20000.www2.hp.com/bc/docs/support/SupportManual/c01804533/c01804533.pdf

Quoted 6 lines · Eric Dumazet

BIOS : Version: I27 Release Date: 09/30/2010

[    0.021849] CPU0: Intel(R) Xeon(R) CPU           E5640  @ 2.67GHz stepping 02
[    0.123904] Performance Events: PEBS fmt1+, Westmere events, Broken BIOS detected, using software events only.
[    0.124125] [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 38d is 330) 

Do the below instructions yield a working system?:

Quoted 15 lines

Quoted 15 lines

On the BL460c G6/G7, one could perform the following steps:

To configure BIOS low-latency options using RBSU
1. Press F9 during POST to enter RBSU.
2. Press CTRL-A to open the menu.
3. Select Service Options.
4. Disable either or both of the following options:
o Processor Power and Utilization Monitoring
o Memory Pre-Failure Notification

By doing the above the BIOS should release the performance counter, and the kernel's "perf" should not encounter a conflict.

The document that you are looking for is here: (please see page 8 and later):
http://h20000.www2.hp.com/bc/docs/support/SupportManual/c01804533/c01804533.pdf

Quoted 28 lines · Peter Zijlstra

Quoted 6 lines · Eric Dumazet

BIOS : Version: I27 Release Date: 09/30/2010

[    0.021849] CPU0: Intel(R) Xeon(R) CPU           E5640  @ 2.67GHz stepping 02
[    0.123904] Performance Events: PEBS fmt1+, Westmere events, Broken BIOS detected, using software events only.
[    0.124125] [Firmware Bug]: the BIOS has corrupted hw-PMU resources (MSR 38d is 330) 

Do the below instructions yield a working system?:

Quoted 15 lines

Quoted 15 lines

On the BL460c G6/G7, one could perform the following steps:

To configure BIOS low-latency options using RBSU
1. Press F9 during POST to enter RBSU.
2. Press CTRL-A to open the menu.
3. Select Service Options.
4. Disable either or both of the following options:
o Processor Power and Utilization Monitoring
o Memory Pre-Failure Notification

By doing the above the BIOS should release the performance counter, and the kernel's "perf" should not encounter a conflict.

The document that you are looking for is here: (please see page 8 and later):
http://h20000.www2.hp.com/bc/docs/support/SupportManual/c01804533/c01804533.pdf

Thanks Peter !

I was not aware of the hidden "CTRL-A" feature on RBSU, so I never met these configuration settings ("Service Options" added in main menu)

I already was in "Power regulator mode : Static High mode" QPI Link power management : Disabled

I have latest released I24 BIOS
DMI: HP ProLiant BL460c G6, BIOS I24 01/29/2011

1) I disabled "Power and Utilization Monitoring" _and_ "Memory Pre-Failure Notification"

Still I got the 'MSR 38d is 330' error

2) I then disabled Intel Turbo boost

Same error

3) Minimum Processor idle Power state
From C1E-State --> No C-states

Same error

Tricky isnt it ?

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

By doing the above the BIOS should release the performance counter, and the kernel's "perf" should not encounter a conflict.

The document that you are looking for is here: (please see page 8 and later):
http://h20000.www2.hp.com/bc/docs/support/SupportManual/c01804533/c01804533.pdf

Thanks Peter !

I was not aware of the hidden "CTRL-A" feature on RBSU, so I never met these configuration settings ("Service Options" added in main menu)

I already was in "Power regulator mode : Static High mode" QPI Link power management : Disabled

I have latest released I24 BIOS
DMI: HP ProLiant BL460c G6, BIOS I24 01/29/2011

1) I disabled "Power and Utilization Monitoring" _and_ "Memory Pre-Failure Notification"

Still I got the 'MSR 38d is 330' error

2) I then disabled Intel Turbo boost

Same error

3) Minimum Processor idle Power state
From C1E-State --> No C-states

Same error

Tricky isnt it ?

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),

Quoted 10 lines · Andi Kleen

Quoted 7 lines

never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),

There's really just two sane options:

- complain about the BIOS corrupting CPU state and refusing to use the PMU
- complain about the BIOS corrupting CPU state and using the PMU against the BIOS

We went for the first one but i'll be more than glad to implement Linus's much more aggressive second option.

Btw., for the record, the thing you have been advocating in the past was a third option: for the kernel to step aside quietly and to let the BIOS corrupt a counter or two. You even sent us some sort of BIOS specification about how to implement that. That's pretty much the worst solution imaginable.

Quoted 10 lines · Ingo Molnar

Quoted 2 lines · Andi Kleen

For the record I don't think it's a good idea for the BIOS to do this (and I'm not aware of any engineer who does),

There's really just two sane options:

- complain about the BIOS corrupting CPU state and refusing to use the PMU
- complain about the BIOS corrupting CPU state and using the PMU against the BIOS

Quoted 7 lines

We went for the first one but i'll be more than glad to implement Linus's much more aggressive second option.

Btw., for the record, the thing you have been advocating in the past was a third option: for the kernel to step aside quietly and to let the BIOS corrupt a counter or two. You even sent us some sort of BIOS specification about how to implement that. That's pretty much the worst solution imaginable.

Also seriously complicated by the kexec case where the previous kernel didn't clean up PMU state. There is simply no sane way to detect if its actually used and by whoem.

The whole PMU 'sharing' concept championed by Andi is utter crap.

As for simply using it despite the BIOS corrupting it, that might not always work, the BIOS might simply over-write your state because it one-sidedly declares to own the MSRs (observed behaviour).

Its all a big clusterfuck and really the best way (IMO) is what we have now to put pressure on and force the BIOS vendors to play nice.

I assume both HP and DELL will be seriously unhappy with the kernel spewing FIRMWARE BUG messages on boot on their boxen, the question is, will they be unhappy enough to fix it..

Now Ingo's patch keeps the warning and lets you take the PMU back and live with whatever consequences that brings (incorrect counts etc), that might also work but puts less pressure on the vendors because things appear to work.

Quoted 6 lines

Its all a big clusterfuck and really the best way (IMO) is what we have now to put pressure on and force the BIOS vendors to play nice.

I assume both HP and DELL will be seriously unhappy with the kernel spewing FIRMWARE BUG messages on boot on their boxen, the question is, will they be unhappy enough to fix it..

So, we better stick then with option 1. My experience is that new system's bioses try not to claim perfctrs (affected systems I have seen are about 2-3 years old), but I am not really sure here.

Also seriously complicated by the kexec case where the previous kernel didn't clean up PMU state. There is simply no sane way to detect if its

actually used and by whoem.

The whole PMU 'sharing' concept championed by Andi is utter crap.

As for simply using it despite the BIOS corrupting it, that might not always work, the BIOS might simply over-write your state because it one-sidedly declares to own the MSRs (observed behaviour).

Its all a big clusterfuck and really the best way (IMO) is what we have now to put pressure on and force the BIOS vendors to play nice.

Quoted 2 lines

Also seriously complicated by the kexec case where the previous kernel didn't clean up PMU state. There is simply no sane way to detect if its

That's a good point, but we can easily stop the PMU before kexec.

Quoted 1 line

actually used and by whoem.

You check if the counter is enabled. If it's already enabled it's used by someone else.

Quoted 1 line

The whole PMU 'sharing' concept championed by Andi is utter crap.

Why? It's the same thing as having some less counters.

[...] You need to already support that for architectural perfmon with variable counters anyways or for sharing with oprofile.

Quoted 3 lines

As for simply using it despite the BIOS corrupting it, that might not always work, the BIOS might simply over-write your state because it one-sidedly declares to own the MSRs (observed behaviour).

Yes, that doesn't work. If someone else is active you have to step back.

Quoted 2 lines

Its all a big clusterfuck and really the best way (IMO) is what we have now to put pressure on and force the BIOS vendors to play nice.

It just results in users like Eric being screwed. I doubt that any BIOS writer ever heard about it. Congratulations for a great plan.

,, and I'd like to point out that we should just say "screw the f*cking BIOS, it's doing things wrong". And then just take over the PMU events, and make sure that they aren't routed to SCI. Instead of the current "ok, roll over and die when the BIOS does something idiotic".

People continuously claim that the BIOS really needs it, and I have never EVER seen any good explanation of why that particular sh*t argument would b true. It seems to be purely about politics, where some idiotic vendor (namely HP) has convinced Intel that they really need it. To the point where some engineers seem to have bought into the whole thing and actually believe that fairy tale ("firmware can do better" - hah! They must be feeding people some bad drugs at the cafeteria)

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if it finds the bit set already.

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty scales proportionately. So it is best to try and avoid atomic instructions wherever possible. test_and_set_bit_lock() can avoid using LOCK_PREFIX if it finds the bit set already.

Signed-off-by Nikanth Karthikesan <[email protected]>

---

arch/x86/include/asm/bitops.h +19 -13

index 903683b..26a42ff 100644
@@ -203,19 +203,6 @@ static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 }
 
 /**
- * test_and_set_bit_lock - Set a bit and return its old value for lock
- * @nr: Bit to set
- * @addr: Address to count from
- *
- * This is the same as test_and_set_bit on x86.
- */
-static __always_inline int
-test_and_set_bit_lock(int nr, volatile unsigned long *addr)
-{
-	return test_and_set_bit(nr, addr);
-}
-
-/**
  * __test_and_set_bit - Set a bit and return its old value
  * @nr: Bit to set
  * @addr: Address to count from

@@ -339,6 +326,25 @@ static int test_bit(int nr, const volatile unsigned long *addr);
 	 : variable_test_bit((nr), (addr)))
 
 /**
+ * test_and_set_bit_lock - Set a bit and return its old value for lock
+ * @nr: Bit to set
+ * @addr: Address to count from
+ *
+ * This is the same as test_and_set_bit on x86. But atomic operation is
+ * avoided, if the bit was already set.
+ */
+static __always_inline int
+test_and_set_bit_lock(int nr, volatile unsigned long *addr)
+{
+#ifdef CONFIG_SMP
+	barrier();
+	if (test_bit(nr, addr))
+		return 1;
+#endif
+	return test_and_set_bit(nr, addr);
+}
+
+/**
  * __ffs - find first set bit in word
  * @word: The word to search
  *

On x86_64 SMP with lots of CPU atomic instructions which assert the LOCK # signal can stall other CPUs. And as the number of cores increase this penalty