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 Tue, 09 Aug 2016 13:02:54 GMT
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 :)

Regards,

 Luca

Mime
View raw message