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

Address reuse improvements and fixes #3475

Merged
merged 3 commits into from
Apr 19, 2024

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Apr 16, 2024

  • when an address gets reused, establish a happens-before link in the data race model
  • do not reuse stack addresses, and make the reuse rate configurable

Fixes #3450

@RalfJung RalfJung force-pushed the reduce-reuse-recycle branch 6 times, most recently from 6992266 to 2049ee7 Compare April 16, 2024 14:08
@RalfJung
Copy link
Member Author

@bors r

@bors
Copy link
Collaborator

bors commented Apr 16, 2024

📌 Commit 2049ee7 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 16, 2024

⌛ Testing commit 2049ee7 with merge 22895df...

bors added a commit that referenced this pull request Apr 16, 2024
when an address gets reused, establish a happens-before link in the data race model

Fixes #3450
@RalfJung
Copy link
Member Author

Ah... this makes the weak memory emulation basically disappear. We do so much reuse even of stack variables that it is almost guaranteed that you pick up some address from another thread, and therefore synchronize with it...

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 16, 2024
…i-obk

interpret: pass MemoryKind to before_memory_deallocation

This will be needed for rust-lang/miri#3475.

r? `@oli-obk`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 16, 2024
…i-obk

interpret: pass MemoryKind to before_memory_deallocation

This will be needed for rust-lang/miri#3475.

r? ``@oli-obk``
@bors
Copy link
Collaborator

bors commented Apr 16, 2024

💥 Test timed out

rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 16, 2024
Rollup merge of rust-lang#124018 - RalfJung:dealloc-memory-kind, r=oli-obk

interpret: pass MemoryKind to before_memory_deallocation

This will be needed for rust-lang/miri#3475.

r? ``@oli-obk``
github-actions bot pushed a commit that referenced this pull request Apr 17, 2024
interpret: pass MemoryKind to before_memory_deallocation

This will be needed for #3475.

r? ``@oli-obk``
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 17, 2024
…, r=oli-obk

interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang/miri#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang#124018 has been rolled up already so I couldn't add it there any more.

r? `@oli-obk`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Apr 17, 2024
…, r=oli-obk

interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang/miri#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang#124018 has been rolled up already so I couldn't add it there any more.

r? ``@oli-obk``
RalfJung added a commit to RalfJung/rust that referenced this pull request Apr 17, 2024
…, r=oli-obk

interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang/miri#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang#124018 has been rolled up already so I couldn't add it there any more.

r? ``@oli-obk``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 17, 2024
…, r=oli-obk

interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang/miri#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang#124018 has been rolled up already so I couldn't add it there any more.

r? ```@oli-obk```
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 17, 2024
Rollup merge of rust-lang#124030 - RalfJung:adjust_alloc_base_pointer, r=oli-obk

interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang/miri#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang#124018 has been rolled up already so I couldn't add it there any more.

r? ```@oli-obk```
RalfJung pushed a commit to RalfJung/miri that referenced this pull request Apr 17, 2024
interpret: pass MemoryKind to adjust_alloc_base_pointer

Another puzzle piece for rust-lang#3475.

The 2nd commit renames base_pointer -> root_pointer; that's how Tree Borrows already calls them and I think the term is more clear than "base pointer". In particular, this distinguishes it from "base address", since a root pointer can point anywhere into an allocation, not just its base address.

rust-lang/rust#124018 has been rolled up already so I couldn't add it there any more.

r? ```@oli-obk```
@RalfJung RalfJung changed the title when an address gets reused, establish a happens-before link in the data race model Address reuse improvements and fixes Apr 18, 2024
@RalfJung
Copy link
Member Author

I excluded stack allocations from reuse, and made the reuse rate configurable.

@bors r

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

📌 Commit f3b31c3 has been approved by RalfJung

It is now in the queue for this repository.

bors added a commit that referenced this pull request Apr 18, 2024
Address reuse improvements and fixes

- when an address gets reused, establish a happens-before link in the data race model
- do not reuse stack addresses, and make the reuse rate configurable

Fixes #3450
@bors
Copy link
Collaborator

bors commented Apr 18, 2024

⌛ Testing commit f3b31c3 with merge ab25a33...

@RalfJung
Copy link
Member Author

RalfJung commented Apr 18, 2024

This actually still has a quite significant impact on data race detection; I suspect this is due to heap allocations in the thread spawning logic. I'm not sure what the best way forward is here... other than reducing the reuse rate drastically. With a reuse rate of 25%, the simple data race I checked is still found around 70% of the time (but it's 100% without reuse). With the default reuse rate it's about 40% detection rate for data races.

I think the rate will be better with real programs; the issue is that in these tests the threads exist only for a very short time so there's a real chance it never gets preempted and so the first thread is done before the second thread starts, and therefore allocations can be reused. If I add a simple for _ in 0..100 {} to the first thread, the data race is detected ~100% of the time even with the default address reuse rate.

We could of course also just not do any reuse across threads, then we also will never add spurious synchronization. But for heap allocations that's not real either.

@bors
Copy link
Collaborator

bors commented Apr 18, 2024

💔 Test failed - checks-actions

@RalfJung RalfJung force-pushed the reduce-reuse-recycle branch 2 times, most recently from acbcc7a to 34c8db6 Compare April 18, 2024 11:21
@RalfJung
Copy link
Member Author

OTOH, real programs will also do a bunch of stuff on the heap, which again increases the chance of address reuse inducting synchronization.

My current inclination is to add some extra randomness that makes address reuse across thread boundaries a lot less likely than address reuse within a single thread.

@RalfJung
Copy link
Member Author

RalfJung commented Apr 18, 2024

My current inclination is to add some extra randomness that makes address reuse across thread boundaries a lot less likely than address reuse within a single thread.

That's what I now implemented with the last commit.

@rust-lang/miri what do you think? With the cross-thread reuse rate of 10%, the simple data race tests still detect the race 80% of the time when preemption is disabled (and ~100% of the time when preemption is enabled). These tests do basically nothing in their threads, so there's a significant chance the 2nd thread starts after the 1st thread is already done and then address reuse inside thread::spawn can induce synchronization.

I'm thinking maybe I should increase the regular address reuse rate to around 70%; that makes the chance that a free; malloc pair reuses the just-freed address 50% (since we roll dice both on free and on malloc, and sqrt(0.5) is close to 0.7). I previously went back down to 50% to avoid too much accidental synchronization, but now that that is a separate knob it seems fine to go up to 70%. OTOH 50% also seems fine; when doing malloc; free in a loop this means we see about N/2 different addresses in N loop iterations.

@saethlin
Copy link
Member

With the cross-thread reuse rate of 10%, the simple data race tests still detect the race 80% of the time when preemption is disabled (and ~100% of the time when preemption is enabled).

That seems fine to me. Anyone seriously looking to find data races should have preemption enabled.

@RalfJung
Copy link
Member Author

Yeah... though it's hard to say what this will do for other cases. Not sure how many people run concurrency tests in a loop in Miri to give races more than one chance to appear.

But for now, let's go ahead with this, we can always re-adjust later if we realize that data race detection has become too non-deterministic.

@bors r

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

📌 Commit 2155a30 has been approved by RalfJung

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

⌛ Testing commit 2155a30 with merge 9b36914...

@bors
Copy link
Collaborator

bors commented Apr 19, 2024

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 9b36914 to master...

@bors bors merged commit 9b36914 into rust-lang:master Apr 19, 2024
8 checks passed
@RalfJung RalfJung deleted the reduce-reuse-recycle branch April 20, 2024 06:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reusing allocation addresses should imply a happens-before relationship
3 participants