Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c h2_mplx.c h2_mplx.h
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
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
> 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
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
> 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&r1=1783911&r2=1783912&view=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(&m->pool, parent, NULL, allocator); >> -if (!m->pool) { >> -apr_allocator_destroy(allocator); >> +status = apr_pool_create_ex(&m->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(&mutex, 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
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&r1=1783911&r2=1783912&view=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(&m->pool, parent, NULL, allocator); > -if (!m->pool) { > -apr_allocator_destroy(allocator); > +status = apr_pool_create_ex(&m->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(&mutex, 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?