Re: [PATCH v2 3/3] main: Safely free watch_data structures

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2451 bytes --]

Hi Mat,

 >>>
>>> +    watch_free_list = l_queue_new();
>>
>> l_queue_new cannot fail.
>
> Ok. I will remove the similar check for idle_list just above this
> (before the patch context) in a separate patch.
>

Thanks.  Some of this code you are touching was created very early in 
ell's lifetime.  So consistency is not quite there yet.

<snip>

>>> -    l_free(data);
>>> +    if (epoll_running) {
>>> +        /* The epoll events array may point to the same memory as
>>> +         * 'data', so let the event loop free it later
>>> +         */
>>> +        data->callback = watch_nop_callback;
>>> +        l_queue_push_tail(watch_free_list, data);
>>
>> Why don't we do this exactly how the idle events are handled.  E.g.
>> set a DESTROYED flag and prune the event list after the epoll
>> processing is complete.
>>
>
> In typical use, the idle_list linked list is short or empty and all
> entries are cleaned up on each event loop iteration -- otherwise you're
> at 100% CPU. The DESTROYED flag fits well with this, since you plan on
> visiting every entry in the list anyway.
>
> The assumptions around watch_list are the opposite: big list, infrequent
> cleanup. watch_list is an array with 128 entries, and you'd have to
> check all 128 entries on every iteration of the event loop. This could

I'm not sure than walking a 128-entry array would be any slower than 
calling malloc/free.  I bet it would be faster.  Would be fun to find out.

> be optimized (use a bitmap to flag which ones to free), but there's
> another problem: watch_list is indexed by an fd which was likely closed
> by the data->destroy() callback. It's possible for the fd to get re-used
> and a new watch to be created on that fd even before the data->destroy
> callback returns.

But then won't you be calling the watch's callback erroneously anyway? 
If this is a concern, then all new watches need to be postponed until 
after the epoll processing has been completed.

>
>
> Another proposal: have watch_remove set a global flag that breaks out of
> the inner "for (n = 0; n < nfds; n++)" loop if watch_remove was called,
> and retries the epoll_wait() without processing idle_list. While it's
> pretty simple to implement, there are drawbacks:

Lets not do this unless there is no better way.  Re-trapping to the 
kernel is way more expensive than anything else we're discussing here.

Regards,
-Denis