Replace ReentrantMutex by a futex-based one on Linux. #95727

m-ou-se · 2022-04-06T10:54:00Z

Tracking issue: #93740

r? @Amanieu

m-ou-se · 2022-04-06T11:21:59Z

test/ui/runtime/stdout-during-shutdown/stdout-during-shutdown.rs

Hm, who thought it was a good idea to test that we allow std to be used in an atexit() handler?

*runs git blame*

Oh. ^^'

bjorn3 · 2022-04-06T18:54:16Z

library/std/src/sys/unix/locks/futex.rs

+/// The 'owner' field tracks which thread has locked the mutex.
+///
+/// We use current_thread_unique_ptr() as the thread identifier,
+/// which is just the address of a thread local variable.


That might not work if a thread doesn't unlock and then exits. Another new thread may then suddenly acquire the lock if the allocator happens to return the same pointer.

There is a ThreadId type you can use instead.

That might not work if a thread doesn't unlock and then exits. Another new thread may then suddenly acquire the lock if the allocator happens to return the same pointer.

That should work out fine, as the old thread doesn't exist anymore, it's fine for the new thread to take over ownership of the lock.

There is a ThreadId type you can use instead.

I am aware, but the ThreadId type is 64-bit, which is problematic on some 32-bit platforms.

library/std/src/sys/unix/locks/futex.rs

library/std/src/sys_common/thread_info.rs

Amanieu

Should all these methods be marked as #[inline]?

library/std/src/sys_common/thread_info.rs

library/std/src/sys/unix/locks/futex.rs

Amanieu · 2022-04-06T20:15:26Z

Also, this ReentrantMutex implementation is not specific to any OS so we could use it everywhere and remove all the OS-specific ReentrantMutex implementations. I think this would be a good improvement towards simplifying the codebase.

m-ou-se · 2022-04-06T20:25:44Z

Also, this ReentrantMutex implementation is not specific to any OS so we could use it everywhere and remove all the OS-specific ReentrantMutex implementations. I think this would be a good improvement towards simplifying the codebase.

Yes! After updating everything on Linux, my next step will using these implementations on as many platforms as possible. (So the Mutex and Condvar and RwLock on all platforms that support futexes, and this ReentrantMutex on every platform.)

m-ou-se · 2022-04-06T20:32:05Z

Should all these methods be marked as #[inline]?

Maybe? They're more complicated than a simple single compare_exchange like in Mutex though. So I guess it depends on how simple the generated code for current_thread_unique_ptr() will be. If that always results in a call, then I guess not.

m-ou-se · 2022-04-06T20:33:14Z

Should all these methods be marked as #[inline]?

Maybe?

Oh and we only use this type within std, and not expose it anywhere. So rustc/llvm can already decide to inline them in the places we use them, regardless of the attribute.

bors · 2022-04-07T00:17:18Z

