commit fb5ebfb50770062c77534d4db4c6a9c5ad475fa0 Author: Nick Mathewson <nickm@torproject.org> Date: Wed Oct 2 15:11:34 2013 -0400 Avoid chan/circ linear lookups for requests The solution I took is to not free a circuit with a pending uncancellable work item, but rather to set its magic number to a sentinel value. When we get a work item, we check whether the circuit has that magic sentinel, and if so, we free it rather than processing the reply. --- src/or/circuitlist.c | 17 ++++++++++++-- src/or/cpuworker.c | 62 +++++++++++++++++++++----------------------------- src/or/or.h | 6 +++++ 3 files changed, 47 insertions(+), 38 deletions(-) diff --git a/src/or/circuitlist.c b/src/or/circuitlist.c index 36ba3bf..d964e66 100644 --- a/src/or/circuitlist.c +++ b/src/or/circuitlist.c @@ -745,6 +745,7 @@ circuit_free(circuit_t *circ) { void *mem; size_t memlen; + int should_free = 1; if (!circ) return; @@ -784,6 +785,8 @@ circuit_free(circuit_t *circ) memlen = sizeof(or_circuit_t); tor_assert(circ->magic == OR_CIRCUIT_MAGIC); + should_free = (ocirc->workqueue_entry == NULL); + crypto_cipher_free(ocirc->p_crypto); crypto_digest_free(ocirc->p_digest); crypto_cipher_free(ocirc->n_crypto); @@ -826,8 +829,18 @@ circuit_free(circuit_t *circ) * "active" checks will be violated. */ cell_queue_clear(&circ->n_chan_cells); - memwipe(mem, 0xAA, memlen); /* poison memory */ - tor_free(mem); + if (should_free) { + memwipe(mem, 0xAA, memlen); /* poison memory */ + tor_free(mem); + } else { + /* If we made it here, this is an or_circuit_t that still has a pending + * cpuworker request which we weren't able to cancel. Instead, set up + * the magic value so that when the reply comes back, we'll know to discard + * the reply and free this structure. + */ + memwipe(mem, 0xAA, memlen); + circ->magic = DEAD_CIRCUIT_MAGIC; + } } /** Deallocate the linked list circ-><b>cpath</b>, and remove the cpath from diff --git a/src/or/cpuworker.c b/src/or/cpuworker.c index abf1f22..36ca505 100644 --- a/src/or/cpuworker.c +++ b/src/or/cpuworker.c @@ -152,8 +152,7 @@ typedef struct cpuworker_reply_t { } cpuworker_reply_t; typedef struct cpuworker_job_u { - uint64_t chan_id; - uint32_t circ_id; + or_circuit_t *circ; union { cpuworker_request_t request; cpuworker_reply_t reply; @@ -297,16 +296,13 @@ cpuworker_log_onionskin_overhead(int severity, int onionskin_type, onionskin_type_name, (unsigned)overhead, relative_overhead*100); } -/** */ +/** Handle a reply from the worker threads. */ static void cpuworker_onion_handshake_replyfn(void *work_) { cpuworker_job_t *job = work_; cpuworker_reply_t rpl; - uint64_t chan_id; - circid_t circ_id; - channel_t *p_chan = NULL; - circuit_t *circ = NULL; + or_circuit_t *circ = NULL; --total_pending_tasks; @@ -338,46 +334,40 @@ cpuworker_onion_handshake_replyfn(void *work_) } } } - /* Find the circ it was talking about */ - chan_id = job->chan_id; - circ_id = job->circ_id; - - p_chan = channel_find_by_global_id(chan_id); - if (p_chan) - circ = circuit_get_by_circid_channel(circ_id, p_chan); + circ = job->circ; log_debug(LD_OR, - "Unpacking cpuworker reply %p, chan_id is " U64_FORMAT - ", circ_id is %u, p_chan=%p, circ=%p, success=%d", - job, U64_PRINTF_ARG(chan_id), (unsigned)circ_id, - p_chan, circ, rpl.success); + "Unpacking cpuworker reply %p, circ=%p, success=%d", + job, circ, rpl.success); + + if (circ->base_.magic == DEAD_CIRCUIT_MAGIC) { + /* The circuit was supposed to get freed while the reply was + * pending. Instead, it got left for us to free so that we wouldn't freak + * out when the job->circ field wound up pointing to nothing. */ + log_debug(LD_OR, "Circuit died while reply was pending. Freeing memory."); + circ->base_.magic = 0; + tor_free(circ); + goto done_processing; + } + + circ->workqueue_entry = NULL; if (rpl.success == 0) { log_debug(LD_OR, "decoding onionskin failed. " "(Old key or bad software.) Closing."); if (circ) - circuit_mark_for_close(circ, END_CIRC_REASON_TORPROTOCOL); - goto done_processing; - } - if (!circ) { - /* This happens because somebody sends us a destroy cell and the - * circuit goes away, while the cpuworker is working. This is also - * why our tag doesn't include a pointer to the circ, because we'd - * never know if it's still valid. - */ - log_debug(LD_OR,"processed onion for a circ that's gone. Dropping."); + circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_TORPROTOCOL); goto done_processing; } - tor_assert(! CIRCUIT_IS_ORIGIN(circ)); - TO_OR_CIRCUIT(circ)->workqueue_entry = NULL; - if (onionskin_answer(TO_OR_CIRCUIT(circ), + + if (onionskin_answer(circ, &rpl.created_cell, (const char*)rpl.keys, rpl.rend_auth_material) < 0) { log_warn(LD_OR,"onionskin_answer failed. Closing."); - circuit_mark_for_close(circ, END_CIRC_REASON_INTERNAL); + circuit_mark_for_close(TO_CIRCUIT(circ), END_CIRC_REASON_INTERNAL); goto done_processing; } log_debug(LD_OR,"onionskin_answer succeeded. Yay."); @@ -527,8 +517,7 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ, tor_gettimeofday(&req.started_at); job = tor_malloc_zero(sizeof(cpuworker_job_t)); - job->chan_id = circ->p_chan->global_identifier; - job->circ_id = circ->p_circ_id; + job->circ = circ; memcpy(&job->u.request, &req, sizeof(req)); memwipe(&req, 0, sizeof(req)); @@ -542,8 +531,9 @@ assign_onionskin_to_cpuworker(or_circuit_t *circ, tor_free(job); return -1; } - log_debug(LD_OR, "Queued task %p (qe=%p, chanid="U64_FORMAT", circid=%u)", - job, queue_entry, U64_PRINTF_ARG(job->chan_id), job->circ_id); + + log_debug(LD_OR, "Queued task %p (qe=%p, circ=%p)", + job, queue_entry, job->circ); circ->workqueue_entry = queue_entry; diff --git a/src/or/or.h b/src/or/or.h index 4ff3555..5978504 100644 --- a/src/or/or.h +++ b/src/or/or.h @@ -2725,8 +2725,14 @@ typedef struct { time_t expiry_time; } cpath_build_state_t; +/** "magic" value for an origin_circuit_t */ #define ORIGIN_CIRCUIT_MAGIC 0x35315243u +/** "magic" value for an or_circuit_t */ #define OR_CIRCUIT_MAGIC 0x98ABC04Fu +/** "magic" value for a circuit that would have been freed by circuit_free, + * but which we're keeping around until a cpuworker reply arrives. See + * circuit_free() for more documentation. */ +#define DEAD_CIRCUIT_MAGIC 0xdeadc14c struct create_cell_t;