[tor-dev] Parallel relaycrypt data structures for review

[David Goulet]

-----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 at 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,
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.12 (GNU/Linux)

iQEcBAEBCgAGBQJQuNqNAAoJEELoaioR9I025KMIALXE81lm0zpTRUWlM52HawyD
lGVBqMRf/V+F/g8EFqGpyZ3drs9o3+uNhY7/+qdSY6IhWJGUfyL7vrwYSk1/yuBf
RWBFE050zLhd9QwCARCoxb0E+oGsh/Z6hq8NgcDxba28Z2II+azE3KrNkwAuUnob
N/6gUd8ceWpyXmFgCpf3MJLNqZKOcYXax2LFA+ocQdQ368VD+Lq8jqPXfR62Npr4
Ujhp9i2REALbbV6dOmxBVdytK9v2NfFGZIXp9n5onU8/Ym9a8fg0ZvUk+i5bwb+6
eOCYeLeVe7yNK8yzxnk6J8TbwuCRMLTSIhEfov6f+WgQYR9hfmBcY7shqFvpef4=
=OQrF
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: multithread-crypto-spec.txt
URL: <http://lists.torproject.org/pipermail/tor-dev/attachments/20121130/8bb42dca/attachment.txt>