From 79515917449c7e0d92f16db0d1e5af4a0370bbab Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Fri, 3 Jan 2014 13:56:46 -0500 Subject: [PATCH 1/2] Fix bugs in bug10169 bugfix memory tracking The chunk_grow() and chunk_copy() functions weren't adjusting the memory totals properly. Bugfix not on any released Tor version. --- src/or/buffers.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 71b61b1890..856c9b6060 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -245,12 +245,13 @@ static INLINE chunk_t * chunk_grow(chunk_t *chunk, size_t sz) { off_t offset; + size_t memlen_orig = chunk->memlen; tor_assert(sz > chunk->memlen); offset = chunk->data - chunk->mem; chunk = tor_realloc(chunk, CHUNK_ALLOC_SIZE(sz)); chunk->memlen = sz; chunk->data = chunk->mem + offset; - total_bytes_allocated_in_chunks += (sz - chunk->memlen); + total_bytes_allocated_in_chunks += CHUNK_ALLOC_SIZE(sz) - CHUNK_ALLOC_SIZE(memlen_orig); return chunk; } @@ -331,10 +332,11 @@ buf_shrink_freelists(int free_all) } // tor_assert(!n_to_free); freelists[i].cur_length = new_length; + tor_assert(orig_n_to_skip == new_length); log_info(LD_MM, "Cleaned freelist for %d-byte chunks: original " - "length %d, kept %d, dropped %d.", + "length %d, kept %d, dropped %d. New length is %d", (int)freelists[i].alloc_size, orig_length, - orig_n_to_skip, orig_n_to_free); + orig_n_to_skip, orig_n_to_free, new_length); } freelists[i].lowest_length = freelists[i].cur_length; assert_freelist_ok(&freelists[i]); @@ -580,6 +582,7 @@ static chunk_t * chunk_copy(const chunk_t *in_chunk) { chunk_t *newch = tor_memdup(in_chunk, CHUNK_ALLOC_SIZE(in_chunk->memlen)); + total_bytes_allocated_in_chunks += CHUNK_ALLOC_SIZE(in_chunk->memlen); newch->next = NULL; if (in_chunk->data) { off_t offset = in_chunk->data - in_chunk->mem; From fd28754dd3dce0e00304825d531348414c0a354b Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Sun, 12 Jan 2014 12:26:45 -0500 Subject: [PATCH 2/2] Actually release buffer freelists when handling OOM conditions. Otherwise freeing buffers won't help for a little while. --- src/or/buffers.c | 9 +++++++-- src/or/buffers.h | 2 +- src/or/circuitlist.c | 13 +++++++++++++ 3 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/or/buffers.c b/src/or/buffers.c index 856c9b6060..352e60a979 100644 --- a/src/or/buffers.c +++ b/src/or/buffers.c @@ -276,12 +276,14 @@ preferred_chunk_size(size_t target) } /** Remove from the freelists most chunks that have not been used since the - * last call to buf_shrink_freelists(). */ -void + * last call to buf_shrink_freelists(). Return the amount of memory + * freed. */ +size_t buf_shrink_freelists(int free_all) { #ifdef ENABLE_BUF_FREELISTS int i; + size_t total_freed = 0; disable_control_logging(); for (i = 0; freelists[i].alloc_size; ++i) { int slack = freelists[i].slack; @@ -315,6 +317,7 @@ buf_shrink_freelists(int free_all) chunk_t *next = chunk->next; tor_assert(total_bytes_allocated_in_chunks >= CHUNK_ALLOC_SIZE(chunk->memlen)); total_bytes_allocated_in_chunks -= CHUNK_ALLOC_SIZE(chunk->memlen); + total_freed += CHUNK_ALLOC_SIZE(chunk->memlen); tor_free(chunk); chunk = next; --n_to_free; @@ -343,8 +346,10 @@ buf_shrink_freelists(int free_all) } done: enable_control_logging(); + return total_freed; #else (void) free_all; + return 0; #endif } diff --git a/src/or/buffers.h b/src/or/buffers.h index 677d68d83d..b3b9003492 100644 --- a/src/or/buffers.h +++ b/src/or/buffers.h @@ -18,7 +18,7 @@ void buf_free(buf_t *buf); void buf_clear(buf_t *buf); buf_t *buf_copy(const buf_t *buf); void buf_shrink(buf_t *buf); -void buf_shrink_freelists(int free_all); +size_t buf_shrink_freelists(int free_all); void buf_dump_freelist_sizes(int severity); size_t buf_datalen(const buf_t *buf); diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 2e135416c2..ffb5e0cd0e 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1532,6 +1532,17 @@ circuits_handle_oom(size_t current_allocation) "over-long queues. (This behavior is controlled by " "MaxMemInQueues.)"); + { + const size_t recovered = buf_shrink_freelists(1); + if (recovered >= current_allocation) { + log_warn(LD_BUG, "We somehow recovered more memory from freelists " + "than we thought we had allocated"); + current_allocation = 0; + } else { + current_allocation -= recovered; + } + } + { size_t mem_target = (size_t)(get_options()->MaxMemInQueues * FRACTION_OF_DATA_TO_RETAIN_ON_OOM); @@ -1575,6 +1586,8 @@ circuits_handle_oom(size_t current_allocation) } SMARTLIST_FOREACH_END(circ); clean_cell_pool(); /* In case this helps. */ + buf_shrink_freelists(1); /* This is necessary to actually release buffer + chunks. */ log_notice(LD_GENERAL, "Removed "U64_FORMAT" bytes by killing %d circuits.", U64_PRINTF_ARG(mem_recovered),