Attention is currently required from: Hoernchen. pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/26190 ) Change subject: rework the counters and stats so they work with the threaded mgw ...................................................................... Patch Set 13: (9 comments) File include/osmocom/mgcp/mgcp_trunk.h: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/241577c6_fb79c067 PS13, Line 38: struct mgcp_ratectr_trunk ratectr; /* NULL for the threads! */ And this kind of stuff is what makes me speak again about having different structs for the global state and one for each worker thread. File src/libosmo-mgcp/mgcp_endp.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/3f1d0163_347537f3 PS13, Line 128: if (endp->callid) { So these stats are being dropped? why? I don't recall reading about it in the commit description. File src/libosmo-mgcp/mgcp_stat.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/e4e8553e_45ecaa82 PS13, Line 84: atomic_uint_least64_t packets_rx = conn->atomic_counters[RTP_PACKETS_RX_CTR]; AFAIU these don't need to be atomic, since they are basically copied locally and used here, no synchonization needs to happen back. File src/libosmo-mgcp/mgcp_threads.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/43193be7_c55f3b34 PS13, Line 428: for (int i = 0; i < _MGCP_GENERAL_NUM_ENUMS; i++) { This kind of loops make me thing whether we'd be better using a mutex rather than accessing crazy amounts of atomic variables (barriers, etc.) https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/2dbf0814_e82c8eff PS13, Line 433: atomic_uint_least64_t endpoints_used = 0; why does this need to be atomic? It's just a local variable not being accessed by other threads? https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/e891b57e_33e1722b PS13, Line 490: /* wait for the threads to be done with init, so main thread can safely read atomics*/ space: atomics */ File src/libosmo-mgcp/mgcp_threads_vty.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/271344c9_1f30c61f PS13, Line 34: atomic_uint_least64_t tx_packets, tx_bytes; same ehre, why are these atomic? File src/libosmo-mgcp/mgcp_trunk.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/fa73d7f9_686e2888 PS13, Line 143: trunk->number_endpoints = number_endpoints; another stat being removed here? File tests/mgcp/mgcp_test.c: https://gerrit.osmocom.org/c/osmo-mgw/+/26190/comment/008d4859_bb485834 PS13, Line 1315: atomic_uint_least64_t test_ctr_in; no need to be atomic? -- To view, visit https://gerrit.osmocom.org/c/osmo-mgw/+/26190 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-mgw Gerrit-Branch: master Gerrit-Change-Id: Iaab3fc262649cb5fb886f0297a60286bde1ffeb0 Gerrit-Change-Number: 26190 Gerrit-PatchSet: 13 Gerrit-Owner: Hoernchen &lt;ewild(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-CC: dexter &lt;pmaier(a)sysmocom.de&gt; Gerrit-CC: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: Hoernchen &lt;ewild(a)sysmocom.de&gt; Gerrit-Comment-Date: Tue, 26 Apr 2022 09:39:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment