May 2024 - gerrit-log - lists.osmocom.org

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by neels 22 May '24

22 May '24

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 6: (26 comments) Patchset: PS6: > Hi @nhofmeyr@sysmocom. […] Actually it is the same thread running twice. There is one "instruction set" and the caller decides how to split the thread responsibilities. It would not be a problem to run only one thread, or three, which is the point of this implementation. For review of the blocking/nonblocking nature, you may find interesting to know that all requests are handled by the maintenance thread, except that the second thread handles ONLY and ALL the GET_COUNTERS requests. This is also described in a comment in-code IIRC. For review of functionality it should not make any difference how often the thread runs. File include/osmocom/hnbgw/hnbgw.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/c52e8836_5a81c567 PS6, Line 384: /* When rules to count traffic to and from this hNodeB are present, this reflects the state in nftables > I'm not understanding this comment. You first mention "rules", and after "nftables rules". […] "rules" and "nftables rules" is the same thing. is this a better comment? : State that the main thread needs in order to know what to request from the worker threads. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/0d7e16e5_ff928953 PS6, Line 402: struct { > maybe you want to declare the struct type once and use it twice here. […] (in an earlier patch set i actually had the two combined in one struct.) The two nft_kpi_handle and nft_kpi_val are also used in function signatures and static vars, but this combined one (ul, dl) exists only here. I can make an extra struct for that, but that makes more lines of code, not less. should i still do that? i wouldn't do it, but my opinion is not strong https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/63da49ff_ea065842 PS6, Line 446: bool enable; > I actually think they are self-explanatory. nft_kpi.enable means nft_kpi is enabled, the nft_kpi.table_name is used as name for the nft table... =) https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/d018d3ed_0a5347a2 PS6, Line 483: struct { > I actually think they are self-explanatory. I'm not a friend of comments like /* timer to get nftables counters */ nft_kpi.get_counters_timer; https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/c94caac6_48d70682 PS6, Line 514: void hnb_persistent_connected(struct hnb_persistent *hnbp); > difficult to understand what are these exactly only by looking at the name. […] (not sure i understand what you mean by stack level) i'll add comments to describe. they are a late addition, it seems i forgot. File include/osmocom/hnbgw/nft_kpi.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/88db9e0a_5132b9cb PS6, Line 7: struct nft_kpi_handle { > I actually think they are simple, clarly named and self-explanatory. i can explain in a comment that nftables has "handles" required to remove unnamed rules...? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/b46445d9_0114dca7 PS6, Line 19: void nft_kpi_hnb_persistent_init(struct hnb_persistent *hnbp); > These APIs seems to be counterparts, but sounds weird having init vs remove. ah yes, i had names based on "init the ruleset" at some point, this is a leftover. i'll make it "add" instead. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/2c1be44b_7aa4dd63 PS6, Line 21: int nft_kpi_hnb_start(struct hnb_persistent *hnbp, const struct osmo_sockaddr_str *gtpu_remote); > This addr is probably coming from RANAP in binary form, so feels weird having it converted to a osmo […] nftables uses strings (also all logging), so it makes sense to use strings here. (This is the same as in MGCP, also a string based protocol, where it is also often more useful to store the strings than the sockaddr) File src/osmo-hnbgw/hnbgw.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/992ef815_ac9bbe62 PS6, Line 346: hnb_persistent_disconnected(ctx->persistent); > so API is not really protocol related. […] (or "active" and "inactive", s.a.) https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/ae15427e_d9a3ee85 PS6, Line 590: if (osmo_sockaddr_str_from_osa(&remote_str, &osa)) { > there we go, a sockaddr transformed into a sockaddr_str, not sure if there's a real reason to do so, […] yes https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a32a6f58_262c4afe PS6, Line 607: { > Sounds like a self-made FSM right? :P […] exactly what i mean above. But this patch is not about fixing the HNBAP / HNB activity tracking. It only shows up because I add the two functions that should already have existed before this patch. So my idea here is to add these "stubs" for a proper FSM to be done in a future patch. That future FSM should then invoke these properly, instead of how osmo-hnbgw does it now. File src/osmo-hnbgw/hnbgw_hnbap.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/4a88b01d_5dbf52f7 PS6, Line 571: hnb_persistent_connected(ctx->persistent); > Since this happens at registration time, maybe rename it to "hnb_persistent_registered"? I still fin […] yes, but how osmo-hnbgw handles HNBAP is weird. We start up a HNB when HNBAP HNB Register is received, but we practically never see a HNBAP HNB De-Register happen. We mostly just see the HNB's Iuh link drop without prior notice; often even after the same cell has registered again from a new connection. My opinion here is not very strong. I can make it hnb_persistent_registered() and hnb_persistent_deregistered() -- but then again we have the ambiguity that the "deregistered()" is in practice just a disconnected link... I think we need to actually further clarify the connected/disconnected and the HNBAP Register and Deregister behavior of osmo-hnbgw, in general, in separate patches. (e.g. what if HNBAP Deregisters but the Iuh link is never dropped... I think osmo-hnbgw currently does not catch that) Maybe "hnb_active" and "hnb_inactive" are more general terms, where "active" means *both* HNBAP Registered *and* Iuh still connected? (and we can later make osmo-hnbgw actually do this properly in all cases) what do you think File src/osmo-hnbgw/hnbgw_vty.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/3fa54b1c_c4bb85aa PS6, Line 884: DEFUN(cfg_hnbgw_nft_kpi, cfg_hnbgw_nft_kpi_cmd, > I think we have VTY CMD flags to state that changes apply after a process restart, you could use the […] I wanted to, but it does not exist AFAICT: ``` /*! Attributes (flags) for \ref cmd_element */ enum { CMD_ATTR_DEPRECATED = (1 << 0), CMD_ATTR_HIDDEN = (1 << 1), CMD_ATTR_IMMEDIATE = (1 << 2), CMD_ATTR_NODE_EXIT = (1 << 3), CMD_ATTR_LIB_COMMAND = (1 << 4), }; ``` https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/6cae760a_478f759a PS6, Line 899: g_hnbgw->config.nft_kpi.table_name = talloc_strdup(g_hnbgw, set_table_name); > These 3 lines is basically a osmo_talloc_replace_string right? yup, it incrementally became this so i didn't notice, thx File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/0490a540_ff8cff4a PS6, Line 18: #include <inttypes.h> > Some overall architecture comment description here would be great, to quickly figure out the interes […] hm i thought i had that somewhere... https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/3e9f7fdd_5972acb9 PS6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL; > Not exactly imho. […] the nft_thread could be launched any number of times. IMHO this is what the __thread is for, exactly. This is a per-thread global state. For any thread not running an nft command queue, g_nft_thread == NULL (here main() has g_nft_thread == NULL, and the two nft threads have this pointing at the static global state for that thread.) I would like this to stay this way. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/0e65f920_bd46f367 PS6, Line 120: LOGP(DNFT, LOGL_ERROR, "error running nft cmd %s#%u: rc=%d cmd=%s\n", > if nft fails, we have no way to say so to the callback? yes we have the rc that is carried back to the main thread in the queued response. we could argue whether all those failures are handled properly, in nft_thread_t2m_cb(). But the hard part is: what do you want to do when nftables didn't work. All we can do really is output an error for the user to know and otherwise carry on as we did, to not completely disrupt service...? Or should we crash the program? Should we reject the hNodeB because our counters don't work? none of it makes much sense really... "nft just has to work." https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/795a7c3f_17857e37 PS6, Line 182: enum nft_thread_req_type { > These can probably be moved up before the static functions. can, but this is the part of the file that defines the thread request queue, so i'd like this to be here. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/f8daf865_8ced844c PS6, Line 193: static const char * const nft_thread_req_type_name[] = { > osmo_value_string? no. i can give reasons, but i hope "no" is sufficient? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/d21ae530_c830874b PS6, Line 207: struct { > IIUC these are actually the params for the different primitives right? hence the naming yes https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/320add70_b368e60e PS6, Line 251: OSMO_ASSERT(g_nft_thread); > why do you need the TLS g_nft_hread allocated statically? Why not allocate it using talloc here? […] Why allocate it using talloc? If you tried to implement that you'd see that it makes no sense to request that. If at all the main thread would talloc them, but why. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a565f984_6fbc3077 PS6, Line 297: static void nft_t2m_enqueue(struct nft_thread *t, struct nft_thread_req *req) > you may want to use the concept of "worker thread" vs "main thread", because well, main is also a th […] The terms I use so far are "main()" and "(nft-)thread", because main() isn't started by pthread_create(). Do you want this changed to nft_w2m_ and _m2w_? This is either just a fringe naming opinion bikeshed (as in, why bother me with a single letter in a static function's name?) -- or it is an important naming convention i am not aware of. which is it? "ctx": i don't like meaningless words like "foo_context" or "foo_data" or "foo_storage", because every struct foo is all of these, implicitly. It is not the thread's "context", it is the entire thread and all of its state. It simply is the thread, period. I really dislike the name "hnb_context" in osmo-hnbgw. It is just the hnb state, not some context around the hnb state. For talloc it is really an actual ctx, and the name fits well there, but anywhere else we use the "ctx" name wrongly IMHO. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/2ba58d8a_a76cb26d PS6, Line 415: static int update_ctr(struct rate_ctr_group *cg, int cgidx, uint64_t *last_val, uint64_t new_val) > I first though about cgi when reading this. […] but the second thought made you realize that there is a cg counter group involved, right =) https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/22ecfa02_078ef35e PS6, Line 419: rate_ctr_add2(cg, cgidx, new_val - *last_val); > Oh I'm surprised to find this function here. […] update_ctr does what it does: updating counters. The important part is where is it called from: update_ctr() is a code-dedup helper for hnb_update_counters(), that in turn is a make-my-loop-more-readable helper for main_thread_handle_get_counters_resp(). is that not clear enough? =) https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/64391f7e_3ff09bf0 PS6, Line 452: size_t want = g_nft_thread->counters_len + 64; > I'd say usually one does *2 here. […] I used \*2 in an array list for a diff algorithm implementation in the https://gameoftrees.org/ project, and learnt that *2 is bad because it runs away too fast. My code blew up memory usage, quite unreasonably. 64 is a reasonable amount of hNodeBs that we can tolerate a reallocation happening for. since a new hnb showing up is such a rare event, it would even be ok to reallocate for each single new hnb. So now we reduce by factor 64, way way more than good enough. (be aware that once we have enough room for the nr of hnb that are in use, there are no reallocations anymore at all.) -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Wed, 22 May 2024 00:16:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by pespin 21 May '24

21 May '24

Attention is currently required from: laforge, neels. pespin 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 6: (1 comment) File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/94748393_6ce3a7e9 PS6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL; > > Simply have one var per thread, even better when you split stuff into different files. […] Not exactly imho. __thread is useful when you have N (tons of, unknown) threads spawned which do generic work and use generic code. This is not really the case here. You have a well known number of threads (2), each having its own specific purpose and hence context. So it's totally fine having 2 global variables instead of having to use TLS every time each of the threads need to access its context. -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Tue, 21 May 2024 20:32:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: laforge <laforge(a)osmocom.org> Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by laforge 21 May '24

21 May '24

Attention is currently required from: neels, pespin. laforge 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 6: (6 comments) File include/osmocom/hnbgw/hnbgw.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/f24a208b_f76c6399 PS6, Line 394: struct osmo_sockaddr_str addr_remote; > open question: to be seen whether it makes more sense to use a osmo_sockaddr instead here. […] libnftables interfacing is string based, as the patch should show in other places https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/97e043bd_58c8421e PS6, Line 446: bool enable; > some short comment here on what these do would be welcome. I actually think they are self-explanatory. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/0cc2af5c_e83598e1 PS6, Line 483: struct { > same here, some comment welcome. I actually think they are self-explanatory. File include/osmocom/hnbgw/nft_kpi.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/34cfe65b_bb6094e9 PS6, Line 7: struct nft_kpi_handle { > Some comments docummenting these structs and fields may be welcome. I actually think they are simple, clarly named and self-explanatory. File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/30e6cd61_14520ed5 PS6, Line 35: { > I guess these will be filled up by follow-up patches adding hashtables? those are the stubs for when you are compiling osmo-hnbgw without libnftables support https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/9becf34f_bf329996 PS6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL; > Simply have one var per thread, even better when you split stuff into different files. isn't that what __thread is for? -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 21 May 2024 20:16:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by pespin 21 May '24

21 May '24

Attention is currently required from: laforge, neels. pespin 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 6: (1 comment) Patchset: PS6: Hi @nhofmeyr@sysmocom.de, I wrote some comments while digesting the patch, feel free to apply whatever you think makes sense. In general looks promising. What I would really like, is that you split each of the worker threads into its own file, otherwise it's a bit mindblowing trying to digest that (for me now, and for any reader that comes after me when the code is merged and has to debug it). So I say: Please first split those into different files if possible and then I'll give a more through review through each of them on that code. It should be mostly moving functions from one file to another (and if a lot other work is needed, then to me it also means something is entangled where it shouldn't, so it's good it becomes untangled). -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Tue, 21 May 2024 17:48:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by pespin 21 May '24

21 May '24

Attention is currently required from: laforge, neels. pespin 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 6: (27 comments) File include/osmocom/hnbgw/hnbgw.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/e5fba8c1_f58df936 PS6, Line 384: /* When rules to count traffic to and from this hNodeB are present, this reflects the state in nftables I'm not understanding this comment. You first mention "rules", and after "nftables rules". Not sure what are the differences. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/ace1cd1a_3147487a PS6, Line 394: struct osmo_sockaddr_str addr_remote; open question: to be seen whether it makes more sense to use a osmo_sockaddr instead here. It depends on whether we really use strings all the time or whether libnft or alike uses sockaddr. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/b7c35db9_bd89154c PS6, Line 402: struct { maybe you want to declare the struct type once and use it twice here. Maybe add it in osmocom/hnbgw/nft_kpi.h? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a7c66462_c1e777cf PS6, Line 446: bool enable; some short comment here on what these do would be welcome. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/b926458d_eb85ddce PS6, Line 483: struct { same here, some comment welcome. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a17a9596_e9973ccc PS6, Line 514: void hnb_persistent_connected(struct hnb_persistent *hnbp); difficult to understand what are these exactly only by looking at the name. Some functions that get called when hnb is connected or disconnected? at which stack level? File include/osmocom/hnbgw/nft_kpi.h: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/e8146148_bb9b5fc9 PS6, Line 7: struct nft_kpi_handle { Some comments docummenting these structs and fields may be welcome. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/6a3c44ba_8fae36bd PS6, Line 19: void nft_kpi_hnb_persistent_init(struct hnb_persistent *hnbp); These APIs seems to be counterparts, but sounds weird having init vs remove. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/e7e0fa6d_bc83b684 PS6, Line 21: int nft_kpi_hnb_start(struct hnb_persistent *hnbp, const struct osmo_sockaddr_str *gtpu_remote); This addr is probably coming from RANAP in binary form, so feels weird having it converted to a osmo_sockaddr_str instead of keeping it as a osmo_sockaddr. File src/osmo-hnbgw/hnbgw.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/f93ff87e_23b1f35f PS6, Line 346: hnb_persistent_disconnected(ctx->persistent); so API is not really protocol related. More like "attach/detach" maybe? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/dcf2a91b_764b7d1e PS6, Line 590: if (osmo_sockaddr_str_from_osa(&remote_str, &osa)) { there we go, a sockaddr transformed into a sockaddr_str, not sure if there's a real reason to do so, maybe inside nft we use strings? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/c0ac0e59_62a0cb56 PS6, Line 607: { Sounds like a self-made FSM right? :P May make sense to move to osmo_fsm. File src/osmo-hnbgw/hnbgw_hnbap.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/856b23fc_d075cf8f PS6, Line 571: hnb_persistent_connected(ctx->persistent); Since this happens at registration time, maybe rename it to "hnb_persistent_registered"? I still find the "connected" here a bit confusing. File src/osmo-hnbgw/hnbgw_vty.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/9f5d00ea_76ab979f PS6, Line 884: DEFUN(cfg_hnbgw_nft_kpi, cfg_hnbgw_nft_kpi_cmd, I think we have VTY CMD flags to state that changes apply after a process restart, you could use them here. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/272e2fa5_45bacedd PS6, Line 899: g_hnbgw->config.nft_kpi.table_name = talloc_strdup(g_hnbgw, set_table_name); These 3 lines is basically a osmo_talloc_replace_string right? File src/osmo-hnbgw/nft_kpi.c: https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/9840336e_39060b40 PS6, Line 18: #include <inttypes.h> Some overall architecture comment description here would be great, to quickly figure out the interesting APIs. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/a4b157a4_d5851d35 PS6, Line 35: { I guess these will be filled up by follow-up patches adding hashtables? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/36732648_bd0d1060 PS6, Line 86: static __thread struct nft_thread *g_nft_thread = NULL; having a thread-specific variable prefix with "g_" sounds weird, since it's not really globally available :) Maybe "self_nft_thread". EDIT: Ok after having a whole look at the file, I see this is actually a pointer only. But I don't really see a point in having a TLS variable for this. Simply have one var per thread, even better when you split stuff into different files. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/6319c0be_ca4b9197 PS6, Line 120: LOGP(DNFT, LOGL_ERROR, "error running nft cmd %s#%u: rc=%d cmd=%s\n", if nft fails, we have no way to say so to the callback? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/3334d6e0_009a955f PS6, Line 182: enum nft_thread_req_type { These can probably be moved up before the static functions. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/64893254_95ac9253 PS6, Line 193: static const char * const nft_thread_req_type_name[] = { osmo_value_string? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/bdd44b05_d8752923 PS6, Line 207: struct { IIUC these are actually the params for the different primitives right? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/2db3db0a_c904fdec PS6, Line 251: OSMO_ASSERT(g_nft_thread); why do you need the TLS g_nft_hread allocated statically? Why not allocate it using talloc here? This way we don't reserve space for other threads which may not be using it. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/ae1857cf_f251d546 PS6, Line 297: static void nft_t2m_enqueue(struct nft_thread *t, struct nft_thread_req *req) you may want to use the concept of "worker thread" vs "main thread", because well, main is also a thread. May make it more clear. Also renaming nft_thread to nft_thread_ctx. Just ideas in case you are interested. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/14214159_f6159830 PS6, Line 415: static int update_ctr(struct rate_ctr_group *cg, int cgidx, uint64_t *last_val, uint64_t new_val) I first though about cgi when reading this. maybe ctrg_idx? https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/e076794a_623a7154 PS6, Line 419: rate_ctr_add2(cg, cgidx, new_val - *last_val); Oh I'm surprised to find this function here. Mixing functions from both threads in the same section of the file makes everything confusing imho, difficult to fiugre out what runs where. It may even make sense to split this into 2 files, one per thread. Actually, ideally, that would be 3 files. One for each thread type. Alternatively main thread stuff can be moved to some existing file. https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/65e99c7b_5f10f474 PS6, Line 452: size_t want = g_nft_thread->counters_len + 64; I'd say usually one does *2 here. It's not like this will be called lots of times, but anyway you get less fragmentation probably. -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Comment-Date: Tue, 21 May 2024 17:44:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by Jenkins Builder 21 May '24

21 May '24

Attention is currently required from: laforge, neels, pespin. Jenkins Builder 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 6: (1 comment) File src/osmo-hnbgw/nft_kpi.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16230): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/19efdfdf_69942797 PS6, Line 651: if (period_us < 1) suspect code indent for conditional statements (7, 15) -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 21 May 2024 16:39:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by neels 21 May '24

21 May '24

Attention is currently required from: laforge, neels, pespin. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539?usp=email to look at the new patch set (#6). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: per-HNB GTP-U traffic counters via nft ...................................................................... per-HNB GTP-U traffic counters via nft Add optional feature: retrieve GTP-U traffic counters per hNodeB (not per individual subscriber!) using nftables, to provide new rate_ctr stats. This is a "workaround" to get performance indicators per hNodeB, without needing a UPF that supports URR. When an hNodeB registers, set up nftables rules to count GTP-U packets (UDP port 2152) to and from that hNodeB's address -- we are assuming that it is the same address that Iuh is connecting from. From the per-hNodeB packet and byte counters from nftables, also derive a "UE bytes" counter, which is counting only the GTP-U payload. Assume IP header of 20 bytes; UDP and GTP-U headers are 8 bytes each: ue_bytes = total_bytes - packets * (20 + 8 + 8) Query these periodically, as configurable by new timer X34. Default is one second of wait time between querying counters (excluding the time it takes to retrieve and update the counters). Add compile-time switch --enable-nftables, to build with/without external dependency libnftables. Default is without, as before. Add jenkins axis NFTABLES to switch --enable-nftables. Add cfg file option 'hnbgw' / 'nft-kpi' to enable use of nftables. This requires osmo-hnbgw to be run with cap_net_admin. The VTY config commands are always visible -- simplifies VTY testing. Refuse to start osmo-hnbgw when the user is requesting nft-kpi in the config but when built without --enable-nftables. Do nft commands in 2 separate threads. Run the same request queue implementation twice, with two thread workers to handle them: - one thread receives all requests to init the nft table, add and remove hNodeB counters, and start and stop counting for a specific hNodeB. - Another thread handles all retrieval and parsing of counters from nft. The main() thread hence never blocks for nftables commands, and services the responses from nft when they are ready, via an osmo_it_q registered in the main() select loop. Persistently keep an nftables named counter for each seen hNodeB cell id in the nftables ruleset, for the lifetime of a hnb_persistent instance that holds the target rate_ctrs. Add the rules to feed into these persistent counters to the ruleset when the particular cell attaches and detaches via HNBAP HNB (De-)Register. On hnb_persistent_free(), remove all items relating to this cell id from nftables, including the persistent named counters. Loosely related: upcoming patches will implement - a hashtable for faster cell id lookup (important for updating counters) Iecb81eba28263ecf90a09c108995f6fb6f5f81f2 - proper MNC-3-digit support in cell ids (better have a 100% correct primary key). Id9a91c80cd2745424a916aef4736993bb7cd8ba0 - idle timeout for disconnected hnbp, so we are sure stale state does not build up for eternity. Ic819d7cbc03fb39e98c204b70d016c5170dc6307 Related: SYS#6773 Related: OS#6425 Change-Id: Ib2f0a9252715ea4b2fe9c367aa65f771357768ca --- M configure.ac M contrib/jenkins.sh M debian/control M debian/rules M include/osmocom/hnbgw/Makefile.am M include/osmocom/hnbgw/hnbgw.h A include/osmocom/hnbgw/nft_kpi.h M src/osmo-hnbgw/Makefile.am M src/osmo-hnbgw/hnbgw.c M src/osmo-hnbgw/hnbgw_hnbap.c M src/osmo-hnbgw/hnbgw_vty.c A src/osmo-hnbgw/nft_kpi.c M src/osmo-hnbgw/osmo_hnbgw_main.c M src/osmo-hnbgw/tdefs.c M tests/osmo-hnbgw.vty 15 files changed, 1,298 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/39/36539/6 -- 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: 6 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by Jenkins Builder 21 May '24

21 May '24

Attention is currently required from: laforge, neels, pespin. Jenkins Builder 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 5: (5 comments) File src/osmo-hnbgw/nft_kpi.c: Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16223): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/df4ad6ff_ceda3078 PS5, Line 193: static const char *nft_thread_req_type_name[] = { static const char * array should probably be static const char * const Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16223): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/d6770d68_79f2c7f2 PS5, Line 620: while (1) { braces {} are not necessary for single statement blocks Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16223): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/7508f387_d8d5a2c9 PS5, Line 652: if (period_us < 1) suspect code indent for conditional statements (7, 15) Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16223): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/3bdc932d_afd931ff PS5, Line 653: period_us = 1; code indent should use tabs where possible Robot Comment from checkpatch (run ID jenkins-gerrit-lint-16223): https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/7901fdad_9cee6d30 PS5, Line 653: period_us = 1; please, no spaces at the start of a line -- 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: 5 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-Comment-Date: Tue, 21 May 2024 16:33:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[XL] Change in osmo-hnbgw[master]: per-HNB GTP-U traffic counters via nft
by neels 21 May '24

21 May '24

Attention is currently required from: laforge, neels, pespin. Hello Jenkins Builder, laforge, pespin, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified-1 by Jenkins Builder Change subject: per-HNB GTP-U traffic counters via nft ...................................................................... per-HNB GTP-U traffic counters via nft Add optional feature: retrieve GTP-U traffic counters per hNodeB (not per individual subscriber!) using nftables, to provide new rate_ctr stats. This is a "workaround" to get performance indicators per hNodeB, without needing a UPF that supports URR. When an hNodeB registers, set up nftables rules to count GTP-U packets (UDP port 2152) to and from that hNodeB's address -- we are assuming that it is the same address that Iuh is connecting from. From the per-hNodeB packet and byte counters from nftables, also derive a "UE bytes" counter, which is counting only the GTP-U payload. Assume IP header of 20 bytes; UDP and GTP-U headers are 8 bytes each: ue_bytes = total_bytes - packets * (20 + 8 + 8) Query these periodically, as configurable by new timer X34. Default is one second of wait time between querying counters (excluding the time it takes to retrieve and update the counters). Add compile-time switch --enable-nftables, to build with/without external dependency libnftables. Default is without, as before. Add jenkins axis NFTABLES to switch --enable-nftables. Add cfg file option 'hnbgw' / 'nft-kpi' to enable use of nftables. This requires osmo-hnbgw to be run with cap_net_admin. The VTY config commands are always visible -- simplifies VTY testing. Refuse to start osmo-hnbgw when the user is requesting nft-kpi in the config but when built without --enable-nftables. Do nft commands in 2 separate threads. Run the same request queue implementation twice, with two thread workers to handle them: - one thread receives all requests to init the nft table, add and remove hNodeB counters, and start and stop counting for a specific hNodeB. - Another thread handles all retrieval and parsing of counters from nft. The main() thread hence never blocks for nftables commands, and services the responses from nft when they are ready, via an osmo_it_q registered in the main() select loop. Persistently keep an nftables named counter for each seen hNodeB cell id in the nftables ruleset, for the lifetime of a hnb_persistent instance that holds the target rate_ctrs. Add the rules to feed into these persistent counters to the ruleset when the particular cell attaches and detaches via HNBAP HNB (De-)Register. On hnb_persistent_free(), remove all items relating to this cell id from nftables, including the persistent named counters. Loosely related: upcoming patches will implement - a hashtable for faster cell id lookup (important for updating counters) Iecb81eba28263ecf90a09c108995f6fb6f5f81f2 - proper MNC-3-digit support in cell ids (better have a 100% correct primary key). Id9a91c80cd2745424a916aef4736993bb7cd8ba0 - idle timeout for disconnected hnbp, so we are sure stale state does not build up for eternity. Ic819d7cbc03fb39e98c204b70d016c5170dc6307 Related: SYS#6773 Related: OS#6425 Change-Id: Ib2f0a9252715ea4b2fe9c367aa65f771357768ca --- M configure.ac M contrib/jenkins.sh M debian/control M debian/rules M include/osmocom/hnbgw/Makefile.am M include/osmocom/hnbgw/hnbgw.h A include/osmocom/hnbgw/nft_kpi.h M src/osmo-hnbgw/Makefile.am M src/osmo-hnbgw/hnbgw.c M src/osmo-hnbgw/hnbgw_hnbap.c M src/osmo-hnbgw/hnbgw_vty.c A src/osmo-hnbgw/nft_kpi.c M src/osmo-hnbgw/osmo_hnbgw_main.c M src/osmo-hnbgw/tdefs.c M tests/osmo-hnbgw.vty 15 files changed, 1,299 insertions(+), 5 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/39/36539/5 -- 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: 5 Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-Reviewer: pespin <pespin(a)sysmocom.de> Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: newpatchset

1 0

[M] Change in osmo-ttcn3-hacks[master]: asterisk: Use Action PJSIPAccessNetworkInfo
by pespin 21 May '24

21 May '24

Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/36886?usp=email to look at the new patch set (#2). The following approvals got outdated and were removed: Verified+1 by Jenkins Builder Change subject: asterisk: Use Action PJSIPAccessNetworkInfo ...................................................................... asterisk: Use Action PJSIPAccessNetworkInfo Validate P-Access-Network-Info should only be present in 2nd REGISTER sent over ipsec. Change-Id: I2759d12caeaca81a9224997a29541c325d65fe30 --- M asterisk/AMI_Functions.ttcn M asterisk/Asterisk_Tests.ttcn M asterisk/IMS_ConnectionHandler.ttcn M library/SIP_Templates.ttcn 4 files changed, 111 insertions(+), 0 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-ttcn3-hacks refs/changes/86/36886/2 -- To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/36886?usp=email 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: I2759d12caeaca81a9224997a29541c325d65fe30 Gerrit-Change-Number: 36886 Gerrit-PatchSet: 2 Gerrit-Owner: pespin <pespin(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset

1 0

2025

2024

2023

2022

gerrit-log May 2024