fix job consume #1526
fix job consume #1526
Conversation
|
This isn't correct. Consume needs to be per thread. Please explain in numbered steps how you think a deadlock can happen. |
|
Also please note that a deadlock in worker threads wouldn't affect the network thread. I see some people already think it is some kind of magical client-side fix for nicehash having shitty software. With regards to nicehash, I think we should silence them like cast-xmr but make it optional with default to false. |
|
I updated my PR description because I was wrong with the deadlock part. |
psychocrypt
force-pushed
the
fix-jobConsume
branch
from
8a7404d to
10b42a5
Compare
THIRD-PARTY-LICENSES
Outdated
| @@ -5,7 +5,7 @@ This application bundles the following third-party software in accordance with t | |||
| Package: Original NVidia mining code | |||
| Authors: tsiv and KlausT | |||
| License: GNU GPLv3 | |||
| Notes: Improvements are (c) of Xmr-Stak team | |||
| Notes: Improvements are (c) of Xmr-Stak team team and are covered by GNU GPLv3 | |||
|
|
||
| Package: cpputil | ||
| Authors: Will Zhang | ||
| Source: https://github.com/willzhang4a58/cpputil |
There was a problem hiding this comment.
Nice to see. Big thanks for dealing with licensing and big thanks to @willzhang4a58 for the code.
xmrstak/cpputil/read_write_lock.h
Outdated
| */ | ||
|
|
||
| #ifndef CPPUTIL_READ_WRITE_LOCK_H_ | ||
| #define CPPUTIL_READ_WRITE_LOCK_H_ |
There was a problem hiding this comment.
Can we get this to the standard stak format -> tab indent and #pragma once
xmrstak/backend/globalStates.cpp
Outdated
| jobLock.WriteLock(); | ||
|
|
||
| // this notifies all threads that the job has changed | ||
| iGlobalJobNo++; |
There was a problem hiding this comment.
This is in a wrong place. The order that I told you:
when a global job is being written you:
acquire write lock
write the job
release the lock
increase the jobno
when a thread gets a job:
acquire read lock
copy the job
set the job no to global (!!! note the difference with write lock)
release read lock
Is slightly more efficient since the threads only start to read when the resource is available
psychocrypt
force-pushed
the
fix-jobConsume
branch
from
10b42a5 to
cde64af
Compare
|
updated |
|
Needs a rebase before testing |
psychocrypt
force-pushed
the
fix-jobConsume
branch
from
cde64af to
2e2e189
Compare
fix fireice-uk#1505 - fix possible deadlock of the executor thread - fix racecondition during the job consumation - remove switch_work in all classes `minethd` - move `consume_work` into `globalStates`
add log class from Will Zhang: Package: cpputil Source: https://github.com/willzhang4a58/cpputil License: MIT License
user read write locks to be sure that no job is consumend during the job update
- reformat `read_write_lock.h` - fix spelling issue - move job id increase of the write to the buttom
psychocrypt
force-pushed
the
fix-jobConsume
branch
from
2e2e189 to
460aa90
Compare
|
I removed the BUG label. The point that all worker must consume a job before a new job can be added for the worker is no real bug. A drawback of this restriction can be that the miner is mining old jobs even if newer jobs are received but I do not know if this can be defined as BUG. |
fix #1505fix or reduce #1337 #1222minethdconsume_workintoglobalStatesIn the previous implementation it was possible that a worker thread consumed and already old job if the pool is sending jobs faster than a batch of nonces is calculated. The reson is that the current implementation force that each worker must consume the oldest job in the queue before a new job can be added. On a GPU the time to process a job (a batch of nonces) before checking again if the job has changed much longer than on a CPU. A GPU needs typical ~1 sec+ (slow GPUs sometimes 10sec).
example of the PR behavior if the job is faster changed than processed:
Test
[update:] I thought there is a race condition and deadlock in the code I changes. After discussions with @fireice-uk it looks like I was wrong. Therefore I updated the PR description.