Cleaned up memory barrier usage (XCC)
authorBarret Rhoden <brho@cs.berkeley.edu>
Sun, 25 Sep 2011 23:16:34 +0000 (16:16 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:36:07 +0000 (17:36 -0700)
Cleaned up the use of memory barriers in our shared memory
synchronization code (event stuff, locking stuff, etc).  Check out the
documentation if you plan to write any concurrent, shared memory code
that doesn't use locking.

17 files changed:
Documentation/memory_barriers.txt [new file with mode: 0644]
kern/arch/i686/atomic.h
kern/arch/i686/ros/membar.h
kern/arch/riscv/ros/membar.h
kern/arch/sparc/ros/membar.h
kern/include/atomic.h
kern/include/ros/atomic.h
kern/src/event.c
kern/src/process.c
kern/src/syscall.c
user/parlib/event.c
user/parlib/include/i686/vcore.h
user/parlib/mcs.c
user/parlib/ucq.c
user/parlib/uthread.c
user/parlib/vcore.c
user/pthread/pthread.c

diff --git a/Documentation/memory_barriers.txt b/Documentation/memory_barriers.txt
new file mode 100644 (file)
index 0000000..aa8cd13
--- /dev/null
@@ -0,0 +1,224 @@
+memory_barriers.txt
+Barret Rhoden
+
+1. Overview
+2. General Rules
+3. Use in the Code Base
+4. Memory Barriers and Locking
+5. Other Stuff
+
+1. Overview
+====================
+Memory barriers exist to make sure the compiler and the CPU do what we intend.
+The compiler memory barreir (cmb()) (called an optimization barrier in linux)
+prevents the compliler from reordering operations.  However, CPUs can also
+reorder reads and writes, in an architecture-dependent manner.  In most places
+with shared memory synchronization, you'll need some form of memory barriers.
+
+These barriers apply to 'unrelated' reads and writes.  The compiler or the CPU
+cannot detect any relationship between them, so it believes it is safe to
+reorder them.  The problem arises in that we attach some meaning to them,
+often in the form of signalling other cores.
+
+CPU memory barriers only apply when talking to different cores or hardware
+devices.  They do not matter when dealing with your own core (perhaps between
+a uthread and vcore context, running on the same core).  cmb()s still matter,
+even when the synchronizing code runs on the same core.  See Section 3 for
+more details.
+
+2. General Rules
+====================
+2.1: Types of Memory Barriers
+---------------------
+For CPU memory barriers, we have 5 types. 
+- rmb() no reordering of reads with future reads
+- wmb() no reordering of writes with future writes
+- wrmb() no reordering of writes with future reads
+- rwmb() no reordering of reads with future writes
+- mb() no reordering of reads or writes with future reads or writes
+
+All 5 of these have a cmb() built in. (a "memory" clobber).
+
+Part of the reason for the distinction between wrmb/rwmb and the full mb is
+that on many machines (x86), rwmb() is a noop (for us).
+
+These barriers are used on 'normal' reads and writes, and they do not include
+streaming/SSE instructions and string ops (on x86), and they do not include
+talking with hardware devices.  For memory barriers for those types of
+operations, use the _f variety (force), e.g. rmb() -> rmb_f().
+
+2.2: Atomics
+---------------------
+Most atomic operations, such as atomic_inc(), provide some form of memory
+barrier protection.  Specifically, all read-modify-write (RMW) atomic ops act
+as a CPU memory barrier (like an mb()), but do *not* provide a cmb().  They
+only provide a cmb() on the variables they apply to (i.e., variables in the
+clobber list).  
+
+I considered making all atomics clobber "memory" (like the cmb()), but
+sync_fetch_and_add() and friends do not do this by default, and it also means
+that any use of atomics (even when we don't require a cmb()) then provides a
+cmb().
+
+Also note that not all atomic operations are RMW.  atomic_set(), _init(), and
+_read() do not enforce a memory barrier in the CPU.  If in doubt, look for the
+LOCK in the assembly (except for xchg, which is a locking function).  We're
+taking advantage of the LOCK the atomics provide to serialize and synchronize
+our memory.
+
+In a lot of places, even if we have an atomic I'll still put in the expected
+mb (e.g., a rmb()), especially if it clarifies the code.  When I rely on the
+atomic's LOCK, I'll make a note of it (such as in spinlock code).
+
+Finally, note that the atomic RMWs handle the mb_f()s just like they handle
+the regular memory barriers.  The LOCK prefix does quite a bit.
+
+These rules are a bit x86 specific, so for now other architectures will need
+to implement their atomics such that they provide the same effects.
+
+2.3: Locking
+---------------------
+If you access shared memory variables only when inside locks, then you do not
+need to worry about memory barriers.  The details are sorted out in the
+locking code.
+
+3. Use in the Code Base
+====================
+Figuring out how / when / if to use memory barriers is relatively easy.
+- First, notice when  you are using shared memory to synchronize.  Any time
+  you are using a global variable or working on structs that someone else
+  might look at concurrently, and you aren't using locks or another vetted
+  tool (like krefs), then you need to worry about this.
+- Second, determine what reads and writes you are doing.
+- Third, determine who you are talking to.
+
+If you're talking to other cores or devices, you need CPU mbs.  If not, a cmb
+suffices.  Based on the types of reads and writes you are doing, just pick one
+of the 5 memory barriers.
+
+3.1: What's a Read or Write?
+---------------------
+When writing code that synchronizes with other threads via shared memory, we
+have a variety of patterns.  Most infamous is the "signal, then check if the
+receiver is still listening", which is the critical part of the "check,
+signal, check again" pattern.  For exmaples, look at things like
+'notif_pending' and 'can_rcv_msg'.  
+
+In these examples, "write" and "read" include things such as posting events or
+checking flags (which ultimately involve writes and reads).  You need to be
+aware that some functions, such as TAILQ_REMOVE are writes, even though it is
+not written as *x = 3;.  Whatever your code is, you should be able to point
+out what are the critical variables and their interleavings.  You should see
+how a CPU reordering would break your algorithm just like a poorly timed
+interrupt or other concurrent interleaving.
+
+When looking at a function that we consider a signal/write, you need to know
+what it does.  It might handle its memory barriers internally (protecting its
+own memory operations).  However, it might not.  In general, I err on the side
+of extra mbs, or at the very least leave a comment about what I am doing and
+what sort of barriers I desire from the code.
+
+3.2: Who Are We Talking To?
+---------------------
+CPU memory barriers are necessary when synchronizing/talking with remote cores
+or devices, but not when "talking" with your own core.  For instance, if you
+issue two writes, then read them, you will see both writes (reads may not be
+reorderd with older writes to the same location on a single processor, and
+your reads get served out of the write buffer).  Note, the read can
+pass/happen before the write, but the CPU gives you the correct value that the
+write gave you (store-buffer forwarding).  Other cores may not see both of
+them due to reordering magic.  Put another way, since those remote processors
+didn't do the initial writing, the rule about reading the same location
+doesn't apply to them.
+
+Given this, when finding spots in the code that may require a mb(), I think
+about signalling a concurrent viewer on a different core.  A classic example
+is when we signal to say "process an item".  Say we were on one core and
+filled the struct out and then signalled, if we then started reading from that
+same core, we would see our old write (you see writes you previously wrote),
+but someone else on another core may see the signal before the filled out
+struct.  
+
+There is still a distinction between the compiler reordering code and the
+processor reordering code.  Imagine the case of "filling a struct, then
+posting the pointer to the struct".  If the compiler reorders, the pointer may
+be posted before the struct is filled, and an interrupt may come in.  The
+interrupt handler may look at that structure and see invalid data.  This has
+nothing to do with the CPU reordering - simply the compiler messed with you.
+Note this only matters if we care about a concurrent interleaving (interrupt
+handler with a kthread for instance), and we ought to be aware that there is
+some shared state being mucked with.
+
+For a more complicated example, consider DONT_MIGRATE and reading vcoreid.
+Logically, we want to make sure the vcoreid read doesn't come before the flag
+write (since uthread code now thinks it is on a particular vcore).  This
+write, then read would normally require a wrmb(), but is that overkill?
+Clearly, we need the compiler to issue the writes in order, so we need a cmb()
+at least.  Here's the code that the processor will get:
+       orl    $0x1,0x254(%edi)      (write DONT_MIGRATE)
+       mov    $0xfffffff0,%ebp      (getting ready with the TLS)
+       mov    %gs:0x0(%ebp),%esi    (reading the vcoreid from TLS)
+
+Do we need a wrmb() here?  Any remote code might see the write after the
+vcoreid read, but the read is a bit different than normal ones.  We aren't
+reading global memory, and we aren't trying to synchronize with another core.
+All that matters is that if the thread running this code saw the vcoreid read,
+then whoever reads the flag sees the write.
+
+The 'whoever' is not concurrently running code - it is 2LS code that either
+runs on the vcore due to an IPI/notification, or it is 2LS code running
+remotely that responded to a preemption of that vcore.  Both of these cases
+require an IPI.  AFAIK, interrupts serialize memory operations, so whatever
+writes were issued before the interrupt hit memory (or cache) before we even
+do things like write out the trapframe of the thread.  If this isn't true,
+then the synchronization we do when writing out the trapframe (before allowing
+a remote core to try and recover the preempted uthread), will handle the
+DONT_MIGRATE write.
+
+Anyway, the point is that remote code will look at it, but only when told to
+look.  That "telling" is the write, which happens after the
+synchronizing/serializing events of the IPI path.
+
+4. Memory Barriers and Locking
+====================
+The implementation of locks require memory barriers (both compiler and CPU).
+Regular users of locks do not need to worry about this.  Lock implementers do.
+
+We need to consider reorderings of reads and writes from before and after the
+lock/unlock write.  In these next sections, the reads and writes I talk about
+are from a given thread/CPU.  Protected reads/writes are those that happen
+while the lock is held.  When I say you need a wmb(), you could get by with a
+cmb() and an atomic-RMW op: just so long as you have the cmb() and the
+approrpiate CPU mb() at a minimum.
+
+4.1: Locking
+---------------------
+- Don't care about our reads or writes before the lock happening after the
+  lock write.
+- Don't want protected writes slipping out before the lock write, need a wmb()
+  after the lock write.
+- Don't want protected reads slipping out before the lock write, need a wrmb()
+  after the lock write.
+
+4.2: Unlocking
+---------------------
+- Don't want protected writes slipping out after the unlock, so we need a
+  wmb() before the unlock write.
+- Don't want protected reads slipping out after the unlock, so we need a
+  rwmb() before the unlock write.  Arguably, there is some causality that
+  makes this less meaningful (what did you do with the info? if not a write
+  that was protected, then who cares? )
+- Don't care about reads or writes after the unlock happening earlier than the
+  unlock write.
+
+5. Other Stuff
+====================
+Linux has a lot of work on memory barriers, far more advanced than our stuff.
+Some of it doesn't make any sense.  I've asked our architects about things
+like read_barrier_depends() and a few other things.  They also support some
+non-Intel x86 clones that need wmb_f() in place of a wmb() (support out of
+order writes).  If this pops up, we'll deal with it.
+       
+Also note that none of these barriers deal with things like page talble walks,
+page/segmentation update writes, non-temporal hints on writes, etc.  glhf with
+that, future self!
index 5bf2021..db50465 100644 (file)
@@ -167,13 +167,15 @@ static inline void spin_lock(spinlock_t *lock)
        lock->call_site = (void RACY*CT(1))TC(read_eip());
        lock->calling_core = core_id();
 #endif
+       cmb();  /* need cmb(), the CPU mb() was handled by the xchgb */
 }
 
 static inline void spin_unlock(spinlock_t *lock)
 {
        /* Need to prevent the compiler (and some arches) from reordering older
-        * stores */
+        * stores. */
        wmb();
+       rwmb(); /* x86 makes both of these a cmb() */
        lock->rlock = 0;
 }
 
index 5e38bf9..37ed736 100644 (file)
@@ -1,15 +1,23 @@
-#ifndef _ARCH_MEMBAR_H
-#define _ARCH_MEMBAR_H
+#ifndef ROS_INC_ARCH_MEMBAR_H
+#define ROS_INC_ARCH_MEMBAR_H
 
-/* Adding "memory" to keep the compiler from fucking with us */
+/* Full CPU memory barrier */
 #define mb() ({ asm volatile("mfence" ::: "memory"); })
-#define rmb() ({ asm volatile("lfence" ::: "memory"); })
-#define wmb() ({ asm volatile("" ::: "memory"); })
-/* Compiler memory barrier */
+/* Compiler memory barrier (optimization barrier) */
 #define cmb() ({ asm volatile("" ::: "memory"); })
-/* Force a wmb, used in cases where an IPI could beat a write, even though
- * write-orderings are respected.
- * TODO: this probably doesn't do what you want. */
-#define wmb_f() ({ asm volatile("sfence"); })
+/* Partial CPU memory barriers */
+#define rmb() cmb()
+#define wmb() cmb()
+#define wrmb() mb()
+#define rwmb() cmb()
 
-#endif
+/* Forced barriers, used for string ops, SSE ops, dealing with hardware, or
+ * other places where you avoid 'normal' x86 read/writes (like having an IPI
+ * beat a write) */
+#define mb_f() ({ asm volatile("mfence" ::: "memory"); })
+#define rmb_f() ({ asm volatile("lfence" ::: "memory"); })
+#define wmb_f() ({ asm volatile("sfence" ::: "memory"); })
+#define wrmb_f() mb_f()
+#define rwmb_f() mb_f()
+
+#endif /* ROS_INC_ARCH_MEMBAR_H */
index aa5faf8..46d7860 100644 (file)
@@ -1,13 +1,23 @@
-#ifndef _ARCH_MEMBAR_H
-#define _ARCH_MEMBAR_H
+#ifndef ROS_INC_ARCH_MEMBAR_H
+#define ROS_INC_ARCH_MEMBAR_H
 
+/* Full CPU memory barrier */
 #define mb() __sync_synchronize()
-#define rmb() mb()
-#define wmb() mb()
 /* Compiler memory barrier */
 #define cmb() ({ asm volatile("" ::: "memory"); })
-/* Force a wmb, used in cases where an IPI could beat a write, even though
- * write-orderings are respected. */
-#define wmb_f() mb()
+/* Partial CPU memory barriers */
+#define rmb() mb()
+#define wmb() mb()
+#define wrmb() mb()
+#define rwmb() mb()
+
+/* Forced barriers, used for string ops, SSE ops, dealing with hardware, or
+ * other places where you avoid 'normal' x86 read/writes (like having an IPI
+ * beat a write) */
+#define mb_f() mb()
+#define rmb_f() rmb()
+#define wmb_f() wmb()
+#define wrmb_f() wrmb()
+#define rwmb_f() rwmb()
 
-#endif
+#endif /* ROS_INC_ARCH_MEMBAR_H */
index a8bd24b..1850b58 100644 (file)
@@ -1,13 +1,23 @@
-#ifndef _ARCH_MEMBAR_H
-#define _ARCH_MEMBAR_H
+#ifndef ROS_INC_ARCH_MEMBAR_H
+#define ROS_INC_ARCH_MEMBAR_H
 
+/* Full CPU memory barrier */
 #define mb() {rmb(); wmb();}
-#define rmb()
-#define wmb() ({ __asm__ __volatile__ ("stbar" ::: "memory"); })
-/* Compiler memory barrier */
+/* Compiler memory barrier (optimization barrier) */
 #define cmb() ({ asm volatile("" ::: "memory"); })
-/* Force a wmb, used in cases where an IPI could beat a write, even though
- * write-orderings are respected.  (Used by x86) */
+/* Partial CPU memory barriers */
+#define rmb() cmb()
+#define wmb() ({ __asm__ __volatile__ ("stbar" ::: "memory"); })
+#define wrmb() mb()
+#define rwmb() mb()
+
+/* Forced barriers, used for string ops, SSE ops, dealing with hardware, or
+ * other places where you avoid 'normal' x86 read/writes (like having an IPI
+ * beat a write) */
+#define mb_f() mb()
+#define rmb_f() rmb()
 #define wmb_f() wmb()
+#define wrmb_f() wrmb()
+#define rwmb_f() rwmb()
 
-#endif
+#endif /* ROS_INC_ARCH_MEMBAR_H */
index 0f1094b..279daa2 100644 (file)
@@ -243,7 +243,9 @@ static inline void write_sequnlock(seqlock_t *lock)
 
 static inline seq_ctr_t read_seqbegin(seqlock_t *lock)
 {
-       return lock->r_ctr;
+       seq_ctr_t retval = lock->r_ctr;
+       rmb();  /* don't want future reads to come before our ctr read */
+       return retval;
 }
 
 static inline bool read_seqretry(seqlock_t *lock, seq_ctr_t ctr)
index 29bb562..ffc8f53 100644 (file)
@@ -8,6 +8,7 @@
 #define ROS_INC_ATOMIC_H
 
 #include <ros/common.h>
+#include <ros/arch/membar.h>
 
 typedef void* atomic_t;
 
@@ -36,6 +37,7 @@ static inline bool seq_is_locked(seq_ctr_t seq_ctr)
 
 static inline bool seqctr_retry(seq_ctr_t old_ctr, seq_ctr_t new_ctr)
 {
+       rmb();  /* don't allow protected reads to reorder after the check */
        return (seq_is_locked(old_ctr)) || (old_ctr != new_ctr);        
 }
 
index b71a92a..dcea4e6 100644 (file)
@@ -69,7 +69,7 @@ static void send_indir_to_vcore(struct event_queue *ev_q, uint32_t vcoreid)
        local_msg.ev_arg3 = ev_q;
        post_ev_msg(get_proc_ev_mbox(vcoreid), &local_msg, 0);
        /* Set notif pending, so userspace doesn't miss the INDIR while yielding */
-       wmb();
+       wmb(); /* Ensure ev_msg write is before notif_pending */
        vcpd->notif_pending = TRUE;
 }
 
