You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jim Jagielski <ji...@jaguNET.com> on 2017/02/20 14:16:51 UTC
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
The below got me thinking...
Right now, the pool allocator mutex is only used when, well,
allocator_alloc() is called, which means that sometimes
apr_palloc(), for example, can be thread-safeish and sometimes
not, depending on whether or not the active node has enough
space.
For 1.6 and later, it might be nice to actually protect the
adjustment of the active node, et.al. to, if a mutex is present,
always be thread-safe... that is, always when we "alloc" memory,
even when/if we do/don't called allocator_alloc().
Thoughts?
> On Feb 20, 2017, at 8:38 AM, ylavic@apache.org wrote:
>
> Author: ylavic
> Date: Mon Feb 20 13:38:03 2017
> New Revision: 1783755
>
> URL: http://svn.apache.org/viewvc?rev=1783755&view=rev
> Log:
> mpm_event: use a mutex for ptrans' allocator to be safe with concurrent
> creation and destruction of its subpools, like with mod_http2.
>
>
> Modified:
> httpd/httpd/trunk/server/mpm/event/event.c
>
> Modified: httpd/httpd/trunk/server/mpm/event/event.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1783755&r1=1783754&r2=1783755&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
> +++ httpd/httpd/trunk/server/mpm/event/event.c Mon Feb 20 13:38:03 2017
> @@ -2096,6 +2096,7 @@ static void * APR_THREAD_FUNC listener_t
> }
> if (!listeners_disabled) {
> void *csd = NULL;
> + apr_thread_mutex_t *mutex;
> ap_listen_rec *lr = (ap_listen_rec *) pt->baton;
> apr_pool_t *ptrans; /* Pool for per-transaction stuff */
> ap_pop_pool(&ptrans, worker_queue_info);
> @@ -2105,20 +2106,44 @@ static void * APR_THREAD_FUNC listener_t
> apr_allocator_t *allocator;
>
> apr_allocator_create(&allocator);
> - apr_allocator_max_free_set(allocator,
> - ap_max_mem_free);
> + apr_allocator_max_free_set(allocator, ap_max_mem_free);
> apr_pool_create_ex(&ptrans, pconf, NULL, allocator);
> - apr_allocator_owner_set(allocator, ptrans);
> if (ptrans == NULL) {
> ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
> ap_server_conf, APLOGNO(03097)
> "Failed to create transaction pool");
> + apr_allocator_destroy(allocator);
> signal_threads(ST_GRACEFUL);
> return NULL;
> }
> + apr_allocator_owner_set(allocator, ptrans);
> }
> apr_pool_tag(ptrans, "transaction");
>
> + /* We need a mutex in the allocator to synchronize ptrans'
> + * children creations/destructions, but this mutex ought to
> + * live in ptrans itself to avoid leaks, hence it's cleared
> + * in ap_push_pool(). We could recycle some pconf's mutexes
> + * like we do for ptrans subpools, but that'd need another
> + * synchronization mechanism, whereas creating a pthread
> + * mutex (unix here!) is really as simple/fast as a static
> + * PTHREAD_MUTEX_INIT assignment, so let's not bother and
> + * create the mutex for each ptrans (recycled or not).
> + */
> + rc = apr_thread_mutex_create(&mutex,
> + APR_THREAD_MUTEX_DEFAULT,
> + ptrans);
> + if (rc != APR_SUCCESS) {
> + ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
> + ap_server_conf, APLOGNO()
> + "Failed to create transaction pool mutex");
> + ap_push_pool(worker_queue_info, ptrans);
> + signal_threads(ST_GRACEFUL);
> + return NULL;
> + }
> + apr_allocator_mutex_set(apr_pool_allocator_get(ptrans),
> + mutex);
> +
> get_worker(&have_idle_worker, 1, &workers_were_busy);
> rc = lr->accept_func(&csd, lr, ptrans);
>
>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Not with apr_palloc() or anything that calls apr_palloc (eg apr_pcalloc, et.al...)
> On Feb 20, 2017, at 10:15 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>
>>
>> Am 20.02.2017 um 16:08 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> But isn't that now true for all conn_rec->pool and thus r->pool and
> c->bucket_alloc etc?
>
>>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>>
>
> Stefan Eissing
>
> <green/>bytes GmbH
> Hafenstrasse 16
> 48155 Münster
> www.greenbytes.de
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Not with apr_palloc() or anything that calls apr_palloc (eg apr_pcalloc, et.al...)
> On Feb 20, 2017, at 10:15 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>
>>
>> Am 20.02.2017 um 16:08 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> But isn't that now true for all conn_rec->pool and thus r->pool and
> c->bucket_alloc etc?
>
>>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>>
>
> Stefan Eissing
>
> <green/>bytes GmbH
> Hafenstrasse 16
> 48155 Münster
> www.greenbytes.de
Re: APR pools, mutexes and thread safe allocations
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 20.02.2017 um 16:08 schrieb Jim Jagielski <ji...@jaguNET.com>:
>
> Again, this would ONLY happen if the underlying allocator has
> a mutex!
But isn't that now true for all conn_rec->pool and thus r->pool and
c->bucket_alloc etc?
>
>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>
>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>
>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>
>>>>> The below got me thinking...
>>>>>
>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>> allocator_alloc() is called, which means that sometimes
>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>> not, depending on whether or not the active node has enough
>>>>> space.
>>>>>
>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>> even when/if we do/don't called allocator_alloc().
>>>>>
>>>>> Thoughts?
>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>
>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>> or anything that *uses* apr_palloc().
>>>
>>> The idea being that if the underlying allocator has a mutex, the
>>> assumption should be that the pool using that allocator "wants"
>>> or "expects" to be thread-safe... It seems an easy way to create
>>> thread-safe APR pools, but I could be missing something crucial
>>> here.
>>>
>>> Of course, if the allocator does NOT have a mutex, no change and
>>> no cost.
>>
>>
>> I've always understood that creating subpools is thread safe iff the
>> allocator has a mutex, but allocating from any single pool is not, by
>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>> way to throw away pools' speed advantage compared to malloc().
>>
>> -- Brane
>
Stefan Eissing
<green/>bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
For those interested in the real history of pools, and, in fact,
that actual design of so much of httpd, I refer people to:
http://www.ra.ethz.ch/CDStore/www5/www415/overview.htm
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Fair enuff... sounds like thread safe pools aren't something
people are interested in, so I won't worry about it :)
thx for the discussion!
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Fair enuff... sounds like thread safe pools aren't something
people are interested in, so I won't worry about it :)
thx for the discussion!
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 5:55 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>>
>> It could be done, but who would use it that way?
>>
>
> I think the non-thread safety of pools requires that each
> person/developer implement their own locking esp around areas
> where they might not even understand that there is cross-thread
> issues. Thinking about 1.6 and especially apr 2.0, it makes
> one wonder when/if we should bite the bullet and add thread
> safe pools in a canon and supported and opaque way instead of
> the normal "hacks" around it...
That'd be for the "garbage collector" functionality of pools, not
necessarily in a performance critical environment (where usually the
headaches come from :)
>
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 5:55 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>
>>
>> It could be done, but who would use it that way?
>>
>
> I think the non-thread safety of pools requires that each
> person/developer implement their own locking esp around areas
> where they might not even understand that there is cross-thread
> issues. Thinking about 1.6 and especially apr 2.0, it makes
> one wonder when/if we should bite the bullet and add thread
> safe pools in a canon and supported and opaque way instead of
> the normal "hacks" around it...
That'd be for the "garbage collector" functionality of pools, not
necessarily in a performance critical environment (where usually the
headaches come from :)
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
>
> It could be done, but who would use it that way?
>
I think the non-thread safety of pools requires that each
person/developer implement their own locking esp around areas
where they might not even understand that there is cross-thread
issues. Thinking about 1.6 and especially apr 2.0, it makes
one wonder when/if we should bite the bullet and add thread
safe pools in a canon and supported and opaque way instead of
the normal "hacks" around it...
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
>
> It could be done, but who would use it that way?
>
I think the non-thread safety of pools requires that each
person/developer implement their own locking esp around areas
where they might not even understand that there is cross-thread
issues. Thinking about 1.6 and especially apr 2.0, it makes
one wonder when/if we should bite the bullet and add thread
safe pools in a canon and supported and opaque way instead of
the normal "hacks" around it...
Re: APR pools, mutexes and thread safe allocations
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 20.02.2017 um 17:22 schrieb Jim Jagielski <ji...@jaguNET.com>:
>
>
>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>>
>> So as Brane says, if one sets a mutex to the allocator of a pool
>> because (s)he wants pool_create/destroy() to be thread-safe, it does
>> not necessarily wants the same for palloc() and friends (e.g. the pool
>> is used by a single thread).
>>
>
> OK, I get that... I can see that understanding of pool allocation.
>
>> I think we should not (re)use the allocator mutex if we want the pool
>> allocations to be thread-safe, but precisely have a separate pool
>> mutex.
>
> That also makes sense... My thoughts were that people were NOT
> using the allocator mutex specifically because it did NOT do want
> they wanted or needed and that leveraging the allocator mutex to
> be applied during all real allocations of memory, even local node.
>
> I still tend to think that it being located in the allocator
> makes the most sense, or, at least, a flag that the mutex
> should be "global" or not... That is, someone uses apr_allocator_mutex_set()
> to set the mutex and something like apr_allocator_mutex_coverage_set() to
> provide the finer control over which cases the mutex needs to be used.
It could be done, but who would use it that way?
The thread safety of pools does not make something like mod_http2 safe
by itself. It will not solve the problems of destruction sequences across
threads, nor the problems of double frees or uses after free. I think the
current design has remedied those problems successfully (fingers crossed)
and correct pool use per thread is trivial in comparison.
I am not against adding such a feature. I'm only saying that I do not ask for it... ;-)
-Stefan
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
>
> So as Brane says, if one sets a mutex to the allocator of a pool
> because (s)he wants pool_create/destroy() to be thread-safe, it does
> not necessarily wants the same for palloc() and friends (e.g. the pool
> is used by a single thread).
>
OK, I get that... I can see that understanding of pool allocation.
> I think we should not (re)use the allocator mutex if we want the pool
> allocations to be thread-safe, but precisely have a separate pool
> mutex.
That also makes sense... My thoughts were that people were NOT
using the allocator mutex specifically because it did NOT do want
they wanted or needed and that leveraging the allocator mutex to
be applied during all real allocations of memory, even local node.
I still tend to think that it being located in the allocator
makes the most sense, or, at least, a flag that the mutex
should be "global" or not... That is, someone uses apr_allocator_mutex_set()
to set the mutex and something like apr_allocator_mutex_coverage_set() to
provide the finer control over which cases the mutex needs to be used.
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
apr_pool_create_ex_debug(), yes.
> On Feb 20, 2017, at 11:00 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Well, this is for debug mode only I guess.
>
> On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> Please READ apr_pools.c... Search for '->mutex'
>>
>>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>>>> You are confusing pool mutexes with allocator mutexes...
>>>
>>> I'm not sure to understand, there is no pool mutex per se currently,
>>> the mutex (if any) used by the pool is its allocator's.
>>>
>>> So as Brane says, if one sets a mutex to the allocator of a pool
>>> because (s)he wants pool_create/destroy() to be thread-safe, it does
>>> not necessarily wants the same for palloc() and friends (e.g. the pool
>>> is used by a single thread).
>>>
>>> I think we should not (re)use the allocator mutex if we want the pool
>>> allocations to be thread-safe, but precisely have a separate pool
>>> mutex.
>>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
apr_pool_create_ex_debug(), yes.
> On Feb 20, 2017, at 11:00 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> Well, this is for debug mode only I guess.
>
> On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> Please READ apr_pools.c... Search for '->mutex'
>>
>>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>>
>>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>>>> You are confusing pool mutexes with allocator mutexes...
>>>
>>> I'm not sure to understand, there is no pool mutex per se currently,
>>> the mutex (if any) used by the pool is its allocator's.
>>>
>>> So as Brane says, if one sets a mutex to the allocator of a pool
>>> because (s)he wants pool_create/destroy() to be thread-safe, it does
>>> not necessarily wants the same for palloc() and friends (e.g. the pool
>>> is used by a single thread).
>>>
>>> I think we should not (re)use the allocator mutex if we want the pool
>>> allocations to be thread-safe, but precisely have a separate pool
>>> mutex.
>>
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
Well, this is for debug mode only I guess.
On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Please READ apr_pools.c... Search for '->mutex'
>
>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>>> You are confusing pool mutexes with allocator mutexes...
>>
>> I'm not sure to understand, there is no pool mutex per se currently,
>> the mutex (if any) used by the pool is its allocator's.
>>
>> So as Brane says, if one sets a mutex to the allocator of a pool
>> because (s)he wants pool_create/destroy() to be thread-safe, it does
>> not necessarily wants the same for palloc() and friends (e.g. the pool
>> is used by a single thread).
>>
>> I think we should not (re)use the allocator mutex if we want the pool
>> allocations to be thread-safe, but precisely have a separate pool
>> mutex.
>
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
Well, this is for debug mode only I guess.
On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> Please READ apr_pools.c... Search for '->mutex'
>
>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>>
>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>>> You are confusing pool mutexes with allocator mutexes...
>>
>> I'm not sure to understand, there is no pool mutex per se currently,
>> the mutex (if any) used by the pool is its allocator's.
>>
>> So as Brane says, if one sets a mutex to the allocator of a pool
>> because (s)he wants pool_create/destroy() to be thread-safe, it does
>> not necessarily wants the same for palloc() and friends (e.g. the pool
>> is used by a single thread).
>>
>> I think we should not (re)use the allocator mutex if we want the pool
>> allocations to be thread-safe, but precisely have a separate pool
>> mutex.
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Please READ apr_pools.c... Search for '->mutex'
> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>> You are confusing pool mutexes with allocator mutexes...
>
> I'm not sure to understand, there is no pool mutex per se currently,
> the mutex (if any) used by the pool is its allocator's.
>
> So as Brane says, if one sets a mutex to the allocator of a pool
> because (s)he wants pool_create/destroy() to be thread-safe, it does
> not necessarily wants the same for palloc() and friends (e.g. the pool
> is used by a single thread).
>
> I think we should not (re)use the allocator mutex if we want the pool
> allocations to be thread-safe, but precisely have a separate pool
> mutex.
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Please READ apr_pools.c... Search for '->mutex'
> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
>> You are confusing pool mutexes with allocator mutexes...
>
> I'm not sure to understand, there is no pool mutex per se currently,
> the mutex (if any) used by the pool is its allocator's.
>
> So as Brane says, if one sets a mutex to the allocator of a pool
> because (s)he wants pool_create/destroy() to be thread-safe, it does
> not necessarily wants the same for palloc() and friends (e.g. the pool
> is used by a single thread).
>
> I think we should not (re)use the allocator mutex if we want the pool
> allocations to be thread-safe, but precisely have a separate pool
> mutex.
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <yl...@gmail.com> wrote:
>
>
> So as Brane says, if one sets a mutex to the allocator of a pool
> because (s)he wants pool_create/destroy() to be thread-safe, it does
> not necessarily wants the same for palloc() and friends (e.g. the pool
> is used by a single thread).
>
OK, I get that... I can see that understanding of pool allocation.
> I think we should not (re)use the allocator mutex if we want the pool
> allocations to be thread-safe, but precisely have a separate pool
> mutex.
That also makes sense... My thoughts were that people were NOT
using the allocator mutex specifically because it did NOT do want
they wanted or needed and that leveraging the allocator mutex to
be applied during all real allocations of memory, even local node.
I still tend to think that it being located in the allocator
makes the most sense, or, at least, a flag that the mutex
should be "global" or not... That is, someone uses apr_allocator_mutex_set()
to set the mutex and something like apr_allocator_mutex_coverage_set() to
provide the finer control over which cases the mutex needs to be used.
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
> You are confusing pool mutexes with allocator mutexes...
I'm not sure to understand, there is no pool mutex per se currently,
the mutex (if any) used by the pool is its allocator's.
So as Brane says, if one sets a mutex to the allocator of a pool
because (s)he wants pool_create/destroy() to be thread-safe, it does
not necessarily wants the same for palloc() and friends (e.g. the pool
is used by a single thread).
I think we should not (re)use the allocator mutex if we want the pool
allocations to be thread-safe, but precisely have a separate pool
mutex.
Re: APR pools, mutexes and thread safe allocations
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <ji...@apache.org> wrote:
> You are confusing pool mutexes with allocator mutexes...
I'm not sure to understand, there is no pool mutex per se currently,
the mutex (if any) used by the pool is its allocator's.
So as Brane says, if one sets a mutex to the allocator of a pool
because (s)he wants pool_create/destroy() to be thread-safe, it does
not necessarily wants the same for palloc() and friends (e.g. the pool
is used by a single thread).
I think we should not (re)use the allocator mutex if we want the pool
allocations to be thread-safe, but precisely have a separate pool
mutex.
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@apache.org>.
You are confusing pool mutexes with allocator mutexes...
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 16:08, Jim Jagielski wrote:
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> Sure, but you need an allocator with a mutex in order to safely create
> pools in a multi-threaded environment. For all practical purposes this
> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>
> Now in certain cases you may have enough control of the structure of the
> application that you're able to make a more fine-grained decision about
> whether your allocator must be thread-safe or not. But libraries, such
> as Subversion for example, do not have that luxury.
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
> Your proposed change would drastically slow down existing code or force
> it to use non-thread-safe allocators and invent and retrofit explicit
> locking around subpool creation. Whilst the change is, strictly
> speaking, backward compatible, I can imagine people being slightly
> miffed off if their apps drastically slow down because they happen to be
> linked with a new version of APR.
>
> -- Brane
>
>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
For those interested in the real history of pools, and, in fact,
that actual design of so much of httpd, I refer people to:
http://www.ra.ethz.ch/CDStore/www5/www415/overview.htm
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@apache.org>.
You are confusing pool mutexes with allocator mutexes...
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 16:08, Jim Jagielski wrote:
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> Sure, but you need an allocator with a mutex in order to safely create
> pools in a multi-threaded environment. For all practical purposes this
> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>
> Now in certain cases you may have enough control of the structure of the
> application that you're able to make a more fine-grained decision about
> whether your allocator must be thread-safe or not. But libraries, such
> as Subversion for example, do not have that luxury.
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
> Your proposed change would drastically slow down existing code or force
> it to use non-thread-safe allocators and invent and retrofit explicit
> locking around subpool creation. Whilst the change is, strictly
> speaking, backward compatible, I can imagine people being slightly
> miffed off if their apps drastically slow down because they happen to be
> linked with a new version of APR.
>
> -- Brane
>
>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
I mean, of course, "has a *mutex*"
> On Feb 20, 2017, at 10:39 AM, Jim Jagielski <ji...@jaguNET.com> wrote:
>
> Again, I am only talking about those in which the allocator
> has a pool... The allocator. Via apr_allocator_mutex_set().
>
>> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>>
>> On 20.02.2017 16:08, Jim Jagielski wrote:
>>> Again, this would ONLY happen if the underlying allocator has
>>> a mutex!
>>
>> Sure, but you need an allocator with a mutex in order to safely create
>> pools in a multi-threaded environment. For all practical purposes this
>> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>>
>> Now in certain cases you may have enough control of the structure of the
>> application that you're able to make a more fine-grained decision about
>> whether your allocator must be thread-safe or not. But libraries, such
>> as Subversion for example, do not have that luxury.
>>
>> APR pools were designed with the assumption that separate threads will
>> always use separate pools whenever concurrent allocations are possible.
>> This assumption happens to fit pretty well with the
>> server/worker/thread-pool architecture that APR was designed to be used
>> in, and is not an inordinate burden for other architectures.
>>
>> Your proposed change would drastically slow down existing code or force
>> it to use non-thread-safe allocators and invent and retrofit explicit
>> locking around subpool creation. Whilst the change is, strictly
>> speaking, backward compatible, I can imagine people being slightly
>> miffed off if their apps drastically slow down because they happen to be
>> linked with a new version of APR.
>>
>> -- Brane
>>
>>
>>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>>
>>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>>
>>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>>
>>>>>>> The below got me thinking...
>>>>>>>
>>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>>> not, depending on whether or not the active node has enough
>>>>>>> space.
>>>>>>>
>>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>>
>>>>>>> Thoughts?
>>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>>
>>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>>> or anything that *uses* apr_palloc().
>>>>>
>>>>> The idea being that if the underlying allocator has a mutex, the
>>>>> assumption should be that the pool using that allocator "wants"
>>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>>> thread-safe APR pools, but I could be missing something crucial
>>>>> here.
>>>>>
>>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>>> no cost.
>>>>
>>>> I've always understood that creating subpools is thread safe iff the
>>>> allocator has a mutex, but allocating from any single pool is not, by
>>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>>> way to throw away pools' speed advantage compared to malloc().
>>>>
>>>> -- Brane
>>
>>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
I mean, of course, "has a *mutex*"
> On Feb 20, 2017, at 10:39 AM, Jim Jagielski <ji...@jaguNET.com> wrote:
>
> Again, I am only talking about those in which the allocator
> has a pool... The allocator. Via apr_allocator_mutex_set().
>
>> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>>
>> On 20.02.2017 16:08, Jim Jagielski wrote:
>>> Again, this would ONLY happen if the underlying allocator has
>>> a mutex!
>>
>> Sure, but you need an allocator with a mutex in order to safely create
>> pools in a multi-threaded environment. For all practical purposes this
>> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>>
>> Now in certain cases you may have enough control of the structure of the
>> application that you're able to make a more fine-grained decision about
>> whether your allocator must be thread-safe or not. But libraries, such
>> as Subversion for example, do not have that luxury.
>>
>> APR pools were designed with the assumption that separate threads will
>> always use separate pools whenever concurrent allocations are possible.
>> This assumption happens to fit pretty well with the
>> server/worker/thread-pool architecture that APR was designed to be used
>> in, and is not an inordinate burden for other architectures.
>>
>> Your proposed change would drastically slow down existing code or force
>> it to use non-thread-safe allocators and invent and retrofit explicit
>> locking around subpool creation. Whilst the change is, strictly
>> speaking, backward compatible, I can imagine people being slightly
>> miffed off if their apps drastically slow down because they happen to be
>> linked with a new version of APR.
>>
>> -- Brane
>>
>>
>>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>>
>>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>>
>>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>>
>>>>>>> The below got me thinking...
>>>>>>>
>>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>>> not, depending on whether or not the active node has enough
>>>>>>> space.
>>>>>>>
>>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>>
>>>>>>> Thoughts?
>>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>>
>>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>>> or anything that *uses* apr_palloc().
>>>>>
>>>>> The idea being that if the underlying allocator has a mutex, the
>>>>> assumption should be that the pool using that allocator "wants"
>>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>>> thread-safe APR pools, but I could be missing something crucial
>>>>> here.
>>>>>
>>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>>> no cost.
>>>>
>>>> I've always understood that creating subpools is thread safe iff the
>>>> allocator has a mutex, but allocating from any single pool is not, by
>>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>>> way to throw away pools' speed advantage compared to malloc().
>>>>
>>>> -- Brane
>>
>>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Again, I am only talking about those in which the allocator
has a pool... The allocator. Via apr_allocator_mutex_set().
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 16:08, Jim Jagielski wrote:
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> Sure, but you need an allocator with a mutex in order to safely create
> pools in a multi-threaded environment. For all practical purposes this
> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>
> Now in certain cases you may have enough control of the structure of the
> application that you're able to make a more fine-grained decision about
> whether your allocator must be thread-safe or not. But libraries, such
> as Subversion for example, do not have that luxury.
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
> Your proposed change would drastically slow down existing code or force
> it to use non-thread-safe allocators and invent and retrofit explicit
> locking around subpool creation. Whilst the change is, strictly
> speaking, backward compatible, I can imagine people being slightly
> miffed off if their apps drastically slow down because they happen to be
> linked with a new version of APR.
>
> -- Brane
>
>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Again, I am only talking about those in which the allocator
has a pool... The allocator. Via apr_allocator_mutex_set().
> On Feb 20, 2017, at 10:26 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 16:08, Jim Jagielski wrote:
>> Again, this would ONLY happen if the underlying allocator has
>> a mutex!
>
> Sure, but you need an allocator with a mutex in order to safely create
> pools in a multi-threaded environment. For all practical purposes this
> means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
>
> Now in certain cases you may have enough control of the structure of the
> application that you're able to make a more fine-grained decision about
> whether your allocator must be thread-safe or not. But libraries, such
> as Subversion for example, do not have that luxury.
>
> APR pools were designed with the assumption that separate threads will
> always use separate pools whenever concurrent allocations are possible.
> This assumption happens to fit pretty well with the
> server/worker/thread-pool architecture that APR was designed to be used
> in, and is not an inordinate burden for other architectures.
>
> Your proposed change would drastically slow down existing code or force
> it to use non-thread-safe allocators and invent and retrofit explicit
> locking around subpool creation. Whilst the change is, strictly
> speaking, backward compatible, I can imagine people being slightly
> miffed off if their apps drastically slow down because they happen to be
> linked with a new version of APR.
>
> -- Brane
>
>
>>> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>>
>>>>>> The below got me thinking...
>>>>>>
>>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>>> allocator_alloc() is called, which means that sometimes
>>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>>> not, depending on whether or not the active node has enough
>>>>>> space.
>>>>>>
>>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>>> even when/if we do/don't called allocator_alloc().
>>>>>>
>>>>>> Thoughts?
>>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>>
>>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>>> or anything that *uses* apr_palloc().
>>>>
>>>> The idea being that if the underlying allocator has a mutex, the
>>>> assumption should be that the pool using that allocator "wants"
>>>> or "expects" to be thread-safe... It seems an easy way to create
>>>> thread-safe APR pools, but I could be missing something crucial
>>>> here.
>>>>
>>>> Of course, if the allocator does NOT have a mutex, no change and
>>>> no cost.
>>>
>>> I've always understood that creating subpools is thread safe iff the
>>> allocator has a mutex, but allocating from any single pool is not, by
>>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>>> way to throw away pools' speed advantage compared to malloc().
>>>
>>> -- Brane
>
>
Re: APR pools, mutexes and thread safe allocations
Posted by Branko Čibej <br...@apache.org>.
On 20.02.2017 16:08, Jim Jagielski wrote:
> Again, this would ONLY happen if the underlying allocator has
> a mutex!
Sure, but you need an allocator with a mutex in order to safely create
pools in a multi-threaded environment. For all practical purposes this
means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
Now in certain cases you may have enough control of the structure of the
application that you're able to make a more fine-grained decision about
whether your allocator must be thread-safe or not. But libraries, such
as Subversion for example, do not have that luxury.
APR pools were designed with the assumption that separate threads will
always use separate pools whenever concurrent allocations are possible.
This assumption happens to fit pretty well with the
server/worker/thread-pool architecture that APR was designed to be used
in, and is not an inordinate burden for other architectures.
Your proposed change would drastically slow down existing code or force
it to use non-thread-safe allocators and invent and retrofit explicit
locking around subpool creation. Whilst the change is, strictly
speaking, backward compatible, I can imagine people being slightly
miffed off if their apps drastically slow down because they happen to be
linked with a new version of APR.
-- Brane
>> On Feb 20, 2017, at 10:06 AM, Branko \u010cibej <br...@apache.org> wrote:
>>
>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>
>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>
>>>>> The below got me thinking...
>>>>>
>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>> allocator_alloc() is called, which means that sometimes
>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>> not, depending on whether or not the active node has enough
>>>>> space.
>>>>>
>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>> even when/if we do/don't called allocator_alloc().
>>>>>
>>>>> Thoughts?
>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>
>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>> or anything that *uses* apr_palloc().
>>>
>>> The idea being that if the underlying allocator has a mutex, the
>>> assumption should be that the pool using that allocator "wants"
>>> or "expects" to be thread-safe... It seems an easy way to create
>>> thread-safe APR pools, but I could be missing something crucial
>>> here.
>>>
>>> Of course, if the allocator does NOT have a mutex, no change and
>>> no cost.
>>
>> I've always understood that creating subpools is thread safe iff the
>> allocator has a mutex, but allocating from any single pool is not, by
>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>> way to throw away pools' speed advantage compared to malloc().
>>
>> -- Brane
Re: APR pools, mutexes and thread safe allocations
Posted by Branko Čibej <br...@apache.org>.
On 20.02.2017 16:08, Jim Jagielski wrote:
> Again, this would ONLY happen if the underlying allocator has
> a mutex!
Sure, but you need an allocator with a mutex in order to safely create
pools in a multi-threaded environment. For all practical purposes this
means that the allocator must be thread-safe #ifdef APR_HAS_THREADS.
Now in certain cases you may have enough control of the structure of the
application that you're able to make a more fine-grained decision about
whether your allocator must be thread-safe or not. But libraries, such
as Subversion for example, do not have that luxury.
APR pools were designed with the assumption that separate threads will
always use separate pools whenever concurrent allocations are possible.
This assumption happens to fit pretty well with the
server/worker/thread-pool architecture that APR was designed to be used
in, and is not an inordinate burden for other architectures.
Your proposed change would drastically slow down existing code or force
it to use non-thread-safe allocators and invent and retrofit explicit
locking around subpool creation. Whilst the change is, strictly
speaking, backward compatible, I can imagine people being slightly
miffed off if their apps drastically slow down because they happen to be
linked with a new version of APR.
-- Brane
>> On Feb 20, 2017, at 10:06 AM, Branko \u010cibej <br...@apache.org> wrote:
>>
>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>>
>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>>
>>>>> The below got me thinking...
>>>>>
>>>>> Right now, the pool allocator mutex is only used when, well,
>>>>> allocator_alloc() is called, which means that sometimes
>>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>>> not, depending on whether or not the active node has enough
>>>>> space.
>>>>>
>>>>> For 1.6 and later, it might be nice to actually protect the
>>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>>> even when/if we do/don't called allocator_alloc().
>>>>>
>>>>> Thoughts?
>>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>>
>>> The cost would be the time spent on a lock on each call to apr_palloc()
>>> or anything that *uses* apr_palloc().
>>>
>>> The idea being that if the underlying allocator has a mutex, the
>>> assumption should be that the pool using that allocator "wants"
>>> or "expects" to be thread-safe... It seems an easy way to create
>>> thread-safe APR pools, but I could be missing something crucial
>>> here.
>>>
>>> Of course, if the allocator does NOT have a mutex, no change and
>>> no cost.
>>
>> I've always understood that creating subpools is thread safe iff the
>> allocator has a mutex, but allocating from any single pool is not, by
>> definition. Acquiring a mutex for every apr_palloc() seems like a good
>> way to throw away pools' speed advantage compared to malloc().
>>
>> -- Brane
APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Again, this would ONLY happen if the underlying allocator has
a mutex!
> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 15:55, Jim Jagielski wrote:
>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>
>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>
>>>> The below got me thinking...
>>>>
>>>> Right now, the pool allocator mutex is only used when, well,
>>>> allocator_alloc() is called, which means that sometimes
>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>> not, depending on whether or not the active node has enough
>>>> space.
>>>>
>>>> For 1.6 and later, it might be nice to actually protect the
>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>> even when/if we do/don't called allocator_alloc().
>>>>
>>>> Thoughts?
>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>
>> The cost would be the time spent on a lock on each call to apr_palloc()
>> or anything that *uses* apr_palloc().
>>
>> The idea being that if the underlying allocator has a mutex, the
>> assumption should be that the pool using that allocator "wants"
>> or "expects" to be thread-safe... It seems an easy way to create
>> thread-safe APR pools, but I could be missing something crucial
>> here.
>>
>> Of course, if the allocator does NOT have a mutex, no change and
>> no cost.
>
>
> I've always understood that creating subpools is thread safe iff the
> allocator has a mutex, but allocating from any single pool is not, by
> definition. Acquiring a mutex for every apr_palloc() seems like a good
> way to throw away pools' speed advantage compared to malloc().
>
> -- Brane
APR pools, mutexes and thread safe allocations
Posted by Jim Jagielski <ji...@jaguNET.com>.
Again, this would ONLY happen if the underlying allocator has
a mutex!
> On Feb 20, 2017, at 10:06 AM, Branko Čibej <br...@apache.org> wrote:
>
> On 20.02.2017 15:55, Jim Jagielski wrote:
>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>>
>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>>
>>>> The below got me thinking...
>>>>
>>>> Right now, the pool allocator mutex is only used when, well,
>>>> allocator_alloc() is called, which means that sometimes
>>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>>> not, depending on whether or not the active node has enough
>>>> space.
>>>>
>>>> For 1.6 and later, it might be nice to actually protect the
>>>> adjustment of the active node, et.al. to, if a mutex is present,
>>>> always be thread-safe... that is, always when we "alloc" memory,
>>>> even when/if we do/don't called allocator_alloc().
>>>>
>>>> Thoughts?
>>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>>> the underlying allocator? Hmm, at what cost? would be my question.
>>>
>> The cost would be the time spent on a lock on each call to apr_palloc()
>> or anything that *uses* apr_palloc().
>>
>> The idea being that if the underlying allocator has a mutex, the
>> assumption should be that the pool using that allocator "wants"
>> or "expects" to be thread-safe... It seems an easy way to create
>> thread-safe APR pools, but I could be missing something crucial
>> here.
>>
>> Of course, if the allocator does NOT have a mutex, no change and
>> no cost.
>
>
> I've always understood that creating subpools is thread safe iff the
> allocator has a mutex, but allocating from any single pool is not, by
> definition. Acquiring a mutex for every apr_palloc() seems like a good
> way to throw away pools' speed advantage compared to malloc().
>
> -- Brane
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Branko Čibej <br...@apache.org>.
On 20.02.2017 15:55, Jim Jagielski wrote:
>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>
>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>
>>> The below got me thinking...
>>>
>>> Right now, the pool allocator mutex is only used when, well,
>>> allocator_alloc() is called, which means that sometimes
>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>> not, depending on whether or not the active node has enough
>>> space.
>>>
>>> For 1.6 and later, it might be nice to actually protect the
>>> adjustment of the active node, et.al. to, if a mutex is present,
>>> always be thread-safe... that is, always when we "alloc" memory,
>>> even when/if we do/don't called allocator_alloc().
>>>
>>> Thoughts?
>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>> the underlying allocator? Hmm, at what cost? would be my question.
>>
> The cost would be the time spent on a lock on each call to apr_palloc()
> or anything that *uses* apr_palloc().
>
> The idea being that if the underlying allocator has a mutex, the
> assumption should be that the pool using that allocator "wants"
> or "expects" to be thread-safe... It seems an easy way to create
> thread-safe APR pools, but I could be missing something crucial
> here.
>
> Of course, if the allocator does NOT have a mutex, no change and
> no cost.
I've always understood that creating subpools is thread safe iff the
allocator has a mutex, but allocating from any single pool is not, by
definition. Acquiring a mutex for every apr_palloc() seems like a good
way to throw away pools' speed advantage compared to malloc().
-- Brane
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Branko Čibej <br...@apache.org>.
On 20.02.2017 15:55, Jim Jagielski wrote:
>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>>
>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>>
>>> The below got me thinking...
>>>
>>> Right now, the pool allocator mutex is only used when, well,
>>> allocator_alloc() is called, which means that sometimes
>>> apr_palloc(), for example, can be thread-safeish and sometimes
>>> not, depending on whether or not the active node has enough
>>> space.
>>>
>>> For 1.6 and later, it might be nice to actually protect the
>>> adjustment of the active node, et.al. to, if a mutex is present,
>>> always be thread-safe... that is, always when we "alloc" memory,
>>> even when/if we do/don't called allocator_alloc().
>>>
>>> Thoughts?
>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>> the underlying allocator? Hmm, at what cost? would be my question.
>>
> The cost would be the time spent on a lock on each call to apr_palloc()
> or anything that *uses* apr_palloc().
>
> The idea being that if the underlying allocator has a mutex, the
> assumption should be that the pool using that allocator "wants"
> or "expects" to be thread-safe... It seems an easy way to create
> thread-safe APR pools, but I could be missing something crucial
> here.
>
> Of course, if the allocator does NOT have a mutex, no change and
> no cost.
I've always understood that creating subpools is thread safe iff the
allocator has a mutex, but allocating from any single pool is not, by
definition. Acquiring a mutex for every apr_palloc() seems like a good
way to throw away pools' speed advantage compared to malloc().
-- Brane
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>
>>
>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>
>> The below got me thinking...
>>
>> Right now, the pool allocator mutex is only used when, well,
>> allocator_alloc() is called, which means that sometimes
>> apr_palloc(), for example, can be thread-safeish and sometimes
>> not, depending on whether or not the active node has enough
>> space.
>>
>> For 1.6 and later, it might be nice to actually protect the
>> adjustment of the active node, et.al. to, if a mutex is present,
>> always be thread-safe... that is, always when we "alloc" memory,
>> even when/if we do/don't called allocator_alloc().
>>
>> Thoughts?
>
> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
> the underlying allocator? Hmm, at what cost? would be my question.
>
The cost would be the time spent on a lock on each call to apr_palloc()
or anything that *uses* apr_palloc().
The idea being that if the underlying allocator has a mutex, the
assumption should be that the pool using that allocator "wants"
or "expects" to be thread-safe... It seems an easy way to create
thread-safe APR pools, but I could be missing something crucial
here.
Of course, if the allocator does NOT have a mutex, no change and
no cost.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Feb 21, 2017 at 3:12 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> I think at this stage, let's get all changes/reverts into trunk. When that is all done, we can talk about changes that I "expect" to work additionally.
Reverted now (but the allocator mutex in h2_slave_create(), up to you ;)
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 21.02.2017 um 14:54 schrieb Yann Ylavic <yl...@gmail.com>:
>
> We seem to be talking past each other, I'll _try_ to synchronize ourselves :)
We'll get there! :)
> On Tue, Feb 21, 2017 at 1:57 PM, Stefan Eissing wrote:
>>
>> You have me confused now. If you did that all only for h2, then, I
>> think, we can live without them all nowadays, because:
>
> Actually I made the MPMs change mostly for correctness with regard to
> any possible module.
>
> But since it is useless for http/1 and may hurt common performances, I think
> I'll revert the changes on the MPMs sides (provided mplx->pool is kept
> mutexed!).
That is totally fine with me. Please revert that and I make the necessary changes to h2 and run my stress tests, make a v1.9.1 and let others verify this.
>> 1. master conn_rec->pool (own allocator, all ops in one thread)
>> * h2_session->pool
>> - h2_stream->pool
>> - h2_mplx->pool
>>
>> 2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex):
>> * h2_slave->pool
>
> OK, so for the crashes you reported yesterday, both MPM's ptrans (aka
> master->pool) and mplx->pool were without a mutex. This is what you
> tested right?
> If so, this is "expected" to crash (at least understood now, sf
> pointed us to this in another thread), because nothing is protecting
> concurrent creation/destruction of mplx->pool's subpools.
I think at this stage, let's get all changes/reverts into trunk. When that is all done, we can talk about changes that I "expect" to work additionally.
> [...]
>>
>> This is how it is intended to be used.
>
> Got it (I think :)
:)
> It should be both safe an performant, could you pass your stress tests on it?
> That is, with current trunk, comment out the apr_allocator_mutex_set()
> used in mpm_event's listener_thread() and h2_slave_create().
>
> Unless I (again) misunderstood what you tried to tell me :)
Will do, once you revert.
>
> Thanks!
> Yann.
Stefan Eissing
<green/>bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
We seem to be talking past each other, I'll _try_ to synchronize ourselves :)
On Tue, Feb 21, 2017 at 1:57 PM, Stefan Eissing wrote:
>
> You have me confused now. If you did that all only for h2, then, I
> think, we can live without them all nowadays, because:
Actually I made the MPMs change mostly for correctness with regard to
any possible module.
But since it is useless for http/1 and may hurt common performances, I think
I'll revert the changes on the MPMs sides (provided mplx->pool is kept
mutexed!).
>
> 1. master conn_rec->pool (own allocator, all ops in one thread)
> * h2_session->pool
> - h2_stream->pool
> - h2_mplx->pool
>
> 2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex):
> * h2_slave->pool
OK, so for the crashes you reported yesterday, both MPM's ptrans (aka
master->pool) and mplx->pool were without a mutex. This is what you
tested right?
If so, this is "expected" to crash (at least understood now, sf
pointed us to this in another thread), because nothing is protecting
concurrent creation/destruction of mplx->pool's subpools.
If either ptrans or mplx->pool (or both) is assigned a mutex, nothing
should crash (the allocator is inherited).
And if mplx->pool is mutexed, ptrans doesn't need to because mplxs are
created in the usual MPM worker path (where ptrans is dedicated):
run_process_connection => h2_h2_process_conn => h2_conn_setup =>
h2_session_create => h2_mplx_create (or the Upgrade path which isn't
an issue either).
>
> 3. h2_slave->pool (own allocator, all ops in one thread)
> * h2_task->pool
> * r->pool
OK, I missed that slave => task/request were single threaded, so
slave->pool doesn't need a mutex either.
It can still have its own allocator to remove useless contention on
mplx->pool's mutex, but no mutex needed for itself or its children.
>
> This is how it is intended to be used.
Got it (I think :)
> And 2+3 run without allocator
> mutex at Stefan Priebe's sites.
Not sure, Stefan Priebe runs with many patches AIUI, including
mpm_event ptrans' mutex, mplx->pool's mutex (and also useless
slave->pool's), so he seems to be bullet proof.
To conclude, I think all we need is an unmutexed allocator for MPMs'
ptrans (i.e. revert yesterday's related changes), a mutexed allocator
for mplx->pool, and an unmutexed allocator for slave->pool.
It should be both safe an performant, could you pass your stress tests on it?
That is, with current trunk, comment out the apr_allocator_mutex_set()
used in mpm_event's listener_thread() and h2_slave_create().
Unless I (again) misunderstood what you tried to tell me :)
Thanks!
Yann.
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 21.02.2017 um 13:42 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Mon, Feb 20, 2017 at 6:29 PM, Yann Ylavic <yl...@gmail.com> wrote:
>> On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing
>> <st...@greenbytes.de> wrote:
>>>
>>> 1+2, so the only allocator is from master conn_rec.
>>
>> The allocator with a mutex (after r1783755)?
>
> Sorry to insist Stefan, I see a reason why it would fail without
> r1783755 (this commit) but not with, so maybe there is wolf in there
> (french expression, dunno if it translates literally to english :)
You have me confused now. If you did that all only for h2, then, I think, we can live without them all nowadays, because:
1. master conn_rec->pool (own allocator, all ops in one thread)
* h2_session->pool
- h2_stream->pool
- h2_mplx->pool
2. h2_mplx->pool (own allocator, all ops guarded h2_mplx->lock mutex):
* h2_slave->pool
3. h2_slave->pool (own allocator, all ops in one thread)
* h2_task->pool
* r->pool
This is how it is intended to be used. And 2+3 run without allocator mutex at Stefan Priebe's sites.
Stefan Eissing
<green/>bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 6:29 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> 1+2, so the only allocator is from master conn_rec.
>
> The allocator with a mutex (after r1783755)?
Sorry to insist Stefan, I see a reason why it would fail without
r1783755 (this commit) but not with, so maybe there is wolf in there
(french expression, dunno if it translates literally to english :)
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Feb 20, 2017 at 6:21 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> 1+2, so the only allocator is from master conn_rec.
The allocator with a mutex (after r1783755)?
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 20.02.2017 um 17:41 schrieb Yann Ylavic <yl...@gmail.com>:
>
> [Keeping httpd list here only]
>
> On Mon, Feb 20, 2017 at 3:51 PM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
>> the underlying allocator? Hmm, at what cost? would be my question.
>
> I also fear that it would be costly when not needed (e.g. request's pool).
> Because yes, the allocator is inherited from the parent pool if none
> is specified at creation time, and hence with this commit every child
> pool of ptrans will have a mutexed allocator.
>
>>
>> In regard to thread safety of apr_allocator, there is still something
>> I do not understand. Observations:
>>
>> 1. When I remove the own allocator in h2_mplx, so it inherits the
>> now protected one from the master connection, all runs fine.
>
> I'm not sure this dedicated allocator is needed, mplx seems to race
> only with itself on ptrans (master->pool), but I may miss something.
>
>> 2. When I remove the own allocator from the slave connections also,
>> in h2_conn, so that slave conns also use the protected allocator
>> from the master conn,
>
> Hmm, rather the allocator from mplx->pool (since slave connections are
> children of mplx), right?
>
>> things break. E.g. strange behaviour up
>> to crashes under load.
>
> Is it with or without this commit?
> Also with 1. + 2., or 2. alone?
1+2, so the only allocator is from master conn_rec.
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
[Keeping httpd list here only]
On Mon, Feb 20, 2017 at 3:51 PM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
> the underlying allocator? Hmm, at what cost? would be my question.
I also fear that it would be costly when not needed (e.g. request's pool).
Because yes, the allocator is inherited from the parent pool if none
is specified at creation time, and hence with this commit every child
pool of ptrans will have a mutexed allocator.
>
> In regard to thread safety of apr_allocator, there is still something
> I do not understand. Observations:
>
> 1. When I remove the own allocator in h2_mplx, so it inherits the
> now protected one from the master connection, all runs fine.
I'm not sure this dedicated allocator is needed, mplx seems to race
only with itself on ptrans (master->pool), but I may miss something.
> 2. When I remove the own allocator from the slave connections also,
> in h2_conn, so that slave conns also use the protected allocator
> from the master conn,
Hmm, rather the allocator from mplx->pool (since slave connections are
children of mplx), right?
> things break. E.g. strange behaviour up
> to crashes under load.
Is it with or without this commit?
Also with 1. + 2., or 2. alone?
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Jim Jagielski <ji...@jaguNET.com>.
> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <st...@greenbytes.de> wrote:
>
>>
>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>>
>> The below got me thinking...
>>
>> Right now, the pool allocator mutex is only used when, well,
>> allocator_alloc() is called, which means that sometimes
>> apr_palloc(), for example, can be thread-safeish and sometimes
>> not, depending on whether or not the active node has enough
>> space.
>>
>> For 1.6 and later, it might be nice to actually protect the
>> adjustment of the active node, et.al. to, if a mutex is present,
>> always be thread-safe... that is, always when we "alloc" memory,
>> even when/if we do/don't called allocator_alloc().
>>
>> Thoughts?
>
> So, apr_p*alloc() calls would be thread-safe if a mutex is set in
> the underlying allocator? Hmm, at what cost? would be my question.
>
The cost would be the time spent on a lock on each call to apr_palloc()
or anything that *uses* apr_palloc().
The idea being that if the underlying allocator has a mutex, the
assumption should be that the pool using that allocator "wants"
or "expects" to be thread-safe... It seems an easy way to create
thread-safe APR pools, but I could be missing something crucial
here.
Of course, if the allocator does NOT have a mutex, no change and
no cost.
Re: svn commit: r1783755 -
/httpd/httpd/trunk/server/mpm/event/event.c
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <ji...@jaguNET.com>:
>
> The below got me thinking...
>
> Right now, the pool allocator mutex is only used when, well,
> allocator_alloc() is called, which means that sometimes
> apr_palloc(), for example, can be thread-safeish and sometimes
> not, depending on whether or not the active node has enough
> space.
>
> For 1.6 and later, it might be nice to actually protect the
> adjustment of the active node, et.al. to, if a mutex is present,
> always be thread-safe... that is, always when we "alloc" memory,
> even when/if we do/don't called allocator_alloc().
>
> Thoughts?
So, apr_p*alloc() calls would be thread-safe if a mutex is set in
the underlying allocator? Hmm, at what cost? would be my question.
In regard to thread safety of apr_allocator, there is still something
I do not understand. Observations:
1. When I remove the own allocator in h2_mplx, so it inherits the
now protected one from the master connection, all runs fine.
2. When I remove the own allocator from the slave connections also,
in h2_conn, so that slave conns also use the protected allocator
from the master conn, things break. E.g. strange behaviour up
to crashes under load.
Which indicates that I have not fully understood the contract of these
things, or there is a hidden assumptions in regard to conn_rec->pool.
hidden to me, at least.
Can someone shed light on this?
>
>> On Feb 20, 2017, at 8:38 AM, ylavic@apache.org wrote:
>>
>> Author: ylavic
>> Date: Mon Feb 20 13:38:03 2017
>> New Revision: 1783755
>>
>> URL: http://svn.apache.org/viewvc?rev=1783755&view=rev
>> Log:
>> mpm_event: use a mutex for ptrans' allocator to be safe with concurrent
>> creation and destruction of its subpools, like with mod_http2.
>>
>>
>> Modified:
>> httpd/httpd/trunk/server/mpm/event/event.c
>>
>> Modified: httpd/httpd/trunk/server/mpm/event/event.c
>> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/server/mpm/event/event.c?rev=1783755&r1=1783754&r2=1783755&view=diff
>> ==============================================================================
>> --- httpd/httpd/trunk/server/mpm/event/event.c (original)
>> +++ httpd/httpd/trunk/server/mpm/event/event.c Mon Feb 20 13:38:03 2017
>> @@ -2096,6 +2096,7 @@ static void * APR_THREAD_FUNC listener_t
>> }
>> if (!listeners_disabled) {
>> void *csd = NULL;
>> + apr_thread_mutex_t *mutex;
>> ap_listen_rec *lr = (ap_listen_rec *) pt->baton;
>> apr_pool_t *ptrans; /* Pool for per-transaction stuff */
>> ap_pop_pool(&ptrans, worker_queue_info);
>> @@ -2105,20 +2106,44 @@ static void * APR_THREAD_FUNC listener_t
>> apr_allocator_t *allocator;
>>
>> apr_allocator_create(&allocator);
>> - apr_allocator_max_free_set(allocator,
>> - ap_max_mem_free);
>> + apr_allocator_max_free_set(allocator, ap_max_mem_free);
>> apr_pool_create_ex(&ptrans, pconf, NULL, allocator);
>> - apr_allocator_owner_set(allocator, ptrans);
>> if (ptrans == NULL) {
>> ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
>> ap_server_conf, APLOGNO(03097)
>> "Failed to create transaction pool");
>> + apr_allocator_destroy(allocator);
>> signal_threads(ST_GRACEFUL);
>> return NULL;
>> }
>> + apr_allocator_owner_set(allocator, ptrans);
>> }
>> apr_pool_tag(ptrans, "transaction");
>>
>> + /* We need a mutex in the allocator to synchronize ptrans'
>> + * children creations/destructions, but this mutex ought to
>> + * live in ptrans itself to avoid leaks, hence it's cleared
>> + * in ap_push_pool(). We could recycle some pconf's mutexes
>> + * like we do for ptrans subpools, but that'd need another
>> + * synchronization mechanism, whereas creating a pthread
>> + * mutex (unix here!) is really as simple/fast as a static
>> + * PTHREAD_MUTEX_INIT assignment, so let's not bother and
>> + * create the mutex for each ptrans (recycled or not).
>> + */
>> + rc = apr_thread_mutex_create(&mutex,
>> + APR_THREAD_MUTEX_DEFAULT,
>> + ptrans);
>> + if (rc != APR_SUCCESS) {
>> + ap_log_error(APLOG_MARK, APLOG_CRIT, rc,
>> + ap_server_conf, APLOGNO()
>> + "Failed to create transaction pool mutex");
>> + ap_push_pool(worker_queue_info, ptrans);
>> + signal_threads(ST_GRACEFUL);
>> + return NULL;
>> + }
>> + apr_allocator_mutex_set(apr_pool_allocator_get(ptrans),
>> + mutex);
>> +
>> get_worker(&have_idle_worker, 1, &workers_were_busy);
>> rc = lr->accept_func(&csd, lr, ptrans);
>>
>>
>>
>
Stefan Eissing
<green/>bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
[Oups, meant to reach the list(s)].
On Mon, Feb 20, 2017 at 3:46 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 3:16 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> The below got me thinking...
>>
>> Right now, the pool allocator mutex is only used when, well,
>> allocator_alloc() is called, which means that sometimes
>> apr_palloc(), for example, can be thread-safeish and sometimes
>> not, depending on whether or not the active node has enough
>> space.
>>
>> For 1.6 and later, it might be nice to actually protect the
>> adjustment of the active node, et.al. to, if a mutex is present,
>> always be thread-safe... that is, always when we "alloc" memory,
>> even when/if we do/don't called allocator_alloc().
>>
>> Thoughts?
>
> That could be useful, but at a cost for every allocation (not only
> when the active node is exhausted).
>
> And we don't need it for http/1 requests processing for example for
> now, while it may be interesting for ptrans for requests to easily
> allocate there (not anyhow of course...).
>
> So I'd rather use another mutex for synchronized allocations
> (when/where needed by the user) with an explicit setting (at creation
> time) like apr_pool_set_sync_alloc(), and then a per-call
> apr_palloc_sync() maybe?
Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Posted by Yann Ylavic <yl...@gmail.com>.
[Oups, meant to reach the list(s)].
On Mon, Feb 20, 2017 at 3:46 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Mon, Feb 20, 2017 at 3:16 PM, Jim Jagielski <ji...@jagunet.com> wrote:
>> The below got me thinking...
>>
>> Right now, the pool allocator mutex is only used when, well,
>> allocator_alloc() is called, which means that sometimes
>> apr_palloc(), for example, can be thread-safeish and sometimes
>> not, depending on whether or not the active node has enough
>> space.
>>
>> For 1.6 and later, it might be nice to actually protect the
>> adjustment of the active node, et.al. to, if a mutex is present,
>> always be thread-safe... that is, always when we "alloc" memory,
>> even when/if we do/don't called allocator_alloc().
>>
>> Thoughts?
>
> That could be useful, but at a cost for every allocation (not only
> when the active node is exhausted).
>
> And we don't need it for http/1 requests processing for example for
> now, while it may be interesting for ptrans for requests to easily
> allocate there (not anyhow of course...).
>
> So I'd rather use another mutex for synchronized allocations
> (when/where needed by the user) with an explicit setting (at creation
> time) like apr_pool_set_sync_alloc(), and then a per-call
> apr_palloc_sync() maybe?