New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make AcqRel universally usable as ordering mode #2503

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
@RalfJung
Member

RalfJung commented Jul 19, 2018

Rendered

@TimNN

This comment has been minimized.

Contributor

TimNN commented Jul 19, 2018

👍 For this RFC. I know that I have in the past used AcqRel with load/store and been annoyed at the resultant runtime panic.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 19, 2018

I don't think this is a good idea: AcqRel has different semantics from just Rel or Acq on their own, even if you throw away the result, eg.

let _ = atomic.swap(new_value, AcqRel);

Has different characteristics than:

atomic.store(new_value, Rel);

I think allowing AcqRel in the second example would give the false impression that they are equivalent.

@nagisa

This comment has been minimized.

Contributor

nagisa commented Jul 19, 2018

@Diggsey I’m not seeing your point. Those are two entirely distinct operations, regardless of what ordering you supply them. atomic.swap(new_value, Rel) is not the same as atomic.store(new_value, Rel) either.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 19, 2018

AcqRel has different semantics from just Rel or Acq on their own, even if you throw away the result, eg.

AFAIK it does not -- it literally means "this is a read-modify-write operation; the read part is Acquire and the write part is Release". What do you mean?

EDIT: Ah, I see what you mean. However, I feel the fact that it says swap still makes it clear that this is not just a store. Consistent with your argument, one could say that let _ = atomic.load(Acq); should be a NOP but it is not.

@le-jzr

This comment has been minimized.

le-jzr commented Jul 19, 2018

No, @Diggsey is correct.
AcqRel is defined to synchronize with both Acq and Rel, regardless of what the operation itself does. This might not seem overly relevant if AcqRel is not normally allowed by the operation, but redefining the ordering to mean something slightly different than the established standard is very, very wrong. I have an experience with slightly off-standard implementations, they cause horrible messes for many years to come.

Another thing to consider is the consistency with SeqCst. If AcqRel weakens its definition depending on operation, why doesn't SeqCst? Messy.

If you want to solve the issue with not all combinations being legal or making sense, a vastly superior solution would be to check it statically at compile time, or to trap the "noop" choices at runtime.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 19, 2018

AcqRel is defined to synchronize with both Acq and Rel, regardless of what the operation itself does.

I am pretty sure that is not correct. A compare_and_swap(..., AcqRel) will not be Rel if it only reads because the comparison fails.

EDIT: In fact, you can see this in the code:

                pub fn compare_and_swap(&self,
                                        current: $int_type,
                                        new: $int_type,
                                        order: Ordering) -> $int_type {
                    match self.compare_exchange(current,
                                                new,
                                                order,
                                                strongest_failure_ordering(order)) {
                        Ok(x) => x,
                        Err(x) => x,
                    }
                }

fn strongest_failure_ordering(order: Ordering) -> Ordering {
    match order {
        Release => Relaxed,
        Relaxed => Relaxed,
        SeqCst => SeqCst,
        Acquire => Acquire,
        AcqRel => Acquire,
        __Nonexhaustive => __Nonexhaustive,
    }
}

So, compare_and_swap(..., AcqRel) is the same as compare_exchange(..., AcqRel, Acquire). The failure mode is just Acquire.

@le-jzr

This comment has been minimized.

le-jzr commented Jul 19, 2018

Sorry, my bad, you're right. I'm used to the GCC builtins that have separate success and failure orderings, where this case never comes up, so my understanding has been incomplete.
Still, if the concern is risk of accidentally using Rel for read or Acq for write, just disallow those cases the same way AcqRel is disallowed. Allowing more invalid orderings and actually promoting their use to avoid bugs sounds pretty backwards to me.

