Replace `RecursiveMutex cs_mapLocalHost` with Mutex, and rename it #24099

pull w0xlt wants to merge 2 commits into bitcoin:master from w0xlt:mutex-mapLocalHost changing 4 files +15 −15

w0xlt commented at 11:44 PM on January 18, 2022: contributor

This PR is related to #19303 and gets rid of the RecursiveMutex cs_mapLocalHost.
DrahtBot added the label P2P on Jan 18, 2022
DrahtBot added the label RPC/REST/ZMQ on Jan 18, 2022
DrahtBot commented at 5:46 AM on January 19, 2022: member


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



Conflicts

Reviewers, this pull request conflicts with the following ones:
- #20196 (net: fix GetListenPort() to derive the proper port by vasild)
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.

in src/net.cpp:115 in 3e05ab9537 outdated

 111 | @@ -112,9 +112,9 @@ static const uint64_t RANDOMIZER_ID_ADDRCACHE = 0x1cf2e4ddd306dda9ULL; // SHA256
 112 |  //
 113 |  bool fDiscover = true;
 114 |  bool fListen = true;
 115 | -RecursiveMutex cs_mapLocalHost;
 116 | -std::map<CNetAddr, LocalServiceInfo> mapLocalHost GUARDED_BY(cs_mapLocalHost);
 117 | -static bool vfLimited[NET_MAX] GUARDED_BY(cs_mapLocalHost) = {};
 118 | +Mutex map_localhost_mutex;

hebasto commented at 7:40 AM on January 19, 2022:

Suggesting to add g_ prefix to explicitly note that this mutex belongs to the global namespace:

Mutex g_maplocalhost_mutex;

Also see #19180.

w0xlt commented at 10:33 AM on January 19, 2022:

Thanks for suggestion. Done.

hebasto commented at 7:40 AM on January 19, 2022: member

Approach ACK 3e05ab9537dd4478c66b26f295b63e8081a24bc5.
shaavan commented at 9:21 AM on January 19, 2022: contributor
Approach ACK 3e05ab9537dd4478c66b26f295b63e8081a24bc5

Going commit wise:
1. The renaming of the mutex cs_mapLocalHost -> map_localhost_mutex, is an improvement, in my opinion.
2. The type of map_localhost_mutex is changed from RecursiveMutex to Mutex. I checked through the codebase where map_localhost_mutex is used for any instances of recursive lockings, and I found none. Also, I was able to compile this PR without any UB warnings successfully. So I think it’s safe to convert the type of this mutex from RecursiveMutex to Mutex.
I shall ACK this PR as soon as this suggestion by @hebasto is addressed.

scripted-diff: rename cs_mapLocalHost -> g_maplocalhost_mutex

-BEGIN VERIFY SCRIPT-
s() { sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' $1; }
s src/net.cpp
s src/net.h
s src/rpc/net.cpp
s src/test/net_tests.cpp
-END VERIFY SCRIPT-

a7da1409bc

refactor: replace RecursiveMutex g_maplocalhost_mutex with Mutex 5e7e4c9f6e
w0xlt force-pushed on Jan 19, 2022
shaavan approved
shaavan commented at 12:05 PM on January 19, 2022: contributor
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e

Changes since my last review:
- map_localhost_mutex -> g_maplocalhost_mutex.
The updated PR compiled successfully on Ubuntu 20.04.
theStack approved
theStack commented at 4:47 PM on January 19, 2022: member

ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e

Checked that within all critical sections that are protected by g_maplocalhost_mutex, there is no chance another one is called, i.e. a regular Mutex is sufficient.
hebasto approved
hebasto commented at 8:59 PM on January 19, 2022: member
ACK 5e7e4c9f6e57f5333bd17a20b0c85a78d032998e, I have reviewed the code and it looks OK, I agree it can be merged.

In particular, I've verified that there are no recursive mutex locking on the path GetLocal() --> CNetAddr::GetReachabilityFrom() --> CNetAddr::IsRoutable().

A script in the "scripted-diff" commit could be more robust:
```
sed -i 's/cs_mapLocalHost/g_maplocalhost_mutex/g' -- $(git grep --files-with-matches 'cs_mapLocalHost')
```
MarcoFalke merged this on Jan 20, 2022
MarcoFalke closed this on Jan 20, 2022
sidhujag referenced this in commit e37897e16a on Jan 20, 2022
w0xlt deleted the branch on Jan 24, 2022
DrahtBot locked this on Jan 24, 2023

Contributors

Labels

Linked (view graph)

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

Replace `RecursiveMutex cs_mapLocalHost` with Mutex, and rename it #24099

Conflicts