On Mon, Nov 25, 2013 at 4:53 PM, Jim Jagielski <j...@jagunet.com> wrote:
> > On Nov 25, 2013, at 10:24 AM, Yann Ylavic <ylavic....@gmail.com> wrote: > > > > > As Jeff said in the other thread, I think the test here should be : > > if (prev_idlers <= 1) > > that's because dec32 was returning the new value while add32 now returns > the old one (fetch_and_sub vs sub_and_fetch). > > > > We aren't being consistent.. see below: > > > +static const apr_uint32_t zero_pt = ((apr_uint32_t)1 << 31); > > Hmmmm... for a 32bit int, shouldn't that be << 29 ?? > I don't see, why 29? Suppose idlers is 0: idlers - zero_pt == 0 - 2^31 == INT32_MIN; and when idlers is 2^32-1: (2^32-1) - 2^31 == INT32_MAX. Is there a need for more positives than negatives? > > > > > 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) { > > Note the check here.... > > > - apr_atomic_inc32((apr_uint32_t *)&(queue_info->idlers)); /* > back out dec */ > > + apr_int32_t prev_idlers; > > + prev_idlers = apr_atomic_add32(&queue_info->idlers, -1) - zero_pt; > > + if (prev_idlers <= 1) { > > + apr_atomic_inc32(&queue_info->idlers); /* back out dec */ > > return APR_EAGAIN; > > } > > return APR_SUCCESS; > > @@ -140,11 +142,11 @@ apr_status_t ap_queue_info_wait_for_idler(fd_queue > > 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) - zero_pt; > > + prev_idlers = apr_atomic_add32(&queue_info->idlers, -1) - zero_pt; > > > > /* Block if there weren't any idle workers */ > > if (prev_idlers <= 0) { > > And here.... > Indeed, that was not consistent! This is a second fix then, the check was doubly broken...