| Message ID | 20141020104926.GD23751@e104818-lin.cambridge.arm.com |
|---|---|
| State | New |
| Headers | show |
On Mon, Oct 20, 2014 at 3:49 AM, Catalin Marinas <catalin.marinas@arm.com> wrote: > > Since you mention symmetry, something like below makes the barriers more > explicit. Borken, for two reasons: > 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 @@ > 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(); > } So the thing is, this means that we can't take advantage of the fact that "atomic_inc" is already an atomic. So this is just a performance breakage. But: > > 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); This is just entirely broken. "atomic_read()" isn't really an "atomic op" at all. despite the name, it's just a read that is basically ACCESS_ONCE. So smp_mb__before_atomic() doesn't work for atomic_read(), and the code is nonsensical and doesn't work. It would need to be a full memory barrier. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
On Mon, Oct 20, 2014 at 04:32:00PM +0100, Linus Torvalds wrote: > On Mon, Oct 20, 2014 at 3:49 AM, Catalin Marinas > <catalin.marinas@arm.com> wrote: > > Since you mention symmetry, something like below makes the barriers more > > explicit. > > Borken, for two reasons: > > > 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 @@ > > 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(); > > } > > So the thing is, this means that we can't take advantage of the fact > that "atomic_inc" is already an atomic. So this is just a performance > breakage. But: OK, I looked at this from an ARM perspective only and it would not make any difference. But it seems that MIPS makes a distinction between "before" and "after" barriers with "before" defined as wmb in some configuration, so the hunk below would break it. > > 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); > > This is just entirely broken. > > "atomic_read()" isn't really an "atomic op" at all. despite the name, > it's just a read that is basically ACCESS_ONCE. > > So smp_mb__before_atomic() doesn't work for atomic_read(), and the > code is nonsensical and doesn't work. It would need to be a full > memory barrier. Looking at the semantics of smp_mb__*_atomic(), it would indeed have to be a full smp_mb() here.
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);