Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

vasild · 2021-05-13T12:30:51Z

This is a piece of #21878, chopped off to ease review.

The following pattern was duplicated in CConnman:

lock
create a copy of vNodes, add a reference to each one 
unlock
... use the copy ... 
lock
release each node from the copy
unlock

Put that code in a RAII helper that reduces it to:

create snapshot "snap"
... use the copy ... 
// release happens when "snap" goes out of scope

DrahtBot · 2021-05-13T15:17:05Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#22829 (refactor: various RecursiveMutex replacements in CConnman by theStack)
#21879 (Wrap accept() and extend usage of Sock by vasild)
#21878 (Make all networking code mockable 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.

src/net.cpp

jnewbery · 2021-05-14T09:31:56Z

Concept ACK

vasild · 2021-05-14T11:00:18Z

644726117...92f62d9ea: add an explicit commit that shows we intentionally don't process newly accepted nodes, as discussed in #21943 (comment)

src/net.cpp

practicalswift · 2021-05-15T07:51:32Z

Concept ACK

amitiuttarwar

concept ACK, some thoughts & questions as I familiarize myself with the code

src/net.h

src/net.cpp

src/net.h

vasild · 2021-05-17T15:19:31Z

92f62d9...00a8c94: address review suggestions

promag

Code review ACK 00a8c94.

vasild · 2021-06-22T09:25:08Z

00a8c943d7...072896b7cf: rebase due to conflicts

jonatack · 2021-09-19T17:08:55Z

Concept ACK, particularly if this can reduce vNodes lock contentions in these areas that are among the most frequent and long-lasting of those I'm seeing on my nodes with -debug=lock.

It looks like the CI stalled out on the last push. In any case, it rebases cleanly onto current master, the debug build is clean, and the unit/functional test suite is green for me locally. Reviewing.

jonatack

Tested ACK 072896b rebased to current master. Reviewed, debug built, and ran unit tests on each commit. Ran a node with this branch rebased to master on mainnet for a day (edit: 3 days) with -debug=lock and 30-40 peers and seeing an impressive reduction in the frequent vNodes lock contentions with grep "lock contention cs_vNodes" ~/.bitcoin/debug.log and very few contentions with grep "connman.cs_vNodes" ~/.bitcoin/debug.log

Nice work!

src/net.cpp

src/net.h

src/net.cpp

rebroad · 2021-09-25T09:46:52Z

concept ACK

vasild · 2021-09-28T07:29:44Z

072896b7cf...a4fbf1ba59: rebase and address review suggestions

Invalidates ACK from @jonatack.

jonatack · 2021-09-28T09:10:42Z

Diff-review ACK a4fbf1b per git range-diff a9d0cec 072896b a4fbf1b following my earlier full review (#21943 (review))

promag

Code review ACK a4fbf1b.

vasild · 2021-10-08T09:21:52Z

40e0bc8de2...99c1af5a8f: rebase due to conflicts

Invalidates ACK from @jonatack

Previously invalidated ACK from @promag

jonatack

re-ACK 99c1af5 per git range-diff 927586 40e0bc8 99c1af5 following my earlier full review #21943 (review), only change is rebase

jnewbery

Near ACK with one suggestion for clarifying the logic sequence.

src/net.cpp

LarryRuane

Perhaps this is outside the scope of the PR, but I'd like:

     void Release()
     {
+        assert(nRefCount > 0);
         nRefCount--;
     }

src/net.cpp

src/net.h

src/net.cpp

vasild · 2021-10-26T08:10:39Z

99c1af5a8f...8589c55e27: rebase and reorganize CConnMan::SocketHandler(), following comments from above: 1 and 2.

Accepting new connections does not require the snapshot of the existing connected nodes, so make this explicit by accepting new connections after the snapshot has been destroyed, at the end of CConnMan::SocketHandler(). Move the pieces of code from CConnMan::SocketHandler() that handle existing connections and accept new ones into their own methods and call those methods from CConnMan::SocketHandler() to improve the readability.

Invalidates ACK from @jonatack

Previously invalidated ACK from @promag

promag

Code review ACK 8589c55.

src/net.cpp

vasild · 2021-11-12T09:54:17Z

8589c55e27...db0a01e0d1: remove unnecessary interruptNet check from SocketEvents(), add such a check to SocketHandlerListening(). Thanks, @promag, #21943 (comment) !

Invalidates ACK from @promag

Previously invalidated ACK from @jonatack

promag

Code review ACK db0a01e, moved the early return to a loop where it makes more sense.

jonatack

Following up on my feedback #21943 (comment), as of db0a01e the commits are now out of order. The last commit

style: remove unnecessary braces

They were needed to define the scope of `LOCK(cs_vNodes)` which was
removed in the previous commit.

should be after the second commit net: keep reference to each node during socket wait (or squashed).

Almost-ACK db0a01e with some suggestions below if you retouch.

src/net.cpp

src/net.h

jonatack · 2021-11-17T12:10:10Z

src/net.cpp

-    if (interruptNet) return;
+    std::set<SOCKET> recv_set;
+    std::set<SOCKET> send_set;
+    std::set<SOCKET> error_set;


4097d14 unneeded style change, would leave as-is (and as currently exists in both of the SocketEvents() functions)

- std::set<SOCKET> recv_set; - std::set<SOCKET> send_set; - std::set<SOCKET> error_set; - + std::set<SOCKET> recv_set, send_set, error_set;

I think it is good to always declare one variable per line.

Consider:

struct SocketSets { std::set<SOCKET> recv; std::set<SOCKET> send; std::set<SOCKET> error; }

@LarryRuane, something like this is coming as a subsequent change from #21878 (see commit net: introduce Sock::WaitMany() and the structures WaitEvents and WaitData introduced in that commit).

The current combination of sets stores the sockets in "one pile of sockets that are ready for read" and "one pile of sockets that are ready for write" (notice that a given socket may be in both). It is more convenient to have just one pile of sockets each one with attached ready-for-read and ready-for-write flags.

I like the idea of abstracting this but it seems out of scope of this PR.

It is more convenient to have just one pile of sockets each one with attached ready-for-read and ready-for-write flags.

Eventually it depends on the selection mechanism used isn't it? When it's abstracted, might as well store it in a format ready to feed to say, poll() or select() whatever is used.

Yes, those 3 sets are dragged around as parameters to functions and as local variables. It is an obvious improvement to pack them together somehow. Not the purpose of this PR. Once this is PR is merged I will chop off another piece from #21878 into a smaller/manageable PR that will contain the commit which introduces such a packing net: introduce Sock::WaitMany(). We can discuss it there, I don't have a strong opinion on how they are packed exactly. Or if this PR is stuck we can move the discussion to #21878 where WaitData is introduced.

Once this is PR is merged I will chop off another piece from #21878 into a smaller/manageable PR that will contain the commit which introduces such a packing net: introduce Sock::WaitMany()

There are too many conflicts if I try to cherry-pick the WaitMany() stuff on top of bare master - it touches the same areas of code as the preceding commits from #21878. I chopped off those preceding commits in separate PRs instead: #23601 and #23604.

WaitMany | net: use Sock::WaitMany() instead of CConnman::SocketEvents() stuff | net: introduce Sock::WaitMany() | net: also wait for exceptional events in Sock::Wait() #23601 | net: don't check if the listening socket is valid | scripted-diff: rename CNode::cs_hSocket to CNode::m_sock_mutex #23604 | net: use Sock in CNode | fuzz: move FuzzedSock earlier in src/test/fuzz/util.h | net: change CreateNodeFromAcceptedSocket() to take Sock #21879 | net: use Sock in CConnman::ListenSocket | net: add new method Sock::Accept() that wraps accept()

src/net.cpp

src/net.h

The following pattern was duplicated in CConnman: ```cpp lock create a copy of vNodes, add a reference to each one unlock ... use the copy ... lock release each node from the copy unlock ``` Put that code in a RAII helper that reduces it to: ```cpp create snapshot "snap" ... use the copy ... // release happens when "snap" goes out of scope ```

Create the snapshot of `CConnman::vNodes` to operate on earlier in `CConnman::SocketHandler()`, before calling `CConnman::SocketEvents()` and pass the `vNodes` copy from the snapshot to `SocketEvents()`. This will keep the refcount of each node incremented during `SocketEvents()` so that the `CNode` object is not destroyed before `SocketEvents()` has finished. Currently in `SocketEvents()` we only remember file descriptor numbers (when not holding `CConnman::cs_vNodes`) which is safe, but we will change this to remember pointers to `CNode::m_sock`.

They were needed to define the scope of `LOCK(cs_vNodes)` which was removed in the previous commit. Re-indent in a separate commit to ease review (use `--ignore-space-change`).

`CConnman::SocketHandler()` does 3 things: 1. Check sockets for readiness 2. Process ready listening sockets 3. Process ready connected sockets Split the processing (2. and 3.) into separate methods to make the code easier to grasp. Also, move the processing of listening sockets after the processing of connected sockets to make it obvious that there is no dependency and also explicitly release the snapshot before dealing with listening sockets - it is only necessary for the connected sockets part.

vasild · 2021-11-18T12:41:45Z

db0a01e0d1...f52b6b2d9f: pick nits

Invalidates ACK from @promag

Previously invalidated ACK from @jonatack

LarryRuane

code review ACK f52b6b2
comments are optional

src/net.cpp

LarryRuane · 2021-11-19T21:22:21Z

src/net.cpp

-    if (interruptNet) return;
+    std::set<SOCKET> recv_set;
+    std::set<SOCKET> send_set;
+    std::set<SOCKET> error_set;


Consider:

struct SocketSets { std::set<SOCKET> recv; std::set<SOCKET> send; std::set<SOCKET> error; }

promag

Code review ACK f52b6b2, only format changes and comment tweaks since last review.

jonatack · 2021-11-23T21:55:24Z

ACK f52b6b2 changes since last review are reordered commits, removing an unneeded local variable, and code formatting and documentation improvements

… of CConnman::vNodes f52b6b2 net: split CConnman::SocketHandler() (Vasil Dimov) c7eb19e style: remove unnecessary braces (Vasil Dimov) 664ac22 net: keep reference to each node during socket wait (Vasil Dimov) 75e8bf5 net: dedup and RAII-fy the creation of a copy of CConnman::vNodes (Vasil Dimov) Pull request description: _This is a piece of bitcoin/bitcoin#21878, chopped off to ease review._ The following pattern was duplicated in CConnman: ```cpp lock create a copy of vNodes, add a reference to each one unlock ... use the copy ... lock release each node from the copy unlock ``` Put that code in a RAII helper that reduces it to: ```cpp create snapshot "snap" ... use the copy ... // release happens when "snap" goes out of scope ACKs for top commit: jonatack: ACK f52b6b2 changes since last review are reordered commits, removing an unneeded local variable, and code formatting and documentation improvements LarryRuane: code review ACK f52b6b2 promag: Code review ACK f52b6b2, only format changes and comment tweaks since last review. Tree-SHA512: 5ead7b4c641ebe5b215e7baeb7bc0cdab2a588b2871d9a343a1d518535c55c0353d4e46de663f41513cdcc79262938ccea3232f6d5166570fc2230286c985f68

laanwj · 2021-11-24T16:50:17Z

Looks like github didn't detect the merge, closing.

…nman::v…

vasild mentioned this pull request May 13, 2021

Make all networking code mockable #21878

Closed

12 tasks

fanquake added the P2P label May 13, 2021

DrahtBot mentioned this pull request May 13, 2021

net_processing: lock clean up #21527

Closed

promag reviewed May 13, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

vasild force-pushed the raiify_copying_vnodes branch from 6447261 to 92f62d9 Compare May 14, 2021 10:54

jnewbery reviewed May 14, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

amitiuttarwar reviewed May 15, 2021

View reviewed changes

src/net.h Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.h Show resolved Hide resolved

jnewbery reviewed May 15, 2021

View reviewed changes

src/net.h Show resolved Hide resolved

vasild force-pushed the raiify_copying_vnodes branch from 92f62d9 to 00a8c94 Compare May 17, 2021 14:47

promag reviewed May 17, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 4, 2021

Randomize message processing peer order #22144

Merged

DrahtBot added the Needs rebase label Jun 16, 2021

vasild force-pushed the raiify_copying_vnodes branch from 00a8c94 to 072896b Compare June 22, 2021 09:25

DrahtBot removed the Needs rebase label Jun 22, 2021

DrahtBot mentioned this pull request Jun 23, 2021

Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) #20487

Merged

DrahtBot mentioned this pull request Aug 30, 2021

refactor: various RecursiveMutex replacements in CConnman #22829

Merged

jonatack approved these changes Sep 20, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Show resolved Hide resolved

sipa added this to Blockers in High-priority for review Sep 23, 2021

vasild force-pushed the raiify_copying_vnodes branch from 072896b to a4fbf1b Compare September 28, 2021 07:29

promag reviewed Sep 28, 2021

View reviewed changes

DrahtBot removed the Needs rebase label Oct 8, 2021

jonatack approved these changes Oct 8, 2021

View reviewed changes

jnewbery reviewed Oct 8, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

LarryRuane reviewed Oct 14, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

src/net.h Show resolved Hide resolved

src/net.h Outdated Show resolved Hide resolved

src/net.cpp Outdated Show resolved Hide resolved

vasild force-pushed the raiify_copying_vnodes branch from 99c1af5 to 8589c55 Compare October 26, 2021 08:01

DrahtBot mentioned this pull request Oct 27, 2021

refactor: wrap accept() and extend usage of Sock #21879

Merged

promag reviewed Oct 28, 2021

View reviewed changes

src/net.cpp Outdated Show resolved Hide resolved

vasild force-pushed the raiify_copying_vnodes branch from 8589c55 to db0a01e Compare November 12, 2021 09:51