EDIT: After rereading the motivation, I see that you're actually concerned with read-modify-write with accidentally weaker ordering than required. To that I would argue that if you're explicitly weakening the ordering from SeqCst, you already need to be reasoning properly about how the ordering affects the semantics of your code. Tbh I have never seen a case where AcqRel would be incorrect if SeqCst is correct, but that doesn't mean there aren't any, and to promote blindly stuffing AcqRel everywhere without thinking instead of SeqCst is playing with with fire. Such a developer shouldn't mess around with atomics in the first place, no point enabling even more half-assery in that area.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 19, 2018

I am pretty sure that is not correct. A compare_and_swap(..., AcqRel) will not be Rel if it only reads because the comparison fails.

But if it succeeds it will give that ordering. Using AcqRel with a load will never give you that ordering.

TBH, I'm not a fan of the "single argument" version of compare-and-swap to begin with, as I think for this kind of code, the explicitness of the two-argument version is always preferable.

I really don't see the case for making a situation that is both rare (most programs should probably not be using atomics directly) and incredibly error prone (not sure I need to argue this point) less explicit.

It's not like you can just stick AcqRel everywhere and be done with it - you still need to actually think about the memory ordering, so why not specify the actual memory ordering that you want?

@Centril Centril added the T-libs label Jul 19, 2018

@jeehoonkang

This comment has been minimized.

jeehoonkang commented Jul 19, 2018

I don't agree with this proposal for another reason. In my vocabulary, "release" means that the order between the previous instructions and the current release instruction should be preserved; and "acquire" means that the order between the current acquire instruction and later instructions should be preserved. So to me, a "release load", if exists, should mean that the instructions before it should be ordered before the release load. Similarly for "acquire store". Probably that's the reason why release load and acquire store cause panic at run time: there's no way to enforce those orderings, so we bail out.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Jul 19, 2018

I'd prefer if the compiler simply emitted warnings when an invalid ordering is passed to an atomic operation (i.e. it would panic).

@pssalmeida

This comment has been minimized.

pssalmeida commented Jul 19, 2018

From the RFC:

Other languages (notably C and C++) do not permit using the combined acquire-release mode for pure load/store operations. By allowing this in Rust, we might surprise people that expect an exact equivalent of the C/C++ API.

I was trying to find out about this. The general feeling seems to be that an acquire-release with pure load/stores is not allowed in C++. But is it really the case? Looking at:

http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/n4713.pdf

page 1202, 1.2:

memory_order::release, memory_order::acq_rel, and memory_order::seq_cst: a store operation
performs a release operation on the affected memory location.

And similarly for loads. So they should be allowed, but have only either the release or acquire effect.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 19, 2018

@pssalmeida to quote from that paper you linked:

void store(T desired, memory_order order = memory_order::seq_cst) volatile noexcept;
void store(T desired, memory_order order = memory_order::seq_cst) noexcept;
7 Requires: The order argument shall not be memory_order::consume, memory_order::acquire, nor
memory_order::acq_rel.
8 Effects: Atomically replaces the value pointed to by this with the value of desired. Memory is
affected according to the value of order.

And similarly for load.

I'm not sure what "page 1202, 1.2:" is referring to, there's nothing relevant on that page.

@pssalmeida

This comment has been minimized.

pssalmeida commented Jul 19, 2018

@Diggsey well, that seems to clarify that it is indeed forbidden. But the point 1.2 in page 1202 that I quoted above, if not contradictory (according to some definition of "store" which I have not bothered to look-up), it is misleading to say the least. It seems to imply that those orderings (release, acq_rel, and seq_cst) are all valid for a store, and they perform a release operation. I.e., just as seq_cst performs a release for a store, so would acq_rel.

But this was just about that concern regarding C++. Regarding Rust, we could do things differently, but I was not advocating it. Given that these things are incredibly tricky, and that using AcqRel in a pure load/store may be misleading to people reading code, I think the more explicit the better. So, maybe it is better to stick to what we have, have panic in those cases, and force people to consider each ordering at each place, instead of going for AcqRel everywhere as a default with no much thought.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 19, 2018

