This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
dexter gerrit-no-reply at lists.osmocom.orgdexter has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/26454 )
Change subject: select: gather statistics for TCP connections
......................................................................
Patch Set 12:
(12 comments)
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/ctrl/control_if.c
File src/ctrl/control_if.c:
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/ctrl/control_if.c@65
PS3, Line 65: #
> not sure if this works for builddir != srcdir builds
Done
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@173
PS1, Line 173: /* Ensure that priv_stats is set to NULL. */
> I find this comment useless.
Done
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. […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/26454/9/src/select.c
File src/select.c:
https://gerrit.osmocom.org/c/libosmocore/+/26454/9/src/select.c@188
PS9, Line 188: osmo_stats_tcp_osmo_fd_unregister(fd);
> Lets not make this over-complicated.
Done
https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.h
File src/stats_tcp.h:
https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.h@5
PS1, Line 5: void stats_tcp_poll(struct llist_head *osmo_fds);
> A header file just for one function? Are you planning to add more stuff here? If not, let's rather d […]
Done
https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.c
File src/stats_tcp.c:
https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.c@64
PS1, Line 64: const struct osmo_stat_item_desc stats_tcp_item_desc[] = {
> static?
Done
https://gerrit.osmocom.org/c/libosmocore/+/26454/1/src/stats_tcp.c@74
PS1, Line 74: const struct osmo_stat_item_group_desc stats_tcp_desc = {
> static?
Done
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c
File src/stats_tcp.c:
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@54
PS3, Line 54: static LLIST_HEAD(stats_tcp);
> I wonder if this should also be __thread, as the osmo_fd list is per-thread. […]
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...
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@170
PS3, Line 170: return;
> comment does not agree with function. […]
I am a bit surprised. There seem to be no way to find out via getsockopt if a socket is a TCP socket.
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@227
PS3, Line 227: llist_for_each_entry(stats_tcp_entry, &stats_tcp, entry) {
: fill_stats(stats_tcp_entry);
: }
> doing all sockets at once has the danger of creating high latency. […]
I agree that this might be a problem. This is addressed in the current patch set.
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp.c@247
PS3, Line 247: osmo_timer_setup(&stats_tcp_poll_timer, stats_tcp_poll_timer_cb, NULL);
> are we sure we can call osmo_timer_schedule from this constructor? As constructor call ordering is […]
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.
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp_internal.h
File src/stats_tcp_internal.h:
https://gerrit.osmocom.org/c/libosmocore/+/26454/3/src/stats_tcp_internal.h@11
PS3, Line 11: void stats_tcp_osmo_fd_register(struct osmo_fd *fd);
: void stats_tcp_osmo_fd_unregister(struct osmo_fd *fd);
: int stats_tcp_set_interval(int interval);
> those symbols […]
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.
--
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: 12
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: daniel <dwillmann at sysmocom.de>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Comment-Date: Tue, 21 Dec 2021 14:59:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: dexter <pmaier at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211221/324e3dce/attachment.htm>