From d2c61fc207e226ef3a00aaef3a6e14567363f3ed Mon Sep 17 00:00:00 2001 From: Stefan Eissing Date: Sat, 29 Oct 2016 22:17:11 +0000 Subject: [PATCH] mod_http2: earlier slave connection allocator destroy, code cleanups git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1767128 13f79535-47bb-0310-9956-ffa450edef68 --- CHANGES | 4 ++ modules/http2/h2_conn.c | 20 ++------ modules/http2/h2_conn.h | 5 +- modules/http2/h2_mplx.c | 96 ++++++++++++++++--------------------- modules/http2/h2_ngn_shed.c | 2 +- modules/http2/h2_session.c | 12 ++--- modules/http2/h2_session.h | 1 - modules/http2/h2_task.c | 8 ++-- 8 files changed, 58 insertions(+), 90 deletions(-) diff --git a/CHANGES b/CHANGES index 31b0fac848..12b5f46f77 100644 --- a/CHANGES +++ b/CHANGES @@ -1,6 +1,10 @@ -*- coding: utf-8 -*- Changes with Apache 2.5.0 + *) mod_http2: allocators from slave connections is released earlier, resulting + in less overall memory use on busy, long lived connections. + [Stefan Eissing] + *) mpm_unix: Apache fails to start if previously crashed then restarted with the same PID (e.g. in container). PR 60261. [Val , Yann Ylavic] diff --git a/modules/http2/h2_conn.c b/modules/http2/h2_conn.c index a0915c3eb4..6f3a8cfe8e 100644 --- a/modules/http2/h2_conn.c +++ b/modules/http2/h2_conn.c @@ -241,9 +241,9 @@ apr_status_t h2_conn_pre_close(struct h2_ctx *ctx, conn_rec *c) return status; } -conn_rec *h2_slave_create(conn_rec *master, int slave_id, - apr_pool_t *parent, apr_allocator_t *allocator) +conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent) { + apr_allocator_t *allocator; apr_pool_t *pool; conn_rec *c; void *cfg; @@ -257,9 +257,7 @@ conn_rec *h2_slave_create(conn_rec *master, int slave_id, * independant of its parent pool in the sense that it can work in * another thread. */ - if (!allocator) { - apr_allocator_create(&allocator); - } + apr_allocator_create(&allocator); apr_pool_create_ex(&pool, parent, NULL, allocator); apr_pool_tag(pool, "h2_slave_conn"); apr_allocator_owner_set(allocator, pool); @@ -311,21 +309,11 @@ conn_rec *h2_slave_create(conn_rec *master, int slave_id, return c; } -void h2_slave_destroy(conn_rec *slave, apr_allocator_t **pallocator) +void h2_slave_destroy(conn_rec *slave) { - apr_pool_t *parent; - apr_allocator_t *allocator = apr_pool_allocator_get(slave->pool); ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, slave, "h2_slave_conn(%ld): destroy (task=%s)", slave->id, apr_table_get(slave->notes, H2_TASK_ID_NOTE)); - /* Attache the allocator to the parent pool and return it for - * reuse, otherwise the own is still the slave pool and it will - * get destroyed with it. */ - parent = apr_pool_parent_get(slave->pool); - if (pallocator && parent) { - apr_allocator_owner_set(allocator, parent); - *pallocator = allocator; - } apr_pool_destroy(slave->pool); } diff --git a/modules/http2/h2_conn.h b/modules/http2/h2_conn.h index 13b20539b1..79948644ae 100644 --- a/modules/http2/h2_conn.h +++ b/modules/http2/h2_conn.h @@ -66,9 +66,8 @@ typedef enum { h2_mpm_type_t h2_conn_mpm_type(void); -conn_rec *h2_slave_create(conn_rec *master, int slave_id, - apr_pool_t *parent, apr_allocator_t *allocator); -void h2_slave_destroy(conn_rec *slave, apr_allocator_t **pallocator); +conn_rec *h2_slave_create(conn_rec *master, int slave_id, apr_pool_t *parent); +void h2_slave_destroy(conn_rec *slave); apr_status_t h2_slave_run_pre_connection(conn_rec *slave, apr_socket_t *csd); void h2_slave_run_connection(conn_rec *slave); diff --git a/modules/http2/h2_mplx.c b/modules/http2/h2_mplx.c index 0cff7b60fa..72f11106ed 100644 --- a/modules/http2/h2_mplx.c +++ b/modules/http2/h2_mplx.c @@ -226,11 +226,17 @@ static void purge_streams(h2_mplx *m) static void h2_mplx_destroy(h2_mplx *m) { + conn_rec **pslave; ap_assert(m); ap_log_cerror(APLOG_MARK, APLOG_TRACE1, 0, m->c, "h2_mplx(%ld): destroy, tasks=%d", m->id, (int)h2_ihash_count(m->tasks)); check_tx_free(m); + + while (m->spare_slaves->nelts > 0) { + pslave = (conn_rec **)apr_array_pop(m->spare_slaves); + h2_slave_destroy(*pslave); + } if (m->pool) { apr_pool_destroy(m->pool); } @@ -297,6 +303,7 @@ h2_mplx *h2_mplx_create(conn_rec *c, apr_pool_t *parent, m->q = h2_iq_create(m->pool, m->max_streams); m->sready = h2_ihash_create(m->pool, offsetof(h2_stream,id)); m->tasks = h2_ihash_create(m->pool, offsetof(h2_task,stream_id)); + m->redo_tasks = h2_ihash_create(m->pool, offsetof(h2_task, stream_id)); m->stream_timeout = stream_timeout; m->workers = workers; @@ -363,24 +370,20 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master) } } - if (task->output.beam) { - h2_beam_on_produced(task->output.beam, NULL, NULL); - ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, - APLOGNO(03385) "h2_task(%s): destroy " - "output beam empty=%d, holds proxies=%d", - task->id, - h2_beam_empty(task->output.beam), - h2_beam_holds_proxies(task->output.beam)); - } + h2_beam_on_produced(task->output.beam, NULL, NULL); + ap_log_cerror(APLOG_MARK, APLOG_DEBUG, 0, m->c, + APLOGNO(03385) "h2_task(%s): destroy " + "output beam empty=%d, holds proxies=%d", + task->id, + h2_beam_empty(task->output.beam), + h2_beam_holds_proxies(task->output.beam)); slave = task->c; reuse_slave = ((m->spare_slaves->nelts < m->spare_slaves->nalloc) && !task->rst_error); h2_ihash_remove(m->tasks, task->stream_id); - if (m->redo_tasks) { - h2_ihash_remove(m->redo_tasks, task->stream_id); - } + h2_ihash_remove(m->redo_tasks, task->stream_id); h2_task_destroy(task); if (slave) { @@ -389,7 +392,7 @@ static void task_destroy(h2_mplx *m, h2_task *task, int called_from_master) } else { slave->sbh = NULL; - h2_slave_destroy(slave, NULL); + h2_slave_destroy(slave); } } @@ -434,18 +437,15 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error) h2_iq_remove(m->q, stream->id); h2_ihash_remove(m->sready, stream->id); h2_ihash_remove(m->streams, stream->id); - if (stream->input) { - m->tx_handles_reserved += h2_beam_get_files_beamed(stream->input); - h2_beam_on_consumed(stream->input, NULL, NULL); - /* Let anyone blocked reading know that there is no more to come */ - h2_beam_abort(stream->input); - /* Remove mutex after, so that abort still finds cond to signal */ - h2_beam_mutex_set(stream->input, NULL, NULL, NULL); - } - if (stream->output) { - m->tx_handles_reserved += h2_beam_get_files_beamed(stream->output); - } + h2_stream_cleanup(stream); + m->tx_handles_reserved += h2_beam_get_files_beamed(stream->input); + h2_beam_on_consumed(stream->input, NULL, NULL); + /* Let anyone blocked reading know that there is no more to come */ + h2_beam_abort(stream->input); + /* Remove mutex after, so that abort still finds cond to signal */ + h2_beam_mutex_set(stream->input, NULL, NULL, NULL); + m->tx_handles_reserved += h2_beam_get_files_beamed(stream->output); task = h2_ihash_get(m->tasks, stream->id); if (task) { @@ -459,7 +459,7 @@ static void stream_done(h2_mplx *m, h2_stream *stream, int rst_error) } else { /* already finished */ - task_destroy(m, task, 0); + task_destroy(m, task, 1); } } h2_stream_destroy(stream); @@ -532,12 +532,8 @@ static int task_abort_connection(void *ctx, void *val) if (task->c) { task->c->aborted = 1; } - if (task->input.beam) { - h2_beam_abort(task->input.beam); - } - if (task->output.beam) { - h2_beam_abort(task->output.beam); - } + h2_beam_abort(task->input.beam); + h2_beam_abort(task->output.beam); } return 1; } @@ -772,11 +768,9 @@ static apr_status_t out_close(h2_mplx *m, h2_task *task) ap_log_cerror(APLOG_MARK, APLOG_TRACE2, status, m->c, "h2_mplx(%s): close", task->id); - if (task->output.beam) { - status = h2_beam_close(task->output.beam); - h2_beam_log(task->output.beam, task->stream_id, "out_close", m->c, - APLOG_TRACE2); - } + status = h2_beam_close(task->output.beam); + h2_beam_log(task->output.beam, task->stream_id, "out_close", m->c, + APLOG_TRACE2); output_consumed_signal(m, task); have_out_data_for(m, stream, 0); return status; @@ -897,7 +891,7 @@ static h2_task *next_stream_task(h2_mplx *m) slave = *pslave; } else { - slave = h2_slave_create(m->c, stream->id, m->pool, NULL); + slave = h2_slave_create(m->c, stream->id, m->pool); new_conn = 1; } @@ -919,16 +913,13 @@ static h2_task *next_stream_task(h2_mplx *m) m->max_stream_started = sid; } - if (stream->input) { - h2_beam_timeout_set(stream->input, m->stream_timeout); - h2_beam_on_consumed(stream->input, stream_input_consumed, m); - h2_beam_on_file_beam(stream->input, can_beam_file, m); - h2_beam_mutex_set(stream->input, beam_enter, task->cond, m); - } - if (stream->output) { - h2_beam_buffer_size_set(stream->output, m->stream_max_mem); - h2_beam_timeout_set(stream->output, m->stream_timeout); - } + h2_beam_timeout_set(stream->input, m->stream_timeout); + h2_beam_on_consumed(stream->input, stream_input_consumed, m); + h2_beam_on_file_beam(stream->input, can_beam_file, m); + h2_beam_mutex_set(stream->input, beam_enter, task->cond, m); + + h2_beam_buffer_size_set(stream->output, m->stream_max_mem); + h2_beam_timeout_set(stream->output, m->stream_timeout); ++m->workers_busy; } } @@ -977,10 +968,8 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) if (ngn) { apr_off_t bytes = 0; - if (task->output.beam) { - h2_beam_send(task->output.beam, NULL, APR_NONBLOCK_READ); - bytes += h2_beam_get_buffered(task->output.beam); - } + h2_beam_send(task->output.beam, NULL, APR_NONBLOCK_READ); + bytes += h2_beam_get_buffered(task->output.beam); if (bytes > 0) { /* we need to report consumed and current buffered output * to the engine. The request will be streamed out or cancelled, @@ -1001,7 +990,7 @@ static void task_done(h2_mplx *m, h2_task *task, h2_req_engine *ngn) } stream = h2_ihash_get(m->streams, task->stream_id); - if (!m->aborted && stream && m->redo_tasks + if (!m->aborted && stream && h2_ihash_get(m->redo_tasks, task->stream_id)) { /* reset and schedule again */ h2_task_redo(task); @@ -1152,9 +1141,6 @@ static apr_status_t unschedule_slow_tasks(h2_mplx *m) h2_task *task; int n; - if (!m->redo_tasks) { - m->redo_tasks = h2_ihash_create(m->pool, offsetof(h2_task, stream_id)); - } /* Try to get rid of streams that occupy workers. Look for safe requests * that are repeatable. If none found, fail the connection. */ diff --git a/modules/http2/h2_ngn_shed.c b/modules/http2/h2_ngn_shed.c index 2f5b729617..45329102f7 100644 --- a/modules/http2/h2_ngn_shed.c +++ b/modules/http2/h2_ngn_shed.c @@ -72,7 +72,7 @@ struct h2_req_engine { const char *type; /* name of the engine type */ apr_pool_t *pool; /* pool for engine specific allocations */ conn_rec *c; /* connection this engine is assigned to */ - h2_task *task; /* the task this engine is base on, running in */ + h2_task *task; /* the task this engine is based on, running in */ h2_ngn_shed *shed; unsigned int shutdown : 1; /* engine is being shut down */ diff --git a/modules/http2/h2_session.c b/modules/http2/h2_session.c index 27ed9197b5..f743f4814c 100644 --- a/modules/http2/h2_session.c +++ b/modules/http2/h2_session.c @@ -82,7 +82,6 @@ apr_status_t h2_session_stream_done(h2_session *session, h2_stream *stream) ap_log_cerror(APLOG_MARK, APLOG_TRACE2, 0, session->c, "h2_stream(%ld-%d): EOS bucket cleanup -> done", session->id, stream->id); - h2_ihash_remove(session->streams, stream->id); h2_mplx_stream_done(session->mplx, stream); dispatch_event(session, H2_SESSION_EV_STREAM_DONE, 0, NULL); @@ -94,10 +93,9 @@ typedef struct stream_sel_ctx { h2_stream *candidate; } stream_sel_ctx; -static int find_cleanup_stream(void *udata, void *sdata) +static int find_cleanup_stream(h2_stream *stream, void *ictx) { - stream_sel_ctx *ctx = udata; - h2_stream *stream = sdata; + stream_sel_ctx *ctx = ictx; if (H2_STREAM_CLIENT_INITIATED(stream->id)) { if (!ctx->session->local.accepting && stream->id > ctx->session->local.accepted_max) { @@ -121,7 +119,7 @@ static void cleanup_streams(h2_session *session) ctx.session = session; ctx.candidate = NULL; while (1) { - h2_ihash_iter(session->streams, find_cleanup_stream, &ctx); + h2_mplx_stream_do(session->mplx, find_cleanup_stream, &ctx); if (ctx.candidate) { h2_session_stream_done(session, ctx.candidate); ctx.candidate = NULL; @@ -144,7 +142,6 @@ h2_stream *h2_session_open_stream(h2_session *session, int stream_id, stream = h2_stream_open(stream_id, stream_pool, session, initiated_on); nghttp2_session_set_stream_user_data(session->ngh2, stream_id, stream); - h2_ihash_add(session->streams, stream); if (req) { h2_stream_set_request(stream, req); @@ -713,7 +710,6 @@ static void h2_session_destroy(h2_session *session) { ap_assert(session); - h2_ihash_clear(session->streams); if (session->mplx) { h2_mplx_set_consumed_cb(session->mplx, NULL, NULL); h2_mplx_release_and_join(session->mplx, session->iowait); @@ -927,8 +923,6 @@ static h2_session *h2_session_create_int(conn_rec *c, return NULL; } - session->streams = h2_ihash_create(session->pool, - offsetof(h2_stream, id)); session->mplx = h2_mplx_create(c, session->pool, session->config, session->s->timeout, workers); diff --git a/modules/http2/h2_session.h b/modules/http2/h2_session.h index 4d21cb86f5..69b7a59c44 100644 --- a/modules/http2/h2_session.h +++ b/modules/http2/h2_session.h @@ -86,7 +86,6 @@ typedef struct h2_session { struct h2_workers *workers; /* for executing stream tasks */ struct h2_filter_cin *cin; /* connection input filter context */ h2_conn_io io; /* io on httpd conn filters */ - struct h2_ihash_t *streams; /* streams handled by this session */ struct nghttp2_session *ngh2; /* the nghttp2 session (internal use) */ h2_session_state state; /* state session is in */ diff --git a/modules/http2/h2_task.c b/modules/http2/h2_task.c index 3f70b3aa21..e5e4efc6a0 100644 --- a/modules/http2/h2_task.c +++ b/modules/http2/h2_task.c @@ -403,7 +403,7 @@ static apr_status_t h2_filter_parse_h1(ap_filter_t* f, apr_bucket_brigade* bb) ******************************************************************************/ int h2_task_can_redo(h2_task *task) { - if (task->input.beam && h2_beam_was_received(task->input.beam)) { + if (h2_beam_was_received(task->input.beam)) { /* cannot repeat that. */ return 0; } @@ -420,10 +420,8 @@ void h2_task_redo(h2_task *task) void h2_task_rst(h2_task *task, int error) { task->rst_error = error; - if (task->input.beam) { - h2_beam_abort(task->input.beam); - } - if (!task->worker_done && task->output.beam) { + h2_beam_abort(task->input.beam); + if (!task->worker_done) { h2_beam_abort(task->output.beam); } if (task->c) { -- 2.40.0