@jeehoonkang I do not follow. I am not proposing to allow "Release loads". I am proposing to provide a way to say "give me the release/acquire variant of this operation, please" without having to think about whether this is a load, store or RMW (read-modify-write). So I don't understand how your comment relates to the proposal.

@le-jzr

To that I would argue that if you're explicitly weakening the ordering from SeqCst, you already need to be reasoning properly about how the ordering affects the semantics of your code.

Actually, my own thinking is the other way around -- release/acquire is what you need to do ownership transfer. If you are explicitly strengthening the ordering to SeqCst, I am curious if that is really needed. Concurrent code needs manual care indeed, but SeqCst is only actually required by some really subtle algorithms. However, programming with release/acquire is needlessly complicated because one has to write three different things when all one wants is to say "make all loads acquire and all stores release, please".

@stjepang

I'd prefer if the compiler simply emitted warnings when an invalid ordering is passed to an atomic operation (i.e. it would panic).

That doesn't help when one accidentally writes Release in a compare_and_swap, and meant AcqRel.


So, I understand some folks here are opposed to the fact that AcqRel sometimes actually means "acquire and release", and sometimes it means "actually just Acquire/Release". I think that is already the case (with e.g. compare_and_swap), but that aside, this is just the mechanism that I suggested to solve the problem. And the problem is that currently, it is very error-prone to say "make all loads acquire and all stores release".

@pssalmeida, you argue that this is actually good because it is "explicit". I could find myself agreeing if getting things wrong would reliably be caught by the compiler, but that is not the case.

Case in point: When I wrote the original version of rust-lang/rust#52349, I was somewhat worried that I had accidentally used the wrong mode at some point. The irony is, I was fairly certain (as certain as I can get without a formal proof) that using release/acquire consistently is sufficient. I feel savvy enough with release/acquire that I could do what @le-jzr calls "reasoning properly about the code" without too much effort. Everything in sync::Once is just ownership transfer.
The only thing I was uncertain about is whether I actually used release/acquire consistently anywhere. I am saying "irony" because I was certain about what is hard (convincing yourself that the ordering modes are correct), bu uncertain about what should be easy (whether I actually used the ordering modes I wanted to use).
That should never be the case in a programming language: Once I know my intent, I should have a clear way to express said intent.

People are wary of using orderings other than SeqCst, and that is fair. However, I don't think it is fair to deliberately make it harder than necessary to use the weaker modes. That is the only way I can describe the current API. As someone who has a pretty solid intuition for how to work with release/acquire concurrency, I feel I should still be able to have the language support me in writing code that consistently uses release/acquire. Right now, the opposite is the case.

So, can we get agreement that there is a problem here that is worth solving?


