Skip to content

Add scheduler deadlock detection#15735

Closed
jamesob wants to merge 2 commits intobitcoin:masterfrom
jamesob:2019-04-serialize-scheduler
Closed

Add scheduler deadlock detection#15735
jamesob wants to merge 2 commits intobitcoin:masterfrom
jamesob:2019-04-serialize-scheduler

Conversation

@jamesob
Copy link
Contributor

@jamesob jamesob commented Apr 3, 2019

Related to #14289 and follows up on #15205. See a relevant IRC chat here: http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-22.html

This helps detect deadlocks in the scheduler (and by association validation interface callbacks) by adding a flag which causes the scheduler to execute tasks synchronously in the calling thread. It adds a Travis job that runs the functional test suite with this flag enabled.

You can verify that the flag works as intended by applying this diff

diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp
index 44432cd0a1..bb4046f119 100644
--- a/src/test/validation_block_tests.cpp
+++ b/src/test/validation_block_tests.cpp
@@ -27,6 +27,7 @@ struct TestSubscriber : public CValidationInterface {
 
     void UpdatedBlockTip(const CBlockIndex* pindexNew, const CBlockIndex* pindexFork, bool fInitialDownload) override
     {
+        SyncWithValidationInterfaceQueue();
         BOOST_CHECK_EQUAL(m_expected_tip, pindexNew->GetBlockHash());
     }

rebuilding with ./configure ... CPPFLAGS="... -DDEBUG_SERIALIZE_SCHEDULER", then running ./src/test/test_bitcoin -t validation_block_tests, and watching as the deadlock detector hulks out.

jamesob added 2 commits April 3, 2019 17:30
@jamesob
Add -DDEBUG_SERIALIZE_SCHEDULER
6470af9 
For use in test environments to detect deadlocks in
CScheduler-managed tasks.
@jamesob
Add a Travis job that checks for CScheduler deadlocks
64c06a4
@jamesob
Copy link
Contributor Author

jamesob commented Apr 4, 2019

This segfaults on functional test run at the moment due to a stack overflow thanks to scheduler.cpp:Repeat(), so I'm gonna think about a workaround.

@jamesob jamesob closed this Apr 4, 2019
@jamesob jamesob mentioned this pull request Apr 18, 2019
Merged
pull bot pushed a commit to jaschadub/bitcoin that referenced this pull request Apr 30, 2019
Merge bitcoin#15849: Thread names in logs and deadlock debug tools
2c35fe6 
8722e54 threads: add thread names to deadlock debugging message (James O'Beirne)
383b186 threads: prefix log messages with thread names (James O'Beirne)
ddd95cc tests: add threadutil tests (James O'Beirne)
ae5f2b6 threads: introduce util/threadnames, refactor thread naming (James O'Beirne)
188ca75 disable HAVE_THREAD_LOCAL on unreliable platforms (James O'Beirne)

Pull request description:

  I'm resurrecting this one (from bitcoin#13168) because I need it to make progress on bitcoin#15735.

  It's now off by default and can be turned on with `-logthreadnames=1`.

  Ran some benchmarks (IBD from local peer from 500_000 -> 504_000) and it's within spitting distance either on or off:

  ### threadnames off (default)

  #### 2018-05-threadnames.3 vs. master (absolute)
  |                      name                      | iterations |   2018-05-threadnames.3    |           master           |
  |------------------------------------------------|-----------:|----------------------------|----------------------------|
  | ibd.local.500000.504000.dbcache=2048           |          3 | 376.1584 (± 9.2944)        | 392.3414 (± 13.4238)       |
  | ibd.local.500000.504000.dbcache=2048.mem-usage |          3 | 2236117.3333 (± 1845.9623) | 2238690.6667 (± 2669.3487) |

  #### 2018-05-threadnames.3 vs. master (relative)
  |                      name                      | iterations | 2018-05-threadnames.3 | master |
  |------------------------------------------------|-----------:|----------------------:|-------:|
  | ibd.local.500000.504000.dbcache=2048           |          3 |                     1 |  1.043 |
  | ibd.local.500000.504000.dbcache=2048.mem-usage |          3 |                     1 |  1.001 |

  ### threadnames on

  #### 2018-05-threadnames-take-2 vs. master (absolute)
  |                      name                      | iterations | 2018-05-threadnames-take-2 |           master           |
  |------------------------------------------------|-----------:|----------------------------|----------------------------|
  | ibd.local.500000.504000.dbcache=2048           |          3 | 367.6861 (± 0.3941)        | 364.1667 (± 0.9776)        |
  | ibd.local.500000.504000.dbcache=2048.mem-usage |          3 | 2238461.3333 (± 3697.8730) | 2237014.6667 (± 3307.6966) |

  #### 2018-05-threadnames-take-2 vs. master (relative)
  |                      name                      | iterations | 2018-05-threadnames-take-2 | master |
  |------------------------------------------------|-----------:|---------------------------:|-------:|
  | ibd.local.500000.504000.dbcache=2048           |          3 |                      1.010 |   1.00 |
  | ibd.local.500000.504000.dbcache=2048.mem-usage |          3 |                      1.001 |   1.00 |
  ```

ACKs for commit 8722e5:
  Empact:
    utACK bitcoin@8722e54
  jnewbery:
    utACK 8722e54
  MarcoFalke:
    re-utACK 8722e54 (Only change since my previous review is DEFAULT_LOGTHREADNAMES=false and stylistic updates

Tree-SHA512: 50af992708295b8d680cf10025262dd964e599a356bdfc1dfc84fb18c00afabcb34d3d12d551b0677ff81f8fccad0e17c1d5b24dfecb953a913bc77fdd1a4577
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@jamesob