avoid deadlock by skipping sampling in libc, libgcc and pthread #85

YangKeao · 2021-10-25T10:01:25Z

Signed-off-by: YangKeao [email protected]

Signed-off-by: YangKeao <[email protected]>

BusyJay · 2021-10-27T10:57:02Z

Please fix warnings. I'm afraid this can mislead users. For example, symbols like pthread_cond_xxx will be skipped while they occupy quite some samples in a typical profile.

Signed-off-by: YangKeao <[email protected]>

BusyJay · 2021-10-29T03:35:42Z

I can see there are still warnings in the changes.

YangKeao · 2021-10-29T04:17:46Z

I can see there are still warnings in the changes.

Fixed 😺

YangKeao · 2021-10-29T04:22:06Z

Please fix warnings. I'm afraid this can mislead users. For example, symbols like pthread_cond_xxx will be skipped while they occupy quite some samples in a typical profile.

That's why it is a feature. I left the choice to the user to trade off the deadlock possibility and the accuracy of the flamegraph

BusyJay · 2021-10-29T05:33:24Z

Can you explain why there will be deadlocks?

YangKeao · 2021-10-29T05:42:40Z

Can you explain why there will be deadlocks?

...
pthread_mutex_lock                          libpthread
dl_iterate_phdr                                     libc.so
_Unwind_Backtrace                            libgcc_s.so
Backtrace::trace_unsynchronized()
...
<---- Signal Arrive
...
dl_iterate_phdr                                     libc.so
_Unwind_Backtrace                            libgcc_s.so
Backtrace::new()
...

Because Backtrace::new() will call dl_iterate_phdr, in which there is a mutex. If a signal arrives right after it acquires the lock, this lock will never be released and this thread will be dead. In tikv-server, when some errors are generated, it will be equipped with a Backtrace, which is the most frequenctly situation where this deadlock may occur.

This lock is acquired during some logic inside the _Unwind_Backtrace and dl_iterate_phdr, so libgcc_s.so and libc.so are all required to be included in the blacklist.

Actually trying to get a lock in the signal handler is (theoretically) unsafe anyway, this PR is only a possible (practical) solution.

Signed-off-by: YangKeao <[email protected]>

YangKeao · 2021-10-29T05:49:34Z

I've thought about shrinking the blacklist by only skipping some symbols (but not whole library).

However, it's only safe to get the ip address of the nearest frame of the signal, but not a full backtrace, so trying to find all possible symbol of the nearest "deadlock" frame is quite complicated (and not stable).

YangKeao · 2021-10-29T07:33:24Z

@BusyJay PTAL

BusyJay · 2021-10-29T07:49:52Z

src/profiler.rs

@@ -23,6 +26,9 @@ pub struct Profiler {
    sample_counter: i32,

    running: bool,
+
+    #[cfg(all(feature = "ignore-libc", target_arch = "x86_64", target_os = "linux"))]


Using a feature seems over complicated to me. Blacklist should be a common requirement. Though I don't have strong opinion on this.

Maybe I should make it configurable through an argument, but not a compile-time feature?

@BusyJay Now I added a ProfilerGuardBuilder to set the configuration (though the ProfilerGuardBuilder sounds chaos).

BusyJay · 2021-10-29T07:53:56Z

src/profiler.rs

@@ -149,14 +173,60 @@ extern "C" fn perf_signal_handler(_signal: c_int) {
    }
 }

+#[cfg(all(feature = "ignore-libc", target_arch = "x86_64", target_os = "linux"))]
+const SHLIB_BLACKLIST: [&str; 3] = ["libc", "libgcc_s", "libpthread"];


What about binaries that linked statically?

They have to be handle case by case (for different libc implementation). The musl implementation of dl_iterate_phdr doesn't have a lock, so the _Unwind_Backtrace will not deadlock at this point.

But I'm not familiar with other static libc implementation.

Signed-off-by: YangKeao <[email protected]>

BusyJay

So it will still deadlock on Arm, right?

src/profiler.rs

Signed-off-by: YangKeao <[email protected]>

YangKeao · 2021-10-29T13:11:57Z

So it will still deadlock on Arm, right?

@BusyJay This PR supports ARM now, but the pprof-rs don't 😭 . #75 describes this problem. A rough investigation on that issue is about the arm special signal frame, but I haven't found a way to resolve it.

YangKeao · 2021-11-01T03:17:39Z

@BusyJay PTAL

BusyJay

Rest LGTM

src/profiler.rs

Signed-off-by: YangKeao <[email protected]>

sticnarf · 2021-11-02T08:16:24Z

A simpler solution might be hacking dl_iterate_phdr. Set a thread local flag when dl_iterate_phdr is called, then we can skip the sample by checking the thread local flag.

* avoid deadlock by skipping sampling in libc, libgcc and pthread Signed-off-by: YangKeao <[email protected]> * make clippy happy Signed-off-by: YangKeao <[email protected]> * skip libc only for linux Signed-off-by: YangKeao <[email protected]> * add null check in the signal handler Signed-off-by: YangKeao <[email protected]> * add more cfg to remove blacklist on non-linux platform Signed-off-by: YangKeao <[email protected]> * fix clippy warning for test Signed-off-by: YangKeao <[email protected]> * fix clippy Signed-off-by: YangKeao <[email protected]> * use a configurable option to set blacklist Signed-off-by: YangKeao <[email protected]> * add document and don't reset blacklist_segments Signed-off-by: YangKeao <[email protected]> * support aarch64 Signed-off-by: YangKeao <[email protected]> * replace blacklist into blocklist Signed-off-by: YangKeao <[email protected]>

