<p style="white-space: pre-wrap; word-wrap: break-word;">I think this is completely inacceptable.</p><p style="white-space: pre-wrap; word-wrap: break-word;">osmo_select_main() is about the 'hottest' code path we have in the entire osmocom universe. It's the single most called function.  You are adding potentially many hundreds of system calls (multiple for each fd, to find out if it's a stream socket, then the getsockopt) to _each interation_ of osmo_select_main.  I would expect this to completely kill performance of software like osmo-mgw.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think the proper way to address this is to<br>a) keep the status of what is a TCP stream socket in osmo_fd so we can just read it from there as opposed to invoking a system call (super expensive)<br>b) gather those TCP_INFO statistics rarely.  That could be a timer firing every 1/5/10 seconds (could e vty-configurable even down to 0, in case a user doesn't want this feature and the related overhead). Even then, iterating over all fd's in one timer and potentially issuing many consecutive TCP_INFO syscalls before returning to osmo_select_main() might introduce more delay than we want in some more performance critical code. One could also consider obtaining the statistics "every [Nth] read on the specific TCP stream socket".  This would mean it is triggered whenever the fd is marked readable-or-writable, and before we call the fd_cb, we would issue the getsockopt.  The nice part about this approach is that we avoid potentially long chains of getsockopt calls before returning back to osmo_select_main, i.e. avoid introducing high amounts of latency.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If one decides to work in the 'one timer to issue TCP_INFO for all fds' approach: One idea would be to keep a separate list of only the TCP stream sockets, so that we don't have to iterate the whole list every time. This would add one more llist_add / llist_del in osmo_fd_{register,unregister}, which is a relatively "cold" path, i.e. not executed thousands of times per second.</p><p style="white-space: pre-wrap; word-wrap: break-word;">keping a separate flag in osmo_fd (or having the separate linked-list of 'stats enabled sockets') also adds the capability to selectively enable this feature for those TCP sockets where it really matters.  In your current approach we would enable it for VTY and CTRL, for example, which I think is of very limited [if at all] benefit and can hence be avoided.</p><p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/libosmocore/+/26454">View Change</a></p><p>1 comment:</p><ul style="list-style: none; padding: 0;"><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@388">Patch Set #1, Line 388:</a> <code style="font-family:monospace,monospace">    stats_tcp_poll(&osmo_fds);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I really don't think you want to poll those statistics every time you enter any osmo_select_main.  This could be thousands (or more!) times per second.</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: 1 </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: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 07 Dec 2021 07:54:42 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>