httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stefan Eissing <stefan.eiss...@greenbytes.de>
Subject Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Date Mon, 20 Feb 2017 14:51:41 GMT

> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <jim@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


Mime
View raw message