Notify uses SeqCst orderings, but acquire/release is enough

[wang384670111]

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：

tokio/tokio/src/sync/notify.rs

Line 570 in 46ff363

let res = self.state.compare_exchange(curr, new, SeqCst, SeqCst);

tokio/tokio/src/sync/notify.rs

Lines 932 to 937 in 46ff363

	let res = notify.state.compare_exchange(
	set_state(curr, EMPTY),
	set_state(curr, WAITING),
	SeqCst,
	SeqCst,
	);

tokio/tokio/src/sync/notify.rs

Line 643 in 46ff363

self.state.store(new_state, SeqCst);

tokio/tokio/src/sync/notify.rs

Line 586 in 46ff363

curr = self.state.load(SeqCst);

tokio/tokio/src/sync/notify.rs

Lines 949 to 954 in 46ff363

	let res = notify.state.compare_exchange(
	set_state(curr, NOTIFIED),
	set_state(curr, EMPTY),
	SeqCst,
	SeqCst,
	);

tokio/tokio/src/sync/notify.rs

Lines 896 to 901 in 46ff363

	let res = notify.state.compare_exchange(
	set_state(curr, NOTIFIED),
	set_state(curr, EMPTY),
	SeqCst,
	SeqCst,
	);

tokio/tokio/src/sync/notify.rs

Line 1037 in 46ff363

let curr = notify.state.load(SeqCst);

(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]

#6268