[PATCH] event-loop: Only read one epoll event per loop dispatch.

[Jonas Ådahl]

On Mon, Mar 19, 2012 at 4:03 PM, Andreas Ericsson <ae at op5.se> wrote:
> On 03/17/2012 09:39 AM, Jonas Ådahl wrote:
>> In order for users of the event loop to manipulate the state of it or
>> others, only one event may be processed per epoll_wait. When multiple
>> events are queued up and a user removes an event input source from an
>> event loop from within a dispatch, if the removed source was one of the
>> queued up events the program may crash with a segmentation faults.
>>
>
> This patch will kill performance completely. I've done this exact
> same thing in an I/O-broker library I wrote, and it's disastrous
> for any kind of throughput.
>
> My testcase served several thousand events per second though from
> about 500 filedescriptors, so this might not be much of an issue
> for Wayland, which I suspect will have fewer fd's and fewer events
> per second.
>

I suspected this as well; I'd like to see it enabled, but the event
source removal issue needs to be solved. Half the point of this patch
was to start a discussion about it.

>
>> In order to allow queuing up multiple events there needs to be a way to
>> manipulate the state without destroying the source event.
>>
>> Signed-off-by: Jonas Ådahl<jadahl at gmail.com>
>> ---
>>   src/event-loop.c |   20 ++++++++++----------
>>   1 files changed, 10 insertions(+), 10 deletions(-)
>>
>> diff --git a/src/event-loop.c b/src/event-loop.c
>> index 2dfe0ae..cc67383 100644
>> --- a/src/event-loop.c
>> +++ b/src/event-loop.c
>> @@ -447,23 +447,23 @@ dispatch_idle_sources(struct wl_event_loop *loop)
>>   WL_EXPORT int
>>   wl_event_loop_dispatch(struct wl_event_loop *loop, int timeout)
>>   {
>> -     struct epoll_event ep[32];
>> +     struct epoll_event ep;
>>       struct wl_event_source *source;
>> -     int i, count, n;
>> +     int count, n;
>>
>>       dispatch_idle_sources(loop);
>>
>> -     count = epoll_wait(loop->epoll_fd, ep, ARRAY_LENGTH(ep), timeout);
>> +     count = epoll_wait(loop->epoll_fd,&ep, 1, timeout);
>>       if (count<  0)
>>               return -1;
>> -     n = 0;
>> -     for (i = 0; i<  count; i++) {
>> -             source = ep[i].data.ptr;
>
>
> How about just
>
>                if (!source)
>                        continue;
>
> here? In particular since removing a source *must* remove it from
> the event handling for it not to crash out later.
>

That would not work because ep[i].data.ptr will already be loaded with
the source pointer pointing to deallocated memory. It doesn't matter
if that particular source is free:ed and removed from the epoll state
since it has already been triggered to dispatch. Another approach
would be to flag an event source for removal if it already has been
queued up then have the event_loop_dispatch function remove it after
or instead of handling it. Something like:

epoll_wait(...);
forall event in events
    event.source.queued = true
forall event in events
    if (event.source.destroy)
        free(event.source)
    else
        handle(event)
        event.source.queued = false


And then instead of free:ing and removing the event source from the
dispatch loop the user could:

if (source.queued)
    source.destroy = true
else
    free(source)

or similar.

I've also considered that one can disallow such behavior and adding
interfaces for enabling or disabling event sources which would be
usable by third party users, but didn't seem optimal.

Jonas