If we can get that, I think there might be other ways to solve it than repurposing AcqRel (though I still think that this is a natural extension to AcqRel's current role). For example, we could add an entirely new element to the Ordering enum (name to be bikeshed) that has the proposed meaning of "make all loads acquire and all stores release". In this proposal, there is no risk of confusing that with the more precise AcqRel which means "the read is acquire and if a write happens, that will be release".

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 19, 2018

@RalfJung I would 100% be in support of any way to enforce valid uses of memory orderings at compile time, I'm only opposed to the specific solution proposed here of changing the meaning of those orderings.

Some ideas:

  • Attribute which could be placed on function arguments to restrict which variants of an enum are allowed. Similar in nature to #[must_use].

  • Use unit structs instead of enum variants: the atomic functions would be generic (load<T: LoadOrdering>(&self, ordering: T)) where LoadOrdering is implemented for Acquire, Relaxed and SeqCst unit structs. For backwards compatibility, it can also be implemented for the Ordering enum, but that enum would become deprecated.

  • Make AcqRel work for loads/stores, but make it actually have AcqRel semantics. Loads with this ordering would not be able to be re-ordered before stores with Release ordering, and likewise for stores. I've definitely had cases where this would be useful - maybe there's a good reason why memory orderings are tied to loads vs stores, but I've never seen a good explanation of why. I assume it's just because of the way the hardware works for common CPU architectures, and because it's far more common to want Acquire semantics for loads and Release for stores?

@parched

This comment has been minimized.

parched commented Jul 20, 2018

IMO the thing that actually needs doing here is correctly the docs for compare_and_swap saying the ordering specified is for when it succeeds, when it fails it is only a release if SeqCst is specified.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 20, 2018

@parched that's not correct, compare_and_swap will always use SeqCst ordering if you specify that.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 20, 2018

@Diggsey

I would 100% be in support of any way to enforce valid uses of memory orderings at compile time, I'm only opposed to the specific solution proposed here of changing the meaning of those orderings.

The problem is not just whether the ordering is valid for the operation. Release is a valid ordering for compare_and_swap. However, it actually makes the load part of this a relaxed load! That's super-dangerous, as far as I am concerned. I want to never do this mistake. With this proposal implemented, I would just always write AcqRel for all operations, and would be sure not to make this mistake.

Your first two proposed solutions fail to address this case. The last would address it, but it doesn't actually express the intent I would like to express: I want to say "make all loads acquire and all stores release". I have reasonable intuition for what my program will and will not do if this is the case. There is no reason to make a load have release-semantics.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 20, 2018

The problem is not just whether the ordering is valid for the operation. Release is a valid ordering for compare_and_swap. However, it actually makes the load part of this a relaxed load! That's super-dangerous, as far as I am concerned. I want to never do this mistake. With this proposal implemented, I would just always write AcqRel for all operations, and would be sure not to make this mistake.

This just seems to mirror my complaint against the single argument version of the function existing at all - I agree it's dangerous, even with AcqRel I think it's dangerous. I'd much rather you always had to specify both orderings.

There is no reason to make a load have release-semantics.

But this is my point: there are reasons to want loads to have release semantics, or a store to have acquire semantics. For example, this pseudocode:

# Thread 1
- Lock mutex (compare_and_swap with AcqRel)
- <do stuff>
- Unlock mutex (store with Release)
- Wait for flag to be set (load with Acquire)

# Thread 2
- Lock mutex
- Set flag
- Unlock mutex

This code is broken because the load in thread 1 can be reordered inside the mutex from the perspective of other threads, and this can result in a deadlock. This is an issue I've encountered in real code. To fix it you need the "mutex unlock" to have Acquire ordering, or the "wait for flag" to have Release ordering.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 20, 2018

This just seems to mirror my complaint against the single argument version of the function existing at all - I agree it's dangerous, even with AcqRel I think it's dangerous. I'd much rather you always had to specify both orderings.

No, this is not about the single-argument form. compare_exchange has the same problem: compare_exchange(..., Acquire, Acquire) will make the write (if it happens) a relaxed write.

We'd need a three-argument form that gives the mode for all of success read, success write, fail read separately. Though I think having a way to just say "release or acquire, whatever applies" is simpler and easier to both read and write.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 20, 2018

No, this is not about the single-argument form. compare_exchange has the same problem: compare_exchange(..., Acquire, Acquire) will make the write (if it happens) a relaxed write.

That doesn't make sense? The operation is atomic, there are not separate ordering for the read vs the write, there are just separate orderings for whether the operation succeeds or fails. If you specify Acquire, then both the read and write parts have Acquire semantics. There is no Relaxed operation because it's one atomic operation.

@stjepang

This comment has been minimized.

Contributor

stjepang commented Jul 20, 2018

@Diggsey This is going to be just about terminology:

From my understanding, saying "an acquire write" or "a release read" is non-sensical (it doesn't mean anything). Write operations can only be relaxed/release/seqcst, while read operations can only be relaxed/acquire/seqcst.

CAS/CMPXCHG operations on failure perform just a read (can be relaxed/acquire/seqcst), while on success atomically perform both a read (can be relaxed/acquire/seqcst) and a write (can be relaxed/release/seqcst).

In particular, if the success case in a CMPXCHG has acqrel ordering, that means the read part has acquire ordering and the write part has release ordering. And if the success case is marked with acquire, that means the read part has acquire ordering and the write part has relaxed ordering. There are two parts to the CMPXCHG operation, but they both happen atomically at the same time.

What you mean by "an acquire write" is probabaly an acquire swap operation. And "a release load" would have to be a CAS loop because we need an actual write operation in order to obtain release ordering:

let mut val = a.load(Relaxed);
loop {
    let actual = a.compare_and_swap(val, val, Release);
    if actual == val {
        break;
    } else {
        val = actual;
    }
}

@jeehoonkang ^ Did I get this right?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 20, 2018

@stjepang from my point of view, the lack of "an acquire write" or "a release read" is an architecture- specific curiosity. It's easy to define semantics for those hypothetical operations.

Just look at how load is implemented on x86: in actuality, all memory orderings are equivalent here, they all translate into a single "mov" instruction (putting aside compiler transformations for the moment). The fact is that the C11 memory model implements a super-set of the functionality actually supported on any given architecture.

The reason this works is that the compiler is only required to generate code with at least the specified ordering guarantees, it is free to be more restricted than necessary.

If we're going to extend load/store to accept AcqRel (which I'm not arguing we should do necessarily) then these are the semantics I would expect, and on x86, stores with AcqRel would translate to "xchg" instructions, because that is the minimum required to uphold the specified ordering.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 20, 2018

@Diggsey

This code is broken because the load in thread 1 can be reordered inside the mutex from the perspective of other threads, and this can result in a deadlock.

Interesting. Lifeness guarantees in weak memory concurrency are still a very active area of research, and maybe this is why. However, I don't feel like the solution is to add extra ordering constraints here. The solution is to declare this (moving a release write down below an acquire read of a different location) a compiler bug.

This RFC dos not propose to add any new operations, or change the semantics of existing operations. I think that's a much bigger change. All I am asking for is a good way to write code using release-acquire concurrency. I don't think the problem you are mentioning is related to the problem I am trying to solve.

That doesn't make sense? The operation is atomic, there are not separate ordering for the read vs the write, there are just separate orderings for whether the operation succeeds or fails. If you specify Acquire, then both the read and write parts have Acquire semantics. There is no Relaxed operation because it's one atomic operation.

That's not correct. A read-modify-write operation has a "read end" and a "write end". The "read end" is relevant to answer "does this operation synchronize with the earlier store that is being read here", and the "write end" is relevant to answer "will a future operation that reads from this write synchronize with this operation". That is why the "success" order can be Relaxed, Release, Acquire and AcqRel, to cover the 4 possible combinations. The formal model that was used to find rust-lang/rust#51780 explicitly has three modes for its CAS.

The "write part" of an operation never has Acquire semantics. Acquire comes into play when looking at all reads-from edges (connecting a load to the store it reads from) -- if the load is Acquire (or SeqCst) and the store is Release (or SeqCst), then the two events synchronize. This is the only effect release/acquire ever has. There is no place in the standard where a load has a "release" effect, or a store has "acquire" effect.
Read-modify-write (RMW) operations have both an outgoing reads-from edge (to the store they read from) where whether or not the RMW is Acquire is relevant, and (possibly) any number of incoming reads-from edges (from other loads that load the value written here) where whether or not the RMW is Release is relevant.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 20, 2018

@stjepang

Did I get this right?

I think you did.

@Diggsey

from my point of view, the lack of "an acquire write" or "a release read" is an architecture- specific curiosity. It's easy to define semantics for those hypothetical operations.

No, it is not easy. I invite you to try. Notice that we are talking about a programming language model here, not about a CPU architecture model. On the CPU, an acquire load translates to a fence and a load (at least on ARM), but in the language, an acquire load ins NOT equivalent to a fence and a load. As I discussed above, release/acquire are used to turn reads-from edges into synchronizes-with edges. Both ends of the reads-from edge have to "opt-in" to doing this. When the read end of the edge opts in, we call it an acquire read. When the write end of the egde opts in, we call it a release write. It just makes no sense at all to talk about acquire writes or release reads.

Just look at how load is implemented on x86

This is a very bad argument. Not only (as I just mentioned above) can you not conflate language-level and CPU-level memory models, also using x86 as example is a bad idea because (as you mentioned) every operation in x86 is (strictly stronger than) release/acquire. (Not all orderings are equivalent though, SeqCst is still different from the rest.) The architecture is so strong as to make the distinction between release/acquire and relaxed accesses meaningless. So, how do you want to explain anything by looking at an architecture that does not make these distinctions?

However, the same distinction remains meaningful when compiling x86 code because LLVM can and will optimize your code based on these ordering modes before translating it to the target architecture.

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Jul 20, 2018

The solution is to declare this (moving a release write down below an acquire read of a different location) a compiler bug.

Can you explain why you think this, I'm genuinely interested if I'm missing something here? As far as I can tell this is working exactly as the memory orderings are defined?

No, it is not easy. I invite you to try. Notice that we are talking about a programming language model here, not about a CPU architecture model.

You kind of defined it for me: the compiler/CPU moving an acquire write down below any other atomic operation is forbidden. Normally there would be another half to this about when the store is visible to other threads: an Acquire store simply does not have this component beyond the basic requirements of an atomic store.

This is a very bad argument. Not only (as I just mentioned above) can you not conflate language-level and CPU-level memory models,

I'm not conflating them - I'm arguing that this distinction is important: that just because an operation doesn't exist in the CPU-level model, doesn't prevent it from being added to a language-level model.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Jul 20, 2018

Can you explain why you think this, I'm genuinely interested if I'm missing something here? As far as I can tell this is working exactly as the memory orderings are defined?

It's a compiler transformation that changes a program from "working" to "deadlocking". Your program has no undefined behavior. I don't see anything that gives the compiler the license to do that.

The burden of proof for a reordering to be legal is on the compiler: It most show that doing this reordering will not break programs.

You kind of defined it for me: the compiler/CPU moving an acquire write down below any other atomic operation is forbidden.

That's not at all how these memory models are defined. The set of legal transformations is a consequence of how the model is defined.

A model defines, given a program, which behaviors are possible. "Can this function return 0?" is the kind of question that a model answers. Once the model is defined, one can then try and prove that a certain transformation is correct, by showing that the new program (after the transformation) will not do anything that the old program (before the transformation) did not also possibly do.

For example:

static DATA : AtomicUsize = AtomicUsize::new(0);
static FLAG : AtomicUsize = AtomicUsize::new(0);

fn main() {
  rayon::join(
    || {
      // Initialize data
      DATA.store(1, Ordering::Relaxed);
      // Signal that we are done
      FLAG.store(1, Ordering::Release); },
    || {
      // Wait until the other thread is done
      while FLAG.load(Ordering::Acquire) == 0 {}
      // Print data
      println!("{}", DATA.load(Ordering::Relaxed));
    }
  );
}

The memory model defines that the only possible output of this program is 1. If any of the two accesses to FLAG would be Relaxed, then 0 would also become a legal output according to the model. That is not defined in terms of which operations can be reordered, but in terms of which values any given load may return.
Above, the DATA.store happens-before FLAG.store happens-before the successful FLAG.load happens-before DATA.load, which is why it must print 1. If any of these happens-before edges was missing, it could print either 0 or 1. In general, a load can read from any store, but then there are restrictions placed upon that, and one of the restrictions is that if you want to load from store B, but then there is also store C such that B happens-before C happens-before the load, then you must not use B. (IOW, you must use a store that is "last" in the part of the happens-before order that is before the current load. There could still be multiple such loads though.) In our example, when looking at the final DATA.load, B is the initial store of 0, and C is the later store of 1.

I already explained above how, when load A reads-from store B, that may or may not give rise to a synchronizes-with edge between these two events. This synchronizes-with edge then gives rise to a happens-before relationship, which has effects on other events. There just is logically no place in this for an "acquire write". "acquire" is the property of the load end of this-load-reads-from-that-store that makes them synchronize.
"acquire write" is like saying, uh, round green. "round" is just not a property that green can have.

(This is also getting off-topic because I do not want to define a new memory model in this RFC, just change the interface to talk with an existing memory model.)

@RalfJung

This comment has been minimized.

Member

RalfJung commented Aug 6, 2018

I have updated the RFC to propose AutoAcqOrRel instead. I still think the original proposal would have been better (e.g. by also fixing the compare_and_swap inconsistency), but well, it seems I could not convince many others of that standpoint. ;)

