Merge pull request #16258 from opensourcerouting/tsan-20240620

lib, tests: fix some b0rked tests, then fix TSAN warnings

lib/frrcu.h

@@ -156,7 +156,7 @@ extern void rcu_enqueue(struct rcu_head *head, const struct rcu_action *action);
 #define rcu_call(func, ptr, field)                                             \
 	do {                                                                   \
 		typeof(ptr) _ptr = (ptr);                                      \
-		void (*fptype)(typeof(ptr));                                   \
+		void (*_fptype)(typeof(ptr));                                  \
 		struct rcu_head *_rcu_head = &_ptr->field;                     \
 		static const struct rcu_action _rcu_action = {                 \
 			.type = RCUA_CALL,                                     \

lib/seqlock.c

@@ -26,6 +26,39 @@
  * OS specific synchronization wrappers *
  ****************************************/
 
+#ifndef __has_feature /* not available on old GCC */
+#define __has_feature(x) 0
+#endif
+
+#if (defined(__SANITIZE_THREAD__) || __has_feature(thread_sanitizer))
+/* TSAN really does not understand what is going on with the low-level
+ * futex/umtx calls.  This leads to a whole bunch of warnings, a lot of which
+ * also have _extremely_ misleading text - since TSAN does not understand that
+ * there is in fact a synchronization primitive involved, it can end up pulling
+ * in completely unrelated things.
+ *
+ * What does work is the "unsupported platform" seqlock implementation based
+ * on a pthread mutex + condvar, since TSAN of course suppports these.
+ *
+ * It may be possible to also fix this with TSAN annotations (__tsan_acquire
+ * and __tsan_release), but using those (correctly) is not easy either, and
+ * for now just get things rolling.
+ */
+
+#ifdef HAVE_SYNC_LINUX_FUTEX
+#undef HAVE_SYNC_LINUX_FUTEX
+#endif
+
+#ifdef HAVE_SYNC_OPENBSD_FUTEX
+#undef HAVE_SYNC_OPENBSD_FUTEX
+#endif
+
+#ifdef HAVE_SYNC_UMTX_OP
+#undef HAVE_SYNC_UMTX_OP
+#endif
+
+#endif /* TSAN */
+
 /*
  * Linux: sys_futex()
  */

tests/lib/test_atomlist.c

@@ -62,7 +62,7 @@ static struct asort_head shead;
 static struct testthread {
 	pthread_t pt;
 	struct seqlock sqlo;
-	size_t counter, nullops;
+	_Atomic size_t counter, nullops;
 } thr[NTHREADS];
 
 struct testrun {
@@ -98,9 +98,9 @@ static void trfunc_##name(unsigned int offset) \
 	size_t i = 0, n = 0;
 
 #define endtestrun                                                             \
-	thr[offset].counter = i; \
+	atomic_store_explicit(&thr[offset].counter, i, memory_order_seq_cst);  \
-	thr[offset].nullops = n; \
+	atomic_store_explicit(&thr[offset].nullops, n, memory_order_seq_cst);  \
-}
+	}
 
 deftestrun(add, "add vs. add", 0, false)
 	for (; i < NITEM / NTHREADS; i++)
@@ -288,10 +288,10 @@ static void run_tr(struct testrun *tr)
 	sv = seqlock_bump(&sqlo) - SEQLOCK_INCR;
 	for (size_t i = 0; i < NTHREADS; i++) {
 		seqlock_wait(&thr[i].sqlo, seqlock_cur(&sqlo));
-		s += thr[i].counter;
+		s += atomic_load_explicit(&thr[i].counter, memory_order_seq_cst);
-		n += thr[i].nullops;
+		n += atomic_load_explicit(&thr[i].nullops, memory_order_seq_cst);
-		thr[i].counter = 0;
+		atomic_store_explicit(&thr[i].counter, 0, memory_order_seq_cst);
-		thr[i].nullops = 0;
+		atomic_store_explicit(&thr[i].nullops, 0, memory_order_seq_cst);
 	}
 
 	delta = monotime_since(&tv, NULL);

tests/lib/test_seqlock.c

@@ -82,11 +82,11 @@ int main(int argc, char **argv)
 	assert(seqlock_held(&sqlo));
 
 	assert(seqlock_cur(&sqlo) == 1);
-	assert(seqlock_bump(&sqlo) == 1);
-	assert(seqlock_cur(&sqlo) == 5);
 	assert(seqlock_bump(&sqlo) == 5);
+	assert(seqlock_cur(&sqlo) == 5);
 	assert(seqlock_bump(&sqlo) == 9);
 	assert(seqlock_bump(&sqlo) == 13);
+	assert(seqlock_bump(&sqlo) == 17);
 	assert(seqlock_cur(&sqlo) == 17);
 
 	assert(seqlock_held(&sqlo));
@@ -111,4 +111,5 @@ int main(int argc, char **argv)
 	writestr("main @release\n");
 	seqlock_release(&sqlo);
 	sleep(1);
+	pthread_join(thr1, NULL);
 }