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 32:
(23 comments)
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32//COMMIT_MSG
Commit Message:
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32//COMMIT_MSG@37
PS32, Line 37: - the mgcp msg queue for mgcp commands, which the thread then ansers by
answers
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@38
PS32, Line 38: enum trunkthread_cfg_msg_t;
we don't usually use the _t type suffix. It's actually discouraged for non simple types in the kernel style guide.
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.
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.
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?
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.
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 understandable to pass w->x.msglen here/there.
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?
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 first sight.
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?
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.
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. Having the 3 functions is 12 lines, and easier to read and look up them in the code.
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. I first though it was the one organizing stuff splitting into several threads, but it's actually the main() of each thread. Rather call it thread_main() or thread_func() or alike.
Even better, thread_loop(), since it's calling the osmocom loop at the end.
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.
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".
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?
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.
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?
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?
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@386
PS32, Line 386: /* this trunk does not have rate ctrs or stat items, it only has atomic counters,
And hence is what we could have different structures for different main trunk and per-thread trunk.
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?
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?
https://gerrit.osmocom.org/c/osmo-mgw/+/25432/32/src/libosmo-mgcp/mgcp_threads.c@440
PS32, Line 440: /* currently unused
what about this?
--
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 <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: dexter <pmaier at sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Wed, 10 Nov 2021 14:01:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211110/af44eee0/attachment.htm>