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.
