This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-mgw/+/25432 ) Change subject: Add multithreading for the virtual trunk ...................................................................... Patch Set 14: Code-Review-1 (29 comments) General commnets: This really needs further improvement imho. * Properly split the global trunk config and per-thread worker/information. Proper naming for such structures * Drop the osmux changes or at least move them to a new (previous?) commit * Move changes adding void *ctx to msg related functions to a new (previous?) commit * A second pass over all changes to fix style related stuff. Also, iiuc you are splitting trunks in threads using chunks (offset+size). However, allocation of endpoint to use probably happens incrementally, hence first 1 thread is filled, then the second one, etc. The easiest would probably be doing an endp_index % num_workers. Another option is to track current load of all workers and select the one with less load (not sure if you are doing that already, I saw you were counting the free endpoints for each trunk somewhere). https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14//COMMIT_MSG Commit Message: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14//COMMIT_MSG@37 PS14, Line 37: - the mgcp msg queue for mgcp commands, which the thread then ansers by answers https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h File include/osmocom/mgcp/mgcp.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@160 PS14, Line 160: struct global_osmux_options_t { no need to set a type name for the struct? This can be done in a separate commit btw. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@179 PS14, Line 179: } global_osmux_options; AFAIU everything in this mgcp_config is global, no need to specify it here. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@205 PS14, Line 205: struct to_trunkthread_mgcp_msg; Did you think aobut using "worker" terminology instead of trunkthread and similar? "worker_mgcp_msg". https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@206 PS14, Line 206: struct per_thread_info; worker_info? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@208 PS14, Line 208: struct msgb *mgcp_submit_message_to_trunkthread(struct mgcp_config *cfg, struct to_trunkthread_mgcp_msg* w); mgcp_submit_message_to_worker(), etc. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@209 PS14, Line 209: struct msgb* thread_handle_mgcp_message(struct to_trunkthread_mgcp_msg* w, struct per_thread_info *thread_info); worker_handle_mgcp_message(), etc. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h File include/osmocom/mgcp/mgcp_threads.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@33 PS14, Line 33: int get_trunk_thread_for_ep_name(const char *epname, struct mgcp_trunk *thread_parent_trunk); get_worker_for_ep_name(const char *epname, ...), etc. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@76 PS14, Line 76: union { call union "msg", then drop "msg" suffix from all types. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@77 PS14, Line 77: struct cfgmsg { all these structs can be anonymous since I guess they are never used alone. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@110 PS14, Line 110: } x; x? perhaps something like "hdr", "info",? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@115 PS14, Line 115: struct per_thread_info { struct worker? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@117 PS14, Line 117: struct mgcp_trunk *this_trunk; /* talloced, used as ctx */ IMHO having a mgcp_trunk as parent and the child being the same type looks REALLY strange and suspicious of data model not being proper. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@127 PS14, Line 127: void thread_push_msg(struct mgcp_trunk *trunk, unsigned int threadnum, void *elem); worker_push_msg https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_trunk.h File include/osmocom/mgcp/mgcp_trunk.h: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_trunk.h@22 PS14, Line 22: struct mgcp_trunk { I have the feeling all this is really confusing and it should be spit. Perhaps mgcp_trunk_cfg & mgcp_trunk_worker, or similar. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_osmux.c File src/libosmo-mgcp/mgcp_osmux.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_osmux.c@49 PS14, Line 49: void osmux_set_global_opt(struct global_osmux_options_t *ptr) { Is this really needed? mgcp_cfg is probably already global? In any case it could be split into another commit if really needed. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c File src/libosmo-mgcp/mgcp_protocol.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@186 PS14, Line 186: static struct msgb *create_resp(void* msgctx, struct mgcp_endpoint *endp, int code, This new msgctx pointer being passed all around can probably be done in a previous/next commit. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@330 PS14, Line 330: #define pdata w->x.pdata use pointer variables? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@336 PS14, Line 336: if (rc < sizeof(rq.name)-1) { ASsigning w->x.msglen and then checking against rc here looks really weird and confusing. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@341 PS14, Line 341: memcpy(rq.name, (const char *)&w->msg[0], sizeof(rq.name)-1); osmo_strlcpy? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@399 PS14, Line 399: pdata.trans = (char *)(pdata.trans - &w->msg[0]); using the define here rather than w->x.pdata looks even more confusing. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@405 PS14, Line 405: if (strcmp(rq.name, "CRCX") == 0){ /* crcx -> pick ONE thread that has free endpoints */ spacing ){ https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_stat.c File src/libosmo-mgcp/mgcp_stat.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_stat.c@36 PS14, Line 36: __attribute__((no_sanitize("integer"))) what? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c File src/libosmo-mgcp/mgcp_threads.c: https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@44 PS14, Line 44: #define GETF(yy, zz) \ This stuff is already quite complex to follow, I bet having 4 3line functions takes less space than what you wrote here. Or write 4 one-line macros. https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@60 PS14, Line 60: ; ; in same line as while? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@342 PS14, Line 342: void *split_per_thead(void *info) worker_loop? worker_main? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@416 PS14, Line 416: while (1) { what? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@427 PS14, Line 427: prctl(PR_SET_NAME, "main", NULL, NULL, NULL); pthread_setname_np()? https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@453 PS14, Line 453: else We should keep osmo_select_main_ctx() in main() imho. Actually, it is kept there if trunk->use_threads is used, but otherwise the one in split_per_thead() is used for the main thread, which looks really confusing. -- 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: 14 Gerrit-Owner: Hoernchen <ewild at sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin at sysmocom.de> Gerrit-Comment-Date: Fri, 10 Sep 2021 13:02:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210910/bef79066/attachment.htm>