Attention is currently required from: neels, fixeria, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169 )
Change subject: fix umts_cell_id_name(): show CID
......................................................................
Patch Set 4: Code-Review+1
(1 comment)
File src/osmo-hnbgw/hnbgw.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169/comment/a729128b_401e0744
PS2, Line 348: return talloc_asprintf(OTC_SELECT, "%u-%u-L%u-R%u-S%u-C%u", ucid->mcc, ucid->mnc, ucid->lac, ucid->rac,
> > I hope this is not called everytime we log a line :) […]
I did a bit of synthetic benchmarking. In my benchmark (using fully asan enabled builds), I can run the static __thread buffer implementation 1294870 times per second on my laptop, while the OTC_SELECT variant runs "only" 808880 times per second.
I'm calling 5 prints for every osmo_select_main[_ctx]() iteration.
So there is a significant performance impact of 37%.
Of course, in a real-world use case there are more difficult to quantify implications such as potential talloc memory fragmentation, or probably more expensively the cache pollution by having to execute more code at different locations (talloc) and access more data structures (talloc data structures).
However, given the the overall unrealistic high execution count in the benchmark (we will rarely print more than 1000 per second and likely never hit a 10k/second in reality), I do agree that this 37% impact on the relatively small percentage of time we spend in printing overall is likely negligable.
I still don't get in general, why we should use a more expensive OTC_SELECT approach over a static buffer approach *without a clear requirement or reason*. It's not that it makes the code any easier to read or write.
So while I'm willing to unblock this change, I wouldn't want this to become standard practice all over the codebase, and I would certainly match any follow-up patch that converted it back to static buffers.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/33169
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hnbgw
Gerrit-Branch: master
Gerrit-Change-Id: Id903b8579ded8b908e644808e440d373bcca8da4
Gerrit-Change-Number: 33169
Gerrit-PatchSet: 4
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jun 2023 15:23:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: neels, pespin.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hlr/+/33097 )
Change subject: Port to new libosmogsm 'struct osmo_sub_auth_data2'
......................................................................
Patch Set 3:
(2 comments)
File TODO-RELEASE:
https://gerrit.osmocom.org/c/osmo-hlr/+/33097/comment/0564550d_683f2ba2
PS3, Line 10: libosmogsm UPDATE_DEP_VER update libosmogsm version dependency after Ie775fedba4a3fa12314c0f7c8a369662ef6a40df is released
> fyi libosmogsm > 1.8. […]
it didn't occur to me, thanks.
File src/auc.c:
https://gerrit.osmocom.org/c/osmo-hlr/+/33097/comment/635a85b4_3e4341c8
PS3, Line 119: vec[i].res_len = 8;
> maybe add a NOTICE log message if res_len was not 8, not let the user note that this is missing prop […]
I don't really get the comment. The old code never generated any res length != 8, so all this code is doing is enforcing the same behavior as before. Before this line, vec[i].res_len may not be initialized as it is the output variable passed by the function caller. It might be zero-initialized or contain random data. We need to set it to a supported value of libosmocore before calling osmo_auth_gen_vec*2 below. And in ordre to get the old behavior, we must set it to 8.
--
To view, visit https://gerrit.osmocom.org/c/osmo-hlr/+/33097
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-hlr
Gerrit-Branch: master
Gerrit-Change-Id: I3207c7bfb73e9ff5471e5c26b66639549e4d48a2
Gerrit-Change-Number: 33097
Gerrit-PatchSet: 3
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jun 2023 14:59:14 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33314 )
Change subject: fixup! RLCMAC_CSN1_Types: Add release 6 additions to PacketCellChangeNotification
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> > I guess this should be merged in the previous commit? […]
ah, didn't see it was from differenta authors. abandoning this one.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/33314
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I86102d3850c83bc29076bd13574547ea9dd12071
Gerrit-Change-Number: 33314
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jun 2023 14:54:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: fixeria, dexter.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/33310 )
Change subject: mgwpool: Document keepalive feature
......................................................................
Patch Set 3:
(2 comments)
File common/chapters/mgwpool.adoc:
https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/33310/comment/7f8f3f90_50a6…
PS2, Line 184: on the MGW against the MGW
> did you mean "on the MGCP against MGW"?
I'll fix this.
https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/33310/comment/10d430dc_3d5d…
PS2, Line 203: considering
> Not willing to block here, but it reads as: "the timeout considering the MGW to be non-reachable". […]
I really see no problem here: "It consists of a XYZ timeout considering the MGW to be non-reachable". It just means the timeout triggers some logic for the app to consider the MGW to be non-reachable.
I'll anyway rephrase since it seems it is creating problems.
--
To view, visit https://gerrit.osmocom.org/c/osmo-gsm-manuals/+/33310
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-gsm-manuals
Gerrit-Branch: master
Gerrit-Change-Id: I2cb4e2098b71b386278eb6026271a6d786a34c2a
Gerrit-Change-Number: 33310
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <pespin(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jun 2023 14:45:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: laforge, fixeria, pespin.
dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32995 )
Change subject: RLCMAC_CSN1_Types: Add release 6 additions to PacketCellChangeNotification
......................................................................
Patch Set 5:
(1 comment)
File library/RLCMAC_CSN1_Types.ttcn:
https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32995/comment/dce9cb26_d925…
PS1, Line 1030: variant (utran_target_cell) "PRESENCE(arfcn_bsic_presence = '1'B, utran_target_cell_presence = '0'B)"
> > Is this what you have in mind? […]
I have squashed your part into this patch now. Thanks for your input.
--
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/32995
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I4e1c63c06fb89111765df187a93db563e77c3fc4
Gerrit-Change-Number: 32995
Gerrit-PatchSet: 5
Gerrit-Owner: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 14 Jun 2023 14:44:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: dexter <pmaier(a)sysmocom.de>
Gerrit-MessageType: comment
