<p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/ctrl/control_if.c">File src/ctrl/control_if.c:</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/libosmocore/+/26454/3/src/ctrl/control_if.c@65">Patch Set #3, Line 65:</a> <code style="font-family:monospace,monospace">#</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">not sure if this works for builddir != srcdir builds</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/select.c">File src/select.c:</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/libosmocore/+/26454/1/src/select.c@173">Patch Set #1, Line 173:</a> <code style="font-family:monospace,monospace"> /* Ensure that priv_stats is set to NULL. */</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I find this comment useless.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/select.c@388">Patch Set #1, Line 388:</a> <code style="font-family:monospace,monospace">      stats_tcp_poll(&osmo_fds);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I really don't think you want to poll those statistics every time you enter any osmo_select_main. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/9/src/select.c">File src/select.c:</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/libosmocore/+/26454/9/src/select.c@188">Patch Set #9, Line 188:</a> <code style="font-family:monospace,monospace"> osmo_stats_tcp_osmo_fd_unregister(fd);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Lets not make this over-complicated.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.h">File src/stats_tcp.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/libosmocore/+/26454/1/src/stats_tcp.h@5">Patch Set #1, Line 5:</a> <code style="font-family:monospace,monospace">void stats_tcp_poll(struct llist_head *osmo_fds);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">A header file just for one function? Are you planning to add more stuff here? If not, let's rather d […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.c">File src/stats_tcp.c:</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/libosmocore/+/26454/1/src/stats_tcp.c@64">Patch Set #1, Line 64:</a> <code style="font-family:monospace,monospace">const struct osmo_stat_item_desc stats_tcp_item_desc[] = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">static?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.c@74">Patch Set #1, Line 74:</a> <code style="font-family:monospace,monospace">const struct osmo_stat_item_group_desc stats_tcp_desc = {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">static?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c">File src/stats_tcp.c:</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/libosmocore/+/26454/3/src/stats_tcp.c@54">Patch Set #3, Line 54:</a> <code style="font-family:monospace,monospace">static LLIST_HEAD(stats_tcp);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I wonder if this should also be __thread, as the osmo_fd list is per-thread. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I feel more comfortable with keeping the stats_tcp list global and using mutexes to protect the critical path. I am also not sure how well the whole stats stuff integrates with threads. Also I have no experience with handling the different threads, starting the timers, vty etc...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@170">Patch Set #3, Line 170:</a> <code style="font-family:monospace,monospace">              return;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">comment does not agree with function. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I am a bit surprised. There seem to be no way to find out via getsockopt if a socket is a TCP socket.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@227">Patch Set #3, Line 227:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">llist_for_each_entry(stats_tcp_entry, &stats_tcp, entry) {<br>           fill_stats(stats_tcp_entry);<br>  }<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">doing all sockets at once has the danger of creating high latency. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I agree that this might be a problem. This is addressed in the current patch set.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@247">Patch Set #3, Line 247:</a> <code style="font-family:monospace,monospace"> osmo_timer_setup(&stats_tcp_poll_timer, stats_tcp_poll_timer_cb, NULL);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">are we sure we can call osmo_timer_schedule from this constructor?  As constructor call ordering is  […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Probably that is unsafe. We have decided that we want to set the default interval to 0. This means that the timer can never be scheduled from the constructor anyway. So we can remove this line and only do the osmo_timer_setup from here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp_internal.h">File src/stats_tcp_internal.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/libosmocore/+/26454/3/src/stats_tcp_internal.h@11">Patch Set #3, Line 11:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">void stats_tcp_osmo_fd_register(struct osmo_fd *fd);<br>void stats_tcp_osmo_fd_unregister(struct osmo_fd *fd);<br>int stats_tcp_set_interval(int interval);<br></pre></blockquote></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">those symbols […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">In the current patchset I have elimnated the need for stats_tcp_internal.h. Its now more uniform to stats.h, which is better I think.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/libosmocore/+/26454">change 26454</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/c/libosmocore/+/26454"/><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-Change-Id: I1416f95aff2adcf13689646b7574845de169fa3d </div>
<div style="display:none"> Gerrit-Change-Number: 26454 </div>
<div style="display:none"> Gerrit-PatchSet: 12 </div>
<div style="display:none"> Gerrit-Owner: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: daniel <dwillmann@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 21 Dec 2021 14:59:57 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Comment-In-Reply-To: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>