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.

Sign up for GitHub

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

time: update wake_up while holding all the locks of sharded time wheels #6683

Merged
merged 2 commits into from
Jul 16, 2024
Merged

time: update wake_up while holding all the locks of sharded time wheels #6683

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1b818a3
time: update wake_up while holding all the locks of sharded time wheels
wathenjiang Jul 16, 2024
7a3f83e
fmt
wathenjiang Jul 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 10 additions & 5 deletions tokio/src/runtime/time/mod.rs
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we add a loom test for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to add a loom test for this one.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is this an actual data race or just a race condition? It looks like it's just incorrect usage of locks, not any incorrect unsafe code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems that I have confused them. I would like to modify the title to time: update wake_up while holding all the locks of sharded time wheels.

Original file line number Diff line number Diff line change
Expand Up @@ -190,11 +190,13 @@ impl Driver {
assert!(!handle.is_shutdown());

// Finds out the min expiration time to park.
let expiration_time = (0..rt_handle.time().inner.get_shard_size())
.filter_map(|id| {
let lock = rt_handle.time().inner.lock_sharded_wheel(id);
lock.next_expiration_time()
})
let locks = (0..rt_handle.time().inner.get_shard_size())
.map(|id| rt_handle.time().inner.lock_sharded_wheel(id))
.collect::<Vec<_>>();
Comment on lines +193 to +195
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to avoid allocating this vector every time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you have any ideas to achieve this? Can we use smallvec::SmallVec here?

Copy link
Contributor

Choose a reason for hiding this comment

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

We're not going to introduce a new dependency for it, but maybe a solution like that could work. But I don't think it has to be solved before we publish a release containing this fix.

(See also chat on discord.)


let expiration_time = locks
.iter()
.filter_map(|lock| lock.next_expiration_time())
.min();

rt_handle
Expand All @@ -203,6 +205,9 @@ impl Driver {
.next_wake
.store(next_wake_time(expiration_time));

// Safety: After updating the `next_wake`, we drop all the locks.
drop(locks);

match expiration_time {
Some(when) => {
let now = handle.time_source.now(rt_handle.clock());
Expand Down
Loading