Change in libosmocore[master]: select: gather statistics for TCP connections

[laforge]

laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/26454 )

Change subject: select: gather statistics for TCP connections
......................................................................


Patch Set 1: Code-Review-1

(1 comment)

I think this is completely inacceptable.

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.

I think the proper way to address this is to
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)
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.

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.

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.

https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/select.c 
File src/select.c:

https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/select.c@388 
PS1, Line 388: 	stats_tcp_poll(&osmo_fds);
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.



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/26454
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I1416f95aff2adcf13689646b7574845de169fa3d
Gerrit-Change-Number: 26454
Gerrit-PatchSet: 1
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Comment-Date: Tue, 07 Dec 2021 07:54:42 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211207/a8866f36/attachment.htm>