httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Luca Toscano <toscano.l...@gmail.com>
Subject Re: Frequent wake-ups for mpm_event
Date Fri, 12 Aug 2016 13:42:17 GMT
2016-08-11 8:43 GMT+02:00 Luca Toscano <toscano.luca@gmail.com>:

>
>
> 2016-08-09 15:02 GMT+02:00 Luca Toscano <toscano.luca@gmail.com>:
>
>> Thanks a lot for the feedback, trying to answer to everybody:
>>
>> 2016-08-05 15:19 GMT+02:00 Jim Jagielski <jim@jagunet.com>:
>>
>>> If APR_POLLSET_WAKEABLE was more universal and, therefore,
>>> more widely tested, I'd be +1... as it is, let's see what
>>> the feedback is.
>>
>>
>> Definitely, we should find a good way to test this if we'll reach
>> consensus on a patch. Maybe some stress tests plus "incubation" in trunk
>> for a while could be good enough, but I agree that it would be an important
>> change to make with extreme care.
>>
>> 2016-08-05 15:26 GMT+02:00 Yann Ylavic <ylavic.dev@gmail.com>:
>>
>>> On Fri, Aug 5, 2016 at 3:19 PM, Yann Ylavic <ylavic.dev@gmail.com>
>>> wrote:
>>> > Hi Luca,
>>> >
>>> > On Fri, Aug 5, 2016 at 2:58 PM, Luca Toscano <toscano.luca@gmail.com>
>>> wrote:
>>> >>
>>> >> 2016-08-04 17:56 GMT+02:00 Luca Toscano <toscano.luca@gmail.com>:
>>> >>>
>>> >>> Would it be possible to avoid them adding APR_POLLSET_WAKEABLE to
the
>>> >>> event_pollset flags and calling apr_pollset_wakeup right after
>>> >>> apr_skiplist_insert?
>>> >
>>> > That might be possible, but there is more than apr_skiplist_insert()
>>> > to handle I'm afraid, see below.
>>> >
>>> >>
>>> >>  Very high level idea: http://apaste.info/MKE
>>> >
>>> > The timeout_interval also handles the listener_may_exit (and
>>> > connection_count == 0 in graceful mode), your patch seems not handle
>>> > them...
>>>
>>
>> You are right, and IIUC these are the use cases to cover:
>>
>> 1) listener thread(s) blocked on apr_pollset_poll and a call to
>> wakeup_listener (that afaiu is the only one setting listener_may_exit to 1);
>> 2) listener thread(s) blocked on apr_pollset_poll, listener_may_exit = 1
>> and connection_count still greater than zero. The last connection cleanup
>> (that calls decrement_connection_count registered previously
>> with apr_pool_cleanup_register) should be able to wake up the listener
>> that in turns will be able to execute the break in the following block:
>>
>>         if (listener_may_exit) {
>>             close_listeners(process_slot, &closed);
>>             if (terminate_mode == ST_UNGRACEFUL
>>                 || apr_atomic_read32(&connection_count) == 0)
>>                 break;
>>         }
>>
>> I would also add another one, namely if the pollset create does not
>> support APR_POLLSET_WAKEABLE. This would mean that in absence of
>> apr_pollset_wakeup we should rely on apr_pollset_poll with the 100 ms
>> timeout, keeping it as "fallback" to ensure compatibility with most of the
>> systems.
>>
>>
>>> > The listener_may_exit case may be easily handled in wakeup_listener
>>> > (one more apr_pollset_wakeup() there), however the connection_count
>>> > looks more complicated.
>>> >
>>> > IOW, not sure something like the below would be enough...
>>> >
>>> > Index: server/mpm/event/event.c
>>> > ===================================================================
>>> > --- server/mpm/event/event.c    (revision 1755288)
>>> > +++ server/mpm/event/event.c    (working copy)
>>> > @@ -484,6 +484,7 @@ static void close_worker_sockets(void)
>>> >  static void wakeup_listener(void)
>>> >  {
>>> >      listener_may_exit = 1;
>>> > +    apr_pollset_wakeup(event_pollset);
>>> >      if (!listener_os_thread) {
>>> >          /* XXX there is an obscure path that this doesn't handle
>>> perfectly:
>>> >           *     right after listener thread is created but before
>>>
>>> Or rather, in wakeup_listener():
>>>
>>> @@ -493,6 +493,9 @@ static void wakeup_listener(void)
>>>          return;
>>>      }
>>>
>>> +    /* unblock the listener if it's waiting for a timer */
>>> +    apr_pollset_wakeup(event_pollset);
>>> +
>>>      /* unblock the listener if it's waiting for a worker */
>>>      ap_queue_info_term(worker_queue_info);
>>>
>>> _
>>>
>>> > @@ -696,7 +697,9 @@ static apr_status_t decrement_connection_count(voi
>>> >          default:
>>> >              break;
>>> >      }
>>> > -    apr_atomic_dec32(&connection_count);
>>> > +    if (!apr_atomic_dec32(&connection_count) && listener_may_exit)
{
>>> > +        apr_pollset_wakeup(event_pollset);
>>> > +    }
>>> >      return APR_SUCCESS;
>>> >  }
>>>
>>
>>  I like these suggestions, they look good! (This assuming that what I
>> have written above is correct, otherwise I didn't get them :)
>>
>
> I applied Yann's suggestions and added some checks to allow wakeable and
> non wakeable listeners: http://apaste.info/mke
>
> This is a very high level prototype, please let me know if you have more
> suggestions (of course the discussion around testing and reliability of
> APR_POLLSET_WAKEABLE still holds).
>

This patch might also miss another point, namely the calls to
process_timeout_queue to manage Keep Alive timeouts, lingering closes and
write completion. IIUC mpm-event explicitly process them after each wake up
(every 100ms).

Will work more on it, in the meantime thanks to all for the feedback!

Luca

Mime
View raw message