Sorry @nagisa... I tried.^^

@RalfJung RalfJung referenced this pull request Aug 6, 2018

Merged

atomic ordering docs #53106

@RalfJung

This comment has been minimized.

Member

RalfJung commented Aug 6, 2018

I proposed an improvement for our Ordering docs at rust-lang/rust#53106

@comex

This comment has been minimized.

comex commented Aug 7, 2018

:/ I think people will be confused about the difference between AutoAcqOrRel and AcqRel. I like the lint idea better.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Aug 7, 2018

Well, yeah, AcqRel is confusing and encourages (IMHO) a wrong way to think about release-acquire concurrency -- but people didn't like my suggestion for improving the situation there so what can you do. ;)

I don't think a lint can reasonably fix a suboptimal API. I am also not aware of any lint we have that is even remotely as specific and convoluted as the one proposed here.

kennytm added a commit to kennytm/rust that referenced this pull request Aug 9, 2018

Rollup merge of rust-lang#53106 - RalfJung:ordering, r=stjepang
atomic ordering docs

Discussion in rust-lang/rfcs#2503 revealed that this could be improved. I hope this helps.

kennytm added a commit to kennytm/rust that referenced this pull request Aug 9, 2018

Rollup merge of rust-lang#53106 - RalfJung:ordering, r=stjepang
atomic ordering docs

