-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA512
Hi everyone,
This is quite interesting. I wrote back an initial proposal for this kind of work in Tor and I'm happy to see some similarities :).
I've attached to the email the ideas I had based on some discussions with Nick and reading the ticket #1749. (Sorry for any obvious huge English mistakes, it's not my primary language :)
I remember a discussion about parallel relay cell crypto with Nick and there was an important thing which is to prioritize quiet circuit(s). This implies that once a cell is scheduled for crypto. processing, the dispatcher should somehow know the "noise" of the circuit and dispatch accordingly. It brings the concept of priority for cells/circuit in a parallel environment which makes dispatching a bit more tricky.
(More inline).
Nick Mathewson:
On Nov 29, 2012 9:30 PM, "Andrea Shepard" andrea@torproject.org wrote:
Please review first draft proposed parallel relaycrypt structures in my parallel_relay_crypt branch.
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.
Maybe we'll need a next pointer in cells if we're queueing them?
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?
- 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?
What about if the main thread, when dequeuing encrypted cells, could check that and decide if it goes outbound or stays locally for more processing ?
- 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.)
I would also propose the use of a priority queue because (or at least) of the aforementioned use case.
- If you're going to have separate locks, it's important to
document how they nest, to prevent deadlock conditions.
- 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.
Looks like there is ? ..
- 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?
Thanks again! I'll let you know if I think of anything else.
I have question/remark on the circuit_t pointer in relaycrypt_job_s structure. I wonder if a lock might be needed since if the circuit is closed but some cells are still being processed by a relaycrypt worker thread, simply setting the circ to NULL will not indicate the thread to stop using CPU for this invalid circuit.
I don't really know if Tor as some kind of refcount mechanism for circuits but in my experience this is a "time trap" when setting a pointer to NULL but still using some objects associated to that pointer. The cleanup becomes a nightmare if a thread still has ref. to some circuit's object which is invalidated when holding them.
Would it be safer to simply have a STOP/CANCEL mechanism for worker thread and avoid nullifying references that might be in used?
That's it for now. I'm quite happy to see this work going forward! :)
Cheers! David
yrs,