[mythtv-users] Race condition

[Kyle Rose]

I'm bringing this thread back over from -dev because I've found the
problem and it isn't in myth.

class RingBuffer uses a pthread_rwlock to do reader/writer locking
(i.e., enforce the conditions r>0 => w==0 and w>0 => r==0), but at
least on AMD64 it appears to have a bug in which pthread_rwlock_unlock
doesn't always decrement the reader count when called.

I've prepared and attached a patch to replace the rwlock with a mutex
and condition variable, which thankfully does not appear to be broken
in my version of pthreads.  (I've run into problems with condition
variables in the past.)

So, if you see a symptom in which playback usually won't start when
you select live tv or a recording, then you may want to try this,
*especially* if you're using AMD64, and (perhaps) especially if you're
using SMP Opterons.  My glibc version is 2.3.2.ds1-17.0.0.1.amd64 from
the Debian AMD64 gcc-3.4 repository.

Cheers,
Kyle

-------------- next part --------------
Index: libs/libmythtv/RingBuffer.cpp
===================================================================
RCS file: /var/lib/mythcvs/mythtv/libs/libmythtv/RingBuffer.cpp,v
retrieving revision 1.105
diff -u -r1.105 RingBuffer.cpp
--- libs/libmythtv/RingBuffer.cpp	30 Sep 2004 10:22:56 -0000	1.105
+++ libs/libmythtv/RingBuffer.cpp	15 Oct 2004 00:47:51 -0000
@@ -486,14 +486,17 @@
     numfailures = 0;
     commserror = false;
 
-    pthread_rwlock_init(&rwlock, NULL);
+    pthread_mutex_init(&hammerlock, NULL);
+    pthread_cond_init(&hammercond, NULL);
+    readers = 0;
+    writers = 0;
 }
 
 RingBuffer::~RingBuffer(void)
 {
     KillReadAheadThread();
 
-    pthread_rwlock_wrlock(&rwlock);
+    write_lock_wait();
     if (remotefile)
     {
         delete remotefile;
@@ -521,7 +524,7 @@
 void RingBuffer::Reset(void)
 {
     wantseek = true;
-    pthread_rwlock_wrlock(&rwlock);
+    write_lock_wait();
     wantseek = false;
 
     if (!normalfile)
@@ -548,7 +551,7 @@
     numfailures = 0;
     commserror = false;
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
 }
 
 int RingBuffer::safe_read(int fd, void *data, unsigned sz)
@@ -613,12 +616,41 @@
     return ret;
 }
 
+void RingBuffer::read_lock_wait()
+{
+  pthread_mutex_lock(&hammerlock);
+  while (writers > 0) {
+    pthread_cond_wait(&hammercond, &hammerlock);
+  }
+  readers++;
+  pthread_mutex_unlock(&hammerlock);
+}
+
+void RingBuffer::write_lock_wait()
+{
+  pthread_mutex_lock(&hammerlock);
+  while (readers > 0 || writers > 0) {
+    pthread_cond_wait(&hammercond, &hammerlock);
+  }
+  writers = 1;
+  pthread_mutex_unlock(&hammerlock);
+}
+
+void RingBuffer::unlock()
+{
+  pthread_mutex_lock(&hammerlock);
+  if (readers > 0) readers--;
+  else writers = 0;
+  pthread_cond_signal(&hammercond);
+  pthread_mutex_unlock(&hammerlock);
+}
+
 #define READ_AHEAD_SIZE (10 * 256000)
 
 void RingBuffer::CalcReadAheadThresh(int estbitrate)
 {
     wantseek = true;
-    pthread_rwlock_wrlock(&rwlock);
+    write_lock_wait();
     wantseek = false;
 
     fill_threshold = 0;
@@ -650,7 +682,7 @@
     if (fill_min == 0)
         fill_min = -1;
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
 }
 
 int RingBuffer::ReadBufFree(void)
@@ -790,7 +822,7 @@
 
         readaheadpaused = false;
 
-        pthread_rwlock_rdlock(&rwlock);
+        read_lock_wait();
         if (totfree > readblocksize && !commserror)
         {
             // limit the read size
@@ -898,7 +930,7 @@
         }
         availWaitMutex.unlock();
 
-        pthread_rwlock_unlock(&rwlock);
+        unlock();
 
         if ((used >= fill_threshold || wantseek) && !pausereadthread)
             usleep(500);
@@ -1009,7 +1041,7 @@
 
 int RingBuffer::Read(void *buf, int count)
 {
-    pthread_rwlock_rdlock(&rwlock);
+    read_lock_wait();
 
     int ret = -1;
     if (normalfile)
@@ -1065,7 +1097,7 @@
         {
             if (stopreads)
             {
-                pthread_rwlock_unlock(&rwlock);
+                unlock();
                 return 0;
             }
 
@@ -1084,7 +1116,7 @@
                 if (stopreads)
                 {
                     availWaitMutex.unlock();
-                    pthread_rwlock_unlock(&rwlock);
+                    unlock();
                     return 0;
                 }
             }
@@ -1115,7 +1147,7 @@
         }
     }
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
     return ret;
 }
 
@@ -1123,11 +1155,11 @@
 {
     bool ret = false;
     int used, free;
-    pthread_rwlock_rdlock(&rwlock);
+    read_lock_wait();
 
     if (!tfw)
     {
-        pthread_rwlock_unlock(&rwlock);
+        unlock();
         return ret;
     }
 
@@ -1136,7 +1168,7 @@
 
     ret = (used * 5 > free);
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
     return ret;
 }
 
@@ -1144,11 +1176,11 @@
 {
     int ret = -1;
 
-    pthread_rwlock_rdlock(&rwlock);
+    read_lock_wait();
 
     if (!tfw)
     {
-        pthread_rwlock_unlock(&rwlock);
+        unlock();
         return ret;
     }
 
@@ -1197,7 +1229,7 @@
         availWaitMutex.unlock();
     }
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
     return ret;
 }
 
@@ -1214,7 +1246,7 @@
 long long RingBuffer::Seek(long long pos, int whence)
 {
     wantseek = true;
-    pthread_rwlock_wrlock(&rwlock);
+    write_lock_wait();
     wantseek = false;
 
     long long ret = -1;
@@ -1278,7 +1310,7 @@
     if (readaheadrunning)
         ResetReadAhead(readpos);
 
-    pthread_rwlock_unlock(&rwlock);
+    unlock();
 
     return ret;
 }
Index: libs/libmythtv/RingBuffer.h
===================================================================
RCS file: /var/lib/mythcvs/mythtv/libs/libmythtv/RingBuffer.h,v
retrieving revision 1.39
diff -u -r1.39 RingBuffer.h
--- libs/libmythtv/RingBuffer.h	6 Jul 2004 03:46:12 -0000	1.39
+++ libs/libmythtv/RingBuffer.h	15 Oct 2004 00:47:51 -0000
@@ -100,7 +100,13 @@
 
     bool stopreads;
 
-    pthread_rwlock_t rwlock;
+    pthread_mutex_t hammerlock;
+    pthread_cond_t hammercond;
+    int readers;
+    int writers;
+    void read_lock_wait();
+    void write_lock_wait();
+    void unlock();
 
     int recorder_num;
     RemoteEncoder *remoteencoder;