Hi, yes, it's been a while now, sorry for that...
On Mon, Nov 25, 2013 at 10:10 PM, <j...@apache.org> wrote: > Author: jim > Date: Mon Nov 25 21:10:05 2013 > New Revision: 1545408 > > URL: http://svn.apache.org/r1545408 > Log: > naming suggestion re: trawick > > Modified: > httpd/httpd/trunk/server/mpm/event/fdqueue.c [] > --- httpd/httpd/trunk/server/mpm/event/fdqueue.c (original) > +++ httpd/httpd/trunk/server/mpm/event/fdqueue.c Mon Nov 25 21:10:05 2013 > @@ -127,9 +127,9 @@ > > apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info) > { > - int prev_idlers; > - prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), > -1) - zero_pt; > - if (prev_idlers <= 0) { > + int new_idlers; > + new_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1) > - zero_pt; > + if (--new_idlers <= 0) { > apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* back > out dec */ > return APR_EAGAIN; > } This change is a follow up to http://svn.apache.org/r1545286, based on the original code (more than 3 years old, by sf@) : @@ -131,7 +128,7 @@ 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)); + prev_idlers = apr_atomic_add32((apr_uint32_t *)&(queue_info->idlers), -1) - zero_pt; if (prev_idlers <= 0) { apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* back out dec */ return APR_EAGAIN; The whole is consistent with the original code, since apr_atomic_dec32() returned the new value (sub-and-fecth), whereas apr_atomic_add32() now returns the previous value (fetch-and-sub), thus --new_idlers in the new code is equal to prev_idlers in the original code, so r1545286 + r1545408 did not change the semantic of ap_queue_info_try_get_idler(). However I think this semantic was/is incorrect. Let's look at the current (full) code : apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info) { apr_int32_t new_idlers; a. new_idlers = apr_atomic_add32(&(queue_info->idlers), -1) - zero_pt; b. if (--new_idlers <= 0) { apr_atomic_inc32(&(queue_info->idlers)); /* back out dec */ return APR_EAGAIN; } return APR_SUCCESS; } Suppose there is initially one idle woker (ie. queue_info->idlers is 1), no concurrency. After a. new_idlers is 1, and after b. it is 0 => EAGAIN. Shouldn't ap_queue_info_try_get_idler() "consume" the last idler in this case? I think the code should really be (back to r1545286) : apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info) { apr_int32_t prev_idlers; prev_idlers = apr_atomic_add32(&(queue_info->idlers), -1) - zero_pt; if (prev_idlers <= 0) { apr_atomic_inc32(&(queue_info->idlers)); /* back out dec */ return APR_EAGAIN; } return APR_SUCCESS; } or even : apr_status_t ap_queue_info_try_get_idler(fd_queue_info_t * queue_info) { for (;;) { apr_uint32_t prev_idlers; apr_uint32_t loop_idlers = queue_info->idlers; prev_idlers = apr_atomic_cas32(&queue_info->idlers, loop_idlers - (loop_idlers > ZERO_PT), loop_idlers); if (prev_idlers <= ZERO_PT) { return APR_EAGAIN; } if (prev_idlers == loop_idlers) { return APR_SUCCESS; } } } so that we don't need to "back out dec" when there is no idler (thus avoiding a race with the other threads in the time between dec32 and inc32). Am I missing something? Regards, Yann.