You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by ic...@apache.org on 2017/02/21 17:18:27 UTC
svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2: h2_conn.c
h2_mplx.c h2_mplx.h
Author: icing
Date: Tue Feb 21 17:18:27 2017
New Revision: 1783912
URL: http://svn.apache.org/viewvc?rev=1783912&view=rev
Log:
On the trunk:
mod_http2: removing extra mutex for h2 apr_allocators again.
Modified:
httpd/httpd/trunk/modules/http2/h2_conn.c
httpd/httpd/trunk/modules/http2/h2_mplx.c
httpd/httpd/trunk/modules/http2/h2_mplx.h
Modified: httpd/httpd/trunk/modules/http2/h2_conn.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_conn.c?rev=1783912&r1=1783911&r2=1783912&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_conn.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_conn.c Tue Feb 21 17:18:27 2017
@@ -255,7 +255,6 @@ apr_status_t h2_conn_pre_close(struct h2
conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent)
{
apr_allocator_t *allocator;
- apr_thread_mutex_t *mutex;
apr_status_t status;
apr_pool_t *pool;
conn_rec *c;
@@ -274,18 +273,15 @@ conn_rec *h2_slave_create(conn_rec *mast
*/
apr_allocator_create(&allocator);
apr_allocator_max_free_set(allocator, ap_max_mem_free);
- apr_pool_create_ex(&pool, parent, NULL, allocator);
- apr_pool_tag(pool, "h2_slave_conn");
- apr_allocator_owner_set(allocator, pool);
- status = apr_thread_mutex_create(&mutex, APR_THREAD_MUTEX_DEFAULT, pool);
+ status = apr_pool_create_ex(&pool, parent, NULL, allocator);
if (status != APR_SUCCESS) {
ap_log_cerror(APLOG_MARK, APLOG_ERR, status, master,
- APLOGNO(10004) "h2_session(%ld-%d): create slave mutex",
+ APLOGNO(10004) "h2_session(%ld-%d): create slave pool",
master->id, slave_id);
- apr_pool_destroy(pool);
return NULL;
}
- apr_allocator_mutex_set(allocator, mutex);
+ apr_allocator_owner_set(allocator, pool);
+ apr_pool_tag(pool, "h2_slave_conn");
c = (conn_rec *) apr_palloc(pool, sizeof(conn_rec));
if (c == NULL) {
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
@@ -235,7 +235,6 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
{
apr_status_t status = APR_SUCCESS;
apr_allocator_t *allocator;
- apr_thread_mutex_t *mutex;
h2_mplx *m;
ap_assert(conf);
@@ -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);
status = apr_thread_mutex_create(&m->lock, APR_THREAD_MUTEX_DEFAULT,
m->pool);
@@ -284,7 +275,6 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr
return NULL;
}
- m->bucket_alloc = apr_bucket_alloc_create(m->pool);
m->max_streams = h2_config_geti(conf, H2_CONF_MAX_STREAMS);
m->stream_max_mem = h2_config_geti(conf, H2_CONF_STREAM_MAX_MEM);
Modified: httpd/httpd/trunk/modules/http2/h2_mplx.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_mplx.h?rev=1783912&r1=1783911&r2=1783912&view=diff
==============================================================================
--- httpd/httpd/trunk/modules/http2/h2_mplx.h (original)
+++ httpd/httpd/trunk/modules/http2/h2_mplx.h Tue Feb 21 17:18:27 2017
@@ -63,7 +63,6 @@ struct h2_mplx {
long id;
conn_rec *c;
apr_pool_t *pool;
- apr_bucket_alloc_t *bucket_alloc;
APR_RING_ENTRY(h2_mplx) link;
Re: svn commit: r1783912 - in /httpd/httpd/trunk/modules/http2:
h2_conn.c h2_mplx.c h2_mplx.h
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 22.02.2017 um 10:01 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic <yl...@gmail.com>:
>>>
>>> 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
<green/>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
Posted by Yann Ylavic <yl...@gmail.com>.
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
Posted by Stefan Eissing <st...@greenbytes.de>.
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 <yl...@gmail.com>:
>
> On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
> <st...@greenbytes.de> wrote:
>>
>>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic <yl...@gmail.com>:
>>>
>>> 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
<green/>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
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, Feb 22, 2017 at 8:52 AM, Stefan Eissing
<st...@greenbytes.de> wrote:
>
>> Am 21.02.2017 um 18:34 schrieb Yann Ylavic <yl...@gmail.com>:
>>
>> 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
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 21.02.2017 um 18:34 schrieb Yann Ylavic <yl...@gmail.com>:
>
> On Tue, Feb 21, 2017 at 6:18 PM, <ic...@apache.org> 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
<green/>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
Posted by Yann Ylavic <yl...@gmail.com>.
On Tue, Feb 21, 2017 at 6:18 PM, <ic...@apache.org> 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?