Skip to content

Allocation tracker - fix deadlock when calling into logs#425

Merged
r1viollet merged 2 commits into
mainfrom
r1viollet/alloc_tracker_remove_logs
Oct 3, 2024
Merged

Allocation tracker - fix deadlock when calling into logs#425
r1viollet merged 2 commits into
mainfrom
r1viollet/alloc_tracker_remove_logs

Conversation

@r1viollet

Copy link
Copy Markdown
Collaborator

What does this PR do?

When entering error paths in the allocation profiler, we would log. This could create deadlocks while trying to acquire a lock to print timestamps in logs. Here is the backtrace where the deadlock was found:

Thread 1 (Thread 0x7f0dffe31180 (LWP 1773386)):
#0  __lll_lock_wait_private (futex=futex@entry=0x7f0e000211a0 <tzset_lock>) at ./lowlevellock.c:35
#1  0x00007f0dfff04b61 in __tz_convert (timer=1727099936, use_localtime=1, tp=0x7ffcbb868d60) at tzset.c:572
#2  0x00007f0dffd15014 in ddprof::vlprintfln(int, int, char const*, char const*, __va_list_tag*) () from target:/tmp/libdd_profiling-embedded.so-4345326bddecccac9d9da7b291afef63d08085caa69e672db2a97995414b68d7
#3  0x00007f0dffd152c8 in ddprof::olprintfln(int, int, char const*, char const*, ...) () from target:/tmp/libdd_profiling-embedded.so-4345326bddecccac9d9da7b291afef63d08085caa69e672db2a97995414b68d7
#4  0x00007f0dffd0d804 in ddprof::AllocationTracker::push_alloc_sample(unsigned long, unsigned long, ddprof::TrackerThreadLocalState&) () from target:/tmp/libdd_profiling-embedded.so-4345326bddecccac9d9da7b291afef63d08085caa69e672db2a97995414b68d7
#5  0x00007f0dffd0dac5 in ddprof::AllocationTracker::track_allocation(unsigned long, unsigned long, ddprof::TrackerThreadLocalState&) () from target:/tmp/libdd_profiling-embedded.so-4345326bddecccac9d9da7b291afef63d08085caa69e672db2a97995414b68d7

This commit moves the logs to the debug level to make sure they would not print by default. We can still trigger the logs in debug mode.

Motivation

Remove a dead lock.

Additional Notes

NA

How to test the change?

I am not able to reproduce this.
the conflict was with the appsec library also calling into the __tz_convert function.
TBD

@r1viollet

Copy link
Copy Markdown
Collaborator Author

Perhaps the logger could be more simple and wait for safe points ?

nsavoire
nsavoire previously approved these changes Sep 24, 2024
When entering error paths in the allocation profiler, we would log.
This could create deadlocks while trying to acquire a lock to print timestamps in logs.
Here is the backtrace where the deadlock was found:
Thread 1 (Thread 0x7f0dffe31180 (LWP 1773386)):

This commit moves the logs to the debug level to make sure they would not print by default.
We can still trigger the logs in debug mode.
@r1viollet
r1viollet force-pushed the r1viollet/alloc_tracker_remove_logs branch from 17a4cfa to 0c9b883 Compare October 1, 2024 08:04
…n tracker unit test.

This seems to be a compiler bug.
@pr-commenter

pr-commenter Bot commented Oct 3, 2024

Copy link
Copy Markdown

Benchmark results for collatz

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.18.0+aa511bcd.45491305 ddprof 0.18.0+36f24982.45686406

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-collatz --preset cpu_only collatz_runner.sh same

@pr-commenter

pr-commenter Bot commented Oct 3, 2024

Copy link
Copy Markdown

Benchmark results for BadBoggleSolver_run

Parameters

Baseline Candidate
config baseline candidate
profiler-version ddprof 0.18.0+aa511bcd.45491305 ddprof 0.18.0+36f24982.45686406

Summary

Found 0 performance improvements and 0 performance regressions! Performance is the same for 1 metrics, 0 unstable metrics.

See unchanged results
scenario Δ mean execution_time
scenario:ddprof -S bench-bad-boggle-solver BadBoggleSolver_run work 1000 same

@r1viollet
r1viollet merged commit 85a7085 into main Oct 3, 2024
@r1viollet
r1viollet deleted the r1viollet/alloc_tracker_remove_logs branch October 3, 2024 09:42
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.

2 participants