Merge lp:~laurynas-biveinis/percona-server/bug1236696 into lp:percona-server/5.6

[Laurynas Biveinis]

3rd MP:
- replaced waiters counts reset to 0 by the unlocking thread with decrements by the woken up waiters themselves.
- fixed bug 1240044.

http://jenkins.percona.com/job/percona-server-5.6-param/377/

2nd MP:

fixed (spurious) compiler warnings
http://jenkins.percona.com/job/percona-server-5.6-param/369/

1st MP:

http://jenkins.percona.com/job/percona-server-5.6-param/368/

Fix bug 1236696.

The issue is a number of race conditions between successful priority
mutex or RW lock locks that happen after having set the waiters flag
(the last locking attempts before waiting), and unlocks.

If a lock (both priority mutex, or a priority RW lock) is successfully
locked this way, the corresponding waiters flag remains set, although
other waiters for this flag do not necessarily exist. Then on unlock,
the flag being set causes the corresponding event to be set (which may
have no waiters), and prevent lower-priority waiters from waking up,
leaving the lock unlocked but its waiters sleeping.

Fix by making ib_prio_mutex_t::high_priority_waiters,
prio_rw_lock_t::high_priority_x_waiters, and
prio_rw_lock_t::high_priority_s_waiters to be actual waiting thread
count instead of 0/1 flag. The counts are atomically incremented by
1 where the flag used to be set to 1, and atomically decremented if
locking after setting the waiters flag has succeeded, instead of doing
nothing. Also fix prio_rw_lock_t::high_priority_wait_ex_waiter flag
handling. Since there can be at most one wait_ex waiter, there is no
need to use atomic inc and dec operations, and it is enough to simply
reset the flag on successful lock. Assert that a high priority
wait_ex waiter does not exist before an X unlock.

Also adjust rw_lock_get_waiters() to use || instead of | for checking
for waiters presence as this function should return only 0 or 1.

[Laurynas Biveinis (laurynas-biveinis)]

[ 78%] Building CXX object storage/innobase/CMakeFiles/innobase.dir/trx/trx0sys.cc.o
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc: In function ‘void rw_lock_s_lock_spin(void*, ulint, bool, bool, const char*, ulint)’:
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc:492: warning: ‘prio_rw_lock’ may be used uninitialized in this function
[ 78%] [ 78%] Building CXX object storage/innobase/CMakeFiles/innobase.dir/trx/trx0trx.cc.o
Building CXX object storage/perfschema/unittest/CMakeFiles/pfs-t.dir/pfs-t.cc.o
[ 78%] Building CXX object storage/perfschema/unittest/CMakeFiles/pfs_account-oom-t.dir/pfs_account-oom-t.cc.o
[ 78%] Building CXX object storage/innobase/CMakeFiles/innobase.dir/trx/trx0undo.cc.o
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc: In function ‘void rw_lock_x_lock_func(prio_rw_lock_t*, ulint, const char*, ulint)’:
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc:756: warning: ‘prio_lock’ may be used uninitialized in this function
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc:756: note: ‘prio_lock’ was declared here
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0sync.cc: In function ‘void mutex_spin_wait(void*, bool, const char*, ulint)’:
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0sync.cc:554: warning: ‘prio_mutex’ may be used uninitialized in this function
[ 78%] Building CXX object storage/innobase/CMakeFiles/innobase.dir/usr/usr0sess.cc.o
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc: In function ‘void rw_lock_x_lock_func(rw_lock_t*, ulint, const char*, ulint, bool, bool)’:
/data/bench/alexey.s/repo/trunk-free-backoff.clean/Percona-Server/storage/innobase/sync/sync0rw.cc:756: warning: ‘prio_lock’ may be used uninitialized in this function

review: Needs Fixing

[Vlad Lesin (vlad-lesin)]

review: Approve (g2)

[Laurynas Biveinis (laurynas-biveinis)]

Thanks for holding this MP, Alexey.

I kept on thinking about Vlad's question on IRC, and realized that waiters sets/resets are indeed unbalanced: if one thread has bumped the waiters flag, and then an unlock happens, and then the first thread wins the lock word race, the resulting waiters value will be one less than it should be. This should be fixable by replacing the waiters count reset in the unlockers with a waiters count decrement upon wakeup.

review: Needs Fixing

[Laurynas Biveinis (laurynas-biveinis)]

While fixing, discovered bug 1240044, will fix it too.

review: Needs Fixing

[Alexey Kopytov (akopytov)]

review: Approve

