Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Yann Ylavic]

On Wed, Feb 22, 2017 at 10:55 AM, Stefan Eissing wrote:
> Now you and a recent, unrepeatable crash on my stress test made me
> nervous. The mplx alloc mutex goes back in now, I want to sleep at
> night... ;-)

Sorry about that, although I already knew that counting mutexes was
better than counting sheeps :)

Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Stefan Eissing]

Now you and a recent, unrepeatable crash on my stress test made me nervous. The 
mplx alloc mutex goes back in now, I want to sleep at night... ;-)

> Am 22.02.2017 um 10:01 schrieb Yann Ylavic :
> 
> On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
>  wrote:
>> 
>>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic :
>>> 
>>> We are back to initial issue here, no?
>> 
>> Surely hope not. All subpools of mplx->pool are guarded by mplx->lock mutex 
>> already.
> 
> OK, slaves/tasks creations look safe indeed.
> 
> Also (mainly for my guidance), what about streams' creation (from
> session->pool)?
> This is in nghttp2's on_begin_headers_cb(), hence always by the same thread?
> 
> Thanks for your patience ;)

Stefan Eissing

bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de

Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Stefan Eissing]

> Am 22.02.2017 um 10:01 schrieb Yann Ylavic :
> 
> On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
>  wrote:
>> 
>>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic :
>>> 
>>> We are back to initial issue here, no?
>> 
>> Surely hope not. All subpools of mplx->pool are guarded by mplx->lock mutex 
>> already.
> 
> OK, slaves/tasks creations look safe indeed.
> 
> Also (mainly for my guidance), what about streams' creation (from
> session->pool)?
> This is in nghttp2's on_begin_headers_cb(), hence always by the same thread?

Yes, all on the master "side". And lifetime of a stream is now always >= 
lifetime of its slave/task (modulo slave reuse). Go betweens are only h2_mplx 
and h2_bucket_beam, with beams now also handling response headers and trailers.

I am currently thinking about moving the complete stream set from mplx back to 
session and only giving streams to mplx for cleanup. I also want to try the 
possible gains when each beam has its own mutex. 

Another bigger change idea would to bundle to beams to a h2_bucket_pipe and tie 
that to a slave connection, to be reused for multiple requests. That would give 
a bi-directional bucket "connection" for slaves. Make that usable in pollsets. 
Integrate that into mpm and we could get rid of h2_workers...

A man can dream...

> 
> Thanks for your patience ;)

My pleasure. ;)

Stefan Eissing

bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de

Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Yann Ylavic]

On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
 wrote:
>
>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic :
>>
>> We are back to initial issue here, no?
>
> Surely hope not. All subpools of mplx->pool are guarded by mplx->lock mutex 
> already.

OK, slaves/tasks creations look safe indeed.

Also (mainly for my guidance), what about streams' creation (from
session->pool)?
This is in nghttp2's on_begin_headers_cb(), hence always by the same thread?

Thanks for your patience ;)

Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Stefan Eissing]

> Am 21.02.2017 um 18:34 schrieb Yann Ylavic :
> 
> On Tue, Feb 21, 2017 at 6:18 PM,   wrote:
>> 
>> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
>> URL: 
>> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1783912=1783911=1783912=diff
>> ==
>> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
>> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Tue Feb 21 17:18:27 2017
>> 
>> @@ -256,20 +255,12 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>> return NULL;
>> }
>> apr_allocator_max_free_set(allocator, ap_max_mem_free);
>> -apr_pool_create_ex(>pool, parent, NULL, allocator);
>> -if (!m->pool) {
>> -apr_allocator_destroy(allocator);
>> +status = apr_pool_create_ex(>pool, parent, NULL, allocator);
>> +if (status != APR_SUCCESS) {
>> return NULL;
>> }
>> apr_pool_tag(m->pool, "h2_mplx");
>> apr_allocator_owner_set(allocator, m->pool);
>> -status = apr_thread_mutex_create(, APR_THREAD_MUTEX_DEFAULT,
>> - m->pool);
>> -if (status != APR_SUCCESS) {
>> -apr_pool_destroy(m->pool);
>> -return NULL;
>> -}
>> -apr_allocator_mutex_set(allocator, mutex);
> 
> I think this one is needed still (ptans/master->pool is not mutexed
> anymore), otherwise what will synchronize mplx's subpools?
> We are back to initial issue here, no?

Surely hope not. All subpools of mplx->pool are guarded by mplx->lock mutex 
already.

Stefan Eissing

bytes GmbH
Hafenstrasse 16
48155 Münster
www.greenbytes.de

Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h

[Yann Ylavic]

On Tue, Feb 21, 2017 at 6:18 PM,   wrote:
>
> Modified: httpd/httpd/trunk/modules/http2/h2_mplx.c
> URL: 
> http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.c?rev=1783912=1783911=1783912=diff
> ==
> --- httpd/httpd/trunk/modules/http2/h2_mplx.c (original)
> +++ httpd/httpd/trunk/modules/http2/h2_mplx.c Tue Feb 21 17:18:27 2017
>
> @@ -256,20 +255,12 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
>  return NULL;
>  }
>  apr_allocator_max_free_set(allocator, ap_max_mem_free);
> -apr_pool_create_ex(>pool, parent, NULL, allocator);
> -if (!m->pool) {
> -apr_allocator_destroy(allocator);
> +status = apr_pool_create_ex(>pool, parent, NULL, allocator);
> +if (status != APR_SUCCESS) {
>  return NULL;
>  }
>  apr_pool_tag(m->pool, "h2_mplx");
>  apr_allocator_owner_set(allocator, m->pool);
> -status = apr_thread_mutex_create(, APR_THREAD_MUTEX_DEFAULT,
> - m->pool);
> -if (status != APR_SUCCESS) {
> -apr_pool_destroy(m->pool);
> -return NULL;
> -}
> -apr_allocator_mutex_set(allocator, mutex);

I think this one is needed still (ptans/master->pool is not mutexed
anymore), otherwise what will synchronize mplx's subpools?
We are back to initial issue here, no?