You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ji...@apache.org on 2013/11/25 22:10:05 UTC
svn commit: r1545408 - in /httpd/httpd/trunk/server/mpm: event/fdqueue.c
eventopt/fdqueue.c
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/eventopt/fdqueue.c
Modified: httpd/httpd/trunk/server/mpm/event/fdqueue.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/fdqueue.c?rev=1545408&r1=1545407&r2=1545408&view=diff
==============================================================================
--- 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_set_idle(fd_q
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;
}
Modified: httpd/httpd/trunk/server/mpm/eventopt/fdqueue.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/eventopt/fdqueue.c?rev=1545408&r1=1545407&r2=1545408&view=diff
==============================================================================
--- httpd/httpd/trunk/server/mpm/eventopt/fdqueue.c (original)
+++ httpd/httpd/trunk/server/mpm/eventopt/fdqueue.c Mon Nov 25 21:10:05 2013
@@ -127,9 +127,9 @@ apr_status_t ap_queue_info_set_idle(fd_q
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;
}
Re: svn commit: r1545408 - in /httpd/httpd/trunk/server/mpm:
event/fdqueue.c eventopt/fdqueue.c
Posted by Yann Ylavic <yl...@gmail.com>.
Hi,
yes, it's been a while now, sorry for that...
On Mon, Nov 25, 2013 at 10:10 PM, <ji...@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.