From 275741694acc5aa7a732c1476c5eb2af015e7a3e Mon Sep 17 00:00:00 2001 From: Dennis Potman Date: Wed, 6 Sep 2023 18:00:13 +0200 Subject: [PATCH] Fix domain confusion in garbage collector caused by cdds_psmx plugin Signed-off-by: Dennis Potman --- src/core/ddsc/tests/psmx_cdds_impl.c | 3 ++ src/core/ddsi/include/dds/ddsi/ddsi_thread.h | 30 ++++++++++++++++++++ src/core/ddsi/src/ddsi_gc.c | 14 +++++++-- src/core/ddsi/src/ddsi_thread.c | 9 ++++++ src/ddsrt/CMakeLists.txt | 4 +++ src/ddsrt/include/dds/features.h.in | 3 ++ 6 files changed, 61 insertions(+), 2 deletions(-) diff --git a/src/core/ddsc/tests/psmx_cdds_impl.c b/src/core/ddsc/tests/psmx_cdds_impl.c index f1308dae91..83d59fd7e3 100644 --- a/src/core/ddsc/tests/psmx_cdds_impl.c +++ b/src/core/ddsc/tests/psmx_cdds_impl.c @@ -18,6 +18,7 @@ #include "dds/ddsrt/strtol.h" #include "dds/ddsi/ddsi_locator.h" #include "dds/ddsi/ddsi_protocol.h" +#include "dds/ddsi/ddsi_thread.h" #include "dds/ddsc/dds_psmx.h" #include "psmx_cdds_impl.h" #include "psmx_cdds_data.h" @@ -507,6 +508,8 @@ dds_return_t cdds_create_psmx (dds_psmx_t **psmx_out, dds_psmx_instance_id_t ins { assert (psmx_out); + ddsrt_atomic_st32 (&dds_nested_gv_allowed, 1); + struct cdds_psmx *psmx = dds_alloc (sizeof (*psmx)); psmx->c.instance_name = dds_string_dup ("cdds-psmx"); psmx->c.instance_id = instance_id; diff --git a/src/core/ddsi/include/dds/ddsi/ddsi_thread.h b/src/core/ddsi/include/dds/ddsi/ddsi_thread.h index aec1cc7e49..fa291a3ef0 100644 --- a/src/core/ddsi/include/dds/ddsi/ddsi_thread.h +++ b/src/core/ddsi/include/dds/ddsi/ddsi_thread.h @@ -73,10 +73,24 @@ void ddsi_thread_vtime_trace (struct ddsi_thread_state *thrst); #define ddsi_thread_vtime_trace(thrst) do { } while (0) #endif /* DDSI_THREAD_DEBUG */ +/** + * When DDS_ALLOW_NESTED_DOMAIN is set, the thread state can store an additional + * gv pointer, so that in case of domain nesting in a single thread, the gv + * pointer of the nested domain won't overwrite the top-level domain's gv pointer. + * Nesting domains has several limitations, and is used for PSMX testing using the + * Cyclone DDS PSMX implementation. + */ +#ifdef DDS_ALLOW_NESTED_DOMAIN +#define THREAD_BASE_NESTEDGV ddsrt_atomic_voidp_t nested_gv; +#else +#define THREAD_BASE_NESTEDGV +#endif + #define THREAD_BASE \ ddsrt_atomic_uint32_t vtime; \ enum ddsi_thread_state_kind state; \ ddsrt_atomic_voidp_t gv; \ + THREAD_BASE_NESTEDGV \ ddsrt_thread_t tid; \ uint32_t (*f) (void *arg); \ void *f_arg; \ @@ -115,6 +129,10 @@ extern ddsrt_thread_local struct ddsi_thread_state *tsd_thread_state; DDS_EXPORT extern ddsrt_thread_local struct ddsi_thread_state *tsd_thread_state; #endif +#ifdef DDS_ALLOW_NESTED_DOMAIN +extern ddsrt_atomic_uint32_t dds_nested_gv_allowed; +#endif + /** @component thread_support */ void ddsi_thread_states_init (void); @@ -190,7 +208,19 @@ inline void ddsi_thread_state_awake (struct ddsi_thread_state *thrst, const stru assert (gv != NULL); assert (thrst->state != DDSI_THREAD_STATE_ALIVE || gv == ddsrt_atomic_ldvoidp (&thrst->gv)); ddsi_thread_vtime_trace (thrst); +#ifdef DDS_ALLOW_NESTED_DOMAIN + assert ((vt & DDSI_VTIME_NEST_MASK) == 0 || gv == ddsrt_atomic_ldvoidp (&thrst->gv) || gv == ddsrt_atomic_ldvoidp (&thrst->nested_gv) || 0 == ddsrt_atomic_ldvoidp (&thrst->nested_gv)); + if ((vt & DDSI_VTIME_NEST_MASK) == 0) + ddsrt_atomic_stvoidp (&thrst->gv, (struct ddsi_domaingv *) gv); + else if (gv != ddsrt_atomic_ldvoidp (&thrst->gv)) + { + assert (ddsrt_atomic_ld32 (&dds_nested_gv_allowed)); + ddsrt_atomic_stvoidp (&thrst->nested_gv, (struct ddsi_domaingv *) gv); + } +#else + assert ((vt & DDSI_VTIME_NEST_MASK) == 0 || gv == ddsrt_atomic_ldvoidp (&thrst->gv)); ddsrt_atomic_stvoidp (&thrst->gv, (struct ddsi_domaingv *) gv); +#endif ddsrt_atomic_fence_stst (); ddsrt_atomic_st32 (&thrst->vtime, vt + 1u); /* nested calls a rare and an extra fence doesn't break things */ diff --git a/src/core/ddsi/src/ddsi_gc.c b/src/core/ddsi/src/ddsi_gc.c index 7b74a12713..5be145ac69 100644 --- a/src/core/ddsi/src/ddsi_gc.c +++ b/src/core/ddsi/src/ddsi_gc.c @@ -55,7 +55,12 @@ static void threads_vtime_gather_for_wait (const struct ddsi_domaingv *gv, uint3 correct; if instead it has gone through another cycle since loading thrst[i].vtime, then the thread will be dropped from the live threads on the next check. So it won't ever wait with unknown duration for progres of threads stuck in another domain */ - if (gv == ddsrt_atomic_ldvoidp (&cur->thrst[i].gv)) +#ifdef DDS_ALLOW_NESTED_DOMAIN + const bool this_domain = gv == ddsrt_atomic_ldvoidp (&cur->thrst[i].gv) || gv == ddsrt_atomic_ldvoidp (&cur->thrst[i].nested_gv); +#else + const bool this_domain = gv == ddsrt_atomic_ldvoidp (&cur->thrst[i].gv); +#endif + if (this_domain) { assert (dstidx < nthreads); ivs[dstidx].thrst = &cur->thrst[i]; @@ -76,7 +81,12 @@ static int threads_vtime_check (const struct ddsi_domaingv *gv, uint32_t *nivs, { ddsi_vtime_t vtime = ddsrt_atomic_ld32 (&ivs[i].thrst->vtime); assert (ddsi_vtime_awake_p (ivs[i].vtime)); - if (!ddsi_vtime_gt (vtime, ivs[i].vtime) && ddsrt_atomic_ldvoidp (&ivs[i].thrst->gv) == gv) +#ifdef DDS_ALLOW_NESTED_DOMAIN + const bool this_domain = ddsrt_atomic_ldvoidp (&ivs[i].thrst->gv) == gv || ddsrt_atomic_ldvoidp (&ivs[i].thrst->nested_gv) == gv; +#else + const bool this_domain = ddsrt_atomic_ldvoidp (&ivs[i].thrst->gv) == gv; +#endif + if (!ddsi_vtime_gt (vtime, ivs[i].vtime) && this_domain) ++i; else { diff --git a/src/core/ddsi/src/ddsi_thread.c b/src/core/ddsi/src/ddsi_thread.c index 8c1bf5d62a..05f0a59ebb 100644 --- a/src/core/ddsi/src/ddsi_thread.c +++ b/src/core/ddsi/src/ddsi_thread.c @@ -27,6 +27,9 @@ struct ddsi_thread_states thread_states; ddsrt_thread_local struct ddsi_thread_state *tsd_thread_state; +#ifdef DDS_ALLOW_NESTED_DOMAIN +ddsrt_atomic_uint32_t dds_nested_gv_allowed; +#endif extern inline bool ddsi_vtime_awake_p (ddsi_vtime_t vtime); extern inline bool ddsi_vtime_asleep_p (ddsi_vtime_t vtime); @@ -114,6 +117,9 @@ void ddsi_thread_states_init (void) struct ddsi_thread_state * const thrst = ddsi_lookup_thread_state_real (); assert (ts0 == NULL || ts0 == thrst); (void) thrst; +#ifdef DDS_ALLOW_NESTED_DOMAIN + ddsrt_atomic_st32 (&dds_nested_gv_allowed, 0); +#endif } bool ddsi_thread_states_fini (void) @@ -300,6 +306,9 @@ static struct ddsi_thread_state *init_thread_state (const char *tname, const str assert (ddsi_vtime_asleep_p (ddsrt_atomic_ld32 (&thrst->vtime))); ddsrt_atomic_stvoidp (&thrst->gv, (struct ddsi_domaingv *) gv); +#ifdef DDS_ALLOW_NESTED_DOMAIN + ddsrt_atomic_stvoidp (&thrst->nested_gv, NULL); +#endif (void) ddsrt_strlcpy (thrst->name, tname, sizeof (thrst->name)); thrst->state = state; return thrst; diff --git a/src/ddsrt/CMakeLists.txt b/src/ddsrt/CMakeLists.txt index 86ffda30ef..d5c7863654 100644 --- a/src/ddsrt/CMakeLists.txt +++ b/src/ddsrt/CMakeLists.txt @@ -314,6 +314,10 @@ foreach(feature SSL SECURITY LIFESPAN DEADLINE_MISSED NETWORK_PARTITIONS set(DDS_HAS_${feature} ${ENABLE_${feature}}) endforeach() +if(BUILD_TESTING) + set(DDS_ALLOW_NESTED_DOMAIN 1) +endif() + configure_file(include/dds/config.h.in include/dds/config.h) configure_file(include/dds/features.h.in include/dds/features.h) configure_file(include/dds/version.h.in include/dds/version.h) diff --git a/src/ddsrt/include/dds/features.h.in b/src/ddsrt/include/dds/features.h.in index 3b8f280f5c..0bcf6f0bb1 100644 --- a/src/ddsrt/include/dds/features.h.in +++ b/src/ddsrt/include/dds/features.h.in @@ -38,4 +38,7 @@ /* Whether or not support for topic discovery is included */ #cmakedefine DDS_HAS_TOPIC_DISCOVERY 1 +/* Not for general use, specificly for testing psmx Cyclone DDS plugin */ +#cmakedefine DDS_ALLOW_NESTED_DOMAIN 1 + #endif