Discussion in rust-lang/rfcs#2503 revealed that this could be improved. I hope this helps.
@parched

This comment has been minimized.

parched commented Oct 4, 2018

Slightly related: I stumbled upon this code in LLVM the other day. Basically, LLVM will convert a fetch-add-of-zero-release to a load-release (fence+load). So maybe one day Rust will want to support load-release explicitly (and store-acquire which currently you do by swap-acquire discarding the read value, AFAIK there is no LLVM optimisation converting it to store+fence for this currently).

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 5, 2018

@parched On a related note, see this writeup on seqlocks.

But the common theme for these I think is that the underlying concurrency memory model is not expressive enough. Some people are hence proposing entirely different APIs.

But all of that is not very related to this RFC, which is just about a slight change to the API we use to talk with the current memory model.

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Oct 12, 2018

Adding an enum variant would be a breaking change. This simply can't be done; using AcqRel would have to be sufficient.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Oct 12, 2018

Adding an enum variant would be a breaking change.

No it would not. The enum is marked as non_exhaustive for a reason.

@clarcharr

This comment has been minimized.

Contributor

clarcharr commented Oct 12, 2018

Oh, I completely missed that it was marked as non_exhaustive. Carry on.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 1, 2018

This seems like a neat feature, but atomic orderings have almost always been explained in Rust as "it's the exact same as C++" because it's such a complicated topic that we don't want to add any extra complication to the story. While this new ordering can be explained in terms of other orderings, I fear that it'd be a roadblock in an already complicated story, and I'm not sure that it justifies its weight.

