<p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Pro arguments: using a user tracking with</p><ul><li>arbitrary names and</li><li>multiple counts</li><li>has proven extremely useful in osmo-msc.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Con arguments, argued against:</p><p style="white-space: pre-wrap; word-wrap: break-word;">(note that the use count callback implementation by the caller can also enforce single use counts.)</p><p style="white-space: pre-wrap; word-wrap: break-word;">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:</p><ul><li>in osmo-msc I actually avoid all dynamic allocation using static use count entries (see in-code comment below, use_count.h:174).</li></ul><ul><li>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.</li></ul><ul><li>strcmp() to compare use tokens:</li><li>- when the strings differ, strcmp() usually exits on the first or second character.</li><li>- 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)))")</li><li>- 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.</li></ul><ul><li>Compared to get_value_string() which we do *all* *the* *time* (at least when logging is enabled), this API imposes negligible traversal overhead.</li></ul><ul><li>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.</li></ul><ul><li>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.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p><a href="https://gerrit.osmocom.org/13121">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h">File include/osmocom/core/use_count.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@133">Patch Set #2, Line 133:</a> <code style="font-family:monospace,monospace">talloc_object</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">naming issue: this is basically just some opaque void * pointer, right?  If nothing in the libosmoco […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">It is actually used as talloc context to allocate use count entries (in case the static entries are exhausted),<br>see https://gerrit.osmocom.org/c/libosmocore/+/13121/2/src/use_count.c#169</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@150">Patch Set #2, Line 150:</a> <code style="font-family:monospace,monospace">count</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I think it's rather difficult to understand that a given "entry" or "token" can have multiple "count […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Besides CM Service Requests,<br>there can be multiple SMS pending for a subscriber,<br>or multiple CC transactions active (for multiple concurrent incoming calls),<br>multiple Paging Requests for a VLR subscriber,<br>etc etc.</p><p style="white-space: pre-wrap; word-wrap: break-word;">A recursive function could also reserve multiple uses like</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">  void user(struct foo *foo, int i)<br>  {<br>     if (i < 1)<br>        return;<br>     foo_get(foo, __func__);<br>     user(foo, i - 1);<br>     foo_put(foo, __func__);<br>  }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">Though I'm not currently using recursive functions in case you're wondering,<br>some object release code paths enter the same release function multiple times<br>from ricocheting release events.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Current osmo-msc does have problems with tracking conn use properly,<br>because we have a bitmask of flags indicating use, e.g. RAN_CONN_USE_TRANS_SMS.<br>When two or more SMS are active, the first SMS that is done already clears the 'SMS' use flag.<br>Then we get an error log that an "SMS" type use count is released even though it wasn't set,<br>when the next SMS is done. The underlying total count stays useful, but the individual<br>user tracking has this quirk that I really want to get rid of. The only way to get rid<br>of spurious error logging while still tracking every use by name is multiple counts.</p><p style="white-space: pre-wrap; word-wrap: break-word;">You might say "then don't track by name" but that has been super annoying when trying to<br>figure out use count bugs. I would have kept the nameless use count if it were maintainable,<br>because it is so nice and simple... but that IMHO is not an option.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/13121/2/include/osmocom/core/use_count.h@174">Patch Set #2, Line 174:</a> <code style="font-family:monospace,monospace">                                       size_t buf_n_entries);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">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</p><p style="white-space: pre-wrap; word-wrap: break-word;">If the caller ever needs more, dynamic allocations are added to the end and stay around as long as the object exists.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Even if using only dynamic allocation, use count entries are not added and removed, they are simply added and then stay around.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/13121">change 13121</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/13121"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: libosmocore </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ife31e6798b4e728a23913179e346552a7dd338c0 </div>
<div style="display:none"> Gerrit-Change-Number: 13121 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 05 Mar 2019 23:40:51 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>