Notify uses SeqCst orderings, but acquire/release is enough

[wang384670111]

trafficstars

I developed a static analysis tool to detect issues related to memory ordering, thread performance, and security. During my examination of several crates, I encountered alarms triggered by the following code：

     "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:570:34: 570:77"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Acquire is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:932:56: 937:34"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Release is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:643:20: 643:44"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Release is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:586:27: 586:39"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Acquire is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:949:56: 954:34"
      },
      "explanation": "Using an atomic compare_exchange operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead, Using AcqRel and Acquire is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:896:44: 901:22"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Release is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:1037:45: 1037:57"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Acquire is sufficient to ensure the correctness of the program"
    }

I believe the ordering explained in the diagnosis is valid, particularly concerning the following atomic operations in the code： https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L570 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L932-L937 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L643 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L586 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L949-L954 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L896-L901 https://github.com/tokio-rs/tokio/blob/46ff36386d11e52faae038d5afe8d2f7a39dfe39/tokio/src/sync/notify.rs#L1037

(happy to make a PR if this looks reasonable)

[Darksonn]

There is precedent for this in #6018, so I guess I am okay with a PR.

[wang384670111]

There is precedent for this in #6018, so I guess I am okay with a PR.

Does the suggested ordering for the atomic operations seem reasonable? If it is, I'll proceed with submitting a pull request right away.

[Darksonn]

Acquire/release is probably correct in most places, but I'd have to look at the specific code to know for sure.

[wang384670111]

Acquire/release is probably correct in most places, but I'd have to look at the specific code to know for sure.

I'm slightly uncertain about the usage of the following three compare_exchange operations. Based on my detection pattern, it's possible that the code doesn't require the use of AcqRel.

     "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:570:34: 570:77"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Acquire is sufficient to ensure the correctness of the program"
    }
  },
  {
    "AtomicCorrelationViolation": {
      "bug_kind": "AtimicCorrelationViolation",
      "possibility": "Possibly",
      "diagnosis": {
        "atomic": "/Users/wangcheng/.cargo/registry/src/index.crates.io-6f17d22bba15001f/tokio-1.35.1/src/sync/notify.rs:932:56: 937:34"
      },
      "explanation": "Using an atomic operation with a stronger memory ordering than necessary can lead to unnecessary performance overhead. Using Release is sufficient to ensure the correctness of the program"
    }

Perhaps the ordering for these two operations should use AcqRel and Acquire? As for the other orderings, I believe they are reasonable as is😁.

[Darksonn]

You can add AcqRel for now and I will review whether it is necessary or not.

[wang384670111]

https://github.com/tokio-rs/tokio/pull/6268