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

[Andreas Ericsson]

On 03/19/2012 09:44 PM, Bill Spitzak wrote:
> 
> 
> Jonas Ådahl wrote:
>> On Mon, Mar 19, 2012 at 7:10 PM, Bill Spitzak <spitzak at gmail.com>
>> wrote:
>>> Jonas Ådahl wrote:
>>> 
>>>> 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.
>>> 
>>> How about having the data point at another object that points at
>>> the source. This object is not deleted until after the epoll is
>>> used, but when the source is deleted it is set to zero so this is
>>> detectable.
>>> 
>> 
>> An event source container acting as a proxy between epoll and the 
>> event source? When receiving an event, instead of marking the
>> event source directly, one could mark the container as queued. When
>> an event source is removed from epoll then one can check if it has
>> been queued by accessing the container via the event source, and in
>> that case mark it for destruction, or free it directly in case it
>> wasn't. Is this what you mean? It sounds quite reasonable.
> 
> No I meant a simpler version. There is no "mark for destruction", the
> object is destroyed immediately. But a side-effect of destruction is
> that the pointer from the epoll structure pointing at it is set to
> zero. Epoll handling is something like this:
> 
> source** p = (source**)(epoll[n].data); if (*p) ((*p)->stuff());
> 
> The destruction of a source clears *p. p itself is not destroyed
> until the epoll object is.
> 

The epoll object has to be destroyed immediately, lest we poll
closed file descriptors (I'm assuming the removal of a source is
usually accompanied by the closing of its filedescriptor...).

> Assuming the set of source objects is somewhat stable (ie you will
> call epoll many times verses the number of times the source list
> chagnes) I would keep all the p pointers in static memory, perhaps in
> a table. The empty entries are reclaimed at points where you know
> there is no epoll using them.


There are two (interestincag) cases to consider. In both cases, dispatch
for sourceA will run first and want to delete sourceB:

sourceA and sourceB both have input
sourceA has input. dispatchB does not.

Currently, we handle the second case just fine (since we'll never want
to access sourceB's pointer again after A free's it), but the first case
causes crashes.


The ways to handle both are:
* delete it immediately and disallow dispatch functions from removing
  sources from the event loop.
Obviously the simplest solution in terms of code, but really a horrible
idea from an API user standpoint.

* keep a separate way of looking up the data one wants to access
This isn't necessarily as much work as it sounds, since we can prealloc
an array to the size of the max number of filedescriptors we're allowed
to use and simply use the filedescriptor as index. That's negligible
memory overhead and O(1) complexity. I chose this way to do things in
my I/O broker.

* maintain a list of entries that should be free()'d
This has the least memory overhead, but gets cumbersome and may promote
memory fragmentation, since the object to remove has to live beyond its
time, while other objects will potentially want to replace it. Possibly
not a big issue, but I'm raising the flag anyway.
We can't just mark the source as removed when we're running dispatchers,
since we won't encounter it if it doesn't have input. Walking the list
of input events twice to mark the ones with input first is so stupid I
refuse to even consider it.
Fortunately, wayland already maintains a list of sources to check post
dispatch time, so we can safely free() the deathmarked ones there.

I'm wondering if we can get rid of that post_dispatch_check() altogether
by using an index though, but since I can't compile wayland I'm sort of
shooting from the hip with double blindfolds.

-- 
Andreas Ericsson                   andreas.ericsson at op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Considering the successes of the wars on alcohol, poverty, drugs and
terror, I think we should give some serious thought to declaring war
on peace.