Fix bug in bench thread barrier #11
Conversation
|
This is my test code for origin thread barrier. |
|
I would defer to @foxhoundsk for reviewing. |
kevinshieh0225
force-pushed
the
pthread_barrier
branch
from
0bddadc
to
ff9895c
Compare
kevinshieh0225
force-pushed
the
pthread_barrier
branch
from
ff9895c
to
25a1acf
Compare
|
@eecheng87, Please check if the above change makes sense or not. |
bench.c
Outdated
| @@ -59,7 +59,7 @@ static const char *msg_dum = "dummy message"; | |||
| static pthread_t pt[MAX_THREAD]; | |||
|
|
|||
| /* block all workers before they are all ready to benchmarking kecho */ | |||
| static bool ready; | |||
| static int ready; | |||
There was a problem hiding this comment.
You should explain why you change the type of ready within git commit message.
There was a problem hiding this comment.
If ready is used for accumulation, its name would confuse.
There was a problem hiding this comment.
I have change the commit message. Please check whether it is clear enough, thank you.
There was a problem hiding this comment.
The name scheme is fairly ambiguous since the word "ready" does not imply the states other than binary form.
There was a problem hiding this comment.
So maybe changing the variable name to count would be better in this scenerio?
There was a problem hiding this comment.
I have change the name to ready_cnt, is it a better name scheme?
There was a problem hiding this comment.
If you intended to count something, you can use retry or n_retry.
There was a problem hiding this comment.
I have changed the name to n_retry, thank you!
kevinshieh0225
force-pushed
the
pthread_barrier
branch
2 times, most recently
from
f97b15f
to
af1b31b
Compare
kevinshieh0225
force-pushed
the
pthread_barrier
branch
2 times, most recently
from
028ac82
to
b31459c
Compare
@kevinshieh0225 , I think it could be changed into:
|
kevinshieh0225
force-pushed
the
pthread_barrier
branch
2 times, most recently
from
1e10844
to
5188627
Compare
|
Sorry for the wait, I have change the message grammar. |
|
@jserv , I think this PR is ready to be merged. (I don't have the right to merge it) |
The origin version cannot guarantee all the threads start kecho task at the same time. Because the main thread may acquire mutex lock before all the bench_worker have acquired it. Change the barrier condition by using counting: - Change condition variable to int n_retry for counting. - The worker acquire lock to add up the counting and wait. - The last worker reach the condition and broadcast the message.
kevinshieh0225
force-pushed
the
pthread_barrier
branch
from
5188627
to
f543b99
Compare
|
Thank @kevinshieh0225 for this effort! |
The origin version cannot guarantee all the thread starting
kecho task at the same time. Because the main thread
may acquire mutex lock before all the bench_worker have
acquire it.
Change the barrier condition by using counting.