Attention is currently required from: laforge, neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36913?usp=email )
Change subject: add jhash.h, copied from linux/jhash.h
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> btw, I'm, not sure we really want to have all that code as a static inline like in the kernel. […]
I'm not saying we should do it one way or another, just raising the topic here.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36913?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0c9652bbc9e2a18b1200e7d63bb6f64ded7d75fa
Gerrit-Change-Number: 36913
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 23 May 2024 15:59:22 +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: laforge, neels.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/36913?usp=email )
Change subject: add jhash.h, copied from linux/jhash.h
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
btw, I'm, not sure we really want to have all that code as a static inline like in the kernel.
In the kernel it's just built statically but in here we are putting it inside a library, which makes it more difficult to manage if it ever needs improvements/fixing and recompiling.
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/36913?usp=email
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I0c9652bbc9e2a18b1200e7d63bb6f64ded7d75fa
Gerrit-Change-Number: 36913
Gerrit-PatchSet: 2
Gerrit-Owner: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Comment-Date: Thu, 23 May 2024 15:58:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
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 13:
(2 comments)
Patchset:
PS6:
> regarding split files, i kind of like that everything is contained in a single .c file. […]
With thew new comments you added it's "good enough" because one can clearly see which thread executes that function, so we can merge it this way for now.
My point is that, precisely, if you put all the nft thread in a seprate file, it suddenly becomes clear that all that code belongs to that thread, and you don't need to go marking each function specifying who runs it.
Furthermore, it becomes easier to follow interaction of 2 threads by having one file open by the side of the other one.
File src/osmo-hnbgw/nft_kpi.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/3ecb5f62_9a8da3da
PS12, Line 694: unsigned long period_us = osmo_tdef_get(hnbgw_T_defs, -34, OSMO_TDEF_US, 1000000);
> ah yes this is exactly what i thought
yes, this is simple, usual in code, so I'm not asking something out of this world I think. Feel free to submit as a follow-up patch, but I definetly think this should be merged soon after this patch is merged.
--
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: 13
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: Thu, 23 May 2024 15:48:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
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 13:
(1 comment)
File src/osmo-hnbgw/nft_kpi.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/e70b0cfe_bb5550fe
PS9, Line 994: nft_kpi_get_counters_schedule();
> I agree with neels that his approach implements what we discussed (for the reasons we discussed and […]
As mentioned, I'm just asking to account for the elapsed time between triggers by writing a few more lines of code, which I already presented as an example. It's 10 min of work, nothing requiring days or weeks of work...
If it's painful doing it in this same commit, feel free to submit it as a follow-up improvement, but this should be there when merging this feature because it's super easier and will make users not wasting time asking themselves why the timers are triggering out of time.
--
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: 13
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: Thu, 23 May 2024 15:44:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
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 13:
(5 comments)
Patchset:
PS6:
> Actually it is the same thread running twice. […]
regarding split files, i kind of like that everything is contained in a single .c file. I thought a bit and it might be even easier to understand if the nft_thread is in a separate .c file, but then we also need another .h file and so on...
for me it's fine in one .c file (with the new comments describing what is going on), but just say the word and i can try to split to another .c file too.
File src/osmo-hnbgw/hnbgw_hnbap.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/2c4c4abc_2eb93499
PS6, Line 571: hnb_persistent_connected(ctx->persistent);
> I agree pespin's wording is more clear.
Done
File src/osmo-hnbgw/hnbgw_vty.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/82819dea_b42b49ca
PS12, Line 885: "nft-kpi [TABLE_NAME]",
> shouldn't this be in some kind of #ifdef block to suppress offering those vty commands in case the p […]
This is a basic disagreement we encounter repeatedly.
IMHO parsing of a config file should be conceptually independent from what the binary supports.
- First read what the user intends, then error later if that is not possible.
- A command that is described in the docs should not appear as a syntax error; instead as a semantic error.
I much prefer if the VTY commands are always available also for practical reasons:
- to generate automatic xml-output of VTY commands for the manuals,
we don't have to specifically compile a binary with all possible features enabled.
We just dump the VTY with the simplest dependencies and have all the docs.
- allows simpler vty testing: a 'list' always shows the same items, etc.
- allows simpler working with cfg files: if parsing fails for a missing command,
it is hard to understand what is going on. The following instead is very clear:
"you configured nft-kpi but this binary is built without support"
- the user can pass a 'no nft-kpi' command to a binary without nft-kpi support,
which would otherwise be an error.
This patch quits the program if nft-kpi is present in the cfg but there is no support for that compiled in, with a sensible error message, at program startup.
File src/osmo-hnbgw/nft_kpi.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/5a7cbe46_24696b8d
PS9, Line 994: nft_kpi_get_counters_schedule();
> I agree with neels that his approach implements what we discussed (for the reasons we discussed and […]
an idea:
best of both worlds: measure the time it took from the last request, and if there is still some left over, wait only for the remaining time. If time is up, don't wait at all.
We can do that in a separate patch...
File src/osmo-hnbgw/nft_kpi.c:
https://gerrit.osmocom.org/c/osmo-hnbgw/+/36539/comment/1bd57bcc_2bd5dee9
PS12, Line 694: unsigned long period_us = osmo_tdef_get(hnbgw_T_defs, -34, OSMO_TDEF_US, 1000000);
> elasped = now() - prev_start_ts; […]
ah yes this is exactly what i thought
--
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: 13
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: Thu, 23 May 2024 15:44:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: neels <nhofmeyr(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: 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 (#13).
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,381 insertions(+), 5 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-hnbgw refs/changes/39/36539/13
--
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: 13
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: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: newpatchset