Re: [lttng-dev] URCU background threads vs signalfd
On 2022-09-23 18:05, Eric Wong wrote: Mathieu Desnoyers wrote: On 2022-09-23 13:55, Eric Wong wrote: Mathieu Desnoyers wrote: On 2022-09-22 05:15, Eric Wong via lttng-dev wrote: Hello, I'm using urcu-bp + rculfhash + call_rcu to implement malloc instrumentation (via LD_PRELOAD) on an existing single-threaded Perl codebase which uses Linux signalfd. signalfd depends on signals being blocked in all threads of the process, otherwise threads with unblocked signals can receive them and starve the signalfd. While some threads in URCU do block signals (e.g. workqueue worker for rculfhash), the call_rcu thread and rculfhash partition_resize_helper threads do not... Should all threads URCU creates block signals (aside from SIGRCU)? Yes, I think you are right. The SIGRCU signal is only needed for the urcu-signal flavor though. Would you like to submit a patch ? Sure. Is there a way to detect at runtime when urcu-signal is in use so SIGRCU (SIGUSR1) doesn't get unblocked when using other flavors? I actually use SIGUSR1 in my signalfd-using codebase. I also want to remove cds_lfht_worker_init entirely since it's racy. Signal blocking needs to be done in the parent before pthread_create to avoid a window where the child has unblocked signals. Thanks. Anyways, this is my work-in-progress: Perhaps with this on top of your wip patch ? The idea is to always block all signals before creating threads, and only unblock SIGRCU when registering a urcu-signal thread. (compile-tested only) Thanks, that makes sense. It passes: make check short_bench My original signalfd + urcu-bp case works well, too, with my constructor workarounds reverted. (I ported our patches ported to to 0.10.2 for Debian buster (oldstable)). I don't know if the existing test coverage is sufficient, though. Waiting on regtest... Do you mind if I fold our 2 patches together with a Co-developed-by tag and use your Signed-off-by ? Then the question that will arise is whether this change is sufficiently self-contained that we can push it to stable branches, or if it's master branch material only. Thoughts ? Thanks, Mathieu -- 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
[lttng-dev] [RFC PATCH urcu] Disable signals in URCU background threads
Applications using signalfd depend on signals being blocked in all
threads of the process, otherwise threads with unblocked signals
can receive them and starve the signalfd.
While some threads in URCU do block signals (e.g. workqueue
worker for rculfhash), the call_rcu, defer_rcu, and rculfhash
partition_resize_helper threads do not.
Always block all signals before creating threads, and only unblock
SIGRCU when registering a urcu-signal thread. Restore the SIGRCU
signal to its pre-registration blocked state on unregistration.
For rculfhash, cds_lfht_worker_init can be removed, because its only
effect is to block all signals except SIGRCU. Blocking all signals is
already done by the workqueue code, and unbloking SIGRCU is now done by
the urcu signal flavor thread regisration.
Co-developed-by: Eric Wong
Signed-off-by: Eric Wong
Signed-off-by: Mathieu Desnoyers
Change-Id: If78346b15bdc287417b992a8963098c6ea0dc7d2
---
src/rculfhash.c | 36 ++
src/urcu-call-rcu-impl.h | 10 +
src/urcu-defer-impl.h| 9
src/urcu.c | 48
src/workqueue.c | 21 ++
5 files changed, 100 insertions(+), 24 deletions(-)
diff --git a/src/rculfhash.c b/src/rculfhash.c
index 7c0b9fb8..a41cac83 100644
--- a/src/rculfhash.c
+++ b/src/rculfhash.c
@@ -1251,6 +1251,7 @@ void partition_resize_helper(struct cds_lfht *ht,
unsigned long i,
struct partition_resize_work *work;
int ret;
unsigned long thread, nr_threads;
+ sigset_t newmask, oldmask;
urcu_posix_assert(nr_cpus_mask != -1);
if (nr_cpus_mask < 0 || len < 2 * MIN_PARTITION_PER_THREAD)
@@ -1273,6 +1274,12 @@ void partition_resize_helper(struct cds_lfht *ht,
unsigned long i,
dbg_printf("error allocating for resize, single-threading\n");
goto fallback;
}
+
+ ret = sigfillset(&newmask);
+ urcu_posix_assert(!ret);
+ ret = pthread_sigmask(SIG_BLOCK, &newmask, &oldmask);
+ urcu_posix_assert(!ret);
+
for (thread = 0; thread < nr_threads; thread++) {
work[thread].ht = ht;
work[thread].i = i;
@@ -1294,6 +1301,10 @@ void partition_resize_helper(struct cds_lfht *ht,
unsigned long i,
}
urcu_posix_assert(!ret);
}
+
+ ret = pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+ urcu_posix_assert(!ret);
+
for (thread = 0; thread < nr_threads; thread++) {
ret = pthread_join(work[thread].thread_id, NULL);
urcu_posix_assert(!ret);
@@ -2163,29 +2174,6 @@ static struct urcu_atfork cds_lfht_atfork = {
.after_fork_child = cds_lfht_after_fork_child,
};
-/*
- * Block all signals for the workqueue worker thread to ensure we don't
- * disturb the application. The SIGRCU signal needs to be unblocked for
- * the urcu-signal flavor.
- */
-static void cds_lfht_worker_init(
- struct urcu_workqueue *workqueue __attribute__((unused)),
- void *priv __attribute__((unused)))
-{
- int ret;
- sigset_t mask;
-
- ret = sigfillset(&mask);
- if (ret)
- urcu_die(errno);
- ret = sigdelset(&mask, SIGRCU);
- if (ret)
- urcu_die(errno);
- ret = pthread_sigmask(SIG_SETMASK, &mask, NULL);
- if (ret)
- urcu_die(ret);
-}
-
static void cds_lfht_init_worker(const struct rcu_flavor_struct *flavor)
{
flavor->register_rculfhash_atfork(&cds_lfht_atfork);
@@ -2194,7 +2182,7 @@ static void cds_lfht_init_worker(const struct
rcu_flavor_struct *flavor)
if (cds_lfht_workqueue_user_count++)
goto end;
cds_lfht_workqueue = urcu_workqueue_create(0, -1, NULL,
- NULL, cds_lfht_worker_init, NULL, NULL, NULL, NULL, NULL);
+ NULL, NULL, NULL, NULL, NULL, NULL, NULL);
end:
mutex_unlock(&cds_lfht_fork_mutex);
}
diff --git a/src/urcu-call-rcu-impl.h b/src/urcu-call-rcu-impl.h
index e9366b42..9f85d55b 100644
--- a/src/urcu-call-rcu-impl.h
+++ b/src/urcu-call-rcu-impl.h
@@ -434,6 +434,7 @@ static void call_rcu_data_init(struct call_rcu_data **crdpp,
{
struct call_rcu_data *crdp;
int ret;
+ sigset_t newmask, oldmask;
crdp = malloc(sizeof(*crdp));
if (crdp == NULL)
@@ -448,9 +449,18 @@ static void call_rcu_data_init(struct call_rcu_data
**crdpp,
crdp->gp_count = 0;
cmm_smp_mb(); /* Structure initialized before pointer is planted. */
*crdpp = crdp;
+
+ ret = sigfillset(&newmask);
+ urcu_posix_assert(!ret);
+ ret = pthread_sigmask(SIG_BLOCK, &newmask, &oldmask);
+ urcu_posix_assert(!ret);
+
ret = pthread_create(&crdp->tid, NULL, call_rcu_thread, crdp);
if (ret)
urcu_die(ret);
+
+ ret = pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+ urcu_posix_assert(!r
Re: [lttng-dev] URCU background threads vs signalfd
Mathieu Desnoyers wrote: > Do you mind if I fold our 2 patches together with a Co-developed-by tag and > use your Signed-off-by ? Looks good to me, thanks. > Then the question that will arise is whether this change is sufficiently > self-contained that we can push it to stable branches, or if it's master > branch material only. It seems self-contained enough and fixes a real problem encountered in real-world usage, so I think it's suitable for stable. Thanks. ___ lttng-dev mailing list lttng-dev@lists.lttng.org https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev
Re: [lttng-dev] URCU background threads vs signalfd
On 2022-09-26 15:58, Eric Wong wrote: Mathieu Desnoyers wrote: Do you mind if I fold our 2 patches together with a Co-developed-by tag and use your Signed-off-by ? Looks good to me, thanks. Then the question that will arise is whether this change is sufficiently self-contained that we can push it to stable branches, or if it's master branch material only. It seems self-contained enough and fixes a real problem encountered in real-world usage, so I think it's suitable for stable. That's tricky. I try very hard not to introduce any user-observable regressions in stable branches. I can very well imagine an application which, for its own peculiar reasons, would expect signals to be unblocked in call_rcu worker thread callbacks. On the other hand, those worker threads have always had the behavior of having signals unblocked so far, therefore an application that wants to use signalfd() today with those liburcu worker threads already needs to take some care about how those threads get created. Therefore, it's not a small self-contained change: it has effects on the blocked signal masks that are observable from the application call_rcu callbacks. So far, applications which expect to use signalfd() in a non-broken way need to ensure they either have the work-around in place around thread creation, or will need to add a dependency on an upcoming liburcu version (e.g. 0.14). For all those reasons, I think it would be a good time to do a liburcu 0.14 release soon, and I don't think backporting this patch makes sense for stable branches of liburcu. Thanks, Mathieu -- 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
