[tor-bugs] #9682 [Tor]: Better work queue implementation for cpuworkers

[Tor Bug Tracker & Wiki]

#9682: Better work queue implementation for cpuworkers
-----------------------------+---------------------------------------------
     Reporter:  nickm        |      Owner:
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:  Tor: 0.2.5.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  tor-relay performance cpuworker
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+---------------------------------------------

Comment (by nickm):

 Replying to [comment:8 andrea]:
 > Review, part 1:
 >
 > 13a3383b5562ba608e55143a8717bbcb35604ae5:
 >  - We're mostly just moving code out of compat.{c,h} here; looks okay to
 me
 >    but it's a big diff, so consider that conditional on that code not
 actually
 >    being changed a lot in the process.
 >
 > 3b7755c5b9a9f29a07d932dcdd78d8b79613818e:
 >  - Is the timeout arg actually necessary?  Not sure I'm seeing it used
 other
 >    than in the test case.

 It's not necessary, but it may be eventually.

 >  - Is struct timeval the right way to pass it (vs., say, a uint64_t
 count of
 >    microseconds?); both the pthreads and windows versions seem to be
 doing
 >    a bit of dancing around to work with that.

 I like a uint64_t of microseconds, though working with it is going to be a
 PITA due (as you note) to the nutty "abstime" input character of the
 input.

 >  - Otherwise, this code looks okay to me.
 >

  [...]
 > e85f99c1b89e5e44786025b1b6f74cffd0f38ba3:
 >  - I think the thread pool/work queue logic is okay for the feature set
 it
 >    supports, but I don't see any way to shut down a thread pool, or any
 >    way for the workler thread to exit other than being killed from
 another
 >    thread.  Could be a problem if it ever gets used for something other
 than
 >    pure computation where there's cleanup to be done.

 See your note later.

 >  - /* XXXX inside or outside of lock?? */
 >    Looks like you got this right; it'd definitely be a race condition if
 it
 >    were outside.
 >  - From a queueing theory perspective, it's sub-optimal to distribute
 work
 >    up front by assigning it to a thread in threadpool_queue_work();
 you'll
 >    get more efficient utilization and lower latency by having a single
 queue
 >    that threads pull from when they finish a job.

 Wouldn't I get lots of lock contention when they tried to pull off that
 queue?

 Also, I talked to a queue guy and he told me that I should have separate
 queues, but that they should be no more than 1 item deep.  Can you point
 me to the relevant literature here?  Can we come up with a benchmark to
 try to demonstrate the difference?

 > 3c586e98c82b52e0803963f98bc36e17f6c2fb71:
 >  - Looks okay.  Is this threadpool_queue_for_all() mechanism going to
 provide
 >    the thread shutdown capability?

 Yup.  Also the key update mechanism.

 > 52c33d8eff931da43b5d4dd551cf75e934862bff:
 >  - Don't you mean s/reentrant/recursive/ ?

 Yes; fixed that in ffb48b39.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9682#comment:11>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online