☔ The latest upstream changes (presumably #95742) made this pull request unmergeable. Please resolve the merge conflicts.

Amanieu · 2022-04-08T13:54:25Z

@bors r+

bors · 2022-04-08T13:54:26Z

📌 Commit 0664b8a has been approved by Amanieu

m-ou-se · 2022-04-08T17:06:58Z

@bors rollup=never

Just in case.

…nieu Replace ReentrantMutex by a futex-based one on Linux. Tracking issue: rust-lang#93740 r? `@Amanieu`

bors · 2022-04-09T10:49:48Z

⌛ Testing commit 0664b8a with merge 939bbea31a5449d8e597e9daa1b866d9dbe3183c...

bors · 2022-04-12T09:26:16Z

💔 Test failed - checks-actions

m-ou-se · 2022-04-12T11:28:43Z

Hm, src/test/ui/issues/issue-70093.rs is segfaulting on host=x86_64-unknown-linux-gnu target=i686-unknown-linux-musl.

rust/src/test/ui/issues/issue-70093.rs

Lines 1 to 9 in 4e1927d

 // run-pass 

 // compile-flags: -Zlink-native-libraries=no -Cdefault-linker-libraries=yes 

 // ignore-windows - this will probably only work on unixish systems 

 // ignore-fuchsia - missing __libc_start_main for some reason (#84733) 

 #[link(name = "some-random-non-existent-library", kind = "static")] 

 extern "C" {} 

 fn main() {}

rust-log-analyzer · 2022-04-12T11:34:42Z

The job dist-i586-gnu-i586-i686-musl failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

failures:

---- [ui] src/test/ui/issues/issue-70093.rs stdout ----

error: test run failed!
status: signal: 11 (core dumped)
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/issues/issue-70093/a"
stdout: none
stderr: none


failures:
    [ui] src/test/ui/issues/issue-70093.rs

m-ou-se · 2022-04-12T12:15:16Z

I can reproduce it on x86_64-unknown-linux-musl.

Program stopped.
0x0000000000000000 in ?? ()
(gdb) bt
#0  0x0000000000000000 in ?? ()
#1  0x00007ffff7fe2b0c in __register_frame_info ()
#2  0x00007ffff7fe34ad in libc_start_init ()
#3  0x00007ffff7fa8c50 in ?? ()
#4  0x00007ffff7fe34d2 in libc_start_main_stage2 ()
#5  0x00007ffff7fe34b5 in libc_start_init ()
#6  0x0000000000000000 in ?? ()
(gdb) frame 1
#1  0x00007ffff7fe2b0c in __register_frame_info ()
(gdb) disassemble
Dump of assembler code for function __register_frame_info:
[...]
   0x00007ffff7fe2afa <+74>:	test   %rbp,%rbp
   0x00007ffff7fe2afd <+77>:	je     0x7ffff7fe2b0c <__register_frame_info+92>
   0x00007ffff7fe2aff <+79>:	lea    0x1a89a(%rip),%rdi        # 0x7ffff7ffd3a0 <object_mutex>
   0x00007ffff7fe2b06 <+86>:	call   *0x1a3a4(%rip)            # 0x7ffff7ffceb0
=> 0x00007ffff7fe2b0c <+92>:	mov    0x1a8c5(%rip),%rax        # 0x7ffff7ffd3d8 <unseen_objects>
[...]
End of assembler dump.
(gdb) x/g 0x7ffff7ffceb0
0x7ffff7ffceb0:	0x0

m-ou-se · 2022-04-12T12:32:56Z

That call seems to be this line: https://github.com/gcc-mirror/gcc/blob/95533fe4f014c10dd18de649927668aba6117daf/libgcc/unwind-dw2-fde.c#L103

__gthread_mutex_lock (&object_mutex);

m-ou-se · 2022-04-12T12:54:57Z

I don't think this test shoud've worked at all when cross compiling *-gnu to *-musl. It's using the *-gnu C compiler as linker for a *-musl target while enabling its default libs. It in turn then passes -lgcc -lgcc_eh -lc to ld, making it link glibc in a musl binary.

m-ou-se · 2022-04-12T12:59:31Z

cc @jsgf who wrote this test. Should we add // ignore-cross-compile to that test? Or is it expected to always work when cross compiling?

…triplett Don't test -Cdefault-linker-libraries=yes when cross compiling. See rust-lang#95727 (comment) and the five comments below that. Unblocks rust-lang#95727.

m-ou-se · 2022-04-13T09:02:14Z

Disabled the test for cross compiling in #95975

@bors r=Amanieu

bors · 2022-04-13T09:02:16Z

💡 This pull request was already approved, no need to approve it again.

This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
There's another pull request that is currently being tested, blocking this pull request: Use mir constant in thir instead of ty::Const #94255

bors · 2022-04-13T09:02:17Z

📌 Commit d4e44a6 has been approved by Amanieu

m-ou-se · 2022-04-13T09:02:29Z

@bors retry

bors · 2022-04-13T13:42:22Z

⌛ Testing commit d4e44a6 with merge ab33f71...

bors · 2022-04-13T16:03:59Z

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing ab33f71 to master...

rust-timer · 2022-04-13T17:18:09Z

Finished benchmarking commit (ab33f71): comparison url.

Summary:

Primary benchmarks: no relevant changes found
Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count¹	0	1	0	2	0
mean²	N/A	0.2%	N/A	-0.4%	N/A
max	N/A	0.2%	N/A	-0.4%	N/A

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

number of relevant changes ↩
the arithmetic mean of the percent change ↩

quininer · 2022-04-14T10:15:57Z

library/std/src/sys/unix/locks/futex.rs

+/// This can be used as a non-null usize-sized ID.
+pub fn current_thread_unique_ptr() -> usize {
+ // Use a non-drop type to make sure it's still available during thread destruction.
+ thread_local! { static X: u8 = const { 0 } }


Should we avoid creating new thread_local!? Since pthread keys number available on some platforms is rather small, it would be beneficial for std to use as few pthread keys as possible.

That'd be nice.

I used the address of the THREAD_INFO thread local in an earlier version, to avoid creating a new one, but that one needs Drop, which makes it inaccessible during thread shutdown. We don't have a good way of getting the address of such a thread local's storage after its destructor ran, since its getter function just returns None after destruction.

It looks like LOCAL_PANIC_COUNT meets this requirement, but it's a little weird to use it directly. maybe it would be nice to combine the KEYS of map.rs and LOCAL_PANIC_COUNT into one LOCAL_THREAD_STATE .

jsgf · 2022-04-17T02:01:20Z

@m-ou-se:

cc @jsgf who wrote this test. Should we add // ignore-cross-compile to that test? Or is it expected to always work when cross compiling?

Yeah, I wouldn't expect that test to necessarily work cross-compiling, so I think it's fine to exclude it. I was always concerned that test was a bit fragile. (Though I don't really see why it would cause a segfault.)

Use a single ReentrantMutex implementation on all platforms. This replaces all platform specific ReentrantMutex implementations by the one I added in rust-lang#95727 for Linux, since that one does not depend on any platform specific details. r? `@Amanieu`

m-ou-se added A-concurrency Area: Concurrency T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 6, 2022

rust-highfive assigned Amanieu Apr 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 6, 2022

This comment has been minimized.

Sign in to view

m-ou-se added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 6, 2022

bjorn3 reviewed Apr 6, 2022

View reviewed changes

library/std/src/sys/unix/locks/futex.rs Show resolved Hide resolved

bjorn3 reviewed Apr 6, 2022

View reviewed changes

library/std/src/sys_common/thread_info.rs Outdated Show resolved Hide resolved

Amanieu reviewed Apr 6, 2022

View reviewed changes

library/std/src/sys_common/thread_info.rs Outdated Show resolved Hide resolved

library/std/src/sys/unix/locks/futex.rs Show resolved Hide resolved

library/std/src/sys/unix/locks/futex.rs Show resolved Hide resolved

m-ou-se mentioned this pull request Apr 6, 2022

Tracking issue for improving std::sync::{Mutex, RwLock, Condvar} #93740

Closed

63 tasks

m-ou-se force-pushed the futex-reentrantmutex branch from 0865b1f to 0664b8a Compare April 7, 2022 09:31

m-ou-se added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 7, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 8, 2022

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Apr 8, 2022

Rollup merge of rust-lang#95727 - m-ou-se:futex-reentrantmutex, r=Ama…

66b3081

…nieu Replace ReentrantMutex by a futex-based one on Linux. Tracking issue: rust-lang#93740 r? `@Amanieu`

Dylan-DPC mentioned this pull request Apr 8, 2022

Rollup of 7 pull requests #95827

Closed

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Apr 12, 2022

m-ou-se mentioned this pull request Apr 12, 2022

Don't test -Cdefault-linker-libraries=yes when cross compiling. #95975

Merged

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 13, 2022

bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 13, 2022

bors merged commit ab33f71 into rust-lang:master Apr 13, 2022

rustbot added this to the 1.62.0 milestone Apr 13, 2022

m-ou-se deleted the futex-reentrantmutex branch April 13, 2022 16:10

bors mentioned this pull request Apr 13, 2022

Use const initializer for LOCAL_PANIC_COUNT #92461

Merged

m-ou-se mentioned this pull request Apr 14, 2022

Use a single ReentrantMutex implementation on all platforms. #96042

Merged

quininer reviewed Apr 14, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace ReentrantMutex by a futex-based one on Linux. #95727

Replace ReentrantMutex by a futex-based one on Linux. #95727

m-ou-se commented Apr 6, 2022

This comment has been minimized.

m-ou-se commented Apr 6, 2022

bjorn3 Apr 6, 2022

bjorn3 Apr 6, 2022

m-ou-se Apr 6, 2022

Amanieu left a comment

Amanieu commented Apr 6, 2022

m-ou-se commented Apr 6, 2022

m-ou-se commented Apr 6, 2022

m-ou-se commented Apr 6, 2022 •

edited

Loading

bors commented Apr 7, 2022

Amanieu commented Apr 8, 2022

bors commented Apr 8, 2022

m-ou-se commented Apr 8, 2022

bors commented Apr 9, 2022

bors commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

rust-log-analyzer commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 12, 2022 •

edited

Loading

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 13, 2022

bors commented Apr 13, 2022

bors commented Apr 13, 2022

m-ou-se commented Apr 13, 2022

bors commented Apr 13, 2022

bors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

quininer Apr 14, 2022

m-ou-se Apr 14, 2022

quininer Apr 14, 2022

jsgf commented Apr 17, 2022 •

edited

Loading

Replace ReentrantMutex by a futex-based one on Linux. #95727

Replace ReentrantMutex by a futex-based one on Linux. #95727

Conversation

m-ou-se commented Apr 6, 2022

This comment has been minimized.

m-ou-se commented Apr 6, 2022

bjorn3 Apr 6, 2022

Choose a reason for hiding this comment

bjorn3 Apr 6, 2022

Choose a reason for hiding this comment

m-ou-se Apr 6, 2022

Choose a reason for hiding this comment

Amanieu left a comment

Choose a reason for hiding this comment

Amanieu commented Apr 6, 2022

m-ou-se commented Apr 6, 2022

m-ou-se commented Apr 6, 2022

m-ou-se commented Apr 6, 2022 • edited Loading

bors commented Apr 7, 2022

Amanieu commented Apr 8, 2022

bors commented Apr 8, 2022

m-ou-se commented Apr 8, 2022

bors commented Apr 9, 2022

bors commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

rust-log-analyzer commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 12, 2022 • edited Loading

m-ou-se commented Apr 12, 2022

m-ou-se commented Apr 13, 2022

bors commented Apr 13, 2022

bors commented Apr 13, 2022

m-ou-se commented Apr 13, 2022

bors commented Apr 13, 2022

bors commented Apr 13, 2022

rust-timer commented Apr 13, 2022

Footnotes

quininer Apr 14, 2022

Choose a reason for hiding this comment

m-ou-se Apr 14, 2022

Choose a reason for hiding this comment

quininer Apr 14, 2022

Choose a reason for hiding this comment

jsgf commented Apr 17, 2022 • edited Loading

m-ou-se commented Apr 6, 2022 •

edited

Loading

m-ou-se commented Apr 12, 2022 •

edited

Loading

jsgf commented Apr 17, 2022 •

edited

Loading