From mboxrd@z Thu Jan  1 00:00:00 1970
Content-Type: multipart/mixed; boundary="===============3703985767417982781=="
MIME-Version: 1.0
From: Denis Kenzior <denkenz@gmail.com>
Subject: Re: [PATCH v2 3/3] main: Safely free watch_data structures
Date: Wed, 16 Mar 2016 17:40:22 -0500
Message-ID: <56E9E0D6.3080902@gmail.com>
In-Reply-To: <alpine.OSX.2.20.1603161315580.13595@mjmartin-mac01.local>
List-Id: <ell.lists.01.org>
To: ell@lists.01.org

--===============3703985767417982781==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

Hi Mat,

 >>>
>>> +    watch_free_list =3D 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 =3D 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 =3D 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

--===============3703985767417982781==--