@@ -98,7 +98,7 @@ static bool try_alert_vcore(struct proc *p, struct event_queue *ev_q,
         * only critical part is that we __alert, then check can_alert. */
        if (can_alert_vcore(vcoreid)) {
                __alert_vcore(p, ev_q, vcoreid);
-               cmb();
+               wrmb(); /* prev write (notif_pending) must come before following reads*/
                if (can_alert_vcore(vcoreid))
                        return TRUE;
        }
@@ -125,7 +125,7 @@ static bool __alert_list_member(struct vcore_tailq *list, struct proc *p,
                /* post the alert.  Not using the try_alert_vcore() helper since I want
                 * something more customized for the lists. */
                __alert_vcore(p, ev_q, vcoreid);
-               cmb();
+               wrmb(); /* prev write (notif_pending) must come before following reads*/
                /* if they are still alertable after we sent the msg, then they'll get
                 * it before yielding (racing with userspace yield here).  This check is
                 * not as critical as the next one, but will allow us to alert vcores
@@ -133,7 +133,6 @@ static bool __alert_list_member(struct vcore_tailq *list, struct proc *p,
                 * bulk_preempt list. */
                if (can_alert_vcore(vcoreid))
                        return TRUE;
-               cmb();
                /* As a backup, if they are still the first on the list, then they are
                 * still going to get the message.  For the online list, proc_yield()
                 * will return them to userspace (where they will get the message)
@@ -202,6 +201,7 @@ static void alert_vcore(struct proc *p, struct event_queue *ev_q,
         * is probably true. */
        if (ev_q->ev_flags & EVENT_INDIR) {
                ev_q->ev_alert_pending = TRUE;
+               wmb();  /* force this write to happen before any event writes */
        }
        /* Don't care about FALLBACK, just send and be done with it.  TODO:
         * considering getting rid of FALLBACK as an option and making it mandatory
@@ -365,6 +365,7 @@ void send_event(struct proc *p, struct event_queue *ev_q, struct event_msg *msg,
         * vehicle for sending the ev_type. */
        assert(msg);
        post_ev_msg(ev_mbox, msg, ev_q->ev_flags);
+       wmb();  /* ensure ev_msg write is before alert_vcore() */
        /* Help out userspace a bit by checking for a potentially confusing bug */
        if ((ev_mbox == get_proc_ev_mbox(vcoreid)) &&
            (ev_q->ev_flags & EVENT_INDIR))
@@ -386,6 +387,7 @@ void send_kernel_event(struct proc *p, struct event_msg *msg, uint32_t vcoreid)
        uint16_t ev_num = msg->ev_type;
        assert(ev_num < MAX_NR_EVENT);          /* events start at 0 */
        struct event_queue *ev_q = p->procdata->kernel_evts[ev_num];
+       /* linux would put a rmb_depends() here too, i think. */
        if (ev_q)
                send_event(p, ev_q, msg, vcoreid);
 }
index f6390fa..56724d9 100644 (file)
@@ -827,7 +827,7 @@ void proc_yield(struct proc *SAFE p, bool being_nice)
                        TAILQ_REMOVE(&p->online_vcs, vc, list);
                        /* Now that we're off the online list, check to see if an alert made
                         * it through (event.c sets this) */
-                       cmb();
+                       wrmb(); /* prev write must hit before reading notif_pending */
                        if (vcpd->notif_pending) {
                                /* We lost, put it back on the list and abort the yield */
                                TAILQ_INSERT_TAIL(&p->online_vcs, vc, list); /* could go HEAD */
@@ -889,6 +889,7 @@ void proc_notify(struct proc *p, uint32_t vcoreid)
         * use a bool. (wrong answer). */
        if (!vcpd->notif_pending) {
                vcpd->notif_pending = TRUE;
+               wrmb(); /* must write notif_pending before reading notif_enabled */
                if (vcpd->notif_enabled) {
                        /* GIANT WARNING: we aren't using the proc-lock to protect the
                         * vcoremap.  We want to be able to use this from interrupt context,
index 9b0295f..c11e95a 100644 (file)
@@ -1476,7 +1476,7 @@ void __signal_syscall(struct syscall *sysc, struct proc *p)
        struct event_msg local_msg;
        /* User sets the ev_q then atomically sets the flag (races with SC_DONE) */
        if (atomic_read(&sysc->flags) & SC_UEVENT) {
-               rmb();
+               rmb();  /* read the ev_q after reading the flag */
                ev_q = sysc->ev_q;
                if (ev_q) {
                        memset(&local_msg, 0, sizeof(struct event_msg));
index ccb1432..04b27af 100644 (file)
@@ -113,6 +113,7 @@ void enable_kevent(unsigned int ev_type, uint32_t vcoreid, int ev_flags)
        ev_q->ev_flags = ev_flags;
        ev_q->ev_vcore = vcoreid;
        ev_q->ev_handler = 0;
+       wmb();  /* make sure ev_q is filled out before registering */
        register_kevent_q(ev_q, ev_type);
 }
 
@@ -194,7 +195,6 @@ static int handle_mbox(struct event_mbox *ev_mbox, unsigned int flags)
         * to check the bits regardless */
        void bit_handler(unsigned int bit) {
                printd("[event] Bit: ev_type: %d\n", bit);
-               cmb();
                if (ev_handlers[bit])
                        ev_handlers[bit](0, bit);
                retval++;
@@ -224,7 +224,7 @@ void handle_ev_ev(struct event_msg *ev_msg, unsigned int ev_type)
         * with the remaining events in the future - meaning it won't return to
         * here. */
        ev_q->ev_alert_pending = FALSE;
-       wmb();                  /* should be unnecessary, due to the function call */
+       wmb();  /* don't let the pending write pass the signaling of an ev recv */
        handle_event_q(ev_q);
 }
 
index d448e5d..dd6f16f 100644 (file)
@@ -99,6 +99,10 @@ static inline void pop_ros_tf(struct user_trapframe *tf, uint32_t vcoreid)
                      "subl %%eax,%%esp;     " /* move to notif_en_loc slot */
                      "popl %%eax;           " /* load notif_enabaled addr */
                      "movb $0x01,(%%eax);   " /* enable notifications */
+                                 /* Need a wrmb() here so the write of enable_notif can't pass
+                                  * the read of notif_pending (racing with a potential
+                                  * cross-core call with proc_notify()). */
+                                 "lock addl $0,(%%esp); " /* LOCK is a CPU mb() */
                                  /* From here down, we can get interrupted and restarted */
                      "popl %%eax;           " /* get notif_pending status */
                      "testb $0x01,(%%eax);  " /* test if a notif is pending */
index a8fbbe7..9b25f27 100644 (file)
@@ -20,33 +20,39 @@ static inline mcs_lock_qnode_t *mcs_qnode_swap(mcs_lock_qnode_t **addr,
 void mcs_lock_lock(struct mcs_lock *lock, struct mcs_lock_qnode *qnode)
 {
        qnode->next = 0;
-       mcs_lock_qnode_t* predecessor = mcs_qnode_swap(&lock->lock,qnode);
-       if(predecessor)
-       {
+       mcs_lock_qnode_t *predecessor = mcs_qnode_swap(&lock->lock, qnode);
+       if (predecessor) {
                qnode->locked = 1;
+               wmb();
                predecessor->next = qnode;
-               while(qnode->locked)
+               /* no need for a wrmb(), since this will only get unlocked after they
+                * read our previous write */
+               while (qnode->locked)
                        cpu_relax();
        }
+       cmb();  /* just need a cmb, the swap handles the CPU wmb/wrmb() */
 }
 
 void mcs_lock_unlock(struct mcs_lock *lock, struct mcs_lock_qnode *qnode)
 {
-       if(qnode->next == 0)
-       {
-               mcs_lock_qnode_told_tail = mcs_qnode_swap(&lock->lock,0);
-               if(old_tail == qnode)
+       if (qnode->next == 0) {
+               cmb();  /* no need for CPU mbs, since there's an atomic_swap() */
+               mcs_lock_qnode_t *old_tail = mcs_qnode_swap(&lock->lock,0);
+               if (old_tail == qnode)
                        return;
-
-               mcs_lock_qnode_t* usurper = mcs_qnode_swap(&lock->lock,old_tail);
-               while(qnode->next == 0);
-               if(usurper)
+               mcs_lock_qnode_t *usurper = mcs_qnode_swap(&lock->lock,old_tail);
+               while (qnode->next == 0)
+                       cpu_relax();
+               if (usurper)
                        usurper->next = qnode->next;
                else
                        qnode->next->locked = 0;
-       }
-       else
+       } else {
+               /* mb()s necessary since we didn't call an atomic_swap() */
+               wmb();  /* need to make sure any previous writes don't pass unlocking */
+               rwmb(); /* need to make sure any reads happen before the unlocking */
                qnode->next->locked = 0;
+       }
 }
 
 /* We don't bother saving the state, like we do with irqsave, since we can use
@@ -58,9 +64,9 @@ void mcs_lock_notifsafe(struct mcs_lock *lock, struct mcs_lock_qnode *qnode)
        if (!in_vcore_context()) {
                if (current_uthread)
                        current_uthread->flags |= UTHREAD_DONT_MIGRATE;
-               wmb();
+               cmb();  /* don't issue the flag write before the vcore_id() read */
                disable_notifs(vcore_id());
-               wmb();
+               cmb();  /* don't issue the flag write before the disable */
                if (current_uthread)
                        current_uthread->flags &= ~UTHREAD_DONT_MIGRATE;
        }
@@ -73,9 +79,9 @@ void mcs_unlock_notifsafe(struct mcs_lock *lock, struct mcs_lock_qnode *qnode)
        if (!in_vcore_context() && in_multi_mode()) {
                if (current_uthread)
                        current_uthread->flags |= UTHREAD_DONT_MIGRATE;
-               wmb();
+               cmb();  /* don't issue the flag write before the vcore_id() read */
                enable_notifs(vcore_id());
-               wmb();
+               cmb();  /* don't issue the flag write before the enable */
                if (current_uthread)
                        current_uthread->flags &= ~UTHREAD_DONT_MIGRATE;
        }
index 60b0bd6..1e58500 100644 (file)
@@ -138,14 +138,16 @@ claim_slot:
        assert(slot_is_good(my_idx));
        /* Now we have a good slot that we can consume */
        my_msg = slot2msg(my_idx);
+       /* linux would put an rmb_depends() here */
        /* Wait til the msg is ready (kernel sets this flag) */
        while (!my_msg->ready)
                cpu_relax();
+       rmb();  /* order the ready read before the contents */
        /* Copy out */
        *msg = my_msg->ev_msg;
        /* Unset this for the next usage of the container */
        my_msg->ready = FALSE;
-       wmb();
+       wmb();  /* post the ready write before incrementing */
        /* Increment nr_cons, showing we're done */
        atomic_inc(&((struct ucq_page*)PTE_ADDR(my_idx))->header.nr_cons);
        return 0;
index fc182b6..6d93793 100644 (file)
@@ -68,7 +68,7 @@ void __attribute__((noreturn)) uthread_vcore_entry(void)
 
        check_preempt_pending(vcoreid);
        handle_events(vcoreid);
-       assert(in_vcore_context());     /* double check, in case and event changed it */
+       assert(in_vcore_context());     /* double check, in case an event changed it */
        assert(sched_ops->sched_entry);
        sched_ops->sched_entry();
        /* 2LS sched_entry should never return */
@@ -102,7 +102,8 @@ void uthread_init(struct uthread *new_thread)
         * when we were interrupted.  We need to not migrate, since once we know the
         * vcoreid, we depend on being on the same vcore throughout. */
        caller->flags |= UTHREAD_DONT_MIGRATE;
-       wmb();
+       /* not concerned about cross-core memory ordering, so no CPU mbs needed */
+       cmb();  /* don't let the compiler issue the vcore read before the write */
        /* Note the first time we call this, we technically aren't on a vcore */
        vcoreid = vcore_id();
        disable_notifs(vcoreid);
@@ -115,7 +116,7 @@ void uthread_init(struct uthread *new_thread)
         * from vcore context, so only enable if we're in _M and in vcore context. */
        if (!in_vcore_context() && in_multi_mode())
                enable_notifs(vcoreid);
-       wmb();
+       cmb();  /* issue this write after we're done with vcoreid */
        caller->flags &= ~UTHREAD_DONT_MIGRATE;
 }
 
@@ -175,7 +176,7 @@ void uthread_yield(bool save_state)
         * the same vcore throughout (once it disables notifs).  The race is that we
         * read vcoreid, then get interrupted / migrated before disabling notifs. */
        uthread->flags |= UTHREAD_DONT_MIGRATE;
-       wmb();
+       cmb();  /* don't let DONT_MIGRATE write pass the vcoreid read */
        uint32_t vcoreid = vcore_id();
        printd("[U] Uthread %08p is yielding on vcore %d\n", uthread, vcoreid);
        struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
@@ -188,6 +189,7 @@ void uthread_yield(bool save_state)
         * and short ciruit the function.  Don't do this if we're dying. */
        if (save_state)
                save_ros_tf(&uthread->utf);
+       cmb();  /* Force a reread of yielding. Technically save_ros_tf() is enough*/
        /* Restart path doesn't matter if we're dying */
        if (!yielding)
                goto yield_return_path;
@@ -267,6 +269,7 @@ void run_current_uthread(void)
 /* Launches the uthread on the vcore.  Don't call this on current_uthread. */
 void run_uthread(struct uthread *uthread)
 {
+       uint32_t vcoreid;
        assert(uthread != current_uthread);
        if (uthread->state != UT_RUNNABLE) {
                /* had vcore3 throw this, when the UT blocked on vcore1 and didn't come
@@ -276,10 +279,9 @@ void run_uthread(struct uthread *uthread)
        }
        assert(uthread->state == UT_RUNNABLE);
        uthread->state = UT_RUNNING;
-       /* Save a ptr to the pthread running in the transition context's TLS */
-       uint32_t vcoreid = vcore_id();
-       struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
+       /* Save a ptr to the uthread we'll run in the transition context's TLS */
        current_uthread = uthread;
+       vcoreid = vcore_id();
        clear_notif_pending(vcoreid);
        set_tls_desc(uthread->tls_desc, vcoreid);
        /* Load silly state (Floating point) too.  For real */
@@ -318,10 +320,10 @@ bool register_evq(struct syscall *sysc, struct event_queue *ev_q)
 {
        int old_flags;
        sysc->ev_q = ev_q;
-       wmb();
+       wrmb(); /* don't let that write pass any future reads (flags) */
        /* Try and set the SC_UEVENT flag (so the kernel knows to look at ev_q) */
        do {
-               cmb();
+               /* no cmb() needed, the atomic_read will reread flags */
                old_flags = atomic_read(&sysc->flags);
                /* Spin if the kernel is mucking with syscall flags */
                while (old_flags & SC_K_LOCK)
@@ -351,9 +353,10 @@ void deregister_evq(struct syscall *sysc)
 {
        int old_flags;
        sysc->ev_q = 0;
+       wrmb(); /* don't let that write pass any future reads (flags) */
        /* Try and unset the SC_UEVENT flag */
        do {
-               cmb();
+               /* no cmb() needed, the atomic_read will reread flags */
                old_flags = atomic_read(&sysc->flags);
                /* Spin if the kernel is mucking with syscall flags */
                while (old_flags & SC_K_LOCK)
index 1dd7d88..a5c62b2 100644 (file)
@@ -201,7 +201,7 @@ void vcore_yield(bool preempt_pending)
        uint32_t vcoreid = vcore_id();
        struct preempt_data *vcpd = &__procdata.vcore_preempt_data[vcoreid];
        vcpd->can_rcv_msg = FALSE;
-       wmb();
+       /* no wrmb() necessary, clear_notif() has an mb() */
        /* Clears notif pending.  If we had an event outstanding, this will handle
         * it and return TRUE, at which point we want to unwind and return to the
         * 2LS loop (where we may not want to yield anymore).  Note that the kernel
@@ -231,8 +231,10 @@ bool clear_notif_pending(uint32_t vcoreid)
 {
        bool handled_event = FALSE;
        do {
-               cmb();
                __procdata.vcore_preempt_data[vcoreid].notif_pending = 0;
+               /* need a full mb(), since handle events might be just a read or might
+                * be a write, either way, it needs to happen after notif_pending */
+               mb();
                handled_event = handle_events(vcoreid);
        } while (handled_event);
        return handled_event;
@@ -245,6 +247,7 @@ bool clear_notif_pending(uint32_t vcoreid)
 void enable_notifs(uint32_t vcoreid)
 {
        __enable_notifs(vcoreid);
+       wrmb(); /* need to read after the write that enabled notifs */
        if (__procdata.vcore_preempt_data[vcoreid].notif_pending)
                sys_self_notify(vcoreid, EV_NONE, 0);
 }
index 4387785..8b62cb1 100644 (file)
@@ -507,6 +507,9 @@ int pthread_mutex_lock(pthread_mutex_t* m)
                        cpu_relax();
                        spin_to_sleep(PTHREAD_MUTEX_SPINS, &spinner);
                }
+       /* normally we'd need a wmb() and a wrmb() after locking, but the
+        * atomic_swap handles the CPU mb(), so just a cmb() is necessary. */
+       cmb();
        return 0;
 }
 
@@ -517,8 +520,8 @@ int pthread_mutex_trylock(pthread_mutex_t* m)
 
 int pthread_mutex_unlock(pthread_mutex_t* m)
 {
-  /* Need to prevent the compiler (and some arches) from reordering older
-   * stores */
+  /* keep reads and writes inside the protected region */
+  rwmb();
   wmb();
   atomic_set(&m->lock, 0);
   return 0;