Patchwork [4.7] libitm: Fix lost wake-up in serial lock.

login
register
mail settings
Submitter Torvald Riegel
Date March 10, 2012, 5:43 p.m.
Message ID <1331401409.2986.8974.camel@triegel.csb>
Download mbox | patch
Permalink /patch/145878/
State New
Headers show

Comments

Torvald Riegel - March 10, 2012, 5:43 p.m.
This patch fixes PR52526, a lost wake-up in libitm (ie, one ore more
threads could hang and not get woken up anymore).

The problem was missing handling of one corner case in the futex-based
serial lock implementation (config/linux/rwlock.cc, read_lock()):
Multiple readers would set READERS to 1 and only call
futex_wait(&readers, 1) if there were any writers.  Writers would set
READERS to 0 and then call futex_wake(&readers).  That's fine, but
because there are multiple readers, it can happen that some would set
READERS to 1 after the writer's futex_wake() call, enabling the
futex_wait() in other readers (because READERS isn't 0 anymore).  This
patch fixes this by having readers wake up all potentially waiting
readers when they set READERS to 1 without an existing writer (thus
taking over what the writer would do).

OK for trunk?

OK for 4.7 too?  This is a showstopper if users hit it, so I'd prefer if
it could go into 4.7 as well.
commit 07d6d68b423797311bb04d8eb571f053d2078aa4
Author: Torvald Riegel <triegel@redhat.com>
Date:   Sat Mar 10 17:44:37 2012 +0100

    libitm: Fix lost wake-up in serial lock.
    
    	PR libitm/52526
    	* config/linux/rwlock.cc (GTM::gtm_rwlock::read_lock): Fix lost
    	wake-up.
Richard Henderson - March 13, 2012, 4:54 p.m.
On 03/10/12 09:43, Torvald Riegel wrote:
>     libitm: Fix lost wake-up in serial lock.
>     
>     	PR libitm/52526
>     	* config/linux/rwlock.cc (GTM::gtm_rwlock::read_lock): Fix lost
>     	wake-up.

Ok.

Jakub needs to approve this for 4.7.0; otherwise ok for 4.7.1.


r~
Jakub Jelinek - March 13, 2012, 5:34 p.m.
On Tue, Mar 13, 2012 at 09:54:36AM -0700, Richard Henderson wrote:
> On 03/10/12 09:43, Torvald Riegel wrote:
> >     libitm: Fix lost wake-up in serial lock.
> >     
> >     	PR libitm/52526
> >     	* config/linux/rwlock.cc (GTM::gtm_rwlock::read_lock): Fix lost
> >     	wake-up.
> 
> Jakub needs to approve this for 4.7.0; otherwise ok for 4.7.1.

Ok for 4.7.0.

	Jakub

Patch

diff --git a/libitm/config/linux/rwlock.cc b/libitm/config/linux/rwlock.cc
index ad1b042..cf1fdd5 100644
--- a/libitm/config/linux/rwlock.cc
+++ b/libitm/config/linux/rwlock.cc
@@ -74,6 +74,32 @@  gtm_rwlock::read_lock (gtm_thread *tx)
 	  atomic_thread_fence (memory_order_seq_cst);
 	  if (writers.load (memory_order_relaxed))
 	    futex_wait(&readers, 1);
+	  else
+	    {
+	      // There is no writer, actually.  However, we can have enabled
+	      // a futex_wait in other readers by previously setting readers
+	      // to 1, so we have to wake them up because there is no writer
+	      // that will do that.  We don't know whether the wake-up is
+	      // really necessary, but we can get lost wake-up situations
+	      // otherwise.
+	      // No additional barrier nor a nonrelaxed load is required due
+	      // to coherency constraints.  write_unlock() checks readers to
+	      // see if any wake-up is necessary, but it is not possible that
+	      // a reader's store prevents a required later writer wake-up;
+	      // If the waking reader's store (value 0) is in modification
+	      // order after the waiting readers store (value 1), then the
+	      // latter will have to read 0 in the futex due to coherency
+	      // constraints and the happens-before enforced by the futex
+	      // (paragraph 6.10 in the standard, 6.19.4 in the Batty et al
+	      // TR); second, the writer will be forced to read in
+	      // modification order too due to Dekker-style synchronization
+	      // with the waiting reader (see write_unlock()).
+	      // ??? Can we avoid the wake-up if readers is zero (like in
+	      // write_unlock())?  Anyway, this might happen too infrequently
+	      // to improve performance significantly.
+	      readers.store (0, memory_order_relaxed);
+	      futex_wake(&readers, INT_MAX);
+	    }
 	}
 
       // And we try again to acquire a read lock.