summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorrsc <rsc>2007-09-30 14:30:04 +0000
committerrsc <rsc>2007-09-30 14:30:04 +0000
commit9fd9f80431ad85552c0969831a3ccc3e800ac464 (patch)
treee9190c3029d9b3eda767ae9d2cb93e0e54344e7b
parentc840f3ecdc718c4a6eb6fbd9e0cbb0a012c3adf4 (diff)
downloadxv6-labs-9fd9f80431ad85552c0969831a3ccc3e800ac464.tar.gz
xv6-labs-9fd9f80431ad85552c0969831a3ccc3e800ac464.tar.bz2
xv6-labs-9fd9f80431ad85552c0969831a3ccc3e800ac464.zip
Re: why cpuid() in locking code?
rtm wrote: > Why does acquire() call cpuid()? Why does release() call cpuid()? The cpuid in acquire is redundant with the cmpxchg, as you said. I have removed the cpuid from acquire. The cpuid in release is actually doing something important, but not on the hardware. It keeps gcc from reordering the lock->locked assignment above the other two during optimization. (Not that current gcc -O2 would choose to do that, but it is allowed to.) I have replaced the cpuid in release with a "gcc barrier" that keeps gcc from moving things around but has no hardware effect. On a related note, I don't think the cpuid in mpmain is necessary, for the same reason that the cpuid wasn't needed in release. As to the question of whether acquire(); x = protected; release(); might read protected after release(), I still haven't convinced myself whether it can. I'll put the cpuid back into release if we determine that it can. Russ
-rw-r--r--main.c1
-rw-r--r--spinlock.c16
-rw-r--r--x86.h1
3 files changed, 9 insertions, 9 deletions
diff --git a/main.c b/main.c
index 275aa80..2108d95 100644
--- a/main.c
+++ b/main.c
@@ -50,7 +50,6 @@ mpmain(void)
if(cpu() != mp_bcpu())
lapic_init(cpu());
setupsegs(0);
- cpuid(0, 0, 0, 0, 0); // memory barrier
cpus[cpu()].booted = 1;
scheduler();
diff --git a/spinlock.c b/spinlock.c
index a1aa37d..c00c978 100644
--- a/spinlock.c
+++ b/spinlock.c
@@ -10,6 +10,12 @@
extern int use_console_lock;
+// Barrier to gcc's instruction reordering.
+static void inline gccbarrier(void)
+{
+ asm volatile("" : : : "memory");
+}
+
void
initlock(struct spinlock *lock, char *name)
{
@@ -32,10 +38,6 @@ acquire(struct spinlock *lock)
while(cmpxchg(0, 1, &lock->locked) == 1)
;
- // Serialize instructions: now that lock is acquired, make sure
- // we wait for all pending writes from other processors.
- cpuid(0, 0, 0, 0, 0); // memory barrier (see Ch 7, IA-32 manual vol 3)
-
// Record info about lock acquisition for debugging.
// The +10 is only so that we can tell the difference
// between forgetting to initialize lock->cpu
@@ -53,12 +55,10 @@ release(struct spinlock *lock)
lock->pcs[0] = 0;
lock->cpu = 0xffffffff;
-
- // Serialize instructions: before unlocking the lock, make sure
- // to flush any pending memory writes from this processor.
- cpuid(0, 0, 0, 0, 0); // memory barrier (see Ch 7, IA-32 manual vol 3)
+ gccbarrier(); // Keep gcc from moving lock->locked = 0 earlier.
lock->locked = 0;
+
popcli();
}
diff --git a/x86.h b/x86.h
index a24214d..a1c66b5 100644
--- a/x86.h
+++ b/x86.h
@@ -96,6 +96,7 @@ write_eflags(uint eflags)
asm volatile("pushl %0; popfl" : : "r" (eflags));
}
+// XXX: Kill this if not used.
static inline void
cpuid(uint info, uint *eaxp, uint *ebxp, uint *ecxp, uint *edxp)
{