YangKeao added 4 commits October 25, 2021 21:01

avoid deadlock by skipping sampling in libc, libgcc and pthread

7c5b602

Signed-off-by: YangKeao <[email protected]>

make clippy happy

46a3996

Signed-off-by: YangKeao <[email protected]>

skip libc only for linux

a640ff5

Signed-off-by: YangKeao <[email protected]>

add null check in the signal handler

ff4e24e

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch from 6aab8f3 to ff4e24e Compare October 26, 2021 05:28

YangKeao requested a review from BusyJay October 27, 2021 09:37

JaySon-Huang mentioned this pull request Oct 28, 2021

TiFlash get stuck occasionally when mem-profiling is actived pingcap/tiflash#3236

Closed

YangKeao added 2 commits October 28, 2021 15:46

add more cfg to remove blacklist on non-linux platform

a12b5ea

Signed-off-by: YangKeao <[email protected]>

fix clippy warning for test

872f5dc

Signed-off-by: YangKeao <[email protected]>

YangKeao requested review from BusyJay and removed request for BusyJay October 29, 2021 05:30

fix clippy

59bad74

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch from 0ce2dd4 to 59bad74 Compare October 29, 2021 05:45

BusyJay reviewed Oct 29, 2021

View reviewed changes

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch 2 times, most recently from 181bdbb to 6e4cfea Compare October 29, 2021 09:14

use a configurable option to set blacklist

1b7b26c

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch from 6e4cfea to 1b7b26c Compare October 29, 2021 09:25

BusyJay reviewed Oct 29, 2021

View reviewed changes

src/profiler.rs Outdated Show resolved Hide resolved

add document and don't reset blacklist_segments

d2b1fb6

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch 2 times, most recently from 39518c8 to 911afb8 Compare October 29, 2021 11:58

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch 2 times, most recently from fac6459 to cbfe7ea Compare October 29, 2021 12:43

support aarch64

1fc974a

Signed-off-by: YangKeao <[email protected]>

YangKeao force-pushed the avoid-deadlock-inside-backtrace branch from cbfe7ea to 1fc974a Compare October 29, 2021 12:48

BusyJay reviewed Nov 1, 2021

View reviewed changes

src/profiler.rs Outdated Show resolved Hide resolved

replace blacklist into blocklist

6e7bc09

Signed-off-by: YangKeao <[email protected]>

YangKeao requested a review from BusyJay November 1, 2021 07:03

BusyJay approved these changes Nov 1, 2021

View reviewed changes

YangKeao merged commit 55d6f87 into tikv:master Nov 1, 2021

YangKeao mentioned this pull request Nov 2, 2021

status_server: add glibc, pthread, libgcc to the blocklist tikv/tikv#11205

Merged

YangKeao mentioned this pull request Nov 3, 2021

status_server: add glibc, pthread, libgcc to the blocklist (#11205) tikv/tikv#11215

Merged

This was referenced Nov 25, 2021

status_server: add glibc, pthread, libgcc to the blocklist (#11205) tikv/tikv#11473

Closed

status_server: add glibc, pthread, libgcc to the blocklist (#11205) tikv/tikv#11474

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

avoid deadlock by skipping sampling in libc, libgcc and pthread #85

avoid deadlock by skipping sampling in libc, libgcc and pthread #85

YangKeao commented Oct 25, 2021

BusyJay commented Oct 27, 2021

BusyJay commented Oct 29, 2021

YangKeao commented Oct 29, 2021

YangKeao commented Oct 29, 2021

BusyJay commented Oct 29, 2021

YangKeao commented Oct 29, 2021 •

edited

Loading

YangKeao commented Oct 29, 2021

YangKeao commented Oct 29, 2021

BusyJay Oct 29, 2021

YangKeao Oct 29, 2021

YangKeao Oct 29, 2021

BusyJay Oct 29, 2021

YangKeao Oct 29, 2021 •

edited

Loading

BusyJay left a comment

YangKeao commented Oct 29, 2021 •

edited

Loading

YangKeao commented Nov 1, 2021

BusyJay left a comment

sticnarf commented Nov 2, 2021

avoid deadlock by skipping sampling in libc, libgcc and pthread #85

avoid deadlock by skipping sampling in libc, libgcc and pthread #85

Conversation

YangKeao commented Oct 25, 2021

BusyJay commented Oct 27, 2021

BusyJay commented Oct 29, 2021

YangKeao commented Oct 29, 2021

YangKeao commented Oct 29, 2021

BusyJay commented Oct 29, 2021

YangKeao commented Oct 29, 2021 • edited Loading

YangKeao commented Oct 29, 2021

YangKeao commented Oct 29, 2021

BusyJay Oct 29, 2021

Choose a reason for hiding this comment

YangKeao Oct 29, 2021

Choose a reason for hiding this comment

YangKeao Oct 29, 2021

Choose a reason for hiding this comment

BusyJay Oct 29, 2021

Choose a reason for hiding this comment

YangKeao Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

BusyJay left a comment

Choose a reason for hiding this comment

YangKeao commented Oct 29, 2021 • edited Loading

YangKeao commented Nov 1, 2021

BusyJay left a comment

Choose a reason for hiding this comment

sticnarf commented Nov 2, 2021

YangKeao commented Oct 29, 2021 •

edited

Loading

YangKeao Oct 29, 2021 •

edited

Loading

YangKeao commented Oct 29, 2021 •

edited

Loading