Re: bugs in test-lock

[Bruno Haible]

Torvald Riegel wrote:
> > The problem here is: it's a unit test. If it fails, I don't want to search
> > for bugs in the condition variable subsystem, semaphore subsystem, AND the
> > lock subsystem. I want the test to rely essentially only on the lock 
> > subsystem.
> 
> locks are the wrong mechanism.  locks give you mutual exclusion.  If you
> want a simple notification that you can query without blocking, use a
> semaphore.

OK, thanks for this clear advice. I've pushed the change below.

The timings look OK:
EXPLICIT_YIELD = 1 USE_SEMAPHORE = 1      1.79 sec
EXPLICIT_YIELD = 1 USE_SEMAPHORE = 0      1.78 sec
EXPLICIT_YIELD = 0 USE_SEMAPHORE = 1      3.3 .. 3.8 sec
EXPLICIT_YIELD = 0 USE_SEMAPHORE = 0      3.3 .. 3.9 sec

> > That's why I inserted the yield statements in the lock_checker_thread:
> > 
> >   for (;;)
> >     {
> >       pthread_mutex_lock (&lock);
> >       do something;
> >       pthread_mutex_unlock (&lock);
> >       do very little other things;
> >       pthread_yield ();
> >     }
> 
> Why do you think this works?

I think this works because
  - the manual page http://man7.org/linux/man-pages/man3/pthread_yield.3.html
    says "The thread is placed at the end of the run queue". To me, this means
    that if all participating threads call pthread_yield () once in a while,
    all threads have a chance to be woken up and get the lock within a 
reasonable
    amount of time.
  - the timings of EXPLICIT_YIELD = 1 vs. 0 (above) show that it has some
    effect.

> We don't have uniprocessors anymore,
> there's more that's affecting execution order than just the OS
> scheduler.
> ...
> There's no guarantee that this will give fairness to calls before or
> after it.  The OS scheduler may let other runnable processes run
> first, but modern synchronization primitives try not to involve the OS
> as much as possible because of the overheads this typically involves.

Are you suggesting that the pthread_yield manual page is wrong? Or that
some warning should be added to it? I'm sure Michael Kerrisk will accept inputs.

> > For a real notification mechanism, probably a pthread_cond_t would be the 
> > right
> > thing.
> 
> But you don't want to wait for a condition, you want to query whether a
> condition holds.  The latter is not supported by a condvar.

Oh. Really, "wait queue" would be a better name than "condition variable"
for this thing.

Bruno


2017-01-05  Bruno Haible  <address@hidden>

        lock tests: Prefer semaphore over mutex.
        * tests/test-lock.c (USE_SEMAPHORE): New constant.
        (struct atomic_int, init_atomic_int, get_atomic_int_value,
        set_atomic_int_value) [USE_SEMAPHORE]: Define using a POSIX semaphore.
        Suggested by Torvald Riegel <address@hidden>.

diff --git a/tests/test-lock.c b/tests/test-lock.c
index 095511e..f3da4cc 100644
--- a/tests/test-lock.c
+++ b/tests/test-lock.c
@@ -51,12 +51,20 @@
 #define EXPLICIT_YIELD 1
 
 /* Whether to use 'volatile' on some variables that communicate information
-   between threads.  If set to 0, a lock is used to protect these variables.
-   If set to 1, 'volatile' is used; this is theoretically equivalent but can
-   lead to much slower execution (e.g. 30x slower total run time on a 40-core
-   machine.  */
+   between threads.  If set to 0, a semaphore or a lock is used to protect
+   these variables.  If set to 1, 'volatile' is used; this is theoretically
+   equivalent but can lead to much slower execution (e.g. 30x slower total
+   run time on a 40-core machine), because 'volatile' does not imply any
+   synchronization/communication between different CPUs.  */
 #define USE_VOLATILE 0
 
+#if USE_POSIX_THREADS
+/* Whether to use a semaphore to communicate information between threads.
+   If set to 0, a lock is used. If set to 1, a semaphore is used.
+   Uncomment this to reduce the dependencies of this test.  */
+# define USE_SEMAPHORE 1
+#endif
+
 /* Whether to print debugging messages.  */
 #define ENABLE_DEBUGGING 0
 
@@ -97,6 +105,10 @@
 
 #include "glthread/thread.h"
 #include "glthread/yield.h"
+#if USE_SEMAPHORE
+# include <errno.h>
+# include <semaphore.h>
+#endif
 
 #if ENABLE_DEBUGGING
 # define dbgprintf printf
@@ -128,6 +140,41 @@ set_atomic_int_value (struct atomic_int *ai, int new_value)
 {
   ai->value = new_value;
 }
+#elif USE_SEMAPHORE
+/* This atomic_int implementation can only support the values 0 and 1.
+   It is initially 0 and can be set to 1 only once.  */
+struct atomic_int {
+  sem_t semaphore;
+};
+static void
+init_atomic_int (struct atomic_int *ai)
+{
+  sem_init (&ai->semaphore, 0, 0);
+}
+static int
+get_atomic_int_value (struct atomic_int *ai)
+{
+  if (sem_trywait (&ai->semaphore) == 0)
+    {
+      if (sem_post (&ai->semaphore))
+        abort ();
+      return 1;
+    }
+  else if (errno == EAGAIN)
+    return 0;
+  else
+    abort ();
+}
+static void
+set_atomic_int_value (struct atomic_int *ai, int new_value)
+{
+  if (new_value == 0)
+    /* It's already initialized with 0.  */
+    return;
+  /* To set the value 1: */
+  if (sem_post (&ai->semaphore))
+    abort ();
+}
 #else
 struct atomic_int {
   gl_lock_define (, lock)