From 43d53e6d86acaf7555c31730a8230fa0cdf31306 Mon Sep 17 00:00:00 2001 From: Nick Mathewson Date: Thu, 21 Mar 2013 14:51:27 -0400 Subject: Implementation of a fix for bug 7912 I added the code to pass a destroy cell to a queueing function rather than writing it immediately, and the code to remember that we shouldn't reuse the circuit id until the destroy is actually sent, and the code to release the circuit id once the destroy has been sent... and then I finished by hooking destroy_cell_queue into the rest of Tor. --- src/or/relay.c | 19 ++++++++++++++----- 1 file changed, 14 insertions(+), 5 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/relay.c b/src/or/relay.c index 0ca3e56fd..ec860269a 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2140,11 +2140,11 @@ cell_queue_append(cell_queue_t *queue, packed_cell_t *cell) /** Append a newly allocated copy of cell to the end of queue */ void cell_queue_append_packed_copy(cell_queue_t *queue, const cell_t *cell, - int wide_circ_ids) + int wide_circ_ids, int use_stats) { packed_cell_t *copy = packed_cell_copy(cell, wide_circ_ids); /* Remember the time when this cell was put in the queue. */ - if (get_options()->CellStatistics) { + if (get_options()->CellStatistics && use_stats) { struct timeval now; uint32_t added; insertion_time_queue_t *it_queue = queue->insertion_times; @@ -2339,7 +2339,7 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) { circuitmux_t *cmux = NULL; int n_flushed = 0; - cell_queue_t *queue; + cell_queue_t *queue, *destroy_queue=NULL; circuit_t *circ; or_circuit_t *or_circ; int streams_blocked; @@ -2352,7 +2352,16 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) /* Main loop: pick a circuit, send a cell, update the cmux */ while (n_flushed < max) { - circ = circuitmux_get_first_active_circuit(cmux); + circ = circuitmux_get_first_active_circuit(cmux, &destroy_queue); + if (destroy_queue) { + /* this code is duplicated from some of the logic below. Ugly! XXXX */ + tor_assert(destroy_queue->n > 0); + cell = cell_queue_pop(destroy_queue); + channel_write_packed_cell(chan, cell); + cell = NULL; + ++n_flushed; + continue; + } /* If it returns NULL, no cells left to send */ if (!circ) break; assert_cmux_ok_paranoid(chan); @@ -2474,7 +2483,7 @@ append_cell_to_circuit_queue(circuit_t *circ, channel_t *chan, streams_blocked = circ->streams_blocked_on_p_chan; } - cell_queue_append_packed_copy(queue, cell, chan->wide_circ_ids); + cell_queue_append_packed_copy(queue, cell, chan->wide_circ_ids, 1); /* If we have too many cells on the circuit, we should stop reading from * the edge streams for a while. */ -- cgit v1.2.3 From 16f9861b22751bc90666fe1836b8cf740630447a Mon Sep 17 00:00:00 2001 From: Andrea Shepard Date: Wed, 12 Jun 2013 22:22:21 -0700 Subject: Add destroy balance tracking and logging to circuitmux --- src/or/channel.c | 1 - src/or/circuitlist.c | 5 +++-- src/or/circuitmux.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++++++++ src/or/circuitmux.h | 1 + src/or/relay.c | 2 ++ 5 files changed, 61 insertions(+), 3 deletions(-) (limited to 'src/or/relay.c') diff --git a/src/or/channel.c b/src/or/channel.c index e327bda51..33a32102e 100644 --- a/src/or/channel.c +++ b/src/or/channel.c @@ -2652,7 +2652,6 @@ is_destroy_cell(channel_t *chan, return 0; } - /** * Send destroy cell on a channel * diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index deb45b7b6..e50ab603e 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -1717,8 +1717,9 @@ assert_circuit_ok(const circuit_t *c) if (or_circ && or_circ->p_chan) { if (or_circ->p_circ_id) { /* ibid */ - circuit_t *c2 = circuit_get_by_circid_channel_impl(or_circ->p_circ_id, - or_circ->p_chan, NULL); + circuit_t *c2 = + circuit_get_by_circid_channel_impl(or_circ->p_circ_id, + or_circ->p_chan, NULL); tor_assert(c == c2); } } diff --git a/src/or/circuitmux.c b/src/or/circuitmux.c index 198e518bd..a6256f804 100644 --- a/src/or/circuitmux.c +++ b/src/or/circuitmux.c @@ -127,6 +127,10 @@ struct circuitmux_s { * cells completely. */ unsigned int last_cell_was_destroy : 1; + /** Destroy counter: increment this when a destroy gets queued, decrement + * when we unqueue it, so we can test to make sure they don't starve. + */ + int64_t destroy_ctr; /* * Circuitmux policy; if this is non-NULL, it can override the built- @@ -206,6 +210,11 @@ static void circuitmux_assert_okay_pass_one(circuitmux_t *cmux); static void circuitmux_assert_okay_pass_two(circuitmux_t *cmux); static void circuitmux_assert_okay_pass_three(circuitmux_t *cmux); +/* Static global variables */ + +/** Count the destroy balance to debug destroy queue logic */ +static int64_t global_destroy_ctr = 0; + /* Function definitions */ /** @@ -521,6 +530,25 @@ circuitmux_free(circuitmux_t *cmux) tor_free(cmux->chanid_circid_map); } + /* + * We're throwing away some destroys; log the counter and + * adjust the global counter by the queue size. + */ + if (cmux->destroy_cell_queue.n > 0) { + cmux->destroy_ctr -= cmux->destroy_cell_queue.n; + global_destroy_ctr -= cmux->destroy_cell_queue.n; + log_debug(LD_CIRC, + "Freeing cmux at %p with %u queued destroys; the last cmux " + "destroy balance was %ld, global is %ld\n", + cmux, cmux->destroy_cell_queue.n, + cmux->destroy_ctr, global_destroy_ctr); + } else { + log_debug(LD_CIRC, + "Freeing cmux at %p with no queued destroys, the cmux destroy " + "balance was %ld, global is %ld\n", + cmux, cmux->destroy_ctr, global_destroy_ctr); + } + cell_queue_clear(&cmux->destroy_cell_queue); tor_free(cmux); @@ -1502,6 +1530,24 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, circuitmux_assert_okay_paranoid(cmux); } +/** + * Notify the circuitmux that a destroy was sent, so we can update + * the counter. + */ + +void +circuitmux_notify_xmit_destroy(circuitmux_t *cmux) +{ + tor_assert(cmux); + + --(cmux->destroy_ctr); + --(global_destroy_ctr); + log_debug(LD_CIRC, + "Cmux at %p sent a destroy, cmux counter is now %ld, " + "global counter is now %ld\n", + cmux, cmux->destroy_ctr, global_destroy_ctr); +} + /* * Circuitmux consistency checking assertions */ @@ -1798,6 +1844,14 @@ circuitmux_append_destroy_cell(channel_t *chan, cell_queue_append_packed_copy(&cmux->destroy_cell_queue, &cell, chan->wide_circ_ids, 0); + /* Destroy entering the queue, update counters */ + ++(cmux->destroy_ctr); + ++global_destroy_ctr; + log_debug(LD_CIRC, + "Cmux at %p queued a destroy for circ %u, " + "cmux counter is now %ld, global counter is now %ld\n", + cmux, circ_id, cmux->destroy_ctr, global_destroy_ctr); + /* XXXX Duplicate code from append_cell_to_circuit_queue */ if (!channel_has_queued_writes(chan)) { /* There is no data at all waiting to be sent on the outbuf. Add a @@ -1808,3 +1862,4 @@ circuitmux_append_destroy_cell(channel_t *chan, channel_flush_from_first_active_circuit(chan, 1); } } + diff --git a/src/or/circuitmux.h b/src/or/circuitmux.h index da62196b2..9ff29de70 100644 --- a/src/or/circuitmux.h +++ b/src/or/circuitmux.h @@ -124,6 +124,7 @@ circuit_t * circuitmux_get_first_active_circuit(circuitmux_t *cmux, cell_queue_t **destroy_queue_out); void circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ, unsigned int n_cells); +void circuitmux_notify_xmit_destroy(circuitmux_t *cmux); /* Circuit interface */ void circuitmux_attach_circuit(circuitmux_t *cmux, circuit_t *circ, diff --git a/src/or/relay.c b/src/or/relay.c index ec860269a..46bfc442b 100644 --- a/src/or/relay.c +++ b/src/or/relay.c @@ -2358,6 +2358,8 @@ channel_flush_from_first_active_circuit(channel_t *chan, int max) tor_assert(destroy_queue->n > 0); cell = cell_queue_pop(destroy_queue); channel_write_packed_cell(chan, cell); + /* Update the cmux destroy counter */ + circuitmux_notify_xmit_destroy(cmux); cell = NULL; ++n_flushed; continue; -- cgit v1.2.3