refactor: Make CAddrMan::cs non-recursive by hebasto · Pull Request #19238 · bitcoin/bitcoin

hebasto · 2020-06-10T15:39:51Z

This PR replaces RecursiveMutex CAddrMan::cs with Mutex CAddrMan::cs.

All of the related code branches are covered by appropriate lock assertions to insure that the mutex locking policy has not been changed by accident.

Related to #19303.

Based on #22025, and first three commits belong to it.

hebasto · 2020-06-10T15:41:34Z

cc @vasild :)

DrahtBot · 2020-06-10T16:50:51Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

refactor: Mark CAddrMan::Select and GetAddr const #21940 (refactor: Mark CAddrMan::Select const by MarcoFalke)
addrman: Make consistency checks a runtime option #20233 (addrman: Make consistency checks a runtime option by jnewbery)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

maflcko · 2020-06-10T16:51:24Z

Concept ACK. I think the reason that LOCKS_EXCLUDED isn't widely used in the code base is that it doesn't propagate up the call stack, so has a rather limited use case. Though, the new syntax claims to do that.

maflcko · 2020-06-10T16:53:02Z

oh wait you are not adding the annotations. Any reason for that?

hebasto · 2020-06-10T17:01:29Z

@MarcoFalke

oh wait you are not adding the annotations. Any reason for that?

Added :)

hebasto · 2020-06-10T17:04:11Z

@MarcoFalke probably a1071f8 deserves its own PR as it blocks the similar changes in other parts of the code, right?

vasild

Looks good, some suggestions below.

The last two commits:

Add means to handle negative capabilities in thread safety annotations
refactor: Add negative thread safety annotations to CAddrMan

are somewhat unrelated and maybe deserve a separate PR if they cause some controversy or if another reviewer asks to move them away to ease review. Since I already reviewed everything I am fine with them as they are.

src/addrman.h

src/sync.h

hebasto · 2020-06-11T14:20:46Z

Updated d957293 -> b6712ec (pr19238.02 -> pr19238.04):

the commit "Add means to handle negative capabilities in thread safety annotations" has been separated into Add means to handle negative capabilities in the Clang Thread Safety annotations #19249, and this PR has been rebased on the latter one
negative mutex requirements added to each commit opportunistically

vasild

ACK b6712ec

…Thread Safety annotations f8213c0 Add means to handle negative capabilities in thread safety annotations (Hennadii Stepanov) Pull request description: This commit is separated from #19238, and it adds support of [Negative Capabilities](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#negative) in the Clang Thread Safety Analysis attributes. > Negative requirements are an alternative `EXCLUDES` [`LOCKS_EXCLUDED`] that provide a stronger safety guarantee. A negative requirement uses the `REQUIRES` [`EXCLUSIVE_LOCKS_REQUIRED`] attribute, in conjunction with the ! operator, to indicate that a capability should not be held. Examples of usage: - #19238 (for a class) - https://github.com/hebasto/bitcoin/tree/200610-addrman-tsn (for the whole code base) ACKs for top commit: MarcoFalke: Approach ACK f8213c0 vasild: ACK f8213c0 Tree-SHA512: 86d992826b87579661bd228712ae5ee6acca6f70b885ef7e96458974eac184e4874a525c669607ba6b6c861aa4806409a8792d100e6914c858bcab43d31cfb1b

hebasto · 2020-06-17T11:29:07Z

Updated b6712ec -> 9cd0ed8 (pr19238.04 -> pr19238.05):

rebased due to the merging of Add means to handle negative capabilities in the Clang Thread Safety annotations #19249
all of the related code branches are covered by appropriate lock assertions to insure that the locking policy has not been changed by accident

hebasto · 2020-06-17T11:33:37Z

The OP has been updated.

src/addrman.h

hebasto · 2020-06-18T09:09:10Z

@sipa Mind reviewing this PR?

hebasto · 2020-06-18T18:14:05Z

btw, how about convention to add the _cs suffix to the name of the function that is called from within the critical section already guarded by the locked instance of the Mutex?

vasild · 2020-06-18T19:28:14Z

btw, how about convention to add the _cs suffix to the name of the function that is called from within the critical section already guarded by the locked instance of the Mutex?

I like it, and it is shorter than ...NonLockHelper(), but what about the CamelCaseConvention? Also, it would get fuzzy if more than one mutex is involved and a method requires one to be held and the other not...

hebasto · 2021-05-27T14:16:12Z

Rebased ee79105 -> e2787fb (pr19238.10 -> pr19238.11) due to the merging of #22025.

vasild

ACK e2787fb

hebasto · 2021-06-12T12:33:36Z

Rebased e2787fb -> af95505 (pr19238.11 -> pr19238.12) due to the conflict with #18722.

vasild

ACK af95505

jnewbery · 2021-06-14T13:30:24Z

Code review ACK af95505

maflcko · 2021-06-14T14:17:27Z

There might be a silent merge conflict

maflcko · 2021-06-14T14:19:49Z

test/fuzz/addrman.cpp:110:30: error: 'Check' is a private member of 'CAddrMan'
    (void)/*const_*/addr_man.Check();
                             ^
./addrman.h:722:10: note: declared private here
    void Check()
         ^
1 error generated.

The unit test is single threaded, so there's no need to hold the mutex between Good() and Attempt(). This change avoids recursive locking in the CAddrMan::Attempt function. Co-authored-by: John Newbery <[email protected]>

hebasto · 2021-06-14T14:24:18Z

There might be a silent merge conflict

With #21941. Rebasing.

