[PATCH] eloop: Remove epoll and kqueue implementations

[Nicolas Escande]

On Thu Oct 3, 2024 at 4:47 PM CEST, Benjamin Berg wrote:
> On Thu, 2024-10-03 at 15:59 +0200, Nicolas Escande wrote:
> > Hello Benjamin,
> > 
> > On Thu Sep 26, 2024 at 5:34 PM CEST, Benjamin Berg wrote:
> > > From: Benjamin Berg <benjamin.berg at intel.com>
> > > 
> > > Both of these implementations are broken by design currently and will
> > > not work correctly with any socket that needs to be registered for
> > > multiple event types (reading, writing, exception).
> > > 
> > > The reason for this is that the eloop_register_sock API can only take
> > > one event at a time resulting in multiple entries for the FD. However,
> > > epoll_ctl() will refuse to register the FD a second time and kqueue()
> > > will happily overwrite the earlier information. As such, neither
> > > implementation is working correctly.
> > 
> > I never even realized that sometimes hostapd registers a socket for writing.
>
> I think in the DBus case it is exceptions and either reading or
> writing. There are also the DPP/HTTP/Radius clients that register
> writing, but that might work just fine as long as only one event type
> is active at a time.
>
> > > Simply remove both, I doubt anyone ever tried to use them as at least
> > > DBus is broken. Also, I don't expect that hostapd/wpa_supplicant has a
> > > lot of FDs/sockets making this a micro-optimization which is likely
> > > pointless in the first place.
> > > 
> > Actually, in our product we heavily use the epoll implementation, but with a
> > very light config / feature set (no dbus or dpp...) so we never encountered a
> > problem of that nature.
> > 
> > Instead of removing the code, we could $(error ) or # error for incompatible
> > configs ? But it seems hard to maintain in the long run when we add new stuff.
> > Either way, I don't really care as long as the poll implementationworks, we'll
> > switch to it if need be.
>
> To be honest, one motivation for me to remove it entirely was because
> it seemed unnecessarily hard to work with the code (I would have a hard
> time testing kqueue at least). And I was wondering whether it might be
> worthwhile to add a prioritization feature to eloop as that could make
> some event handling code more predictable. For example, one could
> ensure all nl80211 events are processed before the next timeout is
> fired.

I just wanted to point out that in specific configs epoll was not broken. So
maybe, if the only motivation was "it's broken, lets not anybody be deceived
by a silently broken build, lets remove it", we could have gone the error way
instead. But it's more work, and I don't mind removing epoll as long as poll
works instead. kqueue is alien for me, I don't care for it at all.
>
> All that said, do you maybe have an idea if it is actually making a
> relevant performance difference?

No, not at all. I don't really care about performance, we want reliability first
and foremost.

>
> Benjamin

Nicolas