From 457eeeee4e2694bdd284a186405088f5ccdc6fa9 Mon Sep 17 00:00:00 2001 From: Mathieu Desnoyers Date: Fri, 10 Feb 2023 14:55:24 -0500 Subject: [PATCH] Fix: call_rcu: teardown default call_rcu worker on application exit Teardown the default call_rcu worker thread if there are no queued callbacks on process exit. This prevents leaking memory. Here is how an application can ensure graceful teardown of this worker thread: - An application queuing call_rcu callbacks should invoke rcu_barrier() before it exits. - When chaining call_rcu callbacks, the number of calls to rcu_barrier() on application exit must match at least the maximum number of chained callbacks. - If an application chains callbacks endlessly, it would have to be modified to stop chaining callbacks when it detects an application exit (e.g. with a flag), and wait for quiescence with rcu_barrier() after setting that flag. - The statements above apply to a library which queues call_rcu callbacks, only it needs to invoke rcu_barrier in its library destructor. Fixes: #1317 Signed-off-by: Mathieu Desnoyers Change-Id: I40556bc872d3df58a22fb88a0dbb528ce5c9b4af --- src/urcu-bp.c | 3 ++ src/urcu-call-rcu-impl.h | 85 ++++++++++++++++++++++++++++++++++------ src/urcu-qsbr.c | 2 + src/urcu.c | 26 ++++++------ 4 files changed, 93 insertions(+), 23 deletions(-) diff --git a/src/urcu-bp.c b/src/urcu-bp.c index 671becb..35ac9db 100644 --- a/src/urcu-bp.c +++ b/src/urcu-bp.c @@ -121,6 +121,7 @@ static void __attribute__((constructor)) _urcu_bp_init(void); static void __attribute__((destructor)) urcu_bp_exit(void); +static void urcu_call_rcu_exit(void); #ifndef CONFIG_RCU_FORCE_SYS_MEMBARRIER int urcu_bp_has_sys_membarrier; @@ -662,6 +663,8 @@ void _urcu_bp_init(void) static void urcu_bp_exit(void) { + urcu_call_rcu_exit(); + mutex_lock(&init_lock); if (!--urcu_bp_refcount) { struct registry_chunk *chunk, *tmp; diff --git a/src/urcu-call-rcu-impl.h b/src/urcu-call-rcu-impl.h index e34faee..15c1ae6 100644 --- a/src/urcu-call-rcu-impl.h +++ b/src/urcu-call-rcu-impl.h @@ -452,8 +452,8 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp, cds_list_add(&crdp->list, &call_rcu_data_list); crdp->cpu_affinity = cpu_affinity; crdp->gp_count = 0; - cmm_smp_mb(); /* Structure initialized before pointer is planted. */ - *crdpp = crdp; + rcu_set_pointer(crdpp, crdp); + ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp); if (ret) urcu_die(ret); @@ -576,22 +576,27 @@ int alias_set_cpu_call_rcu_data(); /* * Return a pointer to the default call_rcu_data structure, creating - * one if need be. Because we never free call_rcu_data structures, - * we don't need to be in an RCU read-side critical section. + * one if need be. + * + * The call to this function with intent to use the returned + * call_rcu_data should be protected by RCU read-side lock. */ struct call_rcu_data *get_default_call_rcu_data(void) { - if (default_call_rcu_data != NULL) - return rcu_dereference(default_call_rcu_data); + struct call_rcu_data *crdp; + + crdp = rcu_dereference(default_call_rcu_data); + if (crdp != NULL) + return crdp; + call_rcu_lock(&call_rcu_mutex); - if (default_call_rcu_data != NULL) { - call_rcu_unlock(&call_rcu_mutex); - return default_call_rcu_data; - } - call_rcu_data_init(&default_call_rcu_data, 0, -1); + if (default_call_rcu_data == NULL) + call_rcu_data_init(&default_call_rcu_data, 0, -1); + crdp = default_call_rcu_data; call_rcu_unlock(&call_rcu_mutex); - return default_call_rcu_data; + + return crdp; } URCU_ATTR_ALIAS(urcu_stringify(get_default_call_rcu_data)) struct call_rcu_data *alias_get_default_call_rcu_data(); @@ -1099,6 +1104,62 @@ void urcu_unregister_rculfhash_atfork(struct urcu_atfork *atfork __attribute__(( { urcu_die(EPERM); } + URCU_ATTR_ALIAS(urcu_stringify(urcu_unregister_rculfhash_atfork)) __attribute__((noreturn)) void alias_urcu_unregister_rculfhash_atfork(); + +/* + * Teardown the default call_rcu worker thread if there are no queued + * callbacks on process exit. This prevents leaking memory. + * + * Here is how an application can ensure graceful teardown of this + * worker thread: + * + * - An application queuing call_rcu callbacks should invoke + * rcu_barrier() before it exits. + * - When chaining call_rcu callbacks, the number of calls to + * rcu_barrier() on application exit must match at least the maximum + * number of chained callbacks. + * - If an application chains callbacks endlessly, it would have to be + * modified to stop chaining callbacks when it detects an application + * exit (e.g. with a flag), and wait for quiescence with rcu_barrier() + * after setting that flag. + * - The statements above apply to a library which queues call_rcu + * callbacks, only it needs to invoke rcu_barrier in its library + * destructor. + * + * Note that this function does not presume it is being called when the + * application is single-threaded even though this is invoked from a + * destructor: this function synchronizes against concurrent calls to + * get_default_call_rcu_data(). + */ +static void urcu_call_rcu_exit(void) +{ + struct call_rcu_data *crdp; + bool teardown = true; + + if (default_call_rcu_data == NULL) + return; + call_rcu_lock(&call_rcu_mutex); + /* + * If the application leaves callbacks in the default call_rcu + * worker queue, keep the default worker in place. + */ + crdp = default_call_rcu_data; + if (!crdp) { + teardown = false; + goto unlock; + } + if (!cds_wfcq_empty(&crdp->cbs_head, &crdp->cbs_tail)) { + teardown = false; + goto unlock; + } + rcu_set_pointer(&default_call_rcu_data, NULL); +unlock: + call_rcu_unlock(&call_rcu_mutex); + if (teardown) { + synchronize_rcu(); + call_rcu_data_free(crdp); + } +} diff --git a/src/urcu-qsbr.c b/src/urcu-qsbr.c index 5572d39..e5ac053 100644 --- a/src/urcu-qsbr.c +++ b/src/urcu-qsbr.c @@ -53,6 +53,7 @@ #define _LGPL_SOURCE void __attribute__((destructor)) urcu_qsbr_exit(void); +static void urcu_call_rcu_exit(void); /* * rcu_gp_lock ensures mutual exclusion between threads calling @@ -524,6 +525,7 @@ void urcu_qsbr_exit(void) * readers, and left running at exit. * assert(cds_list_empty(®istry)); */ + urcu_call_rcu_exit(); } URCU_ATTR_ALIAS("urcu_qsbr_exit") void rcu_exit_qsbr(); diff --git a/src/urcu.c b/src/urcu.c index a8c033a..d81e84f 100644 --- a/src/urcu.c +++ b/src/urcu.c @@ -113,9 +113,11 @@ void alias_rcu_init(void); static int init_done; void __attribute__((constructor)) rcu_init(void); -void __attribute__((destructor)) rcu_exit(void); #endif +void __attribute__((destructor)) rcu_exit(void); +static void urcu_call_rcu_exit(void); + /* * rcu_gp_lock ensures mutual exclusion between threads calling * synchronize_rcu(). @@ -671,22 +673,24 @@ void rcu_init(void) URCU_ATTR_ALIAS(urcu_stringify(rcu_init)) void alias_rcu_init(void); +/* + * Don't unregister the SIGRCU signal handler anymore, because + * call_rcu threads could still be using it shortly before the + * application exits. + * Assertion disabled because call_rcu threads are now rcu + * readers, and left running at exit. + * assert(cds_list_empty(®istry)); + */ + +#endif /* #ifdef RCU_SIGNAL */ + void rcu_exit(void) { - /* - * Don't unregister the SIGRCU signal handler anymore, because - * call_rcu threads could still be using it shortly before the - * application exits. - * Assertion disabled because call_rcu threads are now rcu - * readers, and left running at exit. - * assert(cds_list_empty(®istry)); - */ + urcu_call_rcu_exit(); } URCU_ATTR_ALIAS(urcu_stringify(rcu_exit)) void alias_rcu_exit(void); -#endif /* #ifdef RCU_SIGNAL */ - DEFINE_RCU_FLAVOR(rcu_flavor); DEFINE_RCU_FLAVOR_ALIAS(rcu_flavor, alias_rcu_flavor); -- 2.34.1