Re: [lttng-dev] userspace-rcu and ThreadSanitizer

[Mathieu Desnoyers via lttng-dev <lttng-dev@lists.lttng.org>]

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@sury.org
> 
> _______________________________________________
> lttng-dev mailing list
> lttng-dev@lists.lttng.org
> https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

-- 
Mathieu Desnoyers
EfficiOS Inc.
https://www.efficios.com

_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev