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@xxxxxxxxxxxxxx> PGP fingerprint: 3611 95A4 0740 ED1B 7EA5 DF7E 4191 13D9 D0CF BDA5
Attachment:
pgpAKkac4zkn5.pgp
Description: PGP signature
_______________________________________________ tor-dev mailing list tor-dev@xxxxxxxxxxxxxxxxxxxx https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-dev