Change in the addrman.h header is move-only.

This change improves readability, and follows Developer Notes.

Co-authored-by: John Newbery <[email protected]>

hebasto · 2021-06-14T14:30:28Z

@MarcoFalke @jnewbery @vasild

Rebased af95505 -> ae98aec (pr19238.12 -> pr19238.13) due to the silent merge conflict with #21941.

vasild

ACK ae98aec

DrahtBot added Refactoring Tests labels Jun 10, 2020

maflcko removed the Tests label Jun 10, 2020

This was referenced Jun 10, 2020

Implement ADDRv2 support (part of BIP155) #19031

Closed

addrman: improve performance by using more suitable containers #18722

Merged

hebasto closed this Jun 11, 2020

hebasto reopened this Jun 11, 2020

vasild reviewed Jun 11, 2020

View reviewed changes

src/addrman.h Outdated Show resolved Hide resolved

src/sync.h Show resolved Hide resolved

hebasto force-pushed the 200610-addrman-mx branch from d957293 to 1817751 Compare June 11, 2020 13:58

hebasto mentioned this pull request Jun 11, 2020

Add means to handle negative capabilities in the Clang Thread Safety annotations #19249

Merged

hebasto force-pushed the 200610-addrman-mx branch from 1817751 to b6712ec Compare June 11, 2020 14:08

vasild approved these changes Jun 11, 2020

View reviewed changes

hebasto force-pushed the 200610-addrman-mx branch from b6712ec to 9cd0ed8 Compare June 17, 2020 11:21

hebasto mentioned this pull request Jun 17, 2020

Replace all of the RecursiveMutex instances with the Mutex ones #19303

Open

35 tasks

vasild reviewed Jun 17, 2020

View reviewed changes

src/addrman.h Outdated Show resolved Hide resolved

hebasto mentioned this pull request Jun 20, 2020

refactor: Replace RecursiveMutex with Mutex in CTxMemPool #19306

Closed

vasild mentioned this pull request Jun 20, 2020

sync: detect double lock from the same thread #19337

Merged

hebasto force-pushed the 200610-addrman-mx branch from ee79105 to e2787fb Compare May 27, 2021 14:12

vasild approved these changes May 28, 2021

View reviewed changes

DrahtBot added the Needs rebase label Jun 12, 2021

hebasto force-pushed the 200610-addrman-mx branch from e2787fb to af95505 Compare June 12, 2021 12:29

DrahtBot removed the Needs rebase label Jun 12, 2021

vasild approved these changes Jun 14, 2021

View reviewed changes

hebasto and others added 2 commits June 14, 2021 17:21

test: Drop excessive locking in CAddrManTest::SimConnFail

2da9554

The unit test is single threaded, so there's no need to hold the mutex between Good() and Attempt(). This change avoids recursive locking in the CAddrMan::Attempt function. Co-authored-by: John Newbery <[email protected]>

refactor: Avoid recursive locking in CAddrMan::size

efc6fac

hebasto and others added 8 commits June 14, 2021 17:28

Make CAddrMan::Check private

0670397

Change in the addrman.h header is move-only.

refactor: Fix CAddrMan::Check style

f77d9c7

This change improves readability, and follows Developer Notes.

refactor: Avoid recursive locking in CAddrMan::Check

187b7d2

refactor: Apply consistent pattern for CAddrMan::Check usage

f79a664

Co-authored-by: John Newbery <[email protected]>

refactor: Avoid recursive locking in CAddrMan::Clear

b138973

Co-authored-by: John Newbery <[email protected]>

Add thread safety annotations to CAddrMan public functions

5ef1d0b

Add AssertLockHeld to CAddrMan private functions

f5d1c7f

refactor: Make CAddrMan::cs non-recursive

ae98aec

hebasto force-pushed the 200610-addrman-mx branch from af95505 to ae98aec Compare June 14, 2021 14:29

vasild approved these changes Jun 14, 2021

View reviewed changes

maflcko merged commit 3a2c84a into bitcoin:master Jun 14, 2021

hebasto deleted the 200610-addrman-mx branch June 14, 2021 15:03

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jun 14, 2021

Merge bitcoin#19238: refactor: Make CAddrMan::cs non-recursive

12d86d6

gwillen pushed a commit to ElementsProject/elements that referenced this pull request Jun 1, 2022

Merge 3a2c84a into merged_master (Bitcoin PR bitcoin/bitcoin#19238)

6b94940

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

Conversation

hebasto commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto commented Jun 10, 2020

Uh oh!

DrahtBot commented Jun 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

maflcko commented Jun 10, 2020

Uh oh!

maflcko commented Jun 10, 2020

Uh oh!

hebasto commented Jun 10, 2020

Uh oh!

hebasto commented Jun 10, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hebasto commented Jun 11, 2020

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 17, 2020

Uh oh!

hebasto commented Jun 17, 2020

Uh oh!

Uh oh!

hebasto commented Jun 18, 2020

Uh oh!

hebasto commented Jun 18, 2020

Uh oh!

vasild commented Jun 18, 2020

Uh oh!

hebasto commented May 27, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto commented Jun 12, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery commented Jun 14, 2021

Uh oh!

maflcko commented Jun 14, 2021

Uh oh!

maflcko commented Jun 14, 2021

Uh oh!

hebasto commented Jun 14, 2021

Uh oh!

hebasto commented Jun 14, 2021

Uh oh!

vasild left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

hebasto commented Jun 10, 2020 •

edited

Loading

DrahtBot commented Jun 10, 2020 •

edited

Loading