You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by br...@apache.org on 2003/01/03 19:33:00 UTC
cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
brianp 2003/01/03 10:32:59
Modified: server/mpm/worker fdqueue.c
Log:
Replace most of the mutex locking in the worker MPM's "queue info"
object with atomic compare-and-swap loops.
Revision Changes Path
1.24 +111 -42 httpd-2.0/server/mpm/worker/fdqueue.c
Index: fdqueue.c
===================================================================
RCS file: /home/cvs/httpd-2.0/server/mpm/worker/fdqueue.c,v
retrieving revision 1.23
retrieving revision 1.24
diff -u -r1.23 -r1.24
--- fdqueue.c 2 Aug 2002 17:37:52 -0000 1.23
+++ fdqueue.c 3 Jan 2003 18:32:59 -0000 1.24
@@ -57,6 +57,12 @@
*/
#include "fdqueue.h"
+#include "apr_atomic.h"
+
+typedef struct recycled_pool {
+ apr_pool_t *pool;
+ struct recycled_pool *next;
+} recycled_pool;
struct fd_queue_info_t {
int idlers;
@@ -64,19 +70,27 @@
apr_thread_cond_t *wait_for_idler;
int terminated;
int max_idlers;
- apr_pool_t **recycled_pools;
- int num_recycled;
+ recycled_pool *recycled_pools;
};
static apr_status_t queue_info_cleanup(void *data_)
{
fd_queue_info_t *qi = data_;
- int i;
apr_thread_cond_destroy(qi->wait_for_idler);
apr_thread_mutex_destroy(qi->idlers_mutex);
- for (i = 0; i < qi->num_recycled; i++) {
- apr_pool_destroy(qi->recycled_pools[i]);
+
+ /* Clean up any pools in the recycled list */
+ for (;;) {
+ struct recycled_pool *first_pool = qi->recycled_pools;
+ if (first_pool == NULL) {
+ break;
+ }
+ if (apr_atomic_casptr(&(qi->recycled_pools), first_pool->next,
+ first_pool) == first_pool) {
+ apr_pool_destroy(first_pool->pool);
+ }
}
+
return APR_SUCCESS;
}
@@ -98,9 +112,7 @@
if (rv != APR_SUCCESS) {
return rv;
}
- qi->recycled_pools = (apr_pool_t **)apr_palloc(pool, max_idlers *
- sizeof(apr_pool_t *));
- qi->num_recycled = 0;
+ qi->recycled_pools = NULL;
qi->max_idlers = max_idlers;
apr_pool_cleanup_register(pool, qi, queue_info_cleanup,
apr_pool_cleanup_null);
@@ -114,24 +126,53 @@
apr_pool_t *pool_to_recycle)
{
apr_status_t rv;
- rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
- if (rv != APR_SUCCESS) {
- return rv;
- }
- AP_DEBUG_ASSERT(queue_info->idlers >= 0);
- AP_DEBUG_ASSERT(queue_info->num_recycled < queue_info->max_idlers);
+ int prev_idlers;
+
+ /* If we have been given a pool to recycle, atomically link
+ * it into the queue_info's list of recycled pools
+ */
if (pool_to_recycle) {
- queue_info->recycled_pools[queue_info->num_recycled++] =
- pool_to_recycle;
+ struct recycled_pool *new_recycle;
+ new_recycle = (struct recycled_pool *)apr_palloc(pool_to_recycle,
+ sizeof(*new_recycle));
+ new_recycle->pool = pool_to_recycle;
+ for (;;) {
+ new_recycle->next = queue_info->recycled_pools;
+ if (apr_atomic_casptr(&(queue_info->recycled_pools),
+ new_recycle, new_recycle->next) ==
+ new_recycle->next) {
+ break;
+ }
+ }
}
- if (queue_info->idlers++ == 0) {
- /* Only signal if we had no idlers before. */
- apr_thread_cond_signal(queue_info->wait_for_idler);
+
+ /* Atomically increment the count of idle workers */
+ for (;;) {
+ prev_idlers = queue_info->idlers;
+ if (apr_atomic_cas(&(queue_info->idlers), prev_idlers + 1, prev_idlers)
+ == prev_idlers) {
+ break;
+ }
}
- rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
- if (rv != APR_SUCCESS) {
- return rv;
+
+ /* If this thread just made the idle worker count nonzero,
+ * wake up the listener. */
+ if (prev_idlers == 0) {
+ rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
+ rv = apr_thread_cond_signal(queue_info->wait_for_idler);
+ if (rv != APR_SUCCESS) {
+ apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ return rv;
+ }
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ return rv;
+ }
}
+
return APR_SUCCESS;
}
@@ -139,34 +180,62 @@
apr_pool_t **recycled_pool)
{
apr_status_t rv;
+
*recycled_pool = NULL;
- rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
- if (rv != APR_SUCCESS) {
- return rv;
- }
- AP_DEBUG_ASSERT(queue_info->idlers >= 0);
- while ((queue_info->idlers == 0) && (!queue_info->terminated)) {
- rv = apr_thread_cond_wait(queue_info->wait_for_idler,
- queue_info->idlers_mutex);
+
+ /* Block if the count of idle workers is zero */
+ if (queue_info->idlers == 0) {
+ rv = apr_thread_mutex_lock(queue_info->idlers_mutex);
if (rv != APR_SUCCESS) {
- apr_status_t rv2;
- rv2 = apr_thread_mutex_unlock(queue_info->idlers_mutex);
- if (rv2 != APR_SUCCESS) {
- return rv2;
+ return rv;
+ }
+ /* Re-check the idle worker count: one of the workers might
+ * have incremented the count since we last checked it a
+ * few lines above. If it's still zero now that we're in
+ * the mutex-protected region, it's safe to block on the
+ * condition variable.
+ */
+ if (queue_info->idlers == 0) {
+ rv = apr_thread_cond_wait(queue_info->wait_for_idler,
+ queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
+ apr_status_t rv2;
+ rv2 = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv2 != APR_SUCCESS) {
+ return rv2;
+ }
+ return rv;
}
+ }
+ rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
+ if (rv != APR_SUCCESS) {
return rv;
}
}
- queue_info->idlers--; /* Oh, and idler? Let's take 'em! */
- if (queue_info->num_recycled) {
- *recycled_pool =
- queue_info->recycled_pools[--queue_info->num_recycled];
+
+ /* Atomically decrement the idle worker count */
+ for (;;) {
+ apr_uint32_t prev_idlers = queue_info->idlers;
+ if (apr_atomic_cas(&(queue_info->idlers), prev_idlers - 1, prev_idlers)
+ == prev_idlers) {
+ break;
+ }
}
- rv = apr_thread_mutex_unlock(queue_info->idlers_mutex);
- if (rv != APR_SUCCESS) {
- return rv;
+
+ /* Atomically pop a pool from the recycled list */
+ for (;;) {
+ struct recycled_pool *first_pool = queue_info->recycled_pools;
+ if (first_pool == NULL) {
+ break;
+ }
+ if (apr_atomic_casptr(&(queue_info->recycled_pools), first_pool->next,
+ first_pool) == first_pool) {
+ *recycled_pool = first_pool->pool;
+ break;
+ }
}
- else if (queue_info->terminated) {
+
+ if (queue_info->terminated) {
return APR_EOF;
}
else {
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
On Mon, 2003-01-13 at 12:21, Greg Ames wrote:
> Brian Pane wrote:
> > I'm planning on spinning around the CAS; i.e., retry if the
> > CAS fails due to a collision with another thread. The queue_info
> > synchronization, which uses that same technique, seems to be
> > working well in practice.
>
> no...we're cool if the CAS fails; the problem is that sometimes it doesn't fail
> when you wish it would.
>
> thread 1 prepares to pop A off the list and picks up its next pointer to B, then
> gets interrupted/swapped out/whatever.
>
> other threads really pop A off the list, pop B off the list and free it, then
> push A back on the list with A's next pointer now pointing at C.
>
> thread 1 wakes back up and uses CAS to compare for A and swap with B. The CAS
> succeeds on most architectures, because the head contains a pointer to A once
> again. This corrupts the head to point to B which has been freed. The problem
> is that A's next pointer had been updated, and a straightforward CAS on a single
> word can't tell the difference between a pointer to old A and a pointer to new
> improved A.
>
> I did look at the queue_info pop and decided it must be single threaded for a
> given pool/qi combo, because it is driven as a pool cleanup and if that wasn't
> single threaded already, the apr pool destroy code would go nuts. I hope that
> assessment is correct.
You're right; the queue_info code is safe because only a single thread
pops from that queue. But for the reason you noted, I can't safely use
the same technique for the fd queue, from which multiple threads can
pop.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
On Mon, 2003-01-13 at 12:21, Greg Ames wrote:
> Brian Pane wrote:
> > I'm planning on spinning around the CAS; i.e., retry if the
> > CAS fails due to a collision with another thread. The queue_info
> > synchronization, which uses that same technique, seems to be
> > working well in practice.
>
> no...we're cool if the CAS fails; the problem is that sometimes it doesn't fail
> when you wish it would.
>
> thread 1 prepares to pop A off the list and picks up its next pointer to B, then
> gets interrupted/swapped out/whatever.
>
> other threads really pop A off the list, pop B off the list and free it, then
> push A back on the list with A's next pointer now pointing at C.
>
> thread 1 wakes back up and uses CAS to compare for A and swap with B. The CAS
> succeeds on most architectures, because the head contains a pointer to A once
> again. This corrupts the head to point to B which has been freed. The problem
> is that A's next pointer had been updated, and a straightforward CAS on a single
> word can't tell the difference between a pointer to old A and a pointer to new
> improved A.
>
> I did look at the queue_info pop and decided it must be single threaded for a
> given pool/qi combo, because it is driven as a pool cleanup and if that wasn't
> single threaded already, the apr pool destroy code would go nuts. I hope that
> assessment is correct.
You're right; the queue_info code is safe because only a single thread
pops from that queue. But for the reason you noted, I can't safely use
the same technique for the fd queue, from which multiple threads can
pop.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Greg Ames <gr...@apache.org>.
Brian Pane wrote:
>>>If we could change the apr_atomic_inc/dec functions to use
>>
> apr_uint32_t,
>
>>>this part of the fdqueue code could become a lot simpler.
> As far as I know, we can make it work with apr_uint32_t on most
> platforms, as long as we declare any inline assembly blocks as
> volatile (thanks to Sascha Schumann for fixing this recently in
> apr_atomic_cas on Linux).
>
> The one platform I'm not sure of is OS/390, due to the special
> data types and API involved:
> http://marc.theaimsgroup.com/?l=apr-dev&m=104129861312811
Don't let that stop you. I'll take care of OS/390. Right off the top of my
head, I can't see how the cs_t could be anything other than a 32 bit integer,
knowing how the cs instruction works.
(sorry for not replying to that msg earlier...I'm still backed up from the
holidays.)
>>>I still have one more change I'm hoping to make in the fdqueue
>>>synchronization logic: move the queue manipulation code in
>>>ap_queue_push/pop outside the mutex protected region by maintaining
>>>a linked list of queue nodes with apr_atomic_casptr.
>>
>>Sounds good, as long as the pops are single threaded somehow, or if
>
> you have
>
>>some other way of getting around the A-B-A cas pop window.
>
>
> I'm planning on spinning around the CAS; i.e., retry if the
> CAS fails due to a collision with another thread. The queue_info
> synchronization, which uses that same technique, seems to be
> working well in practice.
no...we're cool if the CAS fails; the problem is that sometimes it doesn't fail
when you wish it would.
thread 1 prepares to pop A off the list and picks up its next pointer to B, then
gets interrupted/swapped out/whatever.
other threads really pop A off the list, pop B off the list and free it, then
push A back on the list with A's next pointer now pointing at C.
thread 1 wakes back up and uses CAS to compare for A and swap with B. The CAS
succeeds on most architectures, because the head contains a pointer to A once
again. This corrupts the head to point to B which has been freed. The problem
is that A's next pointer had been updated, and a straightforward CAS on a single
word can't tell the difference between a pointer to old A and a pointer to new
improved A.
I did look at the queue_info pop and decided it must be single threaded for a
given pool/qi combo, because it is driven as a pool cleanup and if that wasn't
single threaded already, the apr pool destroy code would go nuts. I hope that
assessment is correct.
A double word CAS can do it safely by incrementing a list version # concurrently
with updating the list head pointer. That's possible on Pentiums with CMPXCHG8B
and __cds on S/390. PowerPCs and others (Alpha??) don't need the double word
version; they can use Load and Reserve followed by Store Conditional safely.
But I'm not aware of such a mechanism on Sparc for example, and I don't know
much about other CPU architectures.
Greg
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Greg Ames <gr...@apache.org>.
Brian Pane wrote:
>>>If we could change the apr_atomic_inc/dec functions to use
>>
> apr_uint32_t,
>
>>>this part of the fdqueue code could become a lot simpler.
> As far as I know, we can make it work with apr_uint32_t on most
> platforms, as long as we declare any inline assembly blocks as
> volatile (thanks to Sascha Schumann for fixing this recently in
> apr_atomic_cas on Linux).
>
> The one platform I'm not sure of is OS/390, due to the special
> data types and API involved:
> http://marc.theaimsgroup.com/?l=apr-dev&m=104129861312811
Don't let that stop you. I'll take care of OS/390. Right off the top of my
head, I can't see how the cs_t could be anything other than a 32 bit integer,
knowing how the cs instruction works.
(sorry for not replying to that msg earlier...I'm still backed up from the
holidays.)
>>>I still have one more change I'm hoping to make in the fdqueue
>>>synchronization logic: move the queue manipulation code in
>>>ap_queue_push/pop outside the mutex protected region by maintaining
>>>a linked list of queue nodes with apr_atomic_casptr.
>>
>>Sounds good, as long as the pops are single threaded somehow, or if
>
> you have
>
>>some other way of getting around the A-B-A cas pop window.
>
>
> I'm planning on spinning around the CAS; i.e., retry if the
> CAS fails due to a collision with another thread. The queue_info
> synchronization, which uses that same technique, seems to be
> working well in practice.
no...we're cool if the CAS fails; the problem is that sometimes it doesn't fail
when you wish it would.
thread 1 prepares to pop A off the list and picks up its next pointer to B, then
gets interrupted/swapped out/whatever.
other threads really pop A off the list, pop B off the list and free it, then
push A back on the list with A's next pointer now pointing at C.
thread 1 wakes back up and uses CAS to compare for A and swap with B. The CAS
succeeds on most architectures, because the head contains a pointer to A once
again. This corrupts the head to point to B which has been freed. The problem
is that A's next pointer had been updated, and a straightforward CAS on a single
word can't tell the difference between a pointer to old A and a pointer to new
improved A.
I did look at the queue_info pop and decided it must be single threaded for a
given pool/qi combo, because it is driven as a pool cleanup and if that wasn't
single threaded already, the apr pool destroy code would go nuts. I hope that
assessment is correct.
A double word CAS can do it safely by incrementing a list version # concurrently
with updating the list head pointer. That's possible on Pentiums with CMPXCHG8B
and __cds on S/390. PowerPCs and others (Alpha??) don't need the double word
version; they can use Load and Reserve followed by Store Conditional safely.
But I'm not aware of such a mechanism on Sparc for example, and I don't know
much about other CPU architectures.
Greg
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
Greg Ames wrote:
> Brian Pane wrote:
> > Greg Ames wrote:
>
>
> >> apr_atomic_dec? That does return something.
>
> > The problem is that, since we have to use apr_atomic_cas for the
> > increment (due to the lack of a return value on apr_atomic_inc),
> > we can't use apr_atomic_dec on the same variable. apr_atomic_cas
> > works on apr_uint32_t, while apr_atomic_dec works on apr_atomic_t.
> > If we could change the apr_atomic_inc/dec functions to use
apr_uint32_t,
> > this part of the fdqueue code could become a lot simpler.
>
> I am certainly in favor of changing apr_atomic_inc/dec so they can be
useful.
> I'm wondering if it's ok to use an ordinary apr type, like
apr_uint32_t? or do
> we need special atomic types marked as volatile or memory-resident or
whatever
> so that gcc won't assign them to registers or optimize them out or
??? I don't
> know the answer, but have seen kernel code do such things (OK Jeff,
I've been
> infected by the GPL...no hope for me).
As far as I know, we can make it work with apr_uint32_t on most
platforms, as long as we declare any inline assembly blocks as
volatile (thanks to Sascha Schumann for fixing this recently in
apr_atomic_cas on Linux).
The one platform I'm not sure of is OS/390, due to the special
data types and API involved:
http://marc.theaimsgroup.com/?l=apr-dev&m=104129861312811
>
> > I still have one more change I'm hoping to make in the fdqueue
> > synchronization logic: move the queue manipulation code in
> > ap_queue_push/pop outside the mutex protected region by maintaining
> > a linked list of queue nodes with apr_atomic_casptr.
>
> Sounds good, as long as the pops are single threaded somehow, or if
you have
> some other way of getting around the A-B-A cas pop window.
I'm planning on spinning around the CAS; i.e., retry if the
CAS fails due to a collision with another thread. The queue_info
synchronization, which uses that same technique, seems to be
working well in practice.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Greg Ames <gr...@apache.org>.
Brian Pane wrote:
> Greg Ames wrote:
>> apr_atomic_dec? That does return something.
> The problem is that, since we have to use apr_atomic_cas for the
> increment (due to the lack of a return value on apr_atomic_inc),
> we can't use apr_atomic_dec on the same variable. apr_atomic_cas
> works on apr_uint32_t, while apr_atomic_dec works on apr_atomic_t.
> If we could change the apr_atomic_inc/dec functions to use apr_uint32_t,
> this part of the fdqueue code could become a lot simpler.
I am certainly in favor of changing apr_atomic_inc/dec so they can be useful.
I'm wondering if it's ok to use an ordinary apr type, like apr_uint32_t? or do
we need special atomic types marked as volatile or memory-resident or whatever
so that gcc won't assign them to registers or optimize them out or ??? I don't
know the answer, but have seen kernel code do such things (OK Jeff, I've been
infected by the GPL...no hope for me).
> I still have one more change I'm hoping to make in the fdqueue
> synchronization logic: move the queue manipulation code in
> ap_queue_push/pop outside the mutex protected region by maintaining
> a linked list of queue nodes with apr_atomic_casptr.
Sounds good, as long as the pops are single threaded somehow, or if you have
some other way of getting around the A-B-A cas pop window.
Greg
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
Greg Ames wrote:
>> + /* Atomically decrement the idle worker count */
>> + for (;;) {
>> + apr_uint32_t prev_idlers = queue_info->idlers;
>> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers - 1,
>> prev_idlers)
>> + == prev_idlers) {
>> + break;
>> + }
>
>
> apr_atomic_dec? That does return something.
The problem is that, since we have to use apr_atomic_cas for the
increment (due to the lack of a return value on apr_atomic_inc),
we can't use apr_atomic_dec on the same variable. apr_atomic_cas
works on apr_uint32_t, while apr_atomic_dec works on apr_atomic_t.
If we could change the apr_atomic_inc/dec functions to use apr_uint32_t,
this part of the fdqueue code could become a lot simpler.
I still have one more change I'm hoping to make in the fdqueue
synchronization logic: move the queue manipulation code in
ap_queue_push/pop outside the mutex protected region by maintaining
a linked list of queue nodes with apr_atomic_casptr. We won't be
able to eliminate the mutex entirely, because it's needed for the
condition variable, but moving the queue update code outside the
critical region should at least help reduce the mutex contention.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
Greg Ames wrote:
> Brian Pane wrote:
> > Greg Ames wrote:
>
>
> >> apr_atomic_dec? That does return something.
>
> > The problem is that, since we have to use apr_atomic_cas for the
> > increment (due to the lack of a return value on apr_atomic_inc),
> > we can't use apr_atomic_dec on the same variable. apr_atomic_cas
> > works on apr_uint32_t, while apr_atomic_dec works on apr_atomic_t.
> > If we could change the apr_atomic_inc/dec functions to use
apr_uint32_t,
> > this part of the fdqueue code could become a lot simpler.
>
> I am certainly in favor of changing apr_atomic_inc/dec so they can be
useful.
> I'm wondering if it's ok to use an ordinary apr type, like
apr_uint32_t? or do
> we need special atomic types marked as volatile or memory-resident or
whatever
> so that gcc won't assign them to registers or optimize them out or
??? I don't
> know the answer, but have seen kernel code do such things (OK Jeff,
I've been
> infected by the GPL...no hope for me).
As far as I know, we can make it work with apr_uint32_t on most
platforms, as long as we declare any inline assembly blocks as
volatile (thanks to Sascha Schumann for fixing this recently in
apr_atomic_cas on Linux).
The one platform I'm not sure of is OS/390, due to the special
data types and API involved:
http://marc.theaimsgroup.com/?l=apr-dev&m=104129861312811
>
> > I still have one more change I'm hoping to make in the fdqueue
> > synchronization logic: move the queue manipulation code in
> > ap_queue_push/pop outside the mutex protected region by maintaining
> > a linked list of queue nodes with apr_atomic_casptr.
>
> Sounds good, as long as the pops are single threaded somehow, or if
you have
> some other way of getting around the A-B-A cas pop window.
I'm planning on spinning around the CAS; i.e., retry if the
CAS fails due to a collision with another thread. The queue_info
synchronization, which uses that same technique, seems to be
working well in practice.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Brian Pane <br...@cnet.com>.
Greg Ames wrote:
>> + /* Atomically decrement the idle worker count */
>> + for (;;) {
>> + apr_uint32_t prev_idlers = queue_info->idlers;
>> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers - 1,
>> prev_idlers)
>> + == prev_idlers) {
>> + break;
>> + }
>
>
> apr_atomic_dec? That does return something.
The problem is that, since we have to use apr_atomic_cas for the
increment (due to the lack of a return value on apr_atomic_inc),
we can't use apr_atomic_dec on the same variable. apr_atomic_cas
works on apr_uint32_t, while apr_atomic_dec works on apr_atomic_t.
If we could change the apr_atomic_inc/dec functions to use apr_uint32_t,
this part of the fdqueue code could become a lot simpler.
I still have one more change I'm hoping to make in the fdqueue
synchronization logic: move the queue manipulation code in
ap_queue_push/pop outside the mutex protected region by maintaining
a linked list of queue nodes with apr_atomic_casptr. We won't be
able to eliminate the mutex entirely, because it's needed for the
condition variable, but moving the queue update code outside the
critical region should at least help reduce the mutex contention.
Brian
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Greg Ames <gr...@apache.org>.
brianp@apache.org wrote:
> brianp 2003/01/03 10:32:59
>
> Modified: server/mpm/worker fdqueue.c
> Log:
> Replace most of the mutex locking in the worker MPM's "queue info"
> object with atomic compare-and-swap loops.
A nice New Year's present :-)
Presumably this gets rid of the O(ThreadsPerProcess) overhead in
_pthread_alt_unlock on Linux SMPs. Unfortunately it's not my box.
> + /* Atomically increment the count of idle workers */
> + for (;;) {
> + prev_idlers = queue_info->idlers;
> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers + 1, prev_idlers)
> + == prev_idlers) {
> + break;
> + }
apr_atomic_inc?
Some architectures provide lighter weight ways of doing this (e.g. a locked XADD
on a 486/Pentium is supposed to be cheaper than a locked CMPXCHG + result test +
loop overhead in software). Plus the source code shrinks and is easier to read.
<takes a look at apr_atomic.h> Oh, cripe, apr_atomic_inc doesn't return
anything, so we don't have a reliable way of knowing the old or new value. We
should fix that. Looking at your code above, the old value would be more
convenient. It would also work well for atomically allocating a slot in an array.
> + /* Atomically decrement the idle worker count */
> + for (;;) {
> + apr_uint32_t prev_idlers = queue_info->idlers;
> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers - 1, prev_idlers)
> + == prev_idlers) {
> + break;
> + }
apr_atomic_dec? That does return something.
Greg
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c
Posted by Greg Ames <gr...@apache.org>.
brianp@apache.org wrote:
> brianp 2003/01/03 10:32:59
>
> Modified: server/mpm/worker fdqueue.c
> Log:
> Replace most of the mutex locking in the worker MPM's "queue info"
> object with atomic compare-and-swap loops.
A nice New Year's present :-)
Presumably this gets rid of the O(ThreadsPerProcess) overhead in
_pthread_alt_unlock on Linux SMPs. Unfortunately it's not my box.
> + /* Atomically increment the count of idle workers */
> + for (;;) {
> + prev_idlers = queue_info->idlers;
> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers + 1, prev_idlers)
> + == prev_idlers) {
> + break;
> + }
apr_atomic_inc?
Some architectures provide lighter weight ways of doing this (e.g. a locked XADD
on a 486/Pentium is supposed to be cheaper than a locked CMPXCHG + result test +
loop overhead in software). Plus the source code shrinks and is easier to read.
<takes a look at apr_atomic.h> Oh, cripe, apr_atomic_inc doesn't return
anything, so we don't have a reliable way of knowing the old or new value. We
should fix that. Looking at your code above, the old value would be more
convenient. It would also work well for atomically allocating a slot in an array.
> + /* Atomically decrement the idle worker count */
> + for (;;) {
> + apr_uint32_t prev_idlers = queue_info->idlers;
> + if (apr_atomic_cas(&(queue_info->idlers), prev_idlers - 1, prev_idlers)
> + == prev_idlers) {
> + break;
> + }
apr_atomic_dec? That does return something.
Greg