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 &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Attention: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Comment-Date: Tue, 21 May 2024 17:44:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment