[tor-dev] Parallel relaycrypt data structures for review

[Andrea Shepard]

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
repack then.

> * 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 Shepard
<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
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20121130/6abb79ce/attachment.pgp>