From 8aeac1f005d5b462314c73db098878042fe6c444 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 19 Jun 2024 21:36:17 +0200 Subject: [PATCH 1/5] tests/lib: fix seqlock test seqlock_bump() used to return the value before bumping, but that's unhelpful if you were to actually need it. I had changed it to return the value after, but the update to the test got lost at some point. The return value is not in fact used anywhere in FRR, so while it is a bug, it has zero impact. NB: yes, test_seqlock is not run, which sounds wrong. The problem here is that (a) the test itself uses sleeps and is timing sensitive, which would raise false positives. And (b), the test is meaningless if executed once. It needs to be run millions of times under various conditions (e.g. load) to catch rare races, and it needs to be run on machines with "odd" memory models (in this case I used BE ppc32 and ppc64 systems as test platforms.) Fixes: 6046b690b53 ("lib/seqlock: avoid syscalls in no-waiter cases") Signed-off-by: David Lamparter --- tests/lib/test_seqlock.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/lib/test_seqlock.c b/tests/lib/test_seqlock.c index 288d4a8c2554..35501cbd4a6a 100644 --- a/tests/lib/test_seqlock.c +++ b/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)); From 1f67dfb1438e74e5258786d4a769084354fd375b Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Wed, 19 Jun 2024 22:32:54 +0200 Subject: [PATCH 2/5] lib: fix typo in rcu_do() I lost an underscore somewhere along the way. Which never caused issues because we don't use that function macro. It is, however, useful for testing, so fix it. Signed-off-by: David Lamparter --- lib/frrcu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/frrcu.h b/lib/frrcu.h index 9f07a69b5226..81ab5528a9dd 100644 --- a/lib/frrcu.h +++ b/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, \ From 4836ac071409bfff65f833ce82bd5f0338d0a8ae Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 20 Jun 2024 10:56:18 +0200 Subject: [PATCH 3/5] tests: silence TSAN warning on test_seqlock exit TSAN warns about leaving the second thread dangling. Doesn't really matter, but just add a pthread_join to get rid of the warning. Signed-off-by: David Lamparter --- tests/lib/test_seqlock.c | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/lib/test_seqlock.c b/tests/lib/test_seqlock.c index 35501cbd4a6a..937b3f34f55f 100644 --- a/tests/lib/test_seqlock.c +++ b/tests/lib/test_seqlock.c @@ -111,4 +111,5 @@ int main(int argc, char **argv) writestr("main @release\n"); seqlock_release(&sqlo); sleep(1); + pthread_join(thr1, NULL); } From b9541fe77fcb706c3f265d704e15f810b0a98c14 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 20 Jun 2024 11:13:20 +0200 Subject: [PATCH 4/5] lib: use seqlock slow path with TSAN TSAN doesn't understand the OS specific "fast" seqlock code. Use the pthread mutex/condvar based path when TSAN is enabled. Signed-off-by: David Lamparter --- lib/seqlock.c | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/lib/seqlock.c b/lib/seqlock.c index 62ce316920dd..e74e6718bfef 100644 --- a/lib/seqlock.c +++ b/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() */ From e14c94f2b77692126210f4b7b51cf097d7f847e0 Mon Sep 17 00:00:00 2001 From: David Lamparter Date: Thu, 20 Jun 2024 12:02:25 +0200 Subject: [PATCH 5/5] tests: fix TSAN warnings in atomlist test The atomlist test consists of a sequence of (MT) sub-tests, from which counters are collected and verified. TSAN doesn't know that these counters are synchronized by way of the sub-test starting and finishing, so it complains. Just use atomics to get rid of the warning. (This is solely an issue with the test, not the atomlist code. There are no warnings from that.) Signed-off-by: David Lamparter --- tests/lib/test_atomlist.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/lib/test_atomlist.c b/tests/lib/test_atomlist.c index b50216cf9292..afcfa9879138 100644 --- a/tests/lib/test_atomlist.c +++ b/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 { @@ -97,10 +97,10 @@ static void trfunc_##name(unsigned int offset) \ { \ size_t i = 0, n = 0; -#define endtestrun \ - thr[offset].counter = i; \ - thr[offset].nullops = n; \ -} +#define endtestrun \ + atomic_store_explicit(&thr[offset].counter, i, memory_order_seq_cst); \ + 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; - n += thr[i].nullops; - thr[i].counter = 0; - thr[i].nullops = 0; + s += atomic_load_explicit(&thr[i].counter, memory_order_seq_cst); + n += atomic_load_explicit(&thr[i].nullops, memory_order_seq_cst); + atomic_store_explicit(&thr[i].counter, 0, memory_order_seq_cst); + atomic_store_explicit(&thr[i].nullops, 0, memory_order_seq_cst); } delta = monotime_since(&tv, NULL);