Closed JayDDee closed 4 years ago
In miner_thread if get-work fails the error message is logged before rrleasing the g_work mutex. It's a trivial issue because the error is fatal but the code will be changes for the sake of correctness.
There is interaction between the work_lock and g_work_lock. work_loc protects the stratum context struct while g_work protects the g_work struct. When copying data from the stratum context to g_work both mutexes are required simultaneously. In short:
The nesting is handled properly, however, the g_work lock is held for a long time. Copying data from stratum ctx to g_work then immediately from g_work to local work is done within a one g_work_lock protected segmment. This segment also includes logging.
The segment can naturally be divided into 2 smaller segments with the first segment copying the data from stratum ctx to g_work and the second to copy the data from g_work to local work. The logging would be in between.
This would result in moving mutex control down from the main thread loop to the functions that actually need it to do its work, an architecturally better implementation as well.
For solo mining the mutex segment is split in 2 but there is more analysis needed to undertsand the full picture.
The miner thread locks g_work to send a request to workio to send an RPC to the server. It then waits, holding the mutex, until the data is received.
Meanwhile the workio thread receives the data from RPC and populates g_work. I've never seen one thread hold a mutex for another and it looks like a bad idea. This results in the miner thread voluntarilly suspending while holding the g_work mutex, defintely not a good thing. In addition it's vulnerable to networking latency with RPC.
This results in a very large protected segment that doesn't actually protect the critical data. It exposes potential thread sybchronization issues and also produces a lot of console output.
This will require a deeper dive.
A minor follow up issue has been discovered that only affects logs. A New Work log can be produces with the same job id.
This seems to be when the nonce range is exhausted and the extranonce is incremented when mining a high hash rate algo.
This creates both a problem and an opportunity. The problem is reporting New Work when it's actually an xnonce incr. The opportunity is to report a New Xnonce.
This easy way is to simply add another check for a jobid change but that test is andexpensive string compare. A better way is needed.
Edit: If stratum_gen_work is called with no new stratum work extranonce increment is assumed. Logging of Extranonce will only occur with --debug.
The use of a mutex in a miner thread to protect data accesses by the workio creates a coupling between the threads. Multithreading is typically used to decouple tasks from one another. Other than that I don't see a problem with the implemenation.
The wide range of the mutex segment is due to the g_work_time test. Moving this test to the get_work function would allow better encapsulation of the protected segment and result in cleaner code in miner_thread.
Due to the difficulty in testing getwork no changes will be made.
Summary:
The changes have been implemented and tested and will be in the next release.
One instance of calculating the global hash rate was not protected by the stats_lock. Mutex will be added.
cpuminer-opt-3.14.2 is released with the above changes.
Another change coming.
g_work_lock is being changed from mutex to rwlock to allow multiple readers. This should reduce blocking with high thread count as miner_threads can refresh their work simultaneously instead of queueing up one at a time.
cpuminer-opt-3.14.3 is released with the additional change.
With the rapid increase in the number of CPU cores the risk of mutex contention increases. This issue is opened to review the use of mutex in cpuminer-opt with the aim of: