Hoernchen has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25432 )
Change subject: Add multithreading for the virtual trunk ...................................................................... Patch Set 32: (20 comments) https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/include/osmocom/mgcp/mgcp_threads.h File include/osmocom/mgcp/mgcp_threads.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/include/osmocom/mgcp/mgcp_threads.h@73 PS32, Line 73: struct to_trunkthread_cfg_msg { > This "to" prefix here is very confusing imho. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/include/osmocom/mgcp/mgcp_threads.h@115 PS32, Line 115: struct per_thread_info { > imho this "per_" prefix can also be dropped, it confuses more than helps. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/include/osmocom/mgcp/mgcp_threads.h@121 PS32, Line 121: int tid; /* thread number handling this subtrunk */ > as in gettid()? or some posix threads id? as in "thread number handling this subtrunk" https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@319 PS32, Line 319: ssize_t rc = w->x.msglen; > this looks a bit weird, assigning a length to rc like this. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@326 PS32, Line 326: if (rc < sizeof(rq->name) - 1) { > afaict you are only using rc here and in line 333 before setting it below, > may be just more understa […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@331 PS32, Line 331: memcpy(rq->name, (const char *)&w->msg[0], sizeof(rq->name) - 1); > no null char at the end of rq->name is required? this copies the 4 byte string into a 5 byte array in a zero-initialized struct. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@396 PS32, Line 396: ti->dlcx_in_queue++; > what's the relation between dlc_in_queue and having free endpoints? I cannot > see the relation at fir […] it might be not empty, and there might be other crcx in it that will be processed first, so just ignoring it means we could accidentally bother a thread that has no free eps after all. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@407 PS32, Line 407: return NULL; > Doesn't "w" need to be freed in any of these paths? no, w lives on the caller stack, and is copied to the queue, so always safe. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_protocol.c@1531 PS32, Line 1531: * endpoints, possible open connections are forcefully dropped */ > need to fix formatting here. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c File src/libosmo-mgcp/mgcp_threads.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@44 PS32, Line 44: #define GETF(yy, zz) \ > That's 10 lines for all the macro + definition stuff. […] Macros are safe and idiot proof and it's not my fault that we enforce kernel style formatting which takes up a lot of space, I'm not going to make code difficult to work with and a copy-paste trap just because formatting makes this look ugly. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@337 PS32, Line 337: void *split_per_thead(void *info) > THis function name is misleading. […] Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@344 PS32, Line 344: top_ctx = talloc_named_const(NULL, 0, "top_thread_ctx"); > idea: maybe add this_thread_number to the string here. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@357 PS32, Line 357: struct mgcp_trunk *t = thread_info->this_trunk; > Please move this to the start, I was not finding it when looking for "t". Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@369 PS32, Line 369: t->ratectr = (struct mgcp_ratectr_trunk){ 0 }; > what about this? I think there's APIs to reset the counters? Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@377 PS32, Line 377: prctl(PR_SET_NAME, thrdname, NULL, NULL, NULL); > POSIX threads have pthread_setname_np() for that. Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@382 PS32, Line 382: log_enable_multithread(); > iirc this needs to be called only once per program? Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@383 PS32, Line 383: //msgb_talloc_ctx_init(OTC_GLOBAL,0); nooo is global! > what about this? Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@397 PS32, Line 397: osmo_stat_item_set(osmo_stat_item_group_get_item(t->stats.common, TRUNK_STAT_ENDPOINTS_TOTAL), > is this safe? No it's not, which is why the next patch that fixes the counters exists, since we don't want to fix libosmocore. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@413 PS32, Line 413: //FIXME: shutdown > something to do with this? Done https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@440 PS32, Line 440: /* currently unused > what about this? Done -- 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: 32 Gerrit-Owner: Hoernchen <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: dexter <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Wed, 17 Nov 2021 20:34:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
