<p style="white-space: pre-wrap; word-wrap: break-word;">General commnets: This really needs further improvement imho.</p><ul><li>Properly split the global trunk config and per-thread worker/information. Proper naming for such structures</li><li>Drop the osmux changes or at least move them to a new (previous?) commit</li><li>Move changes adding void *ctx to msg related functions to a new (previous?) commit</li><li>A second pass over all changes to fix style related stuff.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">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).</p><p>Patch set 14:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432">View Change</a></p><p>29 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14//COMMIT_MSG">Commit Message:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14//COMMIT_MSG@37">Patch Set #14, Line 37:</a> <code style="font-family:monospace,monospace">- the mgcp msg queue for mgcp commands, which the thread then ansers by</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">answers</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h">File include/osmocom/mgcp/mgcp.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@160">Patch Set #14, Line 160:</a> <code style="font-family:monospace,monospace"> struct global_osmux_options_t {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">no need to set a type name for the struct?<br>This can be done in a separate commit btw.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@179">Patch Set #14, Line 179:</a> <code style="font-family:monospace,monospace"> } global_osmux_options;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">AFAIU everything in this mgcp_config is global, no need to specify it here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@205">Patch Set #14, Line 205:</a> <code style="font-family:monospace,monospace">struct to_trunkthread_mgcp_msg;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Did you think aobut using "worker" terminology instead of trunkthread and similar? "worker_mgcp_msg".</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@206">Patch Set #14, Line 206:</a> <code style="font-family:monospace,monospace">struct per_thread_info;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">worker_info?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@208">Patch Set #14, Line 208:</a> <code style="font-family:monospace,monospace">struct msgb *mgcp_submit_message_to_trunkthread(struct mgcp_config *cfg, struct to_trunkthread_mgcp_msg* w);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">mgcp_submit_message_to_worker(), etc.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp.h@209">Patch Set #14, Line 209:</a> <code style="font-family:monospace,monospace">struct msgb* thread_handle_mgcp_message(struct to_trunkthread_mgcp_msg* w, struct per_thread_info *thread_info);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">worker_handle_mgcp_message(), etc.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h">File include/osmocom/mgcp/mgcp_threads.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@33">Patch Set #14, Line 33:</a> <code style="font-family:monospace,monospace">int get_trunk_thread_for_ep_name(const char *epname, struct mgcp_trunk *thread_parent_trunk);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">get_worker_for_ep_name(const char *epname, ...), etc.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@76">Patch Set #14, Line 76:</a> <code style="font-family:monospace,monospace"> union {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">call union "msg", then drop "msg" suffix from all types.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@77">Patch Set #14, Line 77:</a> <code style="font-family:monospace,monospace"> struct cfgmsg {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">all these structs can be anonymous since I guess they are never used alone.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@110">Patch Set #14, Line 110:</a> <code style="font-family:monospace,monospace"> } x;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">x? perhaps something like "hdr", "info",?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@115">Patch Set #14, Line 115:</a> <code style="font-family:monospace,monospace">struct per_thread_info {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">struct worker?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@117">Patch Set #14, Line 117:</a> <code style="font-family:monospace,monospace"> struct mgcp_trunk *this_trunk; /* talloced, used as ctx */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_threads.h@127">Patch Set #14, Line 127:</a> <code style="font-family:monospace,monospace">void thread_push_msg(struct mgcp_trunk *trunk, unsigned int threadnum, void *elem);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">worker_push_msg</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_trunk.h">File include/osmocom/mgcp/mgcp_trunk.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/include/osmocom/mgcp/mgcp_trunk.h@22">Patch Set #14, Line 22:</a> <code style="font-family:monospace,monospace">struct mgcp_trunk {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I have the feeling all this is really confusing and it should be spit.<br>Perhaps mgcp_trunk_cfg & mgcp_trunk_worker, or similar.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_osmux.c">File src/libosmo-mgcp/mgcp_osmux.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_osmux.c@49">Patch Set #14, Line 49:</a> <code style="font-family:monospace,monospace">void osmux_set_global_opt(struct global_osmux_options_t *ptr) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is this really needed? mgcp_cfg is probably already global?<br>In any case it could be split into another commit if really needed.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c">File src/libosmo-mgcp/mgcp_protocol.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@186">Patch Set #14, Line 186:</a> <code style="font-family:monospace,monospace">static struct msgb *create_resp(void* msgctx, struct mgcp_endpoint *endp, int code,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This new msgctx pointer being passed all around can probably be done in a previous/next commit.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@330">Patch Set #14, Line 330:</a> <code style="font-family:monospace,monospace"> #define pdata w->x.pdata</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">use pointer variables?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@336">Patch Set #14, Line 336:</a> <code style="font-family:monospace,monospace"> if (rc < sizeof(rq.name)-1) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ASsigning w->x.msglen and then checking against rc here looks really weird and confusing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@341">Patch Set #14, Line 341:</a> <code style="font-family:monospace,monospace"> memcpy(rq.name, (const char *)&w->msg[0], sizeof(rq.name)-1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">osmo_strlcpy?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@399">Patch Set #14, Line 399:</a> <code style="font-family:monospace,monospace"> pdata.trans = (char *)(pdata.trans - &w->msg[0]);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">using the define here rather than w->x.pdata looks even more confusing.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_protocol.c@405">Patch Set #14, Line 405:</a> <code style="font-family:monospace,monospace"> if (strcmp(rq.name, "CRCX") == 0){ /* crcx -> pick ONE thread that has free endpoints */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">spacing ){</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_stat.c">File src/libosmo-mgcp/mgcp_stat.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_stat.c@36">Patch Set #14, Line 36:</a> <code style="font-family:monospace,monospace">__attribute__((no_sanitize("integer")))</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c">File src/libosmo-mgcp/mgcp_threads.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@44">Patch Set #14, Line 44:</a> <code style="font-family:monospace,monospace">#define GETF(yy, zz) \</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This stuff is already quite complex to follow, I bet having 4 3line functions takes less space than what you wrote here.<br>Or write 4 one-line macros.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@60">Patch Set #14, Line 60:</a> <code style="font-family:monospace,monospace"> ;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">; in same line as while?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@342">Patch Set #14, Line 342:</a> <code style="font-family:monospace,monospace">void *split_per_thead(void *info)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">worker_loop? worker_main?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@416">Patch Set #14, Line 416:</a> <code style="font-family:monospace,monospace"> while (1) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@427">Patch Set #14, Line 427:</a> <code style="font-family:monospace,monospace"> prctl(PR_SET_NAME, "main", NULL, NULL, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">pthread_setname_np()?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432/14/src/libosmo-mgcp/mgcp_threads.c@453">Patch Set #14, Line 453:</a> <code style="font-family:monospace,monospace"> else</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432">change 25432</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-mgw/+/25432"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-mgw </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I31be8253600c8af0a43c967d0d128f5ba7b16260 </div>
<div style="display:none"> Gerrit-Change-Number: 25432 </div>
<div style="display:none"> Gerrit-PatchSet: 14 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Fri, 10 Sep 2021 13:02:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>