Skip to content

Conversation

ADD-SP
Copy link
Member

@ADD-SP ADD-SP commented May 17, 2025

Blocked by #7339 which fixes the CI failures caused by the new compilation error message of the new stable toolchain.

Motivation

queue::Local<T> is never been send to another thread (except while shutting down), the tail is only updated by the worker thread who owned this local queue, so any change to tail is immediately visible to the current thread.

This means the Acquire ordering is not required for loading tail while operating on the local queue.

Solution

Unsync load

Since the lfence is not required for this scenario, the atomic loading was replaced by simple pointer deref.

Remove all methods of the queue::Inner<T>

Because of the following reasons:

  • All method on the Inner<T> can be implemented in differently way for Local<T> and Steal<T>
  • Adding method like len_local or unsync_len creates possibilities for incorrect usage in the future which causes ABA problems in critical path.

I removed all methods on the Inner<T> and re-implement it for both Local<T> and Steal<T>.

Remove the feature gate of Steal<T>::len()

The feature gate cfg_unstable_metrics is removed for Steal<T>::len() to avoid duplicating the same code.

This doesn't seem to break anything, but if it does please let me know and I'll switch to something else.

Anything else?

Ideally, the queue::Local<T> should be marked as !Send to lower the risk that unintentionally send it to another thread, but it required the changes of the starting and shutdown logic, I will try to open another PR for this improvement.

@github-actions github-actions bot added the R-loom-multi-thread Run loom multi-thread tests on this PR label May 17, 2025
@ADD-SP ADD-SP force-pushed the add_sp/runtime-eliminate-lfence branch from cea005b to ad6f9ea Compare May 17, 2025 08:02
@mox692
Copy link
Member

mox692 commented May 17, 2025

I just merged #7339, so merging master should fix the ci issue 👍

@Darksonn Darksonn added A-tokio Area: The main tokio crate M-runtime Module: tokio/runtime labels May 19, 2025
@ADD-SP ADD-SP requested a review from Darksonn May 20, 2025 11:57
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks.

@Darksonn Darksonn merged commit 55e3ed2 into tokio-rs:master May 23, 2025
90 checks passed
@ADD-SP ADD-SP deleted the add_sp/runtime-eliminate-lfence branch May 24, 2025 01:34
@Toby-Shi-cloud
Copy link

I am learning lock-free structure right now. Currently I just read the implementation of this steal-queue.
This PR is great and eliminates unnecessary acquire atomic loads, but I still have a small question:
I wonder is there any reason to use unsync_load instead of relaxed atomic load, since in this implementation, the only use of unsync_load is to read tail immediately after using acquire atomic load to read head (which, I think, already prevents the optimization).

hawkw added a commit that referenced this pull request Jul 1, 2025
# 1.46.0 (July 1st, 2025)

### Fixed

- net: fixed `TcpStream::shutdown` incorrectly returning an error on macOS
  ([#7290])

## Added

- sync: `mpsc::OwnedPermit::{same_channel, same_channel_as_sender}` methods
  ([#7389])
- macros: `biased` option for `join!` and `try_join!`, similar to `select!`
  ([#7307])
- net: support for cygwin ([#7393])
- net: support `pope::OpenOptions::read_write` on Android ([#7426])
- net: add `Clone` implementation for `net::unix::SocketAddr` ([#7422])

## Changed

- runtime: eliminate unnecessary lfence while operating on `queue::Local<T>`
  ([#7340])
- task: disallow blocking in `LocalSet::{poll,drop}` ([#7372])

## Unstable

- runtime: add `TaskMeta::spawn_location` tracking where a task was spawned
  ([#7417])
- runtime: removed borrow from `LocalOptions` parameter to
  `runtime::Builder::build_local` ([#7346])

## Documented

- io: clarify behavior of seeking when `start_seek` is not used ([#7366])
- io: document cancellation safety of `AsyncWriteExt::flush` ([#7364])
- net: fix docs for `recv_buffer_size` method ([#7336])
- net: fix broken link of `RawFd` in `TcpSocket` docs ([#7416])
- net: update `AsRawFd` doc link to current Rust stdlib location ([#7429])
- readme: fix double period in reactor description (#7363)
- runtime: add doc note that `on_*_task_poll` is unstable ([#7311])
- sync: update broadcast docs on allocation failure ([#7352])
- time: add a missing panic scenario of `time::advance` ([#7394])

[#7290]: #7290
[#7307]: #7307
[#7311]: #7311
[#7336]: #7336
[#7340]: #7340
[#7346]: #7346
[#7352]: #7352
[#7364]: #7364
[#7366]: #7366
[#7372]: #7372
[#7389]: #7389
[#7393]: #7393
[#7394]: #7394
[#7416]: #7416
[#7422]: #7422
[#7426]: #7426
[#7429]: #7429
[#7417]: #7417
hawkw added a commit that referenced this pull request Jul 1, 2025
# 1.46.0 (July 1st, 2025)

### Fixed

- net: fixed `TcpStream::shutdown` incorrectly returning an error on macOS
  ([#7290])

## Added

- sync: `mpsc::OwnedPermit::{same_channel, same_channel_as_sender}` methods
  ([#7389])
- macros: `biased` option for `join!` and `try_join!`, similar to `select!`
  ([#7307])
- net: support for cygwin ([#7393])
- net: support `pope::OpenOptions::read_write` on Android ([#7426])
- net: add `Clone` implementation for `net::unix::SocketAddr` ([#7422])

## Changed

- runtime: eliminate unnecessary lfence while operating on `queue::Local<T>`
  ([#7340])
- task: disallow blocking in `LocalSet::{poll,drop}` ([#7372])

## Unstable

- runtime: add `TaskMeta::spawn_location` tracking where a task was spawned
  ([#7417])
- runtime: removed borrow from `LocalOptions` parameter to
  `runtime::Builder::build_local` ([#7346])

## Documented

- io: clarify behavior of seeking when `start_seek` is not used ([#7366])
- io: document cancellation safety of `AsyncWriteExt::flush` ([#7364])
- net: fix docs for `recv_buffer_size` method ([#7336])
- net: fix broken link of `RawFd` in `TcpSocket` docs ([#7416])
- net: update `AsRawFd` doc link to current Rust stdlib location ([#7429])
- readme: fix double period in reactor description (#7363)
- runtime: add doc note that `on_*_task_poll` is unstable ([#7311])
- sync: update broadcast docs on allocation failure ([#7352])
- time: add a missing panic scenario of `time::advance` ([#7394])

[#7290]: #7290
[#7307]: #7307
[#7311]: #7311
[#7336]: #7336
[#7340]: #7340
[#7346]: #7346
[#7352]: #7352
[#7364]: #7364
[#7366]: #7366
[#7372]: #7372
[#7389]: #7389
[#7393]: #7393
[#7394]: #7394
[#7416]: #7416
[#7422]: #7422
[#7426]: #7426
[#7429]: #7429
[#7417]: #7417
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 M-runtime Module: tokio/runtime R-loom-multi-thread Run loom multi-thread tests on this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants