Fixes a bug with sparc's atomic_sub_and_test()
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 15 Oct 2010 01:14:56 +0000 (18:14 -0700)
committerKevin Klues <klueska@cs.berkeley.edu>
Thu, 3 Nov 2011 00:35:55 +0000 (17:35 -0700)
Careful with negative (or large) numbers with these tricksy atomics!
Also touches up a couple other things that were discovered while messing
with sparc.

kern/arch/sparc/atomic.h
kern/include/kref.h
kern/src/vfs.c

index 1dade35..550a2a3 100644 (file)
@@ -113,11 +113,17 @@ static inline bool atomic_add_not_zero(atomic_t *number, long val)
 /* Subtraces val from number, returning True if the new value is 0. */
 static inline bool atomic_sub_and_test(atomic_t *number, long val)
 {
-       long retval = atomic_fetch_and_add(number, -val);
-       if (retval)
-               return FALSE;
-       else
-               return TRUE;
+       long num;
+       bool retval = FALSE;
+       /* this is pretty clever.  the lower 8 bits (i.e byte 3)
+        * of the atomic_t serve as a spinlock.  let's acquire it. */
+       spin_lock((spinlock_t*)number);
+       num = atomic_read(number);
+       num -= val;
+       retval = num ? FALSE : TRUE;
+       /* set the new counter value.  the lock is cleared (for free) */
+       atomic_init(number, num);
+       return retval;
 }
 
 static inline uint32_t atomic_swap(uint32_t* addr, uint32_t val)
index 5bfc89e..15b5ae9 100644 (file)
@@ -51,7 +51,8 @@ static struct kref *kref_get_not_zero(struct kref *kref, unsigned int inc)
 {
        if (atomic_add_not_zero(&kref->refcount, inc))
                return kref;
-       else return 0;
+       else
+               return 0;
 }
 
 /* Will panic on zero */
index 2fb22c8..5462a9d 100644 (file)
@@ -150,6 +150,9 @@ static struct dentry *do_lookup(struct dentry *parent, char *name)
        /* not in the dcache at all, need to consult the FS */
        result = parent->d_inode->i_op->lookup(parent->d_inode, query, 0);
        if (!result) {
+               /* Note the USED flag will get turned off when this gets added to the
+                * LRU in dentry_release().  There's a slight race here that we'll panic
+                * on, but I want to catch it (in dcache_put()) for now. */
                query->d_flags |= DENTRY_NEGATIVE;
                dcache_put(parent->d_sb, query);
                kref_put(&query->d_kref);
@@ -675,14 +678,15 @@ void dentry_release(struct kref *kref)
        /* While locked, we need to double check the kref, in case someone already
         * reup'd it.  Re-up? you're crazy!  Reee-up, you're outta yo mind! */
        if (!kref_refcnt(&dentry->d_kref)) {
-               /* and make sure it wasn't USED, then UNUSED again */
-               /* TODO: think about issues with this */
+               /* Note this is where negative dentries get set UNUSED */
                if (dentry->d_flags & DENTRY_USED) {
                        dentry->d_flags &= ~DENTRY_USED;
                        spin_lock(&dentry->d_sb->s_lru_lock);
                        TAILQ_INSERT_TAIL(&dentry->d_sb->s_lru_d, dentry, d_lru);
                        spin_unlock(&dentry->d_sb->s_lru_lock);
                } else {
+                       /* and make sure it wasn't USED, then UNUSED again */
+                       /* TODO: think about issues with this */
                        warn("This should be rare.  Tell brho this happened.");
                }
        }
@@ -800,6 +804,7 @@ void dcache_put(struct super_block *sb, struct dentry *key_val)
        old = hashtable_remove(sb->s_dcache, key_val);
        if (old) {
                assert(old->d_flags & DENTRY_NEGATIVE);
+               /* This is possible, but rare for now (about to be put on the LRU) */
                assert(!(old->d_flags & DENTRY_USED));
                assert(!kref_refcnt(&old->d_kref));
                spin_lock(&sb->s_lru_lock);