[lttng-dev] userspace-rcu and ThreadSanitizer
Mathieu Desnoyers
mathieu.desnoyers at efficios.com
Fri Mar 17 09:44:22 EDT 2023
On 2023-03-14 08:26, Ondřej Surý via lttng-dev wrote:
> Hey,
>
> we use ThreadSanitizer in BIND 9 CI quite extensively and with userspace-rcu
> it's lit like an American house on Saturnalia ;).
Haha, I have no doubt about it. Userspace RCU is all about concurrent
accesses, and so far possesses no TSAN annotations.
>
> I have two questions:
>
> 1. I've tried to help TSAN by replacing the custom atomics with __atomic gcc
> primitives - that seems to work pretty well. Note that using C11 stdatomics
> is frankly not possible here because it would require wrapping everything into
> _Atomic().
Agreed. gcc __atomic seems to be the way to go.
>
> Do you want me to contribute this back? And how should I plug this into the
> existing structure? This touches:
>
> include/urcu/static/pointer.h
> include/urcu/system.h
> include/urcu/uatomic.h
I would indeed like to remove all the custom atomics assembly code from
liburcu now that there are good atomics support in the major compilers
(gcc and clang). I am also tempted to bump the base compiler version
requirements (for both gcc and clang) to something less ancient than
what we have currently for the next liburcu releases if it helps us rely
on non-buggy atomics implementations. The currently supported compilers
are stated in the README.md file:
"Linux ARM depends on running a Linux kernel 2.6.15 or better, GCC 4.4
or better.
The C compiler used needs to support at least C99. The C++ compiler used
needs to support at least C++11.
The GCC compiler versions 3.3, 3.4, 4.0, 4.1, 4.2, 4.3, 4.4 and 4.5 are
supported, with the following exceptions:
- GCC 3.3 and 3.4 have a bug that prevents them from generating volatile
accesses to offsets in a TLS structure on 32-bit x86. These
versions are
therefore not compatible with `liburcu` on x86 32-bit
(i386, i486, i586, i686).
The problem has been reported to the GCC community:
<http://www.mail-archive.com/gcc-bugs@gcc.gnu.org/msg281255.html>
- GCC 3.3 cannot match the "xchg" instruction on 32-bit x86 build.
See <http://kerneltrap.org/node/7507>
- Alpha, ia64 and ARM architectures depend on GCC 4.x with atomic
builtins
support. For ARM this was introduced with GCC 4.4:
<http://gcc.gnu.org/gcc-4.4/changes.html>.
- Linux aarch64 depends on GCC 5.1 or better because prior versions
perform unsafe access to deallocated stack.
Clang version 3.0 (based on LLVM 3.0) is supported."
For gcc, I wonder if gcc-4.8 has appropriate support for __atomic on all
supported architectures supported by liburcu ?
I also wonder what would be a good conservative baseline version for clang.
As we introduce a newer compiler baseline version, I would be tempted to
add a compiler version detection in include/urcu/compiler.h and #warn
whenever the compiler is too old. This is similar to what we do for the
compiler disallow list with URCU_GCC_VERSION, but enforced with a
warning rather than a #error. The last thing I want is to end up wasting
people's time due to compiling with a buggy old compiler, so I favor a
"fail early" approach.
>
>
> 2. I know there's KTSAN, so it must work somehow, but was there any success
> on using ThreadSanitizer on projects using Userspace-RCU? It mostly seems
> to highlight the CDS parts of the code.
Not AFAIK.
>
> I can help TSAN to understand some of the code or suppress some of the warnings,
> but I do want to prevent the code to be full of stuff like this:
>
> static void
> destroy_adbname_rcu_head(struct rcu_head *rcu_head) {
> dns_adbname_t *adbname = caa_container_of(rcu_head, dns_adbname_t,
> rcu_head);
>
> #ifdef __SANITIZE_THREAD__
> SPINLOCK(&adbname->lock);
> SPINUNLOCK(&adbname->lock);
> #endif
>
> destroy_adbname(adbname);
> }
Indeed, we'd want to improve the liburcu header files and implementation
by adding the appropriate annotation there.
>
> I am absolutely sure that the adbname can be destroyed here (because of the
> reference counting), but TSAN had a problem with it. Doing the "fake" barrier
> with a spinlock here made it stop consider this to be a data race.
>
> I also had to disable the auto_resize of cds_lfht when running under TSAN.
>
> I am also worried that by hiding some code from TSAN, we might miss a legitimate
> error.
>
> All I found using Google was this notice from 2014:
> https://www.mail-archive.com/valgrind-users@lists.sourceforge.net/msg05121.html
>
> and perhaps this:
> https://github.com/google/sanitizers/issues/1415
>
> (Perhaps, I should look into annotating urcu code with TSAN annotations?)
Yes, I suspect we'll want to add TSAN annotation to liburcu code, and
perhaps Helgrind and DRD annotations as well while we are at it. Those
tools are very valuable development tools, which makes it worthwhile to
add the relevant annotations to help them figure out liburcu intricacies.
>
> ~~~~
>
> 3. As an extra bonus, this is going to be needed with clang-17 as noreturn is now
> reserved word:
Sure, can you please submit the patch as a separate email with
subject/commit message/signed-off-by tag ?
Thanks!
Mathieu
>
> diff --git a/include/urcu/uatomic/generic.h b/include/urcu/uatomic/generic.h
> index 89d1cfa..c3762b0 100644
> --- a/include/urcu/uatomic/generic.h
> +++ b/include/urcu/uatomic/generic.h
> @@ -38,7 +38,7 @@ extern "C" {
> #endif
>
> #if !defined __OPTIMIZE__ || defined UATOMIC_NO_LINK_ERROR
> -static inline __attribute__((always_inline, noreturn))
> +static inline __attribute__((always_inline, __noreturn__))
> void _uatomic_link_error(void)
> {
> #ifdef ILLEGAL_INSTR
> diff --git a/src/urcu-call-rcu-impl.h b/src/urcu-call-rcu-impl.h
> index 187727e..cc76f53 100644
> --- a/src/urcu-call-rcu-impl.h
> +++ b/src/urcu-call-rcu-impl.h
> @@ -1055,7 +1055,7 @@ void urcu_register_rculfhash_atfork(struct urcu_atfork *atfork)
> * This unregistration function is deprecated, meant only for internal
> * use by rculfhash.
> */
> -__attribute__((noreturn))
> +__attribute__((__noreturn__))
> void urcu_unregister_rculfhash_atfork(struct urcu_atfork *atfork __attribute__((unused)))
> {
> urcu_die(EPERM);
>
>
> Ondrej
> --
> Ondřej Surý (He/Him)
> ondrej at sury.org
>
> _______________________________________________
> lttng-dev mailing list
> lttng-dev at lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
--
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com
More information about the lttng-dev
mailing list