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.