On Nov 25, 2013, at 10:24 AM, Yann Ylavic <[email protected]> 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 ??
>
> 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....
