You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by E Holyat <eh...@yahoo.com> on 2005/06/28 18:41:36 UTC
apr_pools and thread safety
I assumed that pools were thread safe.
Is apr_palloc intended to be thread safe within a pool object?
Does the entire function need to be mutexed? Looks like it should be
Could just the pool->active be locked until incremented? Maybe
Should it have a read write lock?
apr_palloc isn't thread safe, and allocator_alloc has some holes: The entire function should be locked.
e.g.
if (index <= allocator->max_index) { can be incremented while being accessed
}
/* If we found nothing, seek the sink (at index 0), if
* it is not empty.
*/
else if (allocator->free[0]) { 2 threads can enter.
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: apr_pools and thread safety
Posted by Sander Striker <st...@apache.org>.
E Holyat wrote:
> Then I think I may be running into an allocator_alloc bug during my
> testing. In a tight for loop, I am creating a thread which creates a
> child pool from the parent. I am crashing because my child pool is
> NULL. I am not sure yet, whether the child pool is reusing an index
> from the parent or creating a new one.
>
> But I can see the following still holds true for allocator_alloc and
> allocator_free. allocator_alloc allows a free read on max_index when
> either allocator_alloc / allocator_free can change that value.
>
> If 40 threads from the same parent create child pools. 40 threads can
> fall into the first "if statement". The access within that "if
> statement" is mutually exclusive, but, if max_index is being decremented
> within the "if statement", then the condition that put the 13 or 14th or
> nth thread into that "if statement" may no longer hold true. Wouldn't
> that invalidate the index alignment check and give a thread an
> invalid child pool reference?
>
> The mutex should be moved outside of the if and else if
I'll go have a look at this now to see if we really have a race condition.
Moving the mutex has performance implications, so I want to make sure
we actually need to do that.
Thanks for reporting,
Sander
Re: apr_pools and thread safety
Posted by Garrett Rooney <ro...@electricjellyfish.net>.
Sander Striker wrote:
> E Holyat wrote:
>
>> What if one thread destroys the allocator while this function is
>> trying to dereference it.
>
>
> That means you have lifetime issues and that is considered a bug in your
> application. You should make sure the allocator lives longer than the
> pools allocating out of it.
>
>> No matter what, you are going to lock in 99% of the time,
>
>
> The number is probably high, but I don't know how high.
>
>> why not be safe and lock it before dereferencing it.
>
>
> At the time I tried to make sure we never took out the mutex when not
> 99% sure we were going to use it, due to performance reasons.
>
>> Besides if you are going to fall into an if and wait on a mutex,
>> shouldn't you check the if statement again too? What is the purpose
>> of the if statement when by the time a thread acquires the mutex, the
>> if statement may no longer evaluate to TRUE?
>
>
> The if statement doens't need to be reevaluated because the code that
> is executed contains the same conditions as the if statement. It may
> not directly be obvious, since the code is fairly dense, but this should
> and AFAIK is, the case.
>
> The important part is that there is no race condition leading to your
> NULL bug. Do you agree that so far there is none?
Actually, I'm not sure about that. IIRC there is some platform specific
magic that needs to happen (memory barriers and so forth) to make double
checked locking (the idiom you're using here) work correctly on all
platforms. I know that the double checked locking impl we had at my
previous job needed some special stuff to make it work correctly all the
time on alpha for example. I can't recall all the details, but
searching for double checked locking c/c++ might reveal some details...
-garrett
Re: apr_pools and thread safety
Posted by Sander Striker <st...@apache.org>.
E Holyat wrote:
> What if one thread destroys the allocator while this function is trying
> to dereference it.
That means you have lifetime issues and that is considered a bug in your
application. You should make sure the allocator lives longer than the
pools allocating out of it.
> No matter what, you are going to lock in 99% of the time,
The number is probably high, but I don't know how high.
> why not be safe and lock it before dereferencing it.
At the time I tried to make sure we never took out the mutex when not
99% sure we were going to use it, due to performance reasons.
> Besides if you are going to fall into an if and wait on a mutex,
> shouldn't you check the if statement again too? What is the purpose of
> the if statement when by the time a thread acquires the mutex, the if
> statement may no longer evaluate to TRUE?
The if statement doens't need to be reevaluated because the code that
is executed contains the same conditions as the if statement. It may
not directly be obvious, since the code is fairly dense, but this should
and AFAIK is, the case.
The important part is that there is no race condition leading to your
NULL bug. Do you agree that so far there is none?
Sander
Re: apr_pools and thread safety
Posted by E Holyat <eh...@yahoo.com>.
What if one thread destroys the allocator while this function is trying to dereference it. No matter what, you are going to lock in 99% of the time, why not be safe and lock it before dereferencing it.
Besides if you are going to fall into an if and wait on a mutex, shouldn't you check the if statement again too? What is the purpose of the if statement when by the time a thread acquires the mutex, the if statement may no longer evaluate to TRUE?
Sander Striker <st...@apache.org> wrote:
E Holyat wrote:
[...]
> But I can see the following still holds true for allocator_alloc and
> allocator_free. allocator_alloc allows a free read on max_index when
> either allocator_alloc / allocator_free can change that value.
>
> If 40 threads from the same parent create child pools. 40 threads can
> fall into the first "if statement". The access within that "if
> statement" is mutually exclusive, but, if max_index is being decremented
> within the "if statement", then the condition that put the 13 or 14th or
> nth thread into that "if statement" may no longer hold true. Wouldn't
> that invalidate the index alignment check and give a thread an
> invalid child pool reference?
I don't think this would be the case. Comments inline.
> The mutex should be moved outside of the if and else if
> if (index <= allocator->max_index) { can be changed while being read
While allocator->max_index can indeed be changed, this is just a quick
scan to see if a node (aka chunk of memory) *might* be available. If
this seems to be the case a mutex is taken out and the value is read
again:
apr_pools.c:196
if (index <= allocator->max_index) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
/* Walk the free list to see if there are
* any nodes on it of the requested size
...
max_index = allocator->max_index;
ref = &allocator->free[index];
i = index;
while (*ref == NULL && i < max_index) {
ref++;
i++;
}
At this point we either have a node or we don't. If we do, we unlock
the mutex and return the node. If we don't, we unlock the mutex and
fall through, and back, on this piece of code:
apr_pools.c:297
/* If we haven't got a suitable node, malloc a new one
* and initialize it.
*/
if ((node = malloc(size)) == NULL)
return NULL;
...
return node;
> }
>
> /* If we found nothing, seek the sink (at index 0), if
> * it is not empty.
> */
> else if (allocator->free[0]) { 2 threads can enter.
The same holds true for the above, we grab the lock and check again.
Again with the risk of falling through to the block at line 297:
apr_pools.c:260
else if (allocator->free[0]) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
/* Walk the free list to see if there are
* any nodes on it of the requested size
*/
ref = &allocator->free[0];
while ((node = *ref) != NULL && index > node->index)
ref = &node->next;
Sander
__________________________________________________
Do You Yahoo!?
Tired of spam? Yahoo! Mail has the best spam protection around
http://mail.yahoo.com
Re: apr_pools and thread safety
Posted by Sander Striker <st...@apache.org>.
E Holyat wrote:
[...]
> But I can see the following still holds true for allocator_alloc and
> allocator_free. allocator_alloc allows a free read on max_index when
> either allocator_alloc / allocator_free can change that value.
>
> If 40 threads from the same parent create child pools. 40 threads can
> fall into the first "if statement". The access within that "if
> statement" is mutually exclusive, but, if max_index is being decremented
> within the "if statement", then the condition that put the 13 or 14th or
> nth thread into that "if statement" may no longer hold true. Wouldn't
> that invalidate the index alignment check and give a thread an
> invalid child pool reference?
I don't think this would be the case. Comments inline.
> The mutex should be moved outside of the if and else if
> if (index <= allocator->max_index) { can be changed while being read
While allocator->max_index can indeed be changed, this is just a quick
scan to see if a node (aka chunk of memory) *might* be available. If
this seems to be the case a mutex is taken out and the value is read
again:
apr_pools.c:196
if (index <= allocator->max_index) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
/* Walk the free list to see if there are
* any nodes on it of the requested size
...
max_index = allocator->max_index;
ref = &allocator->free[index];
i = index;
while (*ref == NULL && i < max_index) {
ref++;
i++;
}
At this point we either have a node or we don't. If we do, we unlock
the mutex and return the node. If we don't, we unlock the mutex and
fall through, and back, on this piece of code:
apr_pools.c:297
/* If we haven't got a suitable node, malloc a new one
* and initialize it.
*/
if ((node = malloc(size)) == NULL)
return NULL;
...
return node;
> }
>
> /* If we found nothing, seek the sink (at index 0), if
> * it is not empty.
> */
> else if (allocator->free[0]) { 2 threads can enter.
The same holds true for the above, we grab the lock and check again.
Again with the risk of falling through to the block at line 297:
apr_pools.c:260
else if (allocator->free[0]) {
#if APR_HAS_THREADS
if (allocator->mutex)
apr_thread_mutex_lock(allocator->mutex);
#endif /* APR_HAS_THREADS */
/* Walk the free list to see if there are
* any nodes on it of the requested size
*/
ref = &allocator->free[0];
while ((node = *ref) != NULL && index > node->index)
ref = &node->next;
Sander
Re: apr_pools and thread safety
Posted by E Holyat <eh...@yahoo.com>.
Then I think I may be running into an allocator_alloc bug during my testing. In a tight for loop, I am creating a thread which creates a child pool from the parent. I am crashing because my child pool is NULL. I am not sure yet, whether the child pool is reusing an index from the parent or creating a new one.
But I can see the following still holds true for allocator_alloc and allocator_free. allocator_alloc allows a free read on max_index when either allocator_alloc / allocator_free can change that value.
If 40 threads from the same parent create child pools. 40 threads can fall into the first "if statement". The access within that "if statement" is mutually exclusive, but, if max_index is being decremented within the "if statement", then the condition that put the 13 or 14th or nth thread into that "if statement" may no longer hold true. Wouldn't that invalidate the index alignment check and give a thread an invalid child pool reference?
The mutex should be moved outside of the if and else if
if (index <= allocator->max_index) { can be changed while being read
}
/* If we found nothing, seek the sink (at index 0), if
* it is not empty.
*/
else if (allocator->free[0]) { 2 threads can enter.
Joe Orton <jo...@redhat.com> wrote: On Tue, Jun 28, 2005 at 09:41:36AM -0700, E Holyat wrote:
> I assumed that pools were thread safe.
>
> Is apr_palloc intended to be thread safe within a pool object?
No - for a given pool, creation of subpools is a thread-safe operation,
but not allocation of memory.
Regards,
joe
---------------------------------
Yahoo! Sports
Rekindle the Rivalries. Sign up for Fantasy Football
Re: apr_pools and thread safety
Posted by Joe Orton <jo...@redhat.com>.
On Tue, Jun 28, 2005 at 09:41:36AM -0700, E Holyat wrote:
> I assumed that pools were thread safe.
>
> Is apr_palloc intended to be thread safe within a pool object?
No - for a given pool, creation of subpools is a thread-safe operation,
but not allocation of memory.
Regards,
joe