httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1545408 - in /httpd/httpd/trunk/server/mpm: event/fdqueue.c eventopt/fdqueue.c
Date Tue, 02 Dec 2014 18:37:03 GMT
Hi,

yes, it's been a while now, sorry for that...

On Mon, Nov 25, 2013 at 10:10 PM,  <jim@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.

Mime
View raw message