httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: svn commit: r1783755 - /httpd/httpd/trunk/server/mpm/event/event.c
Date Tue, 21 Feb 2017 13:54:46 GMT
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.

Mime
View raw message