=== modified file 'Percona-Server/storage/innobase/include/sync0rw.h'
--- Percona-Server/storage/innobase/include/sync0rw.h	2013-09-26 14:58:37 +0000
+++ Percona-Server/storage/innobase/include/sync0rw.h	2013-10-15 14:32:10 +0000
@@ -780,13 +780,13 @@
 						provides the lock word etc. for
 						the priority rw lock  */
 	volatile ulint		high_priority_s_waiters;
-						/* If 1, high priority S
-						waiters exist */
+						/* Number of high priority S
+						waiters */
 	os_event_t		high_priority_s_event; /* High priority wait
 						array event for S waiters */
 	volatile ulint		high_priority_x_waiters;
-						/* If 1, high priority X
-						waiters exist */
+						/* Number of high priority X
+						waiters */
 	os_event_t		high_priority_x_event;
 						/* High priority wait arraay
 						event for X waiters */

=== modified file 'Percona-Server/storage/innobase/include/sync0rw.ic'
--- Percona-Server/storage/innobase/include/sync0rw.ic	2013-10-04 14:50:45 +0000
+++ Percona-Server/storage/innobase/include/sync0rw.ic	2013-10-15 14:32:10 +0000
@@ -94,8 +94,8 @@
 	const prio_rw_lock_t*	lock)	/*!< in: rw-lock */
 {
 	return rw_lock_get_waiters(&lock->base_lock)
-		| lock->high_priority_s_waiters
-		| lock->high_priority_x_waiters;
+		|| lock->high_priority_s_waiters
+		|| lock->high_priority_x_waiters;
 }
 
 /********************************************************************//**
@@ -619,32 +619,14 @@
 	if (lock_word == 0) {
 
 		/* A waiting next-writer exists, either high priority or
-		regular.  Wake up the first waiter in this order: 1) high
-		priority next-writer; 2) high priority X waiters; 3) high
-		priority S waiters; 4) regular priority next-waiter.  This
-		allows high priority requests to overtake an already-waiting
-		regular priority next-waiter.  */
+		regular, sharing the same wait event.  */
 		if (lock->high_priority_wait_ex_waiter) {
 
 			lock->high_priority_wait_ex_waiter = 0;
-			/* Note that we do not have a separate high priority
-			next-waiter event.  There can be only one such waiter,
-			here we already know it's high priority, no
-			regular-priority wakeup may happen.  */
-			os_event_set(lock->base_lock.wait_ex_event);
-		} else if (lock->high_priority_x_waiters) {
-
-			lock->high_priority_x_waiters = 0;
-			os_event_set(lock->high_priority_x_event);
-		} else if (lock->high_priority_s_waiters) {
-
-			lock->high_priority_s_waiters = 0;
-			os_event_set(lock->high_priority_s_event);
-		} else {
-
-			os_event_set(lock->base_lock.wait_ex_event);
 		}
+		os_event_set(lock->base_lock.wait_ex_event);
 		sync_array_object_signalled();
