Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

classic Classic list List threaded Threaded
35 messages Options
12
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Jim Jagielski
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, [hidden email] 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);
>
>
>

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Yann Ylavic
[Oups, meant to reach the list(s)].

On Mon, Feb 20, 2017 at 3:46 PM, Yann Ylavic <[hidden email]> wrote:

> On Mon, Feb 20, 2017 at 3:16 PM, Jim Jagielski <[hidden email]> 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?
Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Stefan Eissing
In reply to this post by Jim Jagielski

> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>
> 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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Jim Jagielski

> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>
>>
>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>
>> 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.

Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Branko Čibej
On 20.02.2017 15:55, Jim Jagielski wrote:

>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>
>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>
>>> 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
Reply | Threaded
Open this post in threaded view
|

APR pools, mutexes and thread safe allocations

Jim Jagielski
Again, this would ONLY happen if the underlying allocator has
a mutex!

> On Feb 20, 2017, at 10:06 AM, Branko Čibej <[hidden email]> wrote:
>
> On 20.02.2017 15:55, Jim Jagielski wrote:
>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>
>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>
>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Stefan Eissing

> Am 20.02.2017 um 16:08 schrieb Jim Jagielski <[hidden email]>:
>
> 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 <[hidden email]> wrote:
>>
>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>
>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>
>>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
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 <[hidden email]> wrote:
>
>>
>> Am 20.02.2017 um 16:08 schrieb Jim Jagielski <[hidden email]>:
>>
>> 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 <[hidden email]> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>>
>>>>>> 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

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Branko Čibej
In reply to this post by Jim Jagielski
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 <[hidden email]> wrote:
>>
>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>
>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>
>>>>> 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


Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
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 <[hidden email]> 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 <[hidden email]> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>>
>>>>>> 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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
I mean, of course, "has a *mutex*"

> On Feb 20, 2017, at 10:39 AM, Jim Jagielski <[hidden email]> 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 <[hidden email]> 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 <[hidden email]> wrote:
>>>>
>>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>>>
>>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>>>
>>>>>>> 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
>>
>>
>

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski-2
In reply to this post by Branko Čibej
You are confusing pool mutexes with allocator mutexes...

> On Feb 20, 2017, at 10:26 AM, Branko Čibej <[hidden email]> 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 <[hidden email]> wrote:
>>>
>>> On 20.02.2017 15:55, Jim Jagielski wrote:
>>>>> On Feb 20, 2017, at 9:51 AM, Stefan Eissing <[hidden email]> wrote:
>>>>>
>>>>>> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <[hidden email]>:
>>>>>>
>>>>>> 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
>
>

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
In reply to this post by Branko Čibej

> On Feb 20, 2017, at 10:26 AM, Branko Čibej <[hidden email]> 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



Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Yann Ylavic
In reply to this post by Jim Jagielski-2
On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
Please READ apr_pools.c... Search for '->mutex'

> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <[hidden email]> wrote:
>
> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <[hidden email]> 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.

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Yann Ylavic
Well, this is for debug mode only I guess.

On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <[hidden email]> wrote:

> Please READ apr_pools.c... Search for '->mutex'
>
>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <[hidden email]> wrote:
>>
>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <[hidden email]> 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.
>
Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
apr_pool_create_ex_debug(), yes.

> On Feb 20, 2017, at 11:00 AM, Yann Ylavic <[hidden email]> wrote:
>
> Well, this is for debug mode only I guess.
>
> On Mon, Feb 20, 2017 at 4:59 PM, Jim Jagielski <[hidden email]> wrote:
>> Please READ apr_pools.c... Search for '->mutex'
>>
>>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <[hidden email]> wrote:
>>>
>>> On Mon, Feb 20, 2017 at 4:43 PM, Jim Jagielski <[hidden email]> 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.
>>

Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Jim Jagielski
In reply to this post by Yann Ylavic

> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <[hidden email]> 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.
Reply | Threaded
Open this post in threaded view
|

Re: APR pools, mutexes and thread safe allocations

Stefan Eissing

> Am 20.02.2017 um 17:22 schrieb Jim Jagielski <[hidden email]>:
>
>
>> On Feb 20, 2017, at 10:58 AM, Yann Ylavic <[hidden email]> 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


Reply | Threaded
Open this post in threaded view
|

Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c

Yann Ylavic
In reply to this post by Stefan Eissing
[Keeping httpd list here only]

On Mon, Feb 20, 2017 at 3:51 PM, Stefan Eissing
<[hidden email]> 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?
12