On Thu, Aug 18, 2016 at 11:04 AM, Plüm, Rüdiger, Vodafone Group
<ruediger.pl...@vodafone.com> wrote:
>
> From: Luca Toscano [mailto:toscano.l...@gmail.com]
> Sent: Freitag, 12. August 2016 15:42
> To: Apache HTTP Server Development List
> Subject: Re: Frequent wake-ups for mpm_event
>
>> 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!
>
> Does it make sense to get the shortest timeout from keepalive_q,
> write_completion_q, linger_q and short_linger_q and set this value instead
> of -1?
I was thinking of something like the attached patch, where we compute
the exact/minimal timeout needed for the queues.
It requires walking/locking the queues (looking at the first item
only), but besides getting the exact poll() timeout which helps
spurious wakeups, it also allows to avoid walking this queues after
the poll() (previously each 0.1s, for maintenance) when not necessary.
So overall I think it's a gain.
The patch is only compile-tested and may not work as is, I just wanted
to show the principle for feedbacks. WDYT?
Regards,
Yann.
PS: I seem to have read some proposal on our bz (edit: PR 60006) to
replace the g_timer_skiplist_mtx with a spinlock for performances.
This looks like a good idea if the contention is low (acquired for
each polling loop), possibly also for the queues' mutex...
APR lacks spinlocks, though.
Index: server/mpm/event/event.c
===================================================================
--- server/mpm/event/event.c (revision 1756556)
+++ server/mpm/event/event.c (working copy)
@@ -1742,6 +1742,13 @@ static void * APR_THREAD_FUNC listener_thread(apr_
apr_time_t timeout_time = 0, last_log;
int closed = 0, listeners_disabled = 0;
int have_idle_worker = 0;
+ struct timeout_queue *timeout_queues[] = {
+ keepalive_q,
+ write_completion_q,
+ short_linger_q,
+ linger_q,
+ NULL
+ };
last_log = apr_time_now();
free(ti);
@@ -1776,6 +1783,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
apr_int32_t num = 0;
apr_uint32_t c_count, l_count, i_count;
apr_interval_time_t timeout_interval;
+ struct timeout_queue **qp;
apr_time_t now;
int workers_were_busy = 0;
if (listener_may_exit) {
@@ -1821,20 +1829,9 @@ static void * APR_THREAD_FUNC listener_thread(apr_
#endif
now = apr_time_now();
+ timeout_time = timeout_interval = -1;
apr_thread_mutex_lock(g_timer_skiplist_mtx);
- te = apr_skiplist_peek(timer_skiplist);
- if (te) {
- if (te->when > now) {
- timeout_interval = te->when - now;
- }
- else {
- timeout_interval = 1;
- }
- }
- else {
- timeout_interval = apr_time_from_msec(100);
- }
- while (te) {
+ while ((te = apr_skiplist_peek(timer_skiplist))) {
if (te->when < now + EVENT_FUDGE_FACTOR) {
apr_skiplist_pop(timer_skiplist, NULL);
if (!te->canceled) {
@@ -1853,12 +1850,35 @@ static void * APR_THREAD_FUNC listener_thread(apr_
}
}
else {
+ timeout_interval = te->when - now;
break;
}
- te = apr_skiplist_peek(timer_skiplist);
}
apr_thread_mutex_unlock(g_timer_skiplist_mtx);
+ apr_thread_mutex_lock(timeout_mutex);
+ for (qp = timeout_queues; *qp; ++qp) {
+ struct timeout_queue *q = *qp;
+ do {
+ event_conn_state_t *cs = APR_RING_FIRST(&q->head);
+ if (cs != APR_RING_SENTINEL(&q->head, event_conn_state_t,
+ timeout_list)) {
+ apr_time_t left = cs->queue_timestamp + q->timeout - now;
+ if (left <= 0 || cs->queue_timestamp > now + q->timeout) {
+ timeout_time = now + 1;
+ timeout_interval = 1;
+ }
+ else if (timeout_interval < 0 || timeout_interval > left) {
+ timeout_interval = left;
+ }
+ if (timeout_time < 0 || timeout_time > now + left) {
+ timeout_time = now + left;
+ }
+ }
+ } while ((q = q->next));
+ }
+ apr_thread_mutex_unlock(timeout_mutex);
+
rc = apr_pollset_poll(event_pollset, timeout_interval, &num, &out_pfd);
if (rc != APR_SUCCESS) {
if (APR_STATUS_IS_EINTR(rc)) {
@@ -1871,6 +1891,7 @@ static void * APR_THREAD_FUNC listener_thread(apr_
"shutdown process gracefully");
signal_threads(ST_GRACEFUL);
}
+ num = 0;
}
if (listener_may_exit) {
@@ -2083,12 +2104,8 @@ static void * APR_THREAD_FUNC listener_thread(apr_
/* XXX possible optimization: stash the current time for use as
* r->request_time for new requests
*/
- now = apr_time_now();
- /* We only do this once per 0.1s (TIMEOUT_FUDGE_FACTOR), or on a clock
- * skew (if the system time is set back in the meantime, timeout_time
- * will exceed now + TIMEOUT_FUDGE_FACTOR, can't happen otherwise).
- */
- if (now > timeout_time || now + TIMEOUT_FUDGE_FACTOR < timeout_time ) {
+ /* We only do this when queues' timeout_time computed above expires */
+ if (timeout_time >= 0 && (now = apr_time_now()) > timeout_time) {
struct process_score *ps;
timeout_time = now + TIMEOUT_FUDGE_FACTOR;
@@ -2096,22 +2113,8 @@ static void * APR_THREAD_FUNC listener_thread(apr_
apr_thread_mutex_lock(timeout_mutex);
/* Step 1: keepalive timeouts */
- /* If all workers are busy, we kill older keep-alive connections
so that they
- * may connect to another process.
- */
- if ((workers_were_busy || dying) && *keepalive_q->total) {
- if (!dying)
- ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, ap_server_conf,
- "All workers are busy, will close %d
keep-alive "
- "connections",
- *keepalive_q->total);
- process_timeout_queue(keepalive_q, 0,
- start_lingering_close_nonblocking);
- }
- else {
- process_timeout_queue(keepalive_q, timeout_time,
- start_lingering_close_nonblocking);
- }
+ process_timeout_queue(keepalive_q, timeout_time,
+ start_lingering_close_nonblocking);
/* Step 2: write completion timeouts */
process_timeout_queue(write_completion_q, timeout_time,
start_lingering_close_nonblocking);
@@ -2129,6 +2132,18 @@ static void * APR_THREAD_FUNC listener_thread(apr_
ps->suspended = apr_atomic_read32(&suspended_count);
ps->lingering_close = apr_atomic_read32(&lingering_count);
}
+ else if ((workers_were_busy || dying) && *keepalive_q->total) {
+ if (!dying) {
+ ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, ap_server_conf,
+ "All workers are busy, will close %d keep-alive "
+ "connections", *keepalive_q->total);
+ }
+ apr_thread_mutex_lock(timeout_mutex);
+ process_timeout_queue(keepalive_q, 0,
+ start_lingering_close_nonblocking);
+ apr_thread_mutex_unlock(timeout_mutex);
+ }
+
if (listeners_disabled && !workers_were_busy
&& ((c_count = apr_atomic_read32(&connection_count))
>= (l_count = apr_atomic_read32(&lingering_count))