+
 	} else if (lock_word == X_LOCK_DECR) {
 
 		/* S-waiters may exist during an S unlock if a high-priority
@@ -765,6 +747,8 @@
 #endif
 						     &lock->base_lock);
 
+	ut_ad(lock->high_priority_wait_ex_waiter == 0);
+
 	if (rw_lock_lock_word_incr(&lock->base_lock, x_lock_incr)
 	    == X_LOCK_DECR) {
 
@@ -776,12 +760,10 @@
 
 		if (lock->high_priority_x_waiters) {
 
-			lock->high_priority_x_waiters = 0;
 			os_event_set(lock->high_priority_x_event);
 			sync_array_object_signalled();
 		} else if (lock->high_priority_s_waiters) {
 
-			lock->high_priority_s_waiters = 0;
 			os_event_set(lock->high_priority_s_event);
 			sync_array_object_signalled();
 		} else if (lock->base_lock.waiters) {

=== modified file 'Percona-Server/storage/innobase/include/sync0sync.h'
--- Percona-Server/storage/innobase/include/sync0sync.h	2013-09-26 14:58:37 +0000
+++ Percona-Server/storage/innobase/include/sync0sync.h	2013-10-15 14:32:10 +0000
@@ -977,12 +977,11 @@
 					word etc. for the priority mutex  */
 	os_event_t	high_priority_event; /* High priority wait array
 					event */
-	volatile ulint	high_priority_waiters; /* Set to 1 if there are (or
-					may be) threads that asked for this
-					mutex to be acquired with high priority
-					in the global wait array for this mutex
-					to be released.  Otherwise, this is
-					0.  */
+	volatile ulint	high_priority_waiters; /* Number of threads that asked
+					for this mutex to be acquired with high
+					priority in the global wait array
+					waiting for this mutex to be
+					released. */
 };
 
 /** Constant determining how long spin wait is continued before suspending

=== modified file 'Percona-Server/storage/innobase/include/sync0sync.ic'
--- Percona-Server/storage/innobase/include/sync0sync.ic	2013-09-23 13:53:38 +0000
+++ Percona-Server/storage/innobase/include/sync0sync.ic	2013-10-15 14:32:10 +0000
@@ -227,7 +227,6 @@
 	/* Wake up any high priority waiters first.  */
 	if (mutex->high_priority_waiters != 0) {
 
-		mutex->high_priority_waiters = 0;
 		os_event_set(mutex->high_priority_event);
 		sync_array_object_signalled();
 

=== modified file 'Percona-Server/storage/innobase/sync/sync0arr.cc'
--- Percona-Server/storage/innobase/sync/sync0arr.cc	2013-10-04 04:54:33 +0000
+++ Percona-Server/storage/innobase/sync/sync0arr.cc	2013-10-15 14:32:10 +0000
@@ -490,7 +490,7 @@
 		if (type == SYNC_PRIO_MUTEX) {
 
 			fprintf(file,
-				"high-priority waiters flag %lu\n",
+				"high-priority waiters count %lu\n",
 				(ulong) prio_mutex->high_priority_waiters);
 		}
 
@@ -547,8 +547,8 @@
 			rwlock->last_x_file_name,
 			(ulong) rwlock->last_x_line);
 		if (prio_rwlock) {
-			fprintf(file, "high priority S waiters flag %lu, "
-				"high priority X waiters flag %lu, "
+			fprintf(file, "high priority S waiters count %lu, "
+				"high priority X waiters count %lu, "
 				"wait-exclusive waiter is "
 				"high priority if exists: %lu\n",
 				prio_rwlock->high_priority_s_waiters,

=== modified file 'Percona-Server/storage/innobase/sync/sync0rw.cc'
--- Percona-Server/storage/innobase/sync/sync0rw.cc	2013-10-04 14:50:45 +0000
+++ Percona-Server/storage/innobase/sync/sync0rw.cc	2013-10-15 14:32:10 +0000
@@ -408,8 +408,6 @@
 /*=============*/
 	prio_rw_lock_t*	lock)	/*!< in: rw-lock */
 {
-	ut_ad(lock->high_priority_s_waiters < 2);
-	ut_ad(lock->high_priority_x_waiters < 2);
 	return(rw_lock_validate(&lock->base_lock));
 }
 
@@ -491,6 +489,8 @@
 		return; /* Success */
 	} else {
 
+		prio_rw_lock_t*	prio_rw_lock = NULL;
+
 		if (i > 0 && i < SYNC_SPIN_ROUNDS) {
 			goto lock_loop;
 		}
@@ -507,10 +507,14 @@
 		/* Set waiters before checking lock_word to ensure wake-up
 		signal is sent. This may lead to some unnecessary signals. */
 		if (high_priority) {
-			prio_rw_lock_t*	prio_rw_lock
-				= (prio_rw_lock_t *) _lock;
-			prio_rw_lock->high_priority_s_waiters = 1;
+
+			prio_rw_lock = reinterpret_cast<prio_rw_lock_t *>
+				(_lock);
+			os_atomic_increment_ulint(
+				&prio_rw_lock->high_priority_s_waiters,
+				1);
 		} else {
+
 			rw_lock_set_waiter_flag(lock);
 		}
 
@@ -519,6 +523,12 @@
 		    && (TRUE == rw_lock_s_lock_low(lock, pass,
 						   file_name, line))) {
 			sync_array_free_cell(sync_arr, index);
+			if (prio_rw_lock) {
+
+				os_atomic_decrement_ulint(
+					&prio_rw_lock->high_priority_s_waiters,
+					1);
+			}
 			return; /* Success */
 		}
 
@@ -536,6 +546,13 @@
 
 		sync_array_wait_event(sync_arr, index);
 
+		if (prio_rw_lock) {
+
+			os_atomic_decrement_ulint(
+				&prio_rw_lock->high_priority_s_waiters,
+				1);
+		}
+
 		i = 0;
 		goto lock_loop;
 	}
