[tor-dev] Parallel relaycrypt data structures for review
andrea at torproject.org
Sat Dec 1 06:14:17 UTC 2012
On Fri, Nov 30, 2012 at 08:12:10AM -0500, Nick Mathewson wrote:
> Hi! Here are some initial thoughts:
> * If we're going to do it like this, maybe we need to make cell_t
> packed or something eventually. It's got a fair amount of padding
> overhead right now.
Yeah - but relay_crypt() wants a cell_t, so we'd have to unpack and
> * Maybe we'll need a next pointer in cells if we're queueing them?
Hmmm - yeah, I think that could be made to work. I'm mostly concerned
with avoiding having to have the main thread/worker thread either hold
a lock for the entire time it takes to crypt a bunch of cells/pull them
off the out queue
> * Why is there only an rc_job for outgoing cells on a circuit? It
> seems for symmetry we'd need to have one for inbound cells and one for
> outbound cells. It looks like that code isn't there right now?
I was trying to figure out if we can simplify it by just using the queue
for the other circuit, but yeah, think it is necessary to make rc_jobs
(circuit_t, cell_direction_t) tuples.
> * Maybe I'm confused by these queues. The system of cell queues is
> going to get a little confusing, maybe. Putting cells on the outgoing
> queue isn't always right, since some cells (e.g., relay_data cells at
> an exit node) need to be handled locally rather than relaying them.
> So we need more new queues?
The outgoing queue is the queue of crypted cells for the main thread
to pick up; it isn't the same as the circuit outgoing queue because a
circuit might get closed while a worker is active, and because the thing
to do after the cell is crypted is a bit complicated and I wanted to
minimize the number of other things that would end up being called from
the worker thread.
See circuit_receive_relay_cell() in relay.c; if the cell is 'recognized',
it gets special handling, or else it goes on the queue for the circuit.
That logic would move to the second half of the main-thread processing,
after the cell is removed from the rc_job output queue.
> * Should the jobs be in some data structure other than an smartlist_t?
> A queue would seem to make more sense, since jobs are getting added
> and pulled off. (Yes, protecting the data structure there with a lock
> makes sense.)
Yeah, I just said that as a placeholder - what's really best surely
depends on the selection policy.
> * If you're going to have separate locks, it's important to document
> how they nest, to prevent deadlock conditions.
Yeah - I specified always lock the relaycrypt_dispatcher_t, then the
relaycrypt_job_t in the comments, I believe.
> * Presumably relaycrypt_job_t would need to have a pointer to the
> actual circuit that needs work, and a note about whether it's a job
> for outbound or inbound cells.
Well, it shouldn't be messing with the circuit too much because then a lot
of other stuff that touches the circuit also needs to worry about being
thread-safe, and the case of a circuit being shut down while a worker is
active gets hairy. The worker will only be touching the queues in the
rc_job, but it will need the circuit for the call to relay_crypt() - hmm,
we need a way to make sure the crypto keys don't get freed out from under
it if a circuit goes away.
> * In the non-threaded-relaycrypt case, presumably the intention is
> that there's a function that would otherwise queue a cell for crypto
> but instead just put it directly on the appropriate circuit queue?
Yeah - the non-thready version would just call whatever (possibly refactored)
relay_crypt() that the worker thread calls and then the handler for crypted
cells, all in the main thread, rather than queueing anything.
> Thanks again! I'll let you know if I think of anything else.
Okay, thanks. I'm trying to get some code started this weekend, I think.
<andrea at torproject.org>
PGP fingerprint: 3611 95A4 0740 ED1B 7EA5 DF7E 4191 13D9 D0CF BDA5
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 836 bytes
Desc: not available
More information about the tor-dev