From: Jim Jagielski Date: Mon, 17 Feb 2014 14:16:57 +0000 (+0000) Subject: Merge r1545286, r1545292, r1545325, r1545364, r1545408, r1545411 from trunk: X-Git-Tag: 2.4.8~107 X-Git-Url: https://granicus.if.org/sourcecode?a=commitdiff_plain;h=6e0f3ef5dffc8f4ff63db8fb230de133c54a25af;p=apache Merge r1545286, r1545292, r1545325, r1545364, r1545408, r1545411 from trunk: Use a normalized offset point for idlers... still need to worry that atomics work as "expected", in this case that a add32 of a -1 is the "same" as dec32 (as far as effect on idlers) r1545286 for eventopt Use correct type... Use offset which is smack dab in the middle. naming suggestion re: trawick Consistent types Reviewed/backported by: jim git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/branches/2.4.x@1569008 13f79535-47bb-0310-9956-ffa450edef68 --- diff --git a/server/mpm/event/event.c b/server/mpm/event/event.c index 306837dbd1..7a95498ea3 100644 --- a/server/mpm/event/event.c +++ b/server/mpm/event/event.c @@ -2866,16 +2866,14 @@ static int event_pre_config(apr_pool_t * pconf, apr_pool_t * plog, } ++retained->module_loads; if (retained->module_loads == 2) { - int i; - static apr_uint32_t foo = 0; + /* test for correct operation of fdqueue */ + static apr_uint32_t foo1, foo2; - apr_atomic_inc32(&foo); - apr_atomic_dec32(&foo); - apr_atomic_dec32(&foo); - i = apr_atomic_dec32(&foo); - if (i >= 0) { + apr_atomic_set32(&foo1, 100); + foo2 = apr_atomic_add32(&foo1, -10); + if (foo2 != 100 || foo1 != 90) { ap_log_error(APLOG_MARK, APLOG_CRIT, 0, NULL, APLOGNO(02405) - "atomics not working as expected"); + "atomics not working as expected - add32 of negative number"); return HTTP_INTERNAL_SERVER_ERROR; } rv = apr_pollset_create(&event_pollset, 1, plog, diff --git a/server/mpm/event/fdqueue.c b/server/mpm/event/fdqueue.c index 865b8358df..2fa7e1e52c 100644 --- a/server/mpm/event/fdqueue.c +++ b/server/mpm/event/fdqueue.c @@ -17,6 +17,8 @@ #include "fdqueue.h" #include "apr_atomic.h" +static apr_uint32_t zero_pt = APR_UINT32_MAX/2; + struct recycled_pool { apr_pool_t *pool; @@ -25,10 +27,10 @@ struct recycled_pool struct fd_queue_info_t { - apr_int32_t idlers; /** - * 0 or positive: number of idle worker threads - * negative: number of threads blocked waiting - * for an idle worker + apr_uint32_t idlers; /** + * >= zero_pt: number of idle worker threads + * < zero_pt: number of threads blocked waiting + * for an idle worker */ apr_thread_mutex_t *idlers_mutex; apr_thread_cond_t *wait_for_idler; @@ -82,6 +84,7 @@ apr_status_t ap_queue_info_create(fd_queue_info_t ** queue_info, qi->recycled_pools = NULL; qi->max_recycled_pools = max_recycled_pools; qi->max_idlers = max_idlers; + qi->idlers = zero_pt; apr_pool_cleanup_register(pool, qi, queue_info_cleanup, apr_pool_cleanup_null); @@ -94,17 +97,12 @@ apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info, apr_pool_t * pool_to_recycle) { apr_status_t rv; - int prev_idlers; + apr_int32_t prev_idlers; ap_push_pool(queue_info, pool_to_recycle); /* Atomically increment the count of idle workers */ - /* - * TODO: The atomics expect unsigned whereas we're using signed. - * Need to double check that they work as expected or else - * rework how we determine blocked. - */ - prev_idlers = apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); + prev_idlers = apr_atomic_inc32(&(queue_info->idlers)) - zero_pt; /* If other threads are waiting on a worker, wake one up */ if (prev_idlers < 0) { @@ -129,10 +127,10 @@ apr_status_t ap_queue_info_set_idle(fd_queue_info_t * queue_info, apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info) { - int prev_idlers; - prev_idlers = apr_atomic_dec32((apr_uint32_t *)&(queue_info->idlers)); - if (prev_idlers <= 0) { - apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* back out dec */ + apr_int32_t new_idlers; + new_idlers = apr_atomic_add32(&(queue_info->idlers), -1) - zero_pt; + if (--new_idlers <= 0) { + apr_atomic_inc32(&(queue_info->idlers)); /* back out dec */ return APR_EAGAIN; } return APR_SUCCESS; @@ -142,11 +140,11 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info, int *had_to_block) { apr_status_t rv; - int prev_idlers; + apr_int32_t prev_idlers; /* Atomically decrement the idle worker count, saving the old value */ /* See TODO in ap_queue_info_set_idle() */ - prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1); + prev_idlers = apr_atomic_add32(&(queue_info->idlers), -1) - zero_pt; /* Block if there weren't any idle workers */ if (prev_idlers <= 0) { @@ -154,7 +152,7 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info, if (rv != APR_SUCCESS) { AP_DEBUG_ASSERT(0); /* See TODO in ap_queue_info_set_idle() */ - apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* back out dec */ + apr_atomic_inc32(&(queue_info->idlers)); /* back out dec */ return rv; } /* Re-check the idle worker count to guard against a @@ -173,10 +171,11 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info, * now non-negative, it's safe for this function to * return immediately. * - * A negative value in queue_info->idlers tells how many + * A "negative value" (relative to zero_pt) in + * queue_info->idlers tells how many * threads are waiting on an idle worker. */ - if (queue_info->idlers < 0) { + if (queue_info->idlers < zero_pt) { *had_to_block = 1; rv = apr_thread_cond_wait(queue_info->wait_for_idler, queue_info->idlers_mutex); @@ -207,7 +206,7 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue_info_t * queue_info, apr_uint32_t ap_queue_info_get_idlers(fd_queue_info_t * queue_info) { apr_int32_t val; - val = (apr_int32_t)apr_atomic_read32((apr_uint32_t *)&queue_info->idlers); + val = (apr_int32_t)apr_atomic_read32(&queue_info->idlers) - zero_pt; if (val < 0) return 0; return val;