Neels Hofmeyr has posted comments on this change. ( https://gerrit.osmocom.org/13121 )
Change subject: add osmo_use_count API ...................................................................... Patch Set 2: (3 comments) If I were reading this patch from someone else, I would also be reluctant to merge this at first. So I understand your impression. But in this case I argue that the impression is wrong. Pro arguments: using a user tracking with - arbitrary names and - multiple counts has proven extremely useful in osmo-msc. Con arguments, argued against: (note that the use count callback implementation by the caller can also enforce single use counts.) I've gone through a few iterations, some using limited size arrays, some using enum values, and this is the final result that I am actually quite satisfied with. I know it *looks* like it is spamming on dynamic allocation and list iteration but consider: - in osmo-msc I actually avoid all dynamic allocation using static use count entries (see in-code comment below, use_count.h:174). - use count lists so far have at most six entries in practice, usually more like 3. Traversing six llist->next pointers is less effort than doing your common strlen(); compare with osmo_strlcpy() which always does a strlen() no matter what, and we don't think twice. - strcmp() to compare use tokens: -- when the strings differ, strcmp() usually exits on the first or second character. -- when the strings are identical, they are usually the exact same char* address (from compile-time string constant), meaning that strcmp() is completely skipped. (quote: "if (e->use == use || (use && e->use && !strcmp(e->use, use)))") -- if we specified compile-time string constant use as requirement, we wouldn't need strcmp() at all, but I decided for this minuscule overhead because the benefit is complete correctness for any kinds of use token strings. - Compared to get_value_string() which we do *all* *the* *time* (at least when logging is enabled), this API imposes negligible traversal overhead. - Compared to get_value_string(), use count get() and put() operations are actually quite rare (at least when debug logging is enabled). For example, by using osmo_fsm_inst->id string as logging context, meaning less get_value_string() calls, I reduce the array traversal overhead by order of magnitude more than I add llist traversal with these use counts. - Let alone comparing to ran connection or VLR subscriber or gsm_trans lookup: so far we keep all gsm_trans items for all subscribers in *one single global* llist, and when looking for a trans for a given subscriber, we more often than not traverse the entire list. Long before this API becomes a noticeable performance impact, we'd have to introduce hash map lists for those countless other completely mad list traversals we still harbour in our midst. I can understand that this *seems* quite wasteful at first, but if you take a closer look I expect that you guys will agree with me: it is a fairly small overhead for a very nice API. https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h File include/osmocom/core/use_count.h: https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@133 PS2, Line 133: talloc_object > naming issue: this is basically just some opaque void * pointer, right? If > nothing in the libosmoco […] It is actually used as talloc context to allocate use count entries (in case the static entries are exhausted), see https://gerrit.osmocom.org/c/libosmocore/+/13121/2/src/use_count.c#169 https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@150 PS2, Line 150: count > I think it's rather difficult to understand that a given "entry" or "token" > can have multiple "count […] Besides CM Service Requests, there can be multiple SMS pending for a subscriber, or multiple CC transactions active (for multiple concurrent incoming calls), multiple Paging Requests for a VLR subscriber, etc etc. A recursive function could also reserve multiple uses like void user(struct foo *foo, int i) { if (i < 1) return; foo_get(foo, __func__); user(foo, i - 1); foo_put(foo, __func__); } Though I'm not currently using recursive functions in case you're wondering, some object release code paths enter the same release function multiple times from ricocheting release events. Current osmo-msc does have problems with tracking conn use properly, because we have a bitmask of flags indicating use, e.g. RAN_CONN_USE_TRANS_SMS. When two or more SMS are active, the first SMS that is done already clears the 'SMS' use flag. Then we get an error log that an "SMS" type use count is released even though it wasn't set, when the next SMS is done. The underlying total count stays useful, but the individual user tracking has this quirk that I really want to get rid of. The only way to get rid of spurious error logging while still tracking every use by name is multiple counts. You might say "then don't track by name" but that has been super annoying when trying to figure out use count bugs. I would have kept the nameless use count if it were maintainable, because it is so nice and simple... but that IMHO is not an option. https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@174 PS2, Line 174: size_t buf_n_entries); I am actually avoiding dynamic allocation completely using this function. It sets up a number of static use count entries. See https://gerrit.osmocom.org/c/osmo-msc/+/13136/1/src/libvlr/vlr.c#269 If the caller ever needs more, dynamic allocations are added to the end and stay around as long as the object exists. Even if using only dynamic allocation, use count entries are not added and removed, they are simply added and then stay around. -- To view, visit https://gerrit.osmocom.org/13121 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ife31e6798b4e728a23913179e346552a7dd338c0 Gerrit-Change-Number: 13121 Gerrit-PatchSet: 2 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-CC: Harald Welte <[email protected]> Gerrit-Comment-Date: Tue, 05 Mar 2019 23:40:51 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
