Opened 4 years ago
Last modified 9 months ago
#13737 assigned enhancement
Move circuit building crypto to worker
Reported by: | dgoulet | Owned by: | yawning |
---|---|---|---|
Priority: | High | Milestone: | Tor: unspecified |
Component: | Core Tor/Tor | Version: | Tor: 0.2.7 |
Severity: | Normal | Keywords: | tor-client, tor-hs, multicore, performance, tor-dos, term-project-ideas intro |
Cc: | nickm, andrea, yawning, chelseakomlo | Actual Points: | |
Parent ID: | Points: | 3 | |
Reviewer: | Sponsor: | SponsorU-can |
Description
Make worker able to handle circuit building crypto.
Child Tickets
Change History (36)
comment:1 Changed 4 years ago by
Keywords: | tor-client tor-hs multicore performance added |
---|
comment:2 Changed 4 years ago by
Milestone: | Tor: 0.2.??? → Tor: 0.2.7.x-final |
---|
comment:3 Changed 4 years ago by
Status: | new → assigned |
---|
comment:4 Changed 4 years ago by
Keywords: | 027-triaged-1-in added |
---|
Marking more tickets as triaged-in for 0.2.7
comment:5 Changed 4 years ago by
Points: | → small-remaning |
---|---|
Version: | → Tor: 0.2.7 |
comment:6 Changed 3 years ago by
Milestone: | Tor: 0.2.7.x-final → Tor: 0.2.8.x-final |
---|
comment:8 Changed 3 years ago by
Cc: | yawning added |
---|---|
Severity: | → Normal |
comment:9 Changed 3 years ago by
https://github.com/Yawning/tor/compare/feature13737
Initial pass. I still need to figure out what I want to do when there's max tasks queued and we get a created, and also to support canceling CREATED processing when the circuit gets removed when it's pending. I *think* the way things are structured now, the code is merely inefficient instead of crashing, but adding cancellation support is the right thing to do.
Now that I think about it, I'll probably go and rename a bunch of stuff (again) so I don't have lots of pain when I decide to parallelize onion_skin_create
(Needed when #17272 happens)...
So, this branch will get force-pushed at some point. At least it shows how I think this should work...
comment:10 Changed 3 years ago by
Status: | assigned → needs_review |
---|
Ok. I just squashed down my branch. One day in the future, we should strongly consider also parallelizing onion_skin_create
, but it's reasonably fast for now (~25 usec) vs the client/server side specific parts of NTOR (~160 usec).
One side benefit of the new code is that there is a centralized place to add rate limiting for outgoing handshakes if we decide to do something different under extreme load in the future (origin_pending_add()
/origin_next_task()
).
comment:11 Changed 3 years ago by
I have reviewed the code: it makes sense to me and I don't see any issues:
- There is some code duplication between the client and server onionskins, but I think that's ok.
- The starvation-avoidance mechanism in queue_pending_tasks works as described. I wonder if we will want to favour server, client, or the longer queue in future.
Someone who is more familiar with cpuworkers or circuit crypto could give a more thorough review than I can.
comment:12 Changed 3 years ago by
In the future I was thinking of adding some form of active queue management to the client side queue (we detect extreme backlog for the server side queue and start dropping after a while), but that's a separate ticket, and I need to figure out what sort of algorithm to use (CoDel suggests itself as reasonable behavior with minor modifications).
Until we can get profiler output again, this branch is probably the last of the low hanging fruit for HS performance optimization. :(
comment:13 follow-up: 14 Changed 3 years ago by
Here is the _only_ thing I found (typo) :)
- Comment here should be REPLY instead of "RESPONSE".
+ /** Magic number; must be CPUWORKER_[type]_[REQUEST,RESPONSE]_MAGIC. */
The rest lgtm. I have to agree that there is a lot of code duplication here but I'm also OK with having server/client seperate here even though it's almost the same.
I'm also not super fan of having this ++total_pending_tasks;
done _before_ we actually successfully queue the task but I think that was in before this patch anyway (and highly unlikely also...).
Good work here!
comment:14 Changed 3 years ago by
Replying to dgoulet:
Here is the _only_ thing I found (typo) :)
- Comment here should be REPLY instead of "RESPONSE".
+ /** Magic number; must be CPUWORKER_[type]_[REQUEST,RESPONSE]_MAGIC. */
Fixed in a fixup commit.
The rest lgtm. I have to agree that there is a lot of code duplication here but I'm also OK with having server/client seperate here even though it's almost the same.
Yeah, there's a good amount of boiler plate that's needed, but I was reluctant to merge the code further.
I'm also not super fan of having this
++total_pending_tasks;
done _before_ we actually successfully queue the task but I think that was in before this patch anyway (and highly unlikely also...).
Also fixed.
comment:15 Changed 3 years ago by
Milestone: | Tor: 0.2.??? → Tor: 0.2.8.x-final |
---|
comment:16 Changed 3 years ago by
Status: | needs_review → needs_revision |
---|
1b150bcb9d54ccfe132b3e58ee9ef5f001f70221 -- looks ok.
3e75d22bd496c1238cabee25b8616ea6b02805d8
- The fields in origin_entry_t need documentation. What are they for? Who sets them? The type itself could also use some explicitness. How do they get removed? How do they get removed when the origin_circuit_dies?
- origin_entry_t and origin_queue could use a friendlier name, maybe with "pending_cpu" or "pending_crypto" in it.
- All arguments of origin_pending_add should be documented. It should document which items it takes ownership of. It should also document whether it gives items to cpuworkers, or if not, who does?
- In origin_next_task, must the caller also free hop_out?
- origin_pending_remove has no documentation.
- I am fine with having this linear search in origin_pending_remove(), but we should probably have an XXX about it.
- Since CIRC_STATE_ONIONSKIN_PENDING can now mean something a little different than before, its documentation should be updated.
- I am not in love with having total_pending_tasks and max_pending_tasks be shared by both types of work. (This speaks to the 'queue_pending_tasks' thing.)
- I would like to have queue_pending_*_tasks have a more obvious end condition; I'm worried about infinite loops.
- [META]: I assume this is duplicated from existing code, but I don't like how assign_onionskin_client_to_cpuworker() can return -1 for "I queued the work for later and -1 for "I tried to add the job, but threadpool_queue_work failed!"
META:
- Do we really need to have two separate queues here, one for pre-queued stuff and one for stuff that's on a workqueue? Why not just put stuff straight on the workqueue?
comment:17 Changed 3 years ago by
(I opened #17806 to track the workqueue/other-queue issue as a separate ticket.)
comment:18 Changed 3 years ago by
Owner: | set to yawning |
---|---|
Status: | needs_revision → assigned |
Setting yawning as the owner of this needs_revision ticket.
comment:19 Changed 3 years ago by
Status: | assigned → needs_revision |
---|
comment:20 Changed 3 years ago by
Keywords: | 027-triaged-1-in removed |
---|---|
Milestone: | Tor: 0.2.8.x-final → Tor: 0.2.9.x-final |
Passed feature freeze for 028, changing milestone to 029.
comment:21 Changed 3 years ago by
Sponsor: | → SponsorU-can |
---|
comment:22 Changed 3 years ago by
Priority: | Medium → High |
---|
comment:23 Changed 3 years ago by
Keywords: | tor-dos added |
---|
comment:24 Changed 3 years ago by
Parent ID: | → #17293 |
---|
comment:26 Changed 3 years ago by
Points: | small-remaning → 3 |
---|---|
Summary: | Implement circuit building crypto to worker → Move circuit building crypto to worker |
comment:27 Changed 3 years ago by
Keywords: | TorCoreTeam201608 added |
---|
comment:28 Changed 2 years ago by
Keywords: | nickm-deferred-20160905 added |
---|---|
Milestone: | Tor: 0.2.9.x-final → Tor: 0.2.??? |
Hi, Yawning! I'm deferring these tickets assigned to you from 0.2.9 to 0.2.???, since you're going to be out for September. But if you wind up wanting to do any of them for 0.2.9 anyway, please feel free to move them back.
(This is my ticket-deferring afternoon)
comment:29 Changed 2 years ago by
Parent ID: | #17293 |
---|
Unparenting these from #17293; holding for future work.
comment:31 Changed 2 years ago by
Keywords: | tor-03-unspecified-201612 added |
---|---|
Milestone: | Tor: 0.3.??? → Tor: unspecified |
Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.
comment:32 Changed 21 months ago by
Keywords: | tor-03-unspecified-201612 removed |
---|
Remove an old triaging keyword.
comment:33 Changed 21 months ago by
Keywords: | nickm-deferred-20160905 removed |
---|
comment:34 Changed 21 months ago by
Keywords: | TorCoreTeam201608 removed |
---|
comment:35 Changed 21 months ago by
Keywords: | term-project-ideas intro added |
---|
comment:36 Changed 9 months ago by
Cc: | chelseakomlo added |
---|
These may be worth looking at for 0.2.7.