Details
Description
The following patch is suggested by Stewart Smith:
On POWER, you can set thread priority rather than Intel relax cpu instruction
So the correct way to write a spin loop for POWER is to lower the thread
priority while you spin and raise it up afterwards.
We add the asm volatile("":::"memory"); bit of asm to make GCC not optimize
the spin loop down to a nop. This is a GCC barrier rather than a CPU memory
barrier - and is arguably needed for x86 as well
Index: mysql-5.6.17/storage/innobase/sync/sync0rw.cc
===================================================================
--- mysql-5.6.17.orig/storage/innobase/sync/sync0rw.cc 2014-05-26 12:16:25.622071001 +1000
+++ mysql-5.6.17/storage/innobase/sync/sync0rw.cc 2014-05-26 17:37:14.702071000 +1000
@@ -382,6 +382,7 @@
/* Spin waiting for the writer field to become free */
os_rmb;
+ asm volatile ("or 1,1,1");
while (i < SYNC_SPIN_ROUNDS && lock->lock_word <= 0) {
if (srv_spin_wait_delay) {
ut_delay(ut_rnd_interval(0, srv_spin_wait_delay));
@@ -393,7 +394,9 @@
#else
i++;
#endif /* HAVE_MEMORY_BARRIER */
+ asm volatile ("":::"memory");
}
+ asm volatile ("or 2,2,2");
if (i >= SYNC_SPIN_ROUNDS) {
os_thread_yield();
Index: mysql-5.6.17/storage/innobase/sync/sync0sync.cc
===================================================================
--- mysql-5.6.17.orig/storage/innobase/sync/sync0sync.cc 2014-05-26 12:16:25.622071001 +1000
+++ mysql-5.6.17/storage/innobase/sync/sync0sync.cc 2014-05-26 17:37:52.152071000 +1000
@@ -501,6 +501,7 @@
spin_loop:
os_rmb;
+ asm volatile("or 1,1,1");
while (mutex_get_lock_word(mutex) != 0 && i < SYNC_SPIN_ROUNDS) {
if (srv_spin_wait_delay) {
ut_delay(ut_rnd_interval(0, srv_spin_wait_delay));
@@ -512,7 +513,9 @@
#else
i++;
#endif /* HAVE_MEMORY_BARRIER */
+ asm volatile("":::"memory");
}
+ asm volatile ("or 2,2,2;");
if (i >= SYNC_SPIN_ROUNDS) {
os_thread_yield();
Gliffy Diagrams
Attachments
Issue Links
- is part of
-
MDEV-6530 Examine and apply Power8 patches suggested by Stewart Smith
-
- Open
-
- links to
Activity
- All
- Comments
- Work Log
- History
- Activity
- Transitions
I assume it's ok to add the code to lower cpu priority as long as it's ifdef:ed and only used on power.
Still don't think we need the volatile("":::"memory") part.
Not that instead of using asm instructions directly, it's better to use proper macros:
(this is taken from the linux kernel)
#ifdef powerpc
/* Macros for adjusting thread priority (hardware multi-threading) */
#define HMT_very_low() asm volatile("or 31,31,31 # very low priority")
#define HMT_low() asm volatile("or 1,1,1 # low priority")
#define HMT_medium_low() asm volatile("or 6,6,6 # medium low priority")
#define HMT_medium() asm volatile("or 2,2,2 # medium priority")
#define HMT_medium_high() asm volatile("or 5,5,5 # medium high priority")
#define HMT_high() asm volatile("or 3,3,3 # high priority")
#else
#define HMTL_very_low()
...
#endif
We could add this to a new file include/cpu.h
Looks fine, just a few things for your consideration:
- year in copyrights should be 2014, not 2013
- powerpc is deprecated macro, we should use _powerpc_ instead, see:
http://nadeausoftware.com/articles/2012/02/c_c_tip_how_detect_processor_type_using_compiler_predefined_macros - InnoDB uses tabs for indentation (shouldn't we honor that?)
- why this change in rw_lock_s_lock_spin():
- if (i == SYNC_SPIN_ROUNDS) { + if (lock->lock_word <= 0)
revno: 3413.65.7 revision-id: monty@mariadb.org-20140819162835-sorv0ogd39f7mui8 parent: knielsen@knielsen-hq.org-20140813134639-wk760plnzg5wu4x8 committer: Michael Widenius <monty@mariadb.org> branch nick: maria-5.5 timestamp: Tue 2014-08-19 19:28:35 +0300 message: MDEV-6450 - MariaDB crash on Power8 when built with advance tool chain Part of this work is based on Stewart Smitch's memory barrier and lower priori patches for power8. - Added memory syncronization for innodb & xtradb for power8. - Added HAVE_WINDOWS_MM_FENCE to CMakeList.txt - Added os_isync to fix a syncronization problem on power - Added log_get_lsn_nowait which is now used srv_error_monitor_thread to ensur if log mutex is locked. All changes done both for InnoDB and Xtradb
I would argue that this patch is not needed.
Reason:
The loop in /storage/innobase/sync/sync0sync.cc will never be optimized away by gcc because of the code we have in mutex_get_lock_word(mutex), which is part of the loop:
This should make the original code safe.
The other loop also references a volatile argument, so this is also safe.
I suggest we don't take this patch (unless someone can show us an assembler output where the loop is optimized away).