Fix for Fix (c): broken atomic lock in own bmesh code.
That was a nasty one, Debug build would never have any issue (even tried with 64 threads!), but Release build would deadlock nearly immediately, even with only 2 threads! What happened here (I think) is that gcc optimizer would generate a specific path endlessly looping when initial value of virtual_lock was FLT_MAX, by-passing re-assignment from v_no[0] and the atomic cas completely. Which would have been correct, should v_no[0] not have been shared (and modified) by multiple threads. ;) Idea of that (broken) for loop was to avoid completely calling the atomic cas as long as v_no[0] was locked by some other thread, but... Guess the avoided/missing memory barrier was the root of the issue here. Lesson of the evening: Remember kids, do not trust your compiler to understand all possible threading-related side effects, and be explicit rather than elegant when using atomic ops! Side-effect lesson: do check both release and debug builds when messing with said atomic ops...
This commit is contained in:
parent
dd6c918b2c
commit
3c1f3c02c6
|
@ -412,14 +412,16 @@ static void mesh_verts_calc_normals_accum_cb(void *userdata, MempoolIterData *mp
|
|||
* It also assumes that collisions between threads are highly unlikely,
|
||||
* else performances would be quite bad here. */
|
||||
float virtual_lock = v_no[0];
|
||||
for (virtual_lock = v_no[0];
|
||||
(virtual_lock == FLT_MAX) || (atomic_cas_float(&v_no[0], virtual_lock, FLT_MAX) != virtual_lock);
|
||||
virtual_lock = v_no[0])
|
||||
{
|
||||
while (true) {
|
||||
/* This loops until following conditions are met:
|
||||
* - v_no[0] has same value as virtual_lock (i.e. it did not change since last try).
|
||||
* - v_no_[0] was not FLT_MAX, i.e. it was not locked by another thread.
|
||||
* - v_no[0] was not FLT_MAX, i.e. it was not locked by another thread.
|
||||
*/
|
||||
const float vl = atomic_cas_float(&v_no[0], virtual_lock, FLT_MAX);
|
||||
if (vl == virtual_lock && vl != FLT_MAX) {
|
||||
break;
|
||||
}
|
||||
virtual_lock = vl;
|
||||
}
|
||||
BLI_assert(v_no[0] == FLT_MAX);
|
||||
/* Now we own that normal value, and can change it.
|
||||
|
|
Loading…
Reference in New Issue