Attention is currently required from: laforge, pespin. neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539?usp=email )
Change subject: per-HNB GTP-U traffic counters via nft ...................................................................... Patch Set 10: (8 comments) File include/osmocom/hnbgw/hnbgw.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a936ea4a_eeddd3d8 PS6, Line 402: struct { > Oh I didn't mean joining the dl and ul. […] yes i mean that too. i wouldn't do it here because nothing points to it, but my opinion is not strong. to keep it as is, resolve. to change it, say "change it please". File src/osmo-hnbgw/hnbgw_hnbap.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/056856c9_7e254a03 PS6, Line 571: hnb_persistent_connected(ctx->persistent); > Can actually an HNB be registered without being connected? I don't think so. > […] yes, but a hnb can be connected without being registered. (that is for a different redmine ticket) so do you prefer 'active' and 'inactive', or is 'connect' and 'disconnect' good enough. File src/osmo-hnbgw/hnbgw_vty.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/040c3b3d_0b639b97 PS9, Line 894: vty_out(vty, "%% WARNING: nft configuration changes need a restart of osmo-hnbw%s", VTY_NEWLINE); > still a typo in here. osmo-hnbw. hey! i fixed that! File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/47a5b1fd_bb91d2cc PS6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL; > Ok, I'm not really a fan of having this sort of generic thread which may or > may not do stuff, makes […] Done https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/9fea60c5_2b46deb8 PS6, Line 193: static const char * const nft_thread_req_type_name[] = { > no. […] i want to avoid iteration ala value_string, because all callers are entirely constants within this .c file https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a84eb397_87f47076 PS6, Line 415: static int update_ctr(struct rate_ctr_group *cg, int cgidx, uint64_t *last_val, uint64_t new_val) > Yes, after looking at the surrounding code. […] kk File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/db2f120f_9e4ea83d PS9, Line 626: /* From here on, until we receive the next NFT_THREAD_GET_COUNTERS in this thread, the > Possible future improvement here is to use a double-buffer to fill next batch > while the main thread […] if ever the time it takes to retrieve counters does overlap the wait time configured in timer X34, then we would simply add another nft_counters2 thread, submitting GET_COUNTERS requests round robin between them. repeat to taste. (but i don't expect that to be necessary.) https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/fc2fbb4d_735d293d PS9, Line 994: nft_kpi_get_counters_schedule(); > So you call nft_kpi_get_counters_schedule() once you receive + process the > counters, which will then […] i believe this was the result of our discussion: to wait X34 time *between* getting counters. This is nice because it is simple and avoids all such races you describe. What I don't like about it is that the metric from the user perspective should be "get timers every N seconds" -- when we wait in-between, we will "drift", elongating the period by the time it takes to retrieve counters. This is not very long really, but i'd prefer scheduling to start counters every N seconds, not wait N seconds in-between. Doing so means that we need to ensure avoiding races, making the code more complex. So how ambitious are we? I think I am fine with waiting X34 seconds *between* getting counters. it is good enough as i see it. Other opinions? -- To view, visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-hnbgw Gerrit-Branch: master Gerrit-Change-Id: Ib2f0a9252715ea4b2fe9c367aa65f771357768ca Gerrit-Change-Number: 36539 Gerrit-PatchSet: 10 Gerrit-Owner: neels <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Comment-Date: Wed, 22 May 2024 22:21:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels <[email protected]> Comment-In-Reply-To: laforge <[email protected]> Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