The main point against this to me is that it's not present in any other language, and we don't really want to be trailblazers in the area of atomic orderings. Is there a point in favor, however, of why we should include this when it's not included anywhere else (and maybe not on track to be included anywhere else?)

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

So basically you are saying "let's stick with a sub-par API because it is what everyone else uses"?

I don't have much to say to that, except that that makes it really hard to improve the situation. :( You said yourself that it is such a complicated topic, yet at the same time you are giving an argument to thwart any attempt to improve that situation.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 2, 2018

That's what I'm saying bluntly, yes, but I think it misses the rationale for why I'm saying it. Diverging from a model which we have stated that we match exactly means confusion for Rust users as they have to figure out something new that's not already in the model and also runs the risk of clashing with future extensions to the model. It does not seem worth the increase in convenience to run this risk for an extremely core part of Rust.

@RalfJung

This comment has been minimized.

Member

RalfJung commented Nov 2, 2018

It's not really diverging from the model, it is just a slight extension of the API we use to communicate with the model -- an extension that can be easily translated down to the C++ model (and even the C++ API), but the translation is error-prone, hence the suggestions to automate it.

But I guess you are not actually talking about just the model, but the API as well. And I get the argument, I just don't agree with the weights you are assigning to the different aspects here. Fair enough.

@alexcrichton

