Change in libosmocore[master]: select: Migrate over to poll()

[laforge]

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

Change subject: select: Migrate over to poll()
......................................................................


Patch Set 2:

(9 comments)

https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c 
File src/select.c:

https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@108 
PS2, Line 108: osmo_fd_find
> Is it a public API?
yes, I thought I'd export the symbol (but then forgot to add it to a header file).


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@111 
PS2, Line 111: 	llist_for_each_entry(entry, &osmo_fds, list) {
> Indeed looks like we'd want to store that into an rb tree rather than a list.
as stated, we can optimize later.  The important point is no ABI nor API breakage.


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@155 
PS2, Line 155: 1>
> missing space
Ack


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@158 
PS2, Line 158: 				   g_poll.poll_size + POLL_ALLOC_GRANULARITY);
> rather multiply *2?
what would be the advantage? I don't really care much either way.


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@161 
PS2, Line 161: 		memset(p + g_poll.poll_size, 0, POLL_ALLOC_GRANULARITY*sizeof(struct pollfd));
> Are you sure you can still access g_poll after talloc_realloc? It may have been moved to another reg […]
g_poll itself is a static/global thread-local variable.  So g_poll for sure is still valid.  g_poll.poll is a pointer to dynamically allocated memory, and I'm not using that one here until I update it one line further below.


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@305 
PS2, Line 305: 			p->events |= POLLOUT;
> |= POLLERR ?
Ack


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@312 
PS2, Line 312: /* iterate over first n_fd entries of g_poll.lopp + dispatch */
> .poll
Ack


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@328 
PS2, Line 328: 			/* FD might have been unregistered meanwhile */
> Are you sure this can happen?
well, look at the old select loop function and it's restart hack / goto.  That was exactly added for the fact that one ofd callback has modified the list of file descriptors while we are iterating it.   Here we have the advantage that we're not iterating the list while calling the call-back.  But still, one callback which we just executed might have closed down some socket (and unregistered the osmo_fd) for a fd which is also marked readable/writable and which we want to dispatch to right now.  So I'm pretty sure, yes.


https://gerrit.osmocom.org/c/libosmocore/+/20732/2/src/select.c@363 
PS2, Line 363: 	rc = poll(g_poll.poll, n_poll, polling ? 0 : osmo_timers_nearest_ms());
> Please let's keep both implementations around and decide at compile time which to use. […]
keeping multiple implementations around has the risk that over time only one of them will be tested all the time, while the other one is not. (I have no intention of exploding our jenkins matrix by another axis to test select vs. poll).

poll() is available on probably at least as many operating systems used today as select().  Also, given the amount of linux specifics we have in libosmocore, it is rather unlikely that we'll encounter a system that supports select but not poll.

We will likely see an epoll implementation (and possibly also before ppoll as it does have sub-millisecond timers) at not too distant point in time, and then we'd have three implementations to maintain (and test).

So yes, we can keep it around as a --disable-poll configure option for some limited amount of time (to manually test regressions/compatibility), but I would consider that only a short term thing for a few months or one year max.



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

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I9e80da68a144b36926066610d0d3df06abe09bca
Gerrit-Change-Number: 20732
Gerrit-PatchSet: 2
Gerrit-Owner: laforge <laforge at osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 19 Oct 2020 10:05:00 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201019/001a2ec7/attachment.htm>