[lttng-dev] userspace-rcu and ThreadSanitizer

[lttng-dev] userspace-rcu and ThreadSanitizer

[Ondřej Surý via lttng-dev]

Hey,

we use ThreadSanitizer in BIND 9 CI quite extensively and with userspace-rcu
it's lit like an American house on Saturnalia ;).

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().

  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


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.

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);
}

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?)

~~~~

3. As an extra bonus, this is going to be needed with clang-17 as noreturn is now
reserved word:

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

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

[Mathieu Desnoyers via lttng-dev]

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

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

[Ondřej Surý via lttng-dev]

> On 17. 3. 2023, at 14:44, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> Sure, can you please submit the patch as a separate email with subject/commit message/signed-off-by tag ?


https://gitlab.isc.org/isc-projects/userspace-rcu/-/merge_requests/1.patch

Would this work for you?

Or do you need to have the patch attached?

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

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

[Mathieu Desnoyers via lttng-dev]

On 2023-03-17 11:50, Ondřej Surý wrote:
>> On 17. 3. 2023, at 14:44, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>> Sure, can you please submit the patch as a separate email with subject/commit message/signed-off-by tag ?
> 
> 
> https://gitlab.isc.org/isc-projects/userspace-rcu/-/merge_requests/1.patch
> 
> Would this work for you?
> 
> Or do you need to have the patch attached?

Having the patch attached (e.g. using git send-email) would be better, 
but I don't mind downloading the file for this time. Merged into liburcu 
master branch, thanks!

Mathieu

> 
> Ondrej
> --
> Ondřej Surý (He/Him)
> ondrej@sury.org
> 

-- 
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

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

[Duncan Sands via lttng-dev]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]

Hi Ondřej,

> 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().

I'm using the attached patch which does much the same.

Ciao, Duncan.

[-- Attachment #2: pacify_tsan.diff --]
[-- Type: text/x-patch, Size: 1836 bytes --]

diff --git a/External/UserspaceRCU/userspace-rcu/include/urcu/system.h b/External/UserspaceRCU/userspace-rcu/include/urcu/system.h
index faae390554..82898d1dbe 100644
--- a/External/UserspaceRCU/userspace-rcu/include/urcu/system.h
+++ b/External/UserspaceRCU/userspace-rcu/include/urcu/system.h
@@ -26,34 +26,45 @@
  * Identify a shared load. A cmm_smp_rmc() or cmm_smp_mc() should come
  * before the load.
  */
-#define _CMM_LOAD_SHARED(p)	       CMM_ACCESS_ONCE(p)
+#define _CMM_LOAD_SHARED(p)					\
+	__extension__						\
+	({							\
+		__typeof__(p) v;				\
+		__atomic_load(&p, &v, __ATOMIC_RELAXED);	\
+		v;						\
+	})
 
 /*
  * Load a data from shared memory, doing a cache flush if required.
  */
-#define CMM_LOAD_SHARED(p)			\
-	__extension__			\
-	({				\
-		cmm_smp_rmc();		\
-		_CMM_LOAD_SHARED(p);	\
+#define CMM_LOAD_SHARED(p)					\
+	__extension__						\
+	({							\
+		__typeof__(p) v;				\
+		__atomic_load(&p, &v, __ATOMIC_ACQUIRE);	\
+		v;						\
 	})
 
 /*
  * Identify a shared store. A cmm_smp_wmc() or cmm_smp_mc() should
  * follow the store.
  */
-#define _CMM_STORE_SHARED(x, v)	__extension__ ({ CMM_ACCESS_ONCE(x) = (v); })
+#define _CMM_STORE_SHARED(x, v)					\
+	__extension__						\
+	({							\
+		__typeof__(x) w = v;				\
+		__atomic_store(&x, &w, __ATOMIC_RELAXED);	\
+	})
 
 /*
  * Store v into x, where x is located in shared memory. Performs the
  * required cache flush after writing. Returns v.
  */
-#define CMM_STORE_SHARED(x, v)						\
-	__extension__							\
-	({								\
-		__typeof__(x) _v = _CMM_STORE_SHARED(x, v);		\
-		cmm_smp_wmc();						\
-		_v = _v;	/* Work around clang "unused result" */	\
+#define CMM_STORE_SHARED(x, v)					\
+	__extension__						\
+	({							\
+		__typeof__(x) w = v;				\
+		__atomic_store(&x, &w, __ATOMIC_RELEASE);	\
 	})
 
 #endif /* _URCU_SYSTEM_H */

[-- Attachment #3: Type: text/plain, Size: 156 bytes --]

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

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

[Ondřej Surý via lttng-dev]

> On 17. 3. 2023, at 14:44, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 
> 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). 

Here's very preliminary implementation:

https://gitlab.isc.org/isc-projects/userspace-rcu/-/merge_requests/2

I just did something wrong somewhere along the path and it doesn't compile now,
but it did for me locally.

I am submitting this now as it's 18:00 Friday evening and my kids are starting to
be angry at me :).

This will need some more work - I think some of the cmm_ macros might be dropped
now, and somebody who does that more often than I should take a look at the memory
orderings.

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

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

[Mathieu Desnoyers via lttng-dev]

On 2023-03-17 13:02, Ondřej Surý wrote:
>> On 17. 3. 2023, at 14:44, Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
>>
>> 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).
> 
> Here's very preliminary implementation:
> 
> https://gitlab.isc.org/isc-projects/userspace-rcu/-/merge_requests/2
> 
> I just did something wrong somewhere along the path and it doesn't compile now,
> but it did for me locally.
> 
> I am submitting this now as it's 18:00 Friday evening and my kids are starting to
> be angry at me :).
> 
> This will need some more work - I think some of the cmm_ macros might be dropped
> now, and somebody who does that more often than I should take a look at the memory
> orderings.

A few comments:

cmm_barrier() should rather be __atomic_signal_fence().

Also I notice this macro pattern (coding style):

#define uatomic_set(addr, v) __atomic_store_n((addr), (v), __ATOMIC_RELEASE)

The extra parentheses for parameters are not needed, because the comma is pretty
much the last operator in terms of priority. The following would be preferred
specifically because those are separated by comma:

#define uatomic_set(addr, v) __atomic_store_n(addr, v, __ATOMIC_RELEASE)

Our memory barrier semantic are similar to the Linux kernel, where the following
imply ACQ_REL because they return something: cmpxchg, add_return, sub_return, xchg.

The rest (add, sub, and, or, inc, dec) are __ATOMIC_RELAXED. Note that
cmm_smp_mb__before/after_uatomic_*() need to be implemented as
__atomic_thread_fence(__ATOMIC_ACQ_REL).

There are some architectures where we will want to keep a specialized version
of those add, sub, and, or, inc, dec operations which include the ACQ_REL semantic,
e.g. x86, where this is implied by the LOCK prefix. For those the cmm_smp_mb__before/after_uatomic_*()
will be no-ops.

The CMM_STORE_SHARED is not meant to have a RELEASE semantic. It is meant to
update variables that don't need the release ordering. The ATOMIC_CONSUME was
not the intent at the CMM_LOAD_SHARED level neither.

(this is just from looking around at the patches, it would be better if we can have the
patches posted to the mailing list for further discussion)

Thanks!

Mathieu


> 
> Ondrej
> --
> Ondřej Surý (He/Him)
> ondrej@sury.org
> 

-- 
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