This comment has been minimized.

Member

alexcrichton commented Nov 2, 2018

Ah yes that's true, I'm considering the model and the API one and the same because they have a 1:1 correspondance today (roughly at least). I think it's definitely a good thing this proposal is purely sugar, but I don't think such sugar belongs in the standard library personally at least.

I'm curious to hear what others on @rust-lang/libs think, though, and the best way to figure that out is probably...

@rfcbot fcp close

@rfcbot

This comment has been minimized.

rfcbot commented Nov 2, 2018

Team member @alexcrichton has proposed to close this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once a majority of reviewers approve (and none object), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 2, 2018

Would it be possible at all to do this experimentally as an eRFC?

@Diggsey

This comment has been minimized.

Contributor

Diggsey commented Nov 2, 2018

What would be the value in an eRFC? I don't think there's any uncertainty about what the proposed change would look like in practice, there's just a fair amount of disagreement with it, on a number of different points. An eRFC is not supposed to be a way to just sneak features in with less scrutiny.

I'd like to see an RFC for some kind of static analysis instead. If there's a mistake that is too easy to make (eg. using Acq where AcqRel was meant) then IMO we should try to flag it up rather than deviate from the standard, especially in such a niche area.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 3, 2018

An eRFC is not supposed to be a way to just sneak features in with less scrutiny.

I don't mean it that way. It would be a way to get it into the hands of users who could give feedback of whether it is confusing or not.

@sfackler

This comment has been minimized.

Member

sfackler commented Nov 3, 2018

That's exactly what the existing stability system is.

@mark-i-m

This comment has been minimized.

Contributor

mark-i-m commented Nov 3, 2018

Except that the existing system doesn't allow the degree of experimentation where we implement features just to see what they are like... with no implications for what the final decision will be...

@matthieu-m

This comment has been minimized.

matthieu-m commented Nov 3, 2018

I'd like to see an RFC for some kind of static analysis instead. If there's a mistake that is too easy to make (eg. using Acq where AcqRel was meant) then IMO we should try to flag it up rather than deviate from the standard, especially in such a niche area.

A coworker of mine recently ran the Relacy Race Detector on our C++ codebase and it did highlight a number of issues in a couple wait-free algorithms (which were benign on x86, but formally undefined).

It's a bit awkward to use, in C++, due to the heavy degree of modification of the source code necessary but I think that Rust could offer a much superior interface based on procedural macros.

In any case, I heavily encourage all interested parties to have a look. It's all the more interesting that only does it boast a "no false-positive" property, but it also produces detailed traces of "wrong" executions to help visualize the issue with the faulty orderings, teaching the user at the same time it fixes their mistake, much as rustc diagnostics.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment