laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25432 )
Change subject: Add multithreading for the virtual trunk ...................................................................... Patch Set 27: (7 comments) https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h File include/osmocom/mgcp/mgcp_threads.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_threads.h@90 PS27, Line 90: struct tapmsg { I think 'tap' and 'freep' require some kind of explanation comment. cfg/vty/loop is obvious to me. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h File include/osmocom/mgcp/mgcp_trunk.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@30 PS27, Line 30: struct per_thread_info *this_thread_info; what is the rationale of using a union here? Wasting 8 bytes for a pointer is not all that much. Having separate pointers would allow us to catch "wrong" dereferences, i.e. if a master trunk code accesses this_thread_info, it would get a NULL pointer and crash, instead of operating on "forbidden" dat structures. Likewise also the other way around: If a thread accesses thread_info, it would be NULL in a per-thread mgcp_trunk and thus crash? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/include/osmocom/mgcp/mgcp_trunk.h@65 PS27, Line 65: unsigned int number_endpoints_offset; comment required. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@a1402 PS27, Line 1402: * that we walk over all endpoints on the trunk in order to drop all likewise with this move? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@324 PS27, Line 324: #define rq w->x.rq not sure I'm a fan of this, writing those few more characters make it more cleare what is done in the code. Not a blocker, just my thinking. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1188 PS27, Line 1188: if (!endp || !mgcp_endp_avail(endp)) { is moving this around really related to multithreading, or is it an unrelated change that should come before the multithread patch? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/27/src/libosmo-mgcp/mgcp_protocol.c@1576 PS27, Line 1576: for_each_line (line, rq->pdatap->save) { unrelated whitespace change -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/25432 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: I31be8253600c8af0a43c967d0d128f5ba7b16260 Gerrit-Change-Number: 25432 Gerrit-PatchSet: 27 Gerrit-Owner: Hoernchen <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: dexter <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Tue, 12 Oct 2021 16:42:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