@@ -584,6 +601,7 @@
 	ulint		i = 0;
 	sync_array_t*	sync_arr;
 	size_t		counter_index;
+	prio_rw_lock_t*	prio_rw_lock = NULL;
 
 	/* We reuse the thread id to index into the counter, cache
 	it here for efficiency. */
@@ -612,7 +630,7 @@
 
 		if (high_priority) {
 
-			prio_rw_lock_t*	prio_rw_lock
+			prio_rw_lock
 				= reinterpret_cast<prio_rw_lock_t *>(lock);
 			prio_rw_lock->high_priority_wait_ex_waiter = 1;
 		}
@@ -643,6 +661,10 @@
 			We must pass the while-loop check to proceed.*/
 		} else {
 			sync_array_free_cell(sync_arr, index);
+			if (prio_rw_lock) {
+
+				prio_rw_lock->high_priority_wait_ex_waiter = 0;
+			}
 		}
 	}
 	rw_lock_stats.rw_x_spin_round_count.add(counter_index, i);
@@ -739,6 +761,7 @@
 	sync_array_t*	sync_arr;
 	ibool		spinning = FALSE;
 	size_t		counter_index;
+	prio_rw_lock_t*	prio_lock = NULL;
 
 	/* We reuse the thread id to index into the counter, cache
 	it here for efficiency. */
@@ -820,14 +843,22 @@
 	/* Waiters must be set before checking lock_word, to ensure signal
 	is sent. This could lead to a few unnecessary wake-up signals. */
 	if (high_priority) {
-		prio_rw_lock_t*	prio_lock = (prio_rw_lock_t *)lock;
-		prio_lock->high_priority_x_waiters = 1;
+
+		prio_lock = reinterpret_cast<prio_rw_lock_t *>(lock);
+		os_atomic_increment_ulint(&prio_lock->high_priority_x_waiters,
+					  1);
 	} else {
 		rw_lock_set_waiter_flag(lock);
 	}
 
 	if (rw_lock_x_lock_low(lock, high_priority, pass, file_name, line)) {
 		sync_array_free_cell(sync_arr, index);
+		if (prio_lock) {
+
+			os_atomic_decrement_ulint(
+				&prio_lock->high_priority_x_waiters,
+				1);
+		}
 		return; /* Locking succeeded */
 	}
 
@@ -845,6 +876,12 @@
 
 	sync_array_wait_event(sync_arr, index);
 
+	if (prio_lock) {
+
+		os_atomic_decrement_ulint(&prio_lock->high_priority_x_waiters,
+					  1);
+	}
+
 	i = 0;
 	goto lock_loop;
 }

=== modified file 'Percona-Server/storage/innobase/sync/sync0sync.cc'
--- Percona-Server/storage/innobase/sync/sync0sync.cc	2013-09-23 13:53:38 +0000
+++ Percona-Server/storage/innobase/sync/sync0sync.cc	2013-10-15 14:32:10 +0000
@@ -550,7 +550,8 @@
 	/* The typecast below is performed for some of the priority mutexes
 	too, when !high_priority.  This exploits the fact that regular mutex is
 	a prefix of the priority mutex in memory.  */
-	ib_mutex_t*	mutex = (ib_mutex_t *) _mutex;
+	ib_mutex_t*		mutex = (ib_mutex_t *) _mutex;
+	ib_prio_mutex_t*	prio_mutex = NULL;
 
 	counter_index = (size_t) os_thread_get_curr_id();
 
@@ -624,8 +625,12 @@
 	then the event is set to the signaled state. */
 
 	if (high_priority) {
-		((ib_prio_mutex_t *)_mutex)->high_priority_waiters = 1;
+
+		prio_mutex = reinterpret_cast<ib_prio_mutex_t *>(_mutex);
+		os_atomic_increment_ulint(&prio_mutex->high_priority_waiters,
+					  1);
 	} else {
+
 		mutex_set_waiters(mutex, 1);
 	}
 
@@ -641,6 +646,11 @@
 			mutex_set_debug_info(mutex, file_name, line);
 #endif
 
+			if (prio_mutex) {
+				os_atomic_decrement_ulint(
+					&prio_mutex->high_priority_waiters,
+					1);
+			}
 			return;
 
 			/* Note that in this case we leave the waiters field
@@ -659,6 +669,12 @@
 
 	sync_array_wait_event(sync_arr, index);
 
+	if (prio_mutex) {
+
+		os_atomic_decrement_ulint(&prio_mutex->high_priority_waiters,
+					  1);
+	}
+
 	goto mutex_loop;
 }