> Am 20.02.2017 um 15:16 schrieb Jim Jagielski <j...@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, yla...@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