promag reviewed Nov 12, 2021

View reviewed changes

jonatack reviewed Nov 17, 2021

View reviewed changes

vasild added 4 commits November 18, 2021 13:29

style: remove unnecessary braces

c7eb19e

They were needed to define the scope of `LOCK(cs_vNodes)` which was removed in the previous commit. Re-indent in a separate commit to ease review (use `--ignore-space-change`).

vasild force-pushed the raiify_copying_vnodes branch from db0a01e to f52b6b2 Compare November 18, 2021 12:42

LarryRuane reviewed Nov 19, 2021

View reviewed changes

promag reviewed Nov 23, 2021

View reviewed changes

laanwj removed this from Blockers in High-priority for review Nov 24, 2021

laanwj closed this Nov 24, 2021

vasild deleted the raiify_copying_vnodes branch November 25, 2021 08:35

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Nov 25, 2021

Merge bitcoin#21943: Dedup and RAII-fy the creation of a copy of CCon…

3f723e2

…nman::v…

rebroad mentioned this pull request Dec 2, 2021

DeleteNode() occurring during ProcessMessages() #23657

Closed

bitcoin locked and limited conversation to collaborators Nov 26, 2022

Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

Dedup and RAII-fy the creation of a copy of CConnman::vNodes #21943

Conversation

vasild commented May 13, 2021

DrahtBot commented May 13, 2021 • edited

Conflicts

jnewbery commented May 14, 2021

vasild commented May 14, 2021

practicalswift commented May 15, 2021

amitiuttarwar left a comment

Choose a reason for hiding this comment

vasild commented May 17, 2021

promag left a comment

Choose a reason for hiding this comment

vasild commented Jun 22, 2021

jonatack commented Sep 19, 2021

jonatack left a comment • edited

Choose a reason for hiding this comment

rebroad commented Sep 25, 2021

vasild commented Sep 28, 2021 • edited

jonatack commented Sep 28, 2021

promag left a comment

Choose a reason for hiding this comment

vasild commented Oct 8, 2021

jonatack left a comment

Choose a reason for hiding this comment

jnewbery left a comment

Choose a reason for hiding this comment

LarryRuane left a comment • edited

Choose a reason for hiding this comment

vasild commented Oct 26, 2021 • edited

promag left a comment

Choose a reason for hiding this comment

vasild commented Nov 12, 2021

promag left a comment

Choose a reason for hiding this comment

jonatack left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vasild commented Nov 18, 2021 • edited

LarryRuane left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

promag left a comment

Choose a reason for hiding this comment

jonatack commented Nov 23, 2021 • edited

laanwj commented Nov 24, 2021

DrahtBot commented May 13, 2021 •

edited

jonatack left a comment •

edited

vasild commented Sep 28, 2021 •

edited

LarryRuane left a comment •

edited

vasild commented Oct 26, 2021 •

edited

jonatack left a comment •

edited

vasild commented Nov 18, 2021 •

edited

jonatack commented Nov 23, 2021 •

edited