cellular-infrastructure
/
osmo-hnbgw
mirror of https://gerrit.osmocom.org/osmo-hnbgw
9
0
Fork 0
Code Issues Releases Wiki Activity
osmo-hnbgw/a92e9c966b20200950a8a4581b1...

983 lines
44 KiB
Plaintext
Raw Blame History

{
"comments": [
{
"unresolved": true,
"key": {
"uuid": "f2fbe5f3_0738a72d",
"filename": "/COMMIT_MSG",
"patchSetId": 3
},
"lineNbr": 11,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "nftables rules, not netfilter rules.",
"range": {
"startLine": 11,
"startChar": 33,
"endLine": 11,
"endChar": 42
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "126e348f_4a25f562",
"filename": "/COMMIT_MSG",
"patchSetId": 3
},
"lineNbr": 11,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "i\u0027m indeed often a bit confused with the terms -- nft, nftables, libnftables, netfilter, ruleset, table...",
"parentUuid": "f2fbe5f3_0738a72d",
"range": {
"startLine": 11,
"startChar": 33,
"endLine": 11,
"endChar": 42
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "fb8b29e2_4bebc7ab",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "What I am missing here is doing all those nft operations (which take quite some time in blocking way to complete) in a separate thread. I distinctly remember that during the existing discussion, I originally proposed a separate process, and Pau then suggested to use a separate thread. There was no discussion of doing it in the main thread, which will inevitably significantly slow down processing of a hnbgw under high load.\n\nIn my opinion, that separate thread should take care fo rule insertion, rule deletion and counter reading.\n\nThe 1s default interval is way too low and costly. I\u0027d go for certainly no less than 10s; in large setups maybe even less than that.",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "0f1a1921_84b7f591",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "yes, this patch is just a first step to test if this approach works at all.\nI\u0027d rather have URR than this, so I guess I\u0027m doing this in \"quick hack\" mode...\n\nI listed some things I still want to improve on this patch in some redmine issue, moving to a separate thread isn\u0027t listed because it\u0027s still a premature optimization. But obviously it\u0027s an ace up our sleeve.",
"parentUuid": "fb8b29e2_4bebc7ab",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "42bb927a_9b7ce8d3",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T19:19:15Z",
"side": 1,
"message": "I disagree that it\u0027s premature optimization. We know that the user will use this at scale [to the point we started to optimize osmo-bsc for their 2G use cases to reduce the syscall load, etc.]. We also know that any blocking/synchronous call to nft will stop processing any further signaling messages. So we know from day one that doing it in the main thread will not work in the real-world deployment and only in lab/testing.",
"parentUuid": "0f1a1921_84b7f591",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "066c80bd_1622faf9",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T21:31:32Z",
"side": 1,
"message": "i\u0027m not so sure of that, but if you insist i can already introduce multi-threading to a single-threaded project now, before testing whether single-thread is too slow.",
"parentUuid": "42bb927a_9b7ce8d3",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "358c6ac4_2e363831",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "moving the implementation to a separate thread will follow in another patch, so resolving this, only in the context of this patch.",
"parentUuid": "066c80bd_1622faf9",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "4d943566_3a1d15b6",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000004
},
"writtenOn": "2024-04-02T09:46:35Z",
"side": 1,
"message": "does it really make sense to first introduce it one way and then move it? That approach doe feel awkward to me. But if you say this is cleaner than doing it in one step, I\u0027ll trust you on that.",
"parentUuid": "358c6ac4_2e363831",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "06639526_6fdc3633",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-04-02T22:55:36Z",
"side": 1,
"message": "it\u0027s not so much moving, but introducing either an async step between parsing the counters and storing them in rate_ctr; or a mutex lock around storing them in rate_ctr. Pau suggested osmo_it_q.\n\nthe hard part is that we evaluate all counters at the same time. if we submit all in one it_q entry, it could become a fairly large allocation+data copy. On the other extreme, neither do we want each counter causing a separate select() trigger.\nwe could submit them in batches, e.g. one it_q entry copies 100 counters.\n\nI\u0027m still thinking about the alternatives I see and how complex/performant they would be:\n- osmo_it_q in batches\n- a global mutex lock around rate_ctr (do we have multi-threaded rate_ctr code yet?)\n- volatile counter storage per struct hnb_context instance with a flag signalling the main thread to copy to rate_ctr.",
"parentUuid": "4d943566_3a1d15b6",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "b5ff303c_5d85b6e7",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000004
},
"writtenOn": "2024-04-03T11:25:27Z",
"side": 1,
"message": "\"fairly large allocation + copy\": How large are the counters and how many of them do we have? let\u0027s assume 4x64bit per hnb (packet+byte for each direction) and some overhead, so something like 320 bytes? If we hve (already a lot) 1000 hNB, that is 320kB. This might have been \"a lot\" in the 1990s, but today I wouldn\u0027t call 320kB as a large allocation. Even at 100k hNBs (Very unrealistic) it would \"only\" be 32 MB. Even that wouldn\u0027t be large to allocate or copy on contemporary hardware. The point is that we\u0027re doing this at \u003e\u003d 1s intervals, not thousands of times per second.\n\nWe don\u0027t have multi-threaded rate counters so far anywhere. If there really were multiple producers/incrementors, one would probably have to think of per-cpu counters [like done in the kernel]. However, this is a completely separate topic from what we\u0027re discussing here.\n\nGiven that there\u0027s only one writer/incrementer for the counters here, and given that they only increment: would we even bother if a reader (like stats reporter) would read some out-of-date counter? It\u0027s not that we give any specific precision or claim that there is some absolute time at which the counters are taken. So in the worst case, the reader \nwould read an old counter while the writer is writing a new coutner. \n\nAFAICT, 64bit writes are always atomic if the data aligned on a 64bit boundary on x86 since P5/Pentium (1993!). However, we have other platforms, and the compiler might do stuff we\u0027re not aware.\n\nTo be portable gcc offers portable __atomic builtins, see https://gcc.gnu.org/onlinedocs/gcc/_005f_005fatomic-Builtins.html and https://stackoverflow.com/a/30878480/12210185 for an example. The generated x86_64 code will use a \"lock addl\" instruction, FYI.\n\nC11 introduces https://en.cppreference.com/w/c/atomic - but we probably ton\u0027t want to mandate such a recent C standard for all of our code. So the gcc extensions are likely the best choice, if you want to go atomic",
"parentUuid": "06639526_6fdc3633",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "ae7acf9d_70ed417d",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-04-04T00:41:58Z",
"side": 1,
"message": "Thanks for the cool pointers!\n\nAtomicity:\nEven if we get int64 guaranteed to be atomic, we need to keep \u0027packets\u0027 and \u0027total_bytes\u0027 counts in sync per hnb (at least two uint64_t), to be able to calculate actual UE bytes without GTP headers.\n\nSpeed/volume: This is the same issue as all the time with nft.\nThe actual binary data we\u0027re interested in is very small, but we get it all in one large string that needs parsing.\nAlso each counter result requires lookup of a hnbp by id_str, expensive.\n(I\u0027ll add a hashtable.)\n\nThe decisions are: when to parse it, and when to do the lookups, and how much additional storage/copying is really needed.\n\nIt\u0027s a good idea to do parsing and hnb lookups in the nft thread, because that is going to idle for a bit anyway, and the main thread can continue servicing RUA,SCCP,PFCP,MGCP.\n\nWe don\u0027t need to guard against a hnb disappearing while parsing counters, because we\u0027re using hnb_persistent, which is ... persistent.\n\nThe semantically simplest is to mutex on the stats emitter: make sure stats are not read from while we are writing to them. But that requires libosmocore changes...\n\nWell, we might acknowledge now that this problem will re-appear, so introducing a generally thread-safe way to increment a rate_ctr without it being read from at the same time would be desirable?\nWe do already require that there is a single stats reader/emitter.\nIt would actually be very efficient to just update rate_ctr from the nft thread directly, so I think I\u0027ll experiment with this global stats read mutex idea now.\n\nThe other idea that does not need libosmocore changes is this:\n...it\u0027s the same idea as above, just introducing a separate counter storage to avoid changing libosmocore:\n- have a separate \"volatile\" counter storage in the hnb_persistent struct;\n- in the nft thread, do all parsing and all hnb lookups,\n and place parsed counters in that separate counter storage per hnbp.\n- inter-thread signal (it_q is nice because it integrates in the select())\n to tell the main thread to shovel all hnb volatile counters into the rate_ctr;\n those volatile counters need a mutex (one global one, or maybe one per hnb).",
"parentUuid": "b5ff303c_5d85b6e7",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "8828a9f4_562a4b87",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000005
},
"writtenOn": "2024-04-09T03:55:18Z",
"side": 1,
"message": "see https://gerrit.osmocom.org/c/osmo-hnbgw/+/36540",
"parentUuid": "ae7acf9d_70ed417d",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "782b39da_56396033",
"filename": "/PATCHSET_LEVEL",
"patchSetId": 3
},
"lineNbr": 0,
"author": {
"id": 1000074
},
"writtenOn": "2024-04-09T18:52:32Z",
"side": 1,
"message": "Several pointers below:\n\n- I think that definetly we want to merge all this done in a separate thread from first step. I don\u0027t think it even makes sense to have it separated in a first step where everything happens in the same thread, since it\u0027s broken and distracts the whole thing.\n\n\u003e We don\u0027t have multi-threaded rate counters so far anywhere.\n\n- We do, in osmo-trx. In there since it\u0027s heavily multithreaded we do things a bit adhoc with separate structures and then copying them.\n\n\u003e The other idea that does not need libosmocore changes is this:\n\u003e have a separate \"volatile\" counter storage in the hnb_persistent struct;\n\u003e in the nft thread, do all parsing and all hnb lookups,\n\u003e and place parsed counters in that separate counter storage per hnbp.\n\u003e inter-thread signal (it_q is nice because it integrates in the select())\n\u003e to tell the main thread to shovel all hnb volatile counters into the rate_ctr;\n\u003e those volatile counters need a mutex (one global one, or maybe one per hnb).\n\nThat\u0027s similar to how osmo-trx does it, and I think it\u0027s how it should be done.\nAll other ways imho are calling for inconsistencies and stuff ending up broken.\n\nSo my opinion is: I\u0027m waiting to see a patch like the above described which I can review.",
"parentUuid": "8828a9f4_562a4b87",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "f81ea47b_cbe1f041",
"filename": "include/osmocom/hnbgw/hnbgw.h",
"patchSetId": 3
},
"lineNbr": 140,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "in mobile communications, we alwasy speak of Uplink and Downlink, whcih are uniquely defined and well-understood terms, in that they always are viewing it from the UE perspective.\n\nAlso, all of the counters above are using _UL and _DL suffix, so it would be good to be consistent with the other contents in using that abbreviation and using it at the end.",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "baa8cee6_f32f7d91",
"filename": "include/osmocom/hnbgw/hnbgw.h",
"patchSetId": 3
},
"lineNbr": 140,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "i already fixed that in the counter names, ack",
"parentUuid": "f81ea47b_cbe1f041",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "121895da_559c2c47",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 463,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "none of the other counters have a \".\" in their description. It\u0027s not a full sentence anyway.",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "0e41f659_e905d196",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 463,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "i actually dislike the \u0027.\u0027 in the end for these doc strings, so I gladly will remove. (I copied and adapted from some other counters at first, because i hadn\u0027t found the already existing HNB counters on master yet. Those all had the \u0027.\u0027 and i thought, bummer, i have to keep that \u0027.\u0027 to comply.)",
"parentUuid": "121895da_559c2c47",
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "28892d36_856cfca9",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 466,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "why is it \"gtpbytes\" and not \"bytes\"?",
"range": {
"startLine": 466,
"startChar": 11,
"endLine": 466,
"endChar": 19
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "cd36b8ec_46967624",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 466,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "i want to highlight that it is not UE DL bytes, but IPh+UDPh+GTPh+ UE DL.\nAnd i want to keep room for a future \"bytes\" to reflect just the UE DL, in case we want to add that.\n\nI didn\u0027t like the name from the start, i changed it to \"total_bytes\" now, hope that fits better with your taste, too.",
"parentUuid": "28892d36_856cfca9",
"range": {
"startLine": 466,
"startChar": 11,
"endLine": 466,
"endChar": 19
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "5fc7c633_31eff583",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 466,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "Done",
"parentUuid": "cd36b8ec_46967624",
"range": {
"startLine": 466,
"startChar": 11,
"endLine": 466,
"endChar": 19
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "3640b18f_331b2f25",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 470,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "why is the \"ul\" in the middl,e while all other coutners above have the ul/dl at the end? That\u0027s inconsistent to the user.",
"range": {
"startLine": 470,
"startChar": 3,
"endLine": 470,
"endChar": 18
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "06a63da3_8f8488e8",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 470,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "ack",
"parentUuid": "3640b18f_331b2f25",
"range": {
"startLine": 470,
"startChar": 3,
"endLine": 470,
"endChar": 18
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "38dd43bc_4f44ca98",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 911,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "s/netfilter/nftables/. Mentions \"counters\" twice?",
"range": {
"startLine": 911,
"startChar": 18,
"endLine": 911,
"endChar": 27
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "1fd1aa3d_64aaccc4",
"filename": "src/osmo-hnbgw/hnbgw.c",
"patchSetId": 3
},
"lineNbr": 911,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "the one are the netfilter ```counter``` and the other are the rate counters on stats \u003d)",
"parentUuid": "38dd43bc_4f44ca98",
"range": {
"startLine": 911,
"startChar": 18,
"endLine": 911,
"endChar": 27
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "90d48288_a8de85e9",
"filename": "src/osmo-hnbgw/hnbgw_vty.c",
"patchSetId": 3
},
"lineNbr": 746,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "I think the user doesn\u0027t care if some counters are implemented via nftables or in another way. The user cares about per-hnb counters. If we ever decide to implement those counters in a different way, this command would not exist or be very hard to emulate, and have nothing to do with nft/nftables.\n\nIf this is for debugging only, it should at least be HIDDEN so it\u0027s not documented. Also, it might be worth removing it completely, especially if all of the nft handling moves to a separate thread, as it was originally discussed [which likely makes this command more difficult]",
"range": {
"startLine": 746,
"startChar": 7,
"endLine": 746,
"endChar": 16
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "082f4822_ee795abd",
"filename": "src/osmo-hnbgw/hnbgw_vty.c",
"patchSetId": 3
},
"lineNbr": 746,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "yes it\u0027s a debugging cmd, not sure if we\u0027ll keep it.\n\nif yes, the naming is not so trivial; the name i picked at first was \"hnb-kpi\" but obviously there are more kpi that aren\u0027t implemented via nft. Here I specifically want to query the nft counters only. \u0027show hnb kpi\u0027 wouldn\u0027t be accurate.\n\nit\u0027s a \"conflict\" that shows up often -- at a certain point it no longer makes sense to have semantic abstraction in the UI, instead of naming the things as what they actually are. Often the user needs to configure a very implementation specific detail, at which point a generalized name no longer clicks. For example: the nft table name to use (cfg item still needs to be added to this patch).\n\nwhy would you want me to hide a command like this? surely a user can decide whether to look at things or not?",
"parentUuid": "90d48288_a8de85e9",
"range": {
"startLine": 746,
"startChar": 7,
"endLine": 746,
"endChar": 16
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "64e8471d_92c146cf",
"filename": "src/osmo-hnbgw/hnbgw_vty.c",
"patchSetId": 3
},
"lineNbr": 746,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T19:19:15Z",
"side": 1,
"message": "if it\u0027s something to aid during debugging/development in an early phase of the software, I wouldn\u0027t advertise/export is as a public/documented/expected interface.",
"parentUuid": "082f4822_ee795abd",
"range": {
"startLine": 746,
"startChar": 7,
"endLine": 746,
"endChar": 16
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "843ffc9c_bc682b98",
"filename": "src/osmo-hnbgw/hnbgw_vty.c",
"patchSetId": 3
},
"lineNbr": 746,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "Done",
"parentUuid": "64e8471d_92c146cf",
"range": {
"startLine": 746,
"startChar": 7,
"endLine": 746,
"endChar": 16
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "34c84481_a334b52b",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 120,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "why are we adding a new chain for each and every hnb? Isn\u0027t a rule per hnb (per direction) sufficient?\n\nCounters can be named, and rules can contain a comment field, in case you need some identifier.\n\nchains usually make sense if you have many rules attached to one filter criteria, like first match on the host, then jump to a chain, then execute a list of rules for all the packets matching that host. This is not the case here, as you unconditionally jump to each chain for each packet (no matter which destination), and then you only have a single rule in each chain.",
"range": {
"startLine": 120,
"startChar": 10,
"endLine": 120,
"endChar": 13
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "219b60cb_0e917f65",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 120,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T08:05:32Z",
"side": 1,
"message": "Actually, the only reason for having one chain per hnb-direction would be if you\u0027d use verdict maps, as described (even with counters) in https://wiki.nftables.org/wiki-nftables/index.php/Verdict_Maps_(vmaps)\n\nThis even avoids the need to traverse a list of hundreds of rules for each packet, but implements a more efficient way of matching.\n\nBascially you can look at a verdict map as a \u0027dict\u0027 using the ip address as a key, and the \"jump to a chain\" as a value.",
"parentUuid": "34c84481_a334b52b",
"range": {
"startLine": 120,
"startChar": 10,
"endLine": 120,
"endChar": 13
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "de8110fc_a0875590",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 120,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "also AFAIR the chains are limited to 1024...\nso lots of room for improvement. This patch is still a POC.\n\nhere i took the counters example from\nhttps://wiki.nftables.org/wiki-nftables/index.php/Counters\nI guess this is a bad example.",
"parentUuid": "219b60cb_0e917f65",
"range": {
"startLine": 120,
"startChar": 10,
"endLine": 120,
"endChar": 13
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "8e962702_e3686ce9",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 120,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T19:19:15Z",
"side": 1,
"message": "An example focuses on illustrating the use of something in a simple way. That doesn\u0027t always correlate with the best real-world usage for scalable deployment.",
"parentUuid": "de8110fc_a0875590",
"range": {
"startLine": 120,
"startChar": 10,
"endLine": 120,
"endChar": 13
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "c156e23f_9ec3f6bc",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 120,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "Done",
"parentUuid": "8e962702_e3686ce9",
"range": {
"startLine": 120,
"startChar": 10,
"endLine": 120,
"endChar": 13
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "db911bea_0920b27a",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T08:05:32Z",
"side": 1,
"message": "input and output hooks are clarly wrong here. Those are for packets from/to local sockets. A usual hnbgw does not run either the hnb or the sgsn/msc on the same machine, but they are remotely.",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "f1d6d141_7a1b5d40",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T08:09:54Z",
"side": 1,
"message": "if the hnbgw-colocated UPF is used, then the packets are also visisble in input/output - but that support is not mandatory. So basically the wrong hook results in a feature that only works for the colocated upf use case, while placement in e.g. prerouting and postrouting would cover both/all use cases.",
"parentUuid": "db911bea_0920b27a",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "e329ddc6_af89a3e3",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "i tried to read the docs on this and it didn\u0027t help much, so i decided to stick with that example from netfilter.org.\ni\u0027ll use prerouting for rx and postrouting for tx, correct?\n\nWait, I don\u0027t understand your point about co-located UPF.\nI am assuming that the machine running osmo-hnbgw also is the GTP relay, otherwise setting up nft rules on that same host doesn\u0027t make sense, or does it? How would we pick up GTP if it were relayed on a different machine, or bypasses osmo-hnbgw entirely, going directly to the CN? Can netfilter even see packets that are routed between two remote interfaces?",
"parentUuid": "f1d6d141_7a1b5d40",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "79d3522a_2097f6fc",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T19:19:15Z",
"side": 1,
"message": "I think this diagram explains very clearly what happens where: https://wiki.nftables.org/wiki-nftables/index.php/Netfilter_hooks or https://upload.wikimedia.org/wikipedia/commons/3/37/Netfilter-packet-flow.svg\n\nregarding colocation: The IP host running your hnbgw only needs to run a co-located UPF if your HNB-facing IP network and your CN-facing IP networks are not transparently routed. Like always in networking: If your routing is not t ransparent, you need an application-layer gateway/proxy, port-forwarding or the like.\n\nRemember, we all were using osmo-hnbgw before we had upf support in it. In that situation, the IP packets for GTP-U (and RTP) are transparently routed by the normal kernel routing stack [of the host running osmo-hnbgw].\n\nSo in that routed/transparent case, all GTP-U packets (of both directions) go through prerouting, forward and postrouting.\n\nIn the non-transparent/UPF-proxied case, all GTP-U packets would normally terminate on / originate from local IP addresses of the UPF.\n\nI don\u0027t know how the rewrite rules of osmo-upf are structured to tell you whether in the UPF case they actually go through input / output or not. You should probably know that better, as you have worked both on it and with it.",
"parentUuid": "e329ddc6_af89a3e3",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "61d58f03_10560e1e",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T21:31:32Z",
"side": 1,
"message": "The case where a UPF runs on the same host as osmo-hnbgw is clear.\nGTP-U terminates at \"this\" machine and we obviously can count all packets.\n\nThe other case isn\u0027t as clear to me: when GTP-U flows between two remote hosts and doesn\u0027t terminate or even route on \"this\" machine.\n\nBefore osmo-upf was introduced to osmo-hnbgw, the host running osmo-hnbgw was not involved in GTP-U at all. The hNodeB was told the GSN\u0027s GTP-U address.\nIn this older chart, see how \"GTP-U(3G)\" in the bottom doesn\u0027t touch the HNBGW:\nhttps://osmocom.org/projects/cellular-infrastructure/wiki/Osmocom_Network_In_The_Box/153#Topology\n(I remember you told me elsewhere that this is just a special case, but it\u0027s still the only case i\u0027ve seen without a UPF.)\n\nSo IIUC we can only count GTP-U between two remote hosts when we\u0027re plugged in the same ethernet link and the packets so happen to show on our network stack. Is that assumption an obvious given somehow?\n\nSame question when osmo-upf runs on a different host.",
"parentUuid": "79d3522a_2097f6fc",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "411a4ffd_5981cf98",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 121,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "Done",
"parentUuid": "61d58f03_10560e1e",
"range": {
"startLine": 121,
"startChar": 23,
"endLine": 121,
"endChar": 33
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "eee07875_9f8d81ff",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T07:52:03Z",
"side": 1,
"message": "If theo output is json, please let\u0027s not hand-roll our own JSON parser here but us an existing json parser library. The only argument for rolling our own would be that there is a proven benachmark that in our use case no common exisitng json-parser is anywhere near as fast as the hand-rolled one. And in that case, we\u0027d need a benchmark and some kind of test suite to validate our own parser...",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "4a681572_9539020e",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T18:08:49Z",
"side": 1,
"message": "this is the nft scripting language, not json.\nI already had this hackish parsing code lying around from trial tests with osmo-upf\u0027s nft counters.\n\nlet\u0027s see first how the nft script changes when we make it more efficient... might end up getting a json response after all.",
"parentUuid": "eee07875_9f8d81ff",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "be280a47_bc92e811",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000004
},
"writtenOn": "2024-03-22T19:19:15Z",
"side": 1,
"message": "ah, didn\u0027t know that part, sorry for assuming JSON.",
"parentUuid": "4a681572_9539020e",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "a159e625_990cf2d7",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-22T21:31:32Z",
"side": 1,
"message": "I realized that the output can be switched to JSON (nft -j on the cmdline); still having trouble getting this enabled via flags on the C API but should work any minute now...\n\nStill, the amount of information and complexity in the json output is so much much more complex than the flat numerical information that we need. I think when I try a json parser I will very sceptically scrutinize how it affects load.",
"parentUuid": "be280a47_bc92e811",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": true,
"key": {
"uuid": "e573d4d6_f1a6674a",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-23T01:33:43Z",
"side": 1,
"message": "bpftrace says this:\n\nReservation: this is with only two hNodeBs connected, because our ttcn3 test suite has only two HNB slots.\nI\u0027ll try to have more hNodeBs to see the impact with longer response strings.\n...or i could just fabricate huge strings and test parsing them separately...\nanyway, this is what i see so far:\n\nUsing libjansson to parse a json response with four counters in it:\n\n (this is the part decoding the response string only)\n @decode_nft_response_us:\n [128, 256) 1 | |\n [256, 512) 26 |@@@@@@@@@@@@@@@@@@@@@ |\n [512, 1K) 10 |@@@@@@@@ |\n [1K, 2K) 62 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|\n \n (the overall reading+decoding from nft)\n @nft_kpi_read_counters_us:\n [1K, 2K) 1 | |\n [2K, 4K) 17 |@@@@@@@@@@@@ |\n [4K, 8K) 69 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|\n [8K, 16K) 11 |@@@@@@@@ |\n [16K, 32K) 1 | |\n\nUsing plain \u0027nftables\u0027 strings and decoding the same by manual string parsing:\n\n @decode_nft_response_us: \n [16, 32) 1 |@ |\n [32, 64) 22 |@@@@@@@@@@@@@@@@@@@@@@@ |\n [64, 128) 48 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|\n [128, 256) 30 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@ |\n [256, 512) 1 |@ |\n\n @nft_kpi_read_counters_us: \n [1K, 2K) 7 |@@@@ |\n [2K, 4K) 12 |@@@@@@@ |\n [4K, 8K) 81 |@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@@|\n [8K, 16K) 1 | |\n\nThe string decoding part is about 10 times slower with libjansson.\n0.5 to 2 ms with libjansson, .03 to 0.3 ms with string parsing.\n\nagain, remains to be seen what happens when the lists get longer and hit exponential effects.",
"parentUuid": "a159e625_990cf2d7",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
},
{
"unresolved": false,
"key": {
"uuid": "1c0d5d6a_4517d5dd",
"filename": "src/osmo-hnbgw/nft_kpi.c",
"patchSetId": 3
},
"lineNbr": 249,
"author": {
"id": 1000005
},
"writtenOn": "2024-03-28T02:51:23Z",
"side": 1,
"message": "json is implemented in a separate patch, resolving this in the context of this patch",
"parentUuid": "e573d4d6_f1a6674a",
"range": {
"startLine": 249,
"startChar": 39,
"endLine": 249,
"endChar": 45
},
"revId": "a92e9c966b20200950a8a4581b1c94e72b6f9954",
"serverId": "035e6965-6537-41bd-912c-053f3cf69326"
}
]
}
View Git Blame Copy Permalink