On Sat, Jun 05, 2021 at 10:57:39AM -0400, Alan Stern wrote:

On Fri, Jun 04, 2021 at 03:19:11PM -0700, Linus Torvalds wrote:

quoted

Now, part of this is that I do think that in *general* we should never
use this very suble load-cond-store pattern to begin with. We should
strive to use more smp_load_acquire() and smp_store_release() if we
care about ordering of accesses. They are typically cheap enough, and
if there's much of an ordering issue, they are the right things to do.

I think the whole "load-to-store ordering" subtle non-ordered case is
for very very special cases, when you literally don't have a general
memory ordering, you just have an ordering for *one* very particular
access. Like some of the very magical code in the rw-semaphore case,
or that smp_cond_load_acquire().

IOW, I would expect that we have a handful of uses of this thing. And
none of them have that "the conditional store is the same on both
sides" pattern, afaik.

And immediately when the conditional store is different, you end up
having a dependency on it that orders it.

But I guess I can accept the above made-up example as an "argument",
even though I feel it is entirely irrelevant to the actual issues and
uses we have.

Indeed, the expansion of the currently proposed version of

	volatile_if (A) {
		B;
	} else {
		C;
	}

is basically the same as

	if (A) {
		barrier();
		B;
	} else {
		barrier();
		C;
	}

which is just about as easy to write by hand.  (For some reason my 
fingers don't like typing "volatile_"; the letters tend to get 
scrambled.)

So given that:

	1. Reliance on control dependencies is uncommon in the kernel,
	   and

	2. The loads in A could just be replaced with load_acquires
	   at a low penalty (or store-releases could go into B and C),

it seems that we may not need volatile_if at all!  The only real reason 
for having it in the first place was to avoid the penalty of 
load-acquire on architectures where it has a significant cost, when the 
control dependency would provide the necessary ordering for free.  Such 
architectures are getting less and less common.

That does sound good, but...

Current compilers beg to differ at -O2: https://godbolt.org/z/5K55Gardn

------------------------------------------------------------------------
#define READ_ONCE(x) (*(volatile typeof(x) *)&(x))
#define WRITE_ONCE(x, val) (READ_ONCE(x) = (val))
#define barrier() __asm__ __volatile__("": : :"memory")

int x, y;

int main(int argc, char *argv[])
{
    if (READ_ONCE(x)) {
        barrier();
        WRITE_ONCE(y, 1);
    } else {
        barrier();
        WRITE_ONCE(y, 1);
    }
    return 0;
}
------------------------------------------------------------------------

Both gcc and clang generate a load followed by a store, with no branch.
ARM gets the same results from both compilers.

As Linus suggested, removing one (but not both!) invocations of barrier()
does cause a branch to be emitted, so maybe that is a way forward.
Assuming it is more than just dumb luck, anyway.  :-/

							Thanx, Paul