Messages in this thread | | | Date | Mon, 2 Nov 2015 10:08:24 -0800 | Subject | Re: [PATCH 4/4] locking: Introduce smp_cond_acquire() | From | Linus Torvalds <> |
| |
On Mon, Nov 2, 2015 at 5:29 AM, Peter Zijlstra <peterz@infradead.org> wrote: > +#define smp_cond_acquire(cond) do { \ > + while (!(cond)) \ > + cpu_relax(); \ > + smp_read_barrier_depends(); /* ctrl */ \ > + smp_rmb(); /* ctrl + rmb := acquire */ \ > +} while (0)
This code makes absolutely no sense.
smp_read_barrier_depends() is about a memory barrier where there is a data dependency between two accesses. The "depends" is very much about the data dependency, and very much about *nothing* else.
Your comment talks about control dependencies, but smp_read_barrier_depends() has absolutely nothing to do with a control dependency. In fact, it is explicitly a no-op on architectures like ARM and PowerPC that violate control dependencies.
So the code may end up *working*, but the comments in it are misleading, insane, and nonsensical. And I think the code is actively wrong (although in the sense that it has a barrier too *many*, not lacking one).
Because smp_read_barrier_depends() definitely has *nothing* to do with control barriers in any way, shape or form. The comment is actively and entirely wrong.
As far as I know, even on alpha that 'smp_read_barrier_depends()' is unnecessary. Not because of any barrier semantics (whether smp_read_barrier depends or any other barrier), but simply because a store cannot finalize before the conditional has been resolved, which means that the read must have been done.
So for alpha, you end up depending on the exact same logic that you depend on for every other architecture, since there is no "architectural" memory ordering guarantee of it anywhere else either (ok, so x86 has the explicit "memory ordering is causal" guarantee, so x86 does kind of make that conditional ordering explicit).
Adding that "smp_read_barrier_depends()" doesn't add anything to the whole ctrl argument.
So the code looks insane to me. On everything but alpha, "smp_read_barrier_depends()" is a no-op. And on alpha, a combination of "smp_read_barrier_depends()" and "smp_rmb()" is nonsensical. So in no case can that code make sense, as far as I can tell.
Linus
| |