Skip to content
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

Notify uses SeqCst orderings, but acquire/release is enough #6266

Open
wang384670111 opened this issue Jan 4, 2024 · 6 comments
Open

Notify uses SeqCst orderings, but acquire/release is enough #6266

wang384670111 opened this issue Jan 4, 2024 · 6 comments
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync

Comments

@wang384670111
Copy link

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:

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

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

self.state.store(new_state, SeqCst);

curr = self.state.load(SeqCst);

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

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

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

(happy to make a PR if this looks reasonable)

@wang384670111 wang384670111 added A-tokio Area: The main tokio crate C-bug Category: This is a bug. labels Jan 4, 2024
@Darksonn Darksonn added the M-sync Module: tokio/sync label Jan 4, 2024
@Darksonn Darksonn changed the title Potential to modify ordering for state in notify model Notify uses SeqCst orderings, but acquire/release is enough Jan 4, 2024
@Darksonn
Copy link
Contributor

Darksonn commented Jan 4, 2024

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

@wang384670111
Copy link
Author

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
Copy link
Contributor

Darksonn commented Jan 4, 2024

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

@wang384670111
Copy link
Author

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
Copy link
Contributor

Darksonn commented Jan 4, 2024

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

@wang384670111
Copy link
Author

#6268

kezhuw added a commit to kezhuw/tokio that referenced this issue May 16, 2024
kezhuw added a commit to kezhuw/tokio that referenced this issue May 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync
Projects
None yet
Development

No branches or pull requests

2 participants