Change in osmo-mgw[master]: Add multithreading for the virtual trunk

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.org
Fri Sep 10 13:02:34 UTC 2021


pespin 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>


More information about the gerrit-log mailing list