[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 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.
  - 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.
  - Otherwise, this code looks okay to me.

 5abaa4c2cd9f814616bf16b48b634f35b76b5d40:
  - This one looks fine to me.

 cc88b47fa4441233fd6dce448a0c77860da55d8c:
  - This one looks fine 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.
  - /* 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.

 352d758d843801fb6bfe6b5ac4daeaa997c79c1c:
  - This looks okay to me.

 1b9cbcb9ad2d23b6a1b12cc6866f0fd7e40dd810:
  - This looks good to me.

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

 8ef11d4fd328da71d2c95763bcbfd281268f6359:
  - This looks okay to me.

 47a7888c8cf56c52a47c6f69c9644c48ef4ee76b:
  - Looks fine to me.

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

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