sarah
/
tor
1
0
Fork 0
Code Issues Pull Requests Releases Wiki Activity

cmux: Remove round-robin circuit policy 

Since 0.2.4, tor uses EWMA circuit policy to prioritize. The previous
algorithm, round-robin, hasn't been used since then but was still used as a
fallback.

Now that EWMA is mandatory, remove that code entirely and enforce a cmux
policy to be set.

This is part of a circuitmux cleanup to improve performance and reduce
complexity in the code. We'll be able to address future optimization with this
work.

Closes #25268

Signed-off-by: David Goulet <dgoulet@torproject.org>
This commit is contained in:
David Goulet 2018-02-15 14:23:36 -05:00
parent 9d68647ba3
commit c235c32bbc
5 changed files with 26 additions and 339 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/or/circuitlist.c
View File

@ -403,9 +403,6 @@ circuit_set_p_circid_chan(or_circuit_t *or_circ, circid_t id,
circuit_set_circid_chan_helper(circ, CELL_DIRECTION_IN, id, chan);
if (chan) {
tor_assert(bool_eq(or_circ->p_chan_cells.n,
or_circ->next_active_on_p_chan));
chan->timestamp_last_had_circuits = approx_time();
}
@ -428,8 +425,6 @@ circuit_set_n_circid_chan(circuit_t *circ, circid_t id,
circuit_set_circid_chan_helper(circ, CELL_DIRECTION_OUT, id, chan);
if (chan) {
tor_assert(bool_eq(circ->n_chan_cells.n, circ->next_active_on_n_chan));
chan->timestamp_last_had_circuits = approx_time();
}

338
src/or/circuitmux.c
View File

@ -114,13 +114,6 @@ struct circuitmux_s {
*/
chanid_circid_muxinfo_map_t *chanid_circid_map;
/*
* Double-linked ring of circuits with queued cells waiting for room to
* free up on this connection's outbuf. Every time we pull cells from
* a circuit, we advance this pointer to the next circuit in the ring.
*/
struct circuit_t *active_circuits_head, *active_circuits_tail;
/** List of queued destroy cells */
destroy_cell_queue_t destroy_cell_queue;
/** Boolean: True iff the last cell to circuitmux_get_first_active_circuit
@ -188,18 +181,9 @@ chanid_circid_entry_hash(chanid_circid_muxinfo_t *a);
static chanid_circid_muxinfo_t *
circuitmux_find_map_entry(circuitmux_t *cmux, circuit_t *circ);
static void
circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction);
circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ);
static void
circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction);
static inline void
circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction);
static inline circuit_t **
circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ);
static inline circuit_t **
circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ);
circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ);
/* Static global variables */
@ -208,115 +192,6 @@ static int64_t global_destroy_ctr = 0;
/* Function definitions */
/**
* Linked list helpers
*/
/**
* Move an active circuit to the tail of the cmux's active circuits list;
* used by circuitmux_notify_xmit_cells().
*/
static inline void
circuitmux_move_active_circ_to_tail(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction)
{
circuit_t **next_p = NULL, **prev_p = NULL;
circuit_t **next_prev = NULL, **prev_next = NULL;
circuit_t **tail_next = NULL;
or_circuit_t *or_circ = NULL;
tor_assert(cmux);
tor_assert(circ);
/* Figure out our next_p and prev_p for this cmux/direction */
if (direction) {
if (direction == CELL_DIRECTION_OUT) {
tor_assert(circ->n_mux == cmux);
next_p = &(circ->next_active_on_n_chan);
prev_p = &(circ->prev_active_on_n_chan);
} else {
or_circ = TO_OR_CIRCUIT(circ);
tor_assert(or_circ->p_mux == cmux);
next_p = &(or_circ->next_active_on_p_chan);
prev_p = &(or_circ->prev_active_on_p_chan);
}
} else {
if (circ->n_mux == cmux) {
next_p = &(circ->next_active_on_n_chan);
prev_p = &(circ->prev_active_on_n_chan);
} else {
or_circ = TO_OR_CIRCUIT(circ);
tor_assert(or_circ->p_mux == cmux);
next_p = &(or_circ->next_active_on_p_chan);
prev_p = &(or_circ->prev_active_on_p_chan);
}
}
tor_assert(next_p);
tor_assert(prev_p);
/* Check if this really is an active circuit */
if ((*next_p == NULL && *prev_p == NULL) &&
!(circ == cmux->active_circuits_head ||
circ == cmux->active_circuits_tail)) {
/* Not active, no-op */
return;
}
/* Check if this is already the tail */
if (circ == cmux->active_circuits_tail) return;
/* Okay, we have to move it; figure out next_prev and prev_next */
if (*next_p) next_prev = circuitmux_prev_active_circ_p(cmux, *next_p);
if (*prev_p) prev_next = circuitmux_next_active_circ_p(cmux, *prev_p);
/* Adjust the previous node's next pointer, if any */
if (prev_next) *prev_next = *next_p;
/* Otherwise, we were the head */
else cmux->active_circuits_head = *next_p;
/* Adjust the next node's previous pointer, if any */
if (next_prev) *next_prev = *prev_p;
/* We're out of the list; now re-attach at the tail */
/* Adjust our next and prev pointers */
*next_p = NULL;
*prev_p = cmux->active_circuits_tail;
/* Set the next pointer of the tail, or the head if none */
if (cmux->active_circuits_tail) {
tail_next = circuitmux_next_active_circ_p(cmux,
cmux->active_circuits_tail);
*tail_next = circ;
} else {
cmux->active_circuits_head = circ;
}
/* Set the tail to this circuit */
cmux->active_circuits_tail = circ;
}
static inline circuit_t **
circuitmux_next_active_circ_p(circuitmux_t *cmux, circuit_t *circ)
{
tor_assert(cmux);
tor_assert(circ);
if (circ->n_mux == cmux) return &(circ->next_active_on_n_chan);
else {
tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux);
return &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan);
}
}
static inline circuit_t **
circuitmux_prev_active_circ_p(circuitmux_t *cmux, circuit_t *circ)
{
tor_assert(cmux);
tor_assert(circ);
if (circ->n_mux == cmux) return &(circ->prev_active_on_n_chan);
else {
tor_assert(TO_OR_CIRCUIT(circ)->p_mux == cmux);
return &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan);
}
}
/**
* Helper for chanid_circid_cell_count_map_t hash table: compare the channel
* ID and circuit ID for a and b, and return less than, equal to, or greater
@ -412,7 +287,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out)
*/
if (to_remove->muxinfo.cell_count > 0) {
circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_OUT);
circuitmux_make_circuit_inactive(cmux, circ);
}
/* Clear n_mux */
@ -427,7 +302,7 @@ circuitmux_detach_all_circuits(circuitmux_t *cmux, smartlist_t *detached_out)
*/
if (to_remove->muxinfo.cell_count > 0) {
circuitmux_make_circuit_inactive(cmux, circ, CELL_DIRECTION_IN);
circuitmux_make_circuit_inactive(cmux, circ);
}
/*
@ -978,10 +853,10 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ,
*/
if (hashent->muxinfo.cell_count > 0 && cell_count == 0) {
--(cmux->n_active_circuits);
circuitmux_make_circuit_inactive(cmux, circ, direction);
circuitmux_make_circuit_inactive(cmux, circ);
} else if (hashent->muxinfo.cell_count == 0 && cell_count > 0) {
++(cmux->n_active_circuits);
circuitmux_make_circuit_active(cmux, circ, direction);
circuitmux_make_circuit_active(cmux, circ);
}
cmux->n_cells -= hashent->muxinfo.cell_count;
cmux->n_cells += cell_count;
@ -1029,20 +904,11 @@ circuitmux_attach_circuit,(circuitmux_t *cmux, circuit_t *circ,
if (direction == CELL_DIRECTION_OUT) circ->n_mux = cmux;
else TO_OR_CIRCUIT(circ)->p_mux = cmux;
/* Make sure the next/prev pointers are NULL */
if (direction == CELL_DIRECTION_OUT) {
circ->next_active_on_n_chan = NULL;
circ->prev_active_on_n_chan = NULL;
} else {
TO_OR_CIRCUIT(circ)->next_active_on_p_chan = NULL;
TO_OR_CIRCUIT(circ)->prev_active_on_p_chan = NULL;
}
/* Update counters */
++(cmux->n_circuits);
if (cell_count > 0) {
++(cmux->n_active_circuits);
circuitmux_make_circuit_active(cmux, circ, direction);
circuitmux_make_circuit_active(cmux, circ);
}
cmux->n_cells += cell_count;
}
@ -1106,7 +972,7 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ))
if (hashent->muxinfo.cell_count > 0) {
--(cmux->n_active_circuits);
/* This does policy notifies, so comes before freeing policy data */
circuitmux_make_circuit_inactive(cmux, circ, last_searched_direction);
circuitmux_make_circuit_inactive(cmux, circ);
}
cmux->n_cells -= hashent->muxinfo.cell_count;
@ -1143,81 +1009,16 @@ circuitmux_detach_circuit,(circuitmux_t *cmux, circuit_t *circ))
*/
static void
circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction)
circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ)
{
circuit_t **next_active = NULL, **prev_active = NULL, **next_prev = NULL;
circuitmux_t *circuit_cmux = NULL;
chanid_circid_muxinfo_t *hashent = NULL;
channel_t *chan = NULL;
circid_t circ_id;
int already_active;
tor_assert(cmux);
tor_assert(cmux->policy);
tor_assert(circ);
tor_assert(direction == CELL_DIRECTION_OUT ||
direction == CELL_DIRECTION_IN);
/* Get the right set of active list links for this direction */
if (direction == CELL_DIRECTION_OUT) {
next_active = &(circ->next_active_on_n_chan);
prev_active = &(circ->prev_active_on_n_chan);
circuit_cmux = circ->n_mux;
chan = circ->n_chan;
circ_id = circ->n_circ_id;
} else {
next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan);
prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan);
circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux;
chan = TO_OR_CIRCUIT(circ)->p_chan;
circ_id = TO_OR_CIRCUIT(circ)->p_circ_id;
}
/* Assert that it is attached to this mux and a channel */
tor_assert(cmux == circuit_cmux);
tor_assert(chan != NULL);
/*
* Check if the circuit really was inactive; if it's active, at least one
* of the next_active and prev_active pointers will not be NULL, or this
* circuit will be either the head or tail of the list for this cmux.
*/
already_active = (*prev_active != NULL || *next_active != NULL ||
cmux->active_circuits_head == circ ||
cmux->active_circuits_tail == circ);
/* If we're already active, log a warning and finish */
if (already_active) {
log_warn(LD_CIRC,
"Circuit %u on channel " U64_FORMAT " was already active",
(unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier));
return;
}
/*
* This is going at the head of the list; if the old head is not NULL,
* then its prev pointer should point to this.
*/
*next_active = cmux->active_circuits_head; /* Next is old head */
*prev_active = NULL; /* Prev is NULL (this will be the head) */
if (cmux->active_circuits_head) {
/* The list had an old head; update its prev pointer */
next_prev =
circuitmux_prev_active_circ_p(cmux, cmux->active_circuits_head);
tor_assert(next_prev);
*next_prev = circ;
} else {
/* The list was empty; this becomes the tail as well */
cmux->active_circuits_tail = circ;
}
/* This becomes the new head of the list */
cmux->active_circuits_head = circ;
/* Policy-specific notification */
if (cmux->policy &&
cmux->policy->notify_circ_active) {
if (cmux->policy->notify_circ_active) {
/* Okay, we need to check the circuit for policy data now */
hashent = circuitmux_find_map_entry(cmux, circ);
chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ);
/* We should have found something */
tor_assert(hashent);
/* Notify */
@ -1232,99 +1033,16 @@ circuitmux_make_circuit_active(circuitmux_t *cmux, circuit_t *circ,
*/
static void
circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ,
cell_direction_t direction)
circuitmux_make_circuit_inactive(circuitmux_t *cmux, circuit_t *circ)
{
circuit_t **next_active = NULL, **prev_active = NULL;
circuit_t **next_prev = NULL, **prev_next = NULL;
circuitmux_t *circuit_cmux = NULL;
chanid_circid_muxinfo_t *hashent = NULL;
channel_t *chan = NULL;
circid_t circ_id;
int already_inactive;
tor_assert(cmux);
tor_assert(cmux->policy);
tor_assert(circ);
tor_assert(direction == CELL_DIRECTION_OUT ||
direction == CELL_DIRECTION_IN);
/* Get the right set of active list links for this direction */
if (direction == CELL_DIRECTION_OUT) {
next_active = &(circ->next_active_on_n_chan);
prev_active = &(circ->prev_active_on_n_chan);
circuit_cmux = circ->n_mux;
chan = circ->n_chan;
circ_id = circ->n_circ_id;
} else {
next_active = &(TO_OR_CIRCUIT(circ)->next_active_on_p_chan);
prev_active = &(TO_OR_CIRCUIT(circ)->prev_active_on_p_chan);
circuit_cmux = TO_OR_CIRCUIT(circ)->p_mux;
chan = TO_OR_CIRCUIT(circ)->p_chan;
circ_id = TO_OR_CIRCUIT(circ)->p_circ_id;
}
/* Assert that it is attached to this mux and a channel */
tor_assert(cmux == circuit_cmux);
tor_assert(chan != NULL);
/*
* Check if the circuit really was active; if it's inactive, the
* next_active and prev_active pointers will be NULL and this circuit
* will not be the head or tail of the list for this cmux.
*/
already_inactive = (*prev_active == NULL && *next_active == NULL &&
cmux->active_circuits_head != circ &&
cmux->active_circuits_tail != circ);
/* If we're already inactive, log a warning and finish */
if (already_inactive) {
log_warn(LD_CIRC,
"Circuit %d on channel " U64_FORMAT " was already inactive",
(unsigned)circ_id, U64_PRINTF_ARG(chan->global_identifier));
return;
}
/* Remove from the list; first get next_prev and prev_next */
if (*next_active) {
/*
* If there's a next circuit, its previous circuit becomes this
* circuit's previous circuit.
*/
next_prev = circuitmux_prev_active_circ_p(cmux, *next_active);
} else {
/* Else, the tail becomes this circuit's previous circuit */
next_prev = &(cmux->active_circuits_tail);
}
/* Got next_prev, now prev_next */
if (*prev_active) {
/*
* If there's a previous circuit, its next circuit becomes this circuit's
* next circuit.
*/
prev_next = circuitmux_next_active_circ_p(cmux, *prev_active);
} else {
/* Else, the head becomes this circuit's next circuit */
prev_next = &(cmux->active_circuits_head);
}
/* Assert that we got sensible values for the next/prev pointers */
tor_assert(next_prev != NULL);
tor_assert(prev_next != NULL);
/* Update the next/prev pointers - this removes circ from the list */
*next_prev = *prev_active;
*prev_next = *next_active;
/* Now null out prev_active/next_active */
*prev_active = NULL;
*next_active = NULL;
/* Policy-specific notification */
if (cmux->policy &&
cmux->policy->notify_circ_inactive) {
if (cmux->policy->notify_circ_inactive) {
/* Okay, we need to check the circuit for policy data now */
hashent = circuitmux_find_map_entry(cmux, circ);
chanid_circid_muxinfo_t *hashent = circuitmux_find_map_entry(cmux, circ);
/* We should have found something */
tor_assert(hashent);
/* Notify */
@ -1383,12 +1101,12 @@ circuitmux_set_num_cells(circuitmux_t *cmux, circuit_t *circ,
if (hashent->muxinfo.cell_count > 0 && n_cells == 0) {
--(cmux->n_active_circuits);
hashent->muxinfo.cell_count = n_cells;
circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction);
circuitmux_make_circuit_inactive(cmux, circ);
/* Is the old cell count == 0 and the new cell count > 0 ? */
} else if (hashent->muxinfo.cell_count == 0 && n_cells > 0) {
++(cmux->n_active_circuits);
hashent->muxinfo.cell_count = n_cells;
circuitmux_make_circuit_active(cmux, circ, hashent->muxinfo.direction);
circuitmux_make_circuit_active(cmux, circ);
} else {
hashent->muxinfo.cell_count = n_cells;
}
@ -1417,6 +1135,9 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux,
circuit_t *circ = NULL;
tor_assert(cmux);
tor_assert(cmux->policy);
/* This callback is mandatory. */
tor_assert(cmux->policy->pick_active_circuit);
tor_assert(destroy_queue_out);
*destroy_queue_out = NULL;
@ -1435,14 +1156,7 @@ circuitmux_get_first_active_circuit(circuitmux_t *cmux,
/* We also must have a cell available for this to be the case */
tor_assert(cmux->n_cells > 0);
/* Do we have a policy-provided circuit selector? */
if (cmux->policy && cmux->policy->pick_active_circuit) {
circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data);
}
/* Fall back on the head of the active circuits list */
if (!circ) {
tor_assert(cmux->active_circuits_head);
circ = cmux->active_circuits_head;
}
circ = cmux->policy->pick_active_circuit(cmux, cmux->policy_data);
cmux->last_cell_was_destroy = 0;
} else {
tor_assert(cmux->n_cells == 0);
@ -1492,12 +1206,6 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ,
/* Adjust the mux cell counter */
cmux->n_cells -= n_cells;
/* If we aren't making it inactive later, move it to the tail of the list */
if (!becomes_inactive) {
circuitmux_move_active_circ_to_tail(cmux, circ,
hashent->muxinfo.direction);
}
/*
* We call notify_xmit_cells() before making the circuit inactive if needed,
* so the policy can always count on this coming in on an active circuit.
@ -1514,7 +1222,7 @@ circuitmux_notify_xmit_cells(circuitmux_t *cmux, circuit_t *circ,
*/
if (becomes_inactive) {
--(cmux->n_active_circuits);
circuitmux_make_circuit_inactive(cmux, circ, hashent->muxinfo.direction);
circuitmux_make_circuit_inactive(cmux, circ);
}
}

17
src/or/or.h
View File

@ -3179,15 +3179,6 @@ typedef struct circuit_t {
/** Index in smartlist of all circuits (global_circuitlist). */
int global_circuitlist_idx;
/** Next circuit in the doubly-linked ring of circuits waiting to add
* cells to n_conn. NULL if we have no cells pending, or if we're not
* linked to an OR connection. */
struct circuit_t *next_active_on_n_chan;
/** Previous circuit in the doubly-linked ring of circuits waiting to add
* cells to n_conn. NULL if we have no cells pending, or if we're not
* linked to an OR connection. */
struct circuit_t *prev_active_on_n_chan;
/** Various statistics about cells being added to or removed from this
* circuit's queues; used only if CELL_STATS events are enabled and
* cleared after being sent to control port. */
@ -3467,14 +3458,6 @@ struct onion_queue_t;
typedef struct or_circuit_t {
circuit_t base_;
/** Next circuit in the doubly-linked ring of circuits waiting to add
* cells to p_chan. NULL if we have no cells pending, or if we're not
* linked to an OR connection. */
struct circuit_t *next_active_on_p_chan;
/** Previous circuit in the doubly-linked ring of circuits waiting to add
* cells to p_chan. NULL if we have no cells pending, or if we're not
* linked to an OR connection. */
struct circuit_t *prev_active_on_p_chan;
/** Pointer to an entry on the onion queue, if this circuit is waiting for a
* chance to give an onionskin to a cpuworker. Used only in onion.c */
struct onion_queue_t *onionqueue_entry;

3
src/test/test_channel.c
View File

@ -281,6 +281,7 @@ new_fake_channel(void)
chan->state = CHANNEL_STATE_OPEN;
chan->cmux = circuitmux_alloc();
circuitmux_set_policy(chan->cmux, &ewma_policy);
return chan;
}
@ -582,8 +583,6 @@ test_channel_outbound_cell(void *arg)
* to the channel's cmux as well. */
circuit_set_n_circid_chan(TO_CIRCUIT(circ), 42, chan);
tt_int_op(channel_num_circuits(chan), OP_EQ, 1);
tt_assert(!TO_CIRCUIT(circ)->next_active_on_n_chan);
tt_assert(!TO_CIRCUIT(circ)->prev_active_on_n_chan);
/* Test the cmux state. */
tt_ptr_op(TO_CIRCUIT(circ)->n_mux, OP_EQ, chan->cmux);
tt_int_op(circuitmux_is_circuit_attached(chan->cmux, TO_CIRCUIT(circ)),

2
src/test/test_circuitmux.c
View File

@ -7,6 +7,7 @@
#include "or.h"
#include "channel.h"
#include "circuitmux.h"
#include "circuitmux_ewma.h"
#include "relay.h"
#include "scheduler.h"
#include "test.h"
@ -45,6 +46,7 @@ test_cmux_destroy_cell_queue(void *arg)
cmux = circuitmux_alloc();
tt_assert(cmux);
ch = new_fake_channel();
circuitmux_set_policy(cmux, &ewma_policy);
ch->has_queued_writes = has_queued_writes;
ch->wide_circ_ids = 1;