On Mon, Oct 20, 2014 at 10:11:40AM +0100, Thomas Gleixner wrote:
> On Sat, 18 Oct 2014, Davidlohr Bueso wrote:
> > On Sat, 2014-10-18 at 13:50 -0700, Linus Torvalds wrote:
> > > On Sat, Oct 18, 2014 at 12:58 PM, Davidlohr Bueso <d...@stgolabs.net>
> > > wrote:
> > > >
> > > > And [get/put]_futex_keys() shouldn't even be called for private futexes.
> > > > The following patch had some very minor testing on a 60 core box last
> > > > night, but passes both Darren's and perf's tests. So I *think* this is
> > > > right, but lack of sleep and I overall just don't trust them futexes!
> > >
> > > Hmm. I don't see the advantage of making the code more complex in
> > > order to avoid the functions that are no-ops for the !fshared case?
> > >
> > > IOW, as far as I can tell, this patch doesn't actually really *change*
> > > anything. Am I missing something?
> >
> > Right, all we do is avoid a NOP, but I don't see how this patch makes
> > the code more complex. In fact, the whole idea is to make it easier to
> > read and makes the key referencing differences between shared and
> > private futexes crystal clear, hoping to mitigate future bugs.
>
> I tend to disagree. The current code is symetric versus get/drop and
> you make it unsymetric by avoiding the drop call with a pointless
> extra conditional in all call sites.
Since you mention symmetry, something like below makes the barriers more
explicit. However, it removes the implied barrier for get_futex_key_refs
and the other calling places need to be verified (requeue_futex and
requeue_pi_futex; if barrier is not required here, the result may be
slightly more optimal, not sure you would see the difference though).
diff --git a/kernel/futex.c b/kernel/futex.c
index f3a3a071283c..5b9d857d0816 100644
--- a/kernel/futex.c
+++ b/kernel/futex.c
@@ -143,9 +143,7 @@
*
* Where (A) orders the waiters increment and the futex value read through
* atomic operations (see hb_waiters_inc) and where (B) orders the write
- * to futex and the waiters read -- this is done by the barriers in
- * get_futex_key_refs(), through either ihold or atomic_inc, depending on the
- * futex type.
+ * to futex and the waiters read (see hb_waiters_pending).
*
* This yields the following case (where X:=waiters, Y:=futex):
*
@@ -262,12 +260,6 @@ static struct futex_hash_bucket *futex_queues;
static inline void futex_get_mm(union futex_key *key)
{
atomic_inc(&key->private.mm->mm_count);
- /*
- * Ensure futex_get_mm() implies a full barrier such that
- * get_futex_key() implies a full barrier. This is relied upon
- * as full barrier (B), see the ordering comment above.
- */
- smp_mb__after_atomic();
}
/*
@@ -297,6 +289,10 @@ static inline void hb_waiters_dec(struct futex_hash_bucket
*hb)
static inline int hb_waiters_pending(struct futex_hash_bucket *hb)
{
+ /*
+ * Full barrier (B), see the ordering comment above.
+ */
+ smp_mb__before_atomic();
#ifdef CONFIG_SMP
return atomic_read(&hb->waiters);
#else
@@ -338,13 +334,11 @@ static void get_futex_key_refs(union futex_key *key)
switch (key->both.offset & (FUT_OFF_INODE|FUT_OFF_MMSHARED)) {
case FUT_OFF_INODE:
- ihold(key->shared.inode); /* implies MB (B) */
+ __iget(key->shared.inode);
break;
case FUT_OFF_MMSHARED:
- futex_get_mm(key); /* implies MB (B) */
+ futex_get_mm(key);
break;
- default:
- smp_mb(); /* explicit MB (B) */
}
}
@@ -417,7 +411,7 @@ get_futex_key(u32 __user *uaddr, int fshared, union
futex_key *key, int rw)
if (!fshared) {
key->private.mm = mm;
key->private.address = address;
- get_futex_key_refs(key); /* implies MB (B) */
+ get_futex_key_refs(key);
return 0;
}
@@ -524,7 +518,7 @@ again:
key->shared.pgoff = basepage_index(page);
}
- get_futex_key_refs(key); /* implies MB (B) */
+ get_futex_key_refs(key);
out:
unlock_page(page_head);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
