Use PDR locks for glibc's internal locks (XCC)
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 9 Sep 2016 18:49:06 +0000 (14:49 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 16 Sep 2016 18:35:09 +0000 (14:35 -0400)
Our glibc port was using a simple spinlock for its LLL (low-level lock).
The problem with this is that the lock could be grabbed by vcore context
code, and in general is not safe from preemption.

The fix is to use PDR locks.  There are a couple nasty details.

One is that glibc mostly assumes a lock is an int.  We could hack up the
sysdeps completely (note that the INITIALIZER is compared to 0 directly),
but that's a mess.  Instead, we rely on the fact that spin PDR locks are 32
bits.

The other detail is that ld.so uses the locks, and it doesn't link with
parlib.  Even if it did, or if we moved the PDR locks to glibc directly,
it'd possibly be a mess, since ld grabs the locks before any of our parlib
constructors (I think).  The way it works now is that ld.so uses the
internal versions of the locks, and anything that links against parlib
(i.e. a binary that ld loads) should get the parlib version.  This might
not be working exactly as I think: see my notes in parlib-compat.c for
details.

A minor point to note is the removal of parlib/common.h from x86/atomic.h.
The common.h header pulls in way too much, and now that glibc's LLL needs
parlib/spinlock.h, we run into build issues.  So don't add things to
low-level parlib header files unnecessarily.

Rebuild the world.  AFAIK, even dynamically linked apps need to be rebuilt,
otherwise they may use the version of the locks that ld.so uses.  For
single-threaded apps, this is probably okay.

Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/Versions
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/lowlevellock.h
tools/compilers/gcc-glibc/glibc-2.19-akaros/sysdeps/akaros/parlib-compat.c
user/parlib/include/parlib/x86/atomic.h
user/parlib/spinlock.c

index b9c4d20..055c70f 100644 (file)
@@ -84,5 +84,8 @@ libc {
     _assert_failed;
     nsec2tsc;
     tsc2nsec;
+    spin_pdr_trylock;
+    spin_pdr_lock;
+    spin_pdr_unlock;
   }
 }
index abe4ada..159e20c 100644 (file)
@@ -3,15 +3,24 @@
 
 #include <atomic.h>
 #include <sys/param.h>
+#include <parlib/spinlock.h>
 
+/* Akaros doesn't support private vs shared locks.  That's probably a problem */
 #define LLL_PRIVATE 0
 #define LLL_SHARED 1
 
-#define LLL_LOCK_INITIALIZER (0)
+/* Glibc's locking is a nightmare, and in lieu of rewriting that mess (see
+ * libc-lock.h) to support something other than an integer for the LLL lock
+ * (and thus the libc lock, and the libc recursive lock, etc), we'll just use
+ * an int for storage and cast that to a struct spin_pdr_lock, which happens to
+ * also be 32 bits.  FFS. */
+#define LLL_LOCK_INITIALIZER SPINPDR_UNLOCKED
 
-#define lll_lock(l,p) do { } while(lll_trylock(l))
-#define lll_unlock(l,p) ({ (l) = 0; 0; })
-#define lll_trylock(l) __sync_lock_test_and_set(&(l), 1)
+#define lll_lock(l, p) spin_pdr_lock((struct spin_pdr_lock*)&(l))
+#define lll_unlock(l, p) spin_pdr_unlock((struct spin_pdr_lock*)&(l))
+/* lll_trylock returns 0 on success.  spin_pdr_trylock returns TRUE on
+ * success. */
+#define lll_trylock(l) !spin_pdr_trylock((struct spin_pdr_lock*)&l)
 
 #define lll_futex_wait(m,v,p) do { assert("NO FUTEX_WAIT FOR YOU!" == 0); } while(0)
 #define lll_futex_wake(m,n,p) do { assert("NO FUTEX_WAKE FOR YOU!" == 0); } while(0)
index 0afef27..be133fd 100644 (file)
@@ -8,6 +8,7 @@
 #include <ros/trapframe.h>
 #include <parlib/stdio.h>
 #include <parlib/assert.h>
+#include <parlib/spinlock.h>
 #include <stdbool.h>
 
 /* Here we define functions and variables that are really defined in parlib, but
  *
  * Unfortunately, this trick only works so long as we leave parlib as a static
  * library. If we ever decide to make parlib a .so, then we will have to revisit
- * this and use function pointers at runtime or something similar. */
+ * this and use function pointers at runtime or something similar.
+ *
+ * This also doesn't work for ld.so, which doesn't link against parlib.  That's
+ * probably a good thing (uthread constructors would be a mess for ld, I bet).
+ * But that does mean that these stubs need to actually do something for
+ * functions that ld.so calls.  See the notes below for more info. */
 
 __thread int __weak_vcoreid = 0;
 weak_alias(__weak_vcoreid, __vcoreid);
@@ -54,3 +60,41 @@ uint64_t __tsc2nsec(uint64_t tsc_time)
        assert(0);
 }
 weak_alias(__tsc2nsec, tsc2nsec)
+
+/* ld.so calls these, so we need them to work.  We don't need them to be
+ * thread-safe, since we're single-threaded, but we do need them to use the
+ * right values for 'locked' and 'unlocked'.
+ *
+ * Note that for this change I needed to recompile the binaries that link with
+ * libc - even if they link dynamically.  Otherwise, when they linked with
+ * libc.so, *libc itself* (not the actual program) would not find the parlib
+ * functions - it would still use these functions.  Lots of glibc functions
+ * (printf, fflush, etc) call some version of spin_pdr_lock (lll_lock).
+ *
+ * For an example, if you write(2, "foo\n", 4) on ever lock acquisition, you'll
+ * see one foo per process, which I think comes from ld.  Later functions that
+ * call spin_pdr_lock, whether from the app, parlib, or libc, do not output foo.
+ * This is not the case if the application was not rebuilt before this change
+ * (e.g. bash, ssh, etc). */
+bool __spin_pdr_trylock(struct spin_pdr_lock *pdr_lock)
+{
+       if (pdr_lock->lock != SPINPDR_UNLOCKED)
+               return FALSE;
+       pdr_lock->lock = 0;
+       return TRUE;
+}
+weak_alias(__spin_pdr_trylock, spin_pdr_trylock)
+
+void __spin_pdr_lock(struct spin_pdr_lock *pdr_lock)
+{
+       assert(pdr_lock->lock == SPINPDR_UNLOCKED);
+       /* assume we're vcore 0 */
+       pdr_lock->lock = 0;
+}
+weak_alias(__spin_pdr_lock, spin_pdr_lock)
+
+void __spin_pdr_unlock(struct spin_pdr_lock *pdr_lock)
+{
+       pdr_lock->lock = SPINPDR_UNLOCKED;
+}
+weak_alias(__spin_pdr_unlock, spin_pdr_unlock)
index 1ecd3e2..bc9b9f8 100644 (file)
@@ -1,6 +1,5 @@
 #pragma once
 
-#include <parlib/common.h>
 #include <ros/atomic.h>
 
 __BEGIN_DECLS
index fbf6d25..a833ab8 100644 (file)
@@ -45,6 +45,8 @@ bool spinlock_locked(spinlock_t *lock)
  * lockholder's vcoreid in the lock, and all spinners ensure that vcore runs. */
 void spin_pdr_init(struct spin_pdr_lock *pdr_lock)
 {
+       /* See glibc-2.19-akaros/sysdeps/akaros/lowlevellock.h for details. */
+       parlib_static_assert(sizeof(struct spin_pdr_lock) == sizeof(int));
        pdr_lock->lock = SPINPDR_UNLOCKED;
 }