[Bf-blender-cvs] [dd6c918b2cc] master: Fix broken atomic_cas lock in own recent commit in bmesh.

[Bastien Montagne]

Commit: dd6c918b2ccc6ca546e9a8c5cb5ac169a59e8316
Author: Bastien Montagne
Date:   Sat Nov 25 20:28:12 2017 +0100
Branches: master
https://developer.blender.org/rBdd6c918b2ccc6ca546e9a8c5cb5ac169a59e8316

Fix broken atomic_cas lock in own recent commit in bmesh.

Using atomic cas correctly is really hairy... ;)

In this case, the returned value from cas needs to validate *two*
conditions, it must not be FLT_MAX (which is our 'locked' value and
would mean another thread has already locked it), but it also must be
equal to previously stored value...

This means we need two steps per loop here, hence using a 'for' loop
instead of a 'while' one now.

Note that collisions are (as expected) very rare, less than 1 for 10k
typically, so did not catch the issue initially (also because I was
mostly working with release build to check on performances...).

===================================================================

M	source/blender/bmesh/intern/bmesh_mesh.c

===================================================================

diff --git a/source/blender/bmesh/intern/bmesh_mesh.c b/source/blender/bmesh/intern/bmesh_mesh.c
index f6c235271bf..c0cd2e31753 100644
--- a/source/blender/bmesh/intern/bmesh_mesh.c
+++ b/source/blender/bmesh/intern/bmesh_mesh.c
@@ -412,18 +412,24 @@ 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];
-		while ((virtual_lock = atomic_cas_float(&v_no[0], virtual_lock, FLT_MAX)) == FLT_MAX) {
+		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])
+		{
 			/* 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.
 			 */
 		}
+		BLI_assert(v_no[0] == FLT_MAX);
 		/* Now we own that normal value, and can change it.
 		 * But first scalar of the vector must not be changed yet, it's our lock! */
 		virtual_lock += f_no[0] * fac;
 		v_no[1] += f_no[1] * fac;
 		v_no[2] += f_no[2] * fac;
 		/* Second atomic operation to 'release' our lock on that vector and set its first scalar value. */
+		/* Note that we do not need to loop here, since we 'locked' v_no[0],
+		 * nobody should have changed it in the mean time. */
 		virtual_lock = atomic_cas_float(&v_no[0], FLT_MAX, virtual_lock);
 		BLI_assert(virtual_lock == FLT_MAX);