diff options
author | Bastien Montagne <montagne29@wanadoo.fr> | 2017-11-26 01:14:54 +0300 |
---|---|---|
committer | Bastien Montagne <montagne29@wanadoo.fr> | 2017-11-26 01:14:54 +0300 |
commit | 3c1f3c02c60f6ace330c53299640a6ca6499b6b9 (patch) | |
tree | 3ec4ee323c882963e0761994abdcf63cd9b4926f | |
parent | dd6c918b2ccc6ca546e9a8c5cb5ac169a59e8316 (diff) |
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...
-rw-r--r-- | source/blender/bmesh/intern/bmesh_mesh.c | 12 |
1 files changed, 7 insertions, 5 deletions
diff --git a/source/blender/bmesh/intern/bmesh_mesh.c b/source/blender/bmesh/intern/bmesh_mesh.c index c0cd2e31753..0f97321d9b9 100644 --- a/source/blender/bmesh/intern/bmesh_mesh.c +++ b/source/blender/bmesh/intern/bmesh_mesh.c @@ -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. |