Re: [lttng-dev] URCU background threads vs signalfd

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

On 2022-09-23 13:55, Eric Wong wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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)

diff --git a/src/rculfhash.c b/src/rculfhash.c
index 5f455af3..a41cac83 100644
--- a/src/rculfhash.c
+++ b/src/rculfhash.c
@@ -2174,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);
@@ -2205,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.c b/src/urcu.c
index 59f2e8f1..cf4d6d03 100644
--- a/src/urcu.c
+++ b/src/urcu.c
@@ -110,6 +110,8 @@ static int init_done;

  void __attribute__((constructor)) rcu_init(void);
  void __attribute__((destructor)) rcu_exit(void);
+
+static DEFINE_URCU_TLS(int, rcu_signal_was_blocked);
  #endif

  /*
@@ -537,8 +539,52 @@ int rcu_read_ongoing(void)
  	return _rcu_read_ongoing();
  }

+#ifdef RCU_SIGNAL
+/*
+ * Make sure the signal used by the urcu-signal flavor is unblocked
+ * while the thread is registered.
+ */
+static
+void urcu_signal_unblock(void)
+{
+	sigset_t mask, oldmask;
+	int ret;
+
+	ret = sigemptyset(&mask);
+	urcu_posix_assert(!ret);
+	ret = sigaddset(&mask, SIGRCU);
+	urcu_posix_assert(!ret);
+	ret = pthread_sigmask(SIG_UNBLOCK, &mask, &oldmask);
+	urcu_posix_assert(!ret);
+	URCU_TLS(rcu_signal_was_blocked) = sigismember(&oldmask, SIGRCU);
+}
+
+static
+void urcu_signal_restore(void)
+{
+	sigset_t mask;
+	int ret;
+
+	if (!URCU_TLS(rcu_signal_was_blocked))
+		return;
+	ret = sigemptyset(&mask);
+	urcu_posix_assert(!ret);
+	ret = sigaddset(&mask, SIGRCU);
+	urcu_posix_assert(!ret);
+	ret = pthread_sigmask(SIG_BLOCK, &mask, NULL);
+	urcu_posix_assert(!ret);
+}
+#else
+static
+void urcu_signal_unblock(void) { }
+static
+void urcu_signal_restore(void) { }
+#endif
+
  void rcu_register_thread(void)
  {
+	urcu_signal_unblock();
+
  	URCU_TLS(rcu_reader).tid = pthread_self();
  	urcu_posix_assert(URCU_TLS(rcu_reader).need_mb == 0);
  	urcu_posix_assert(!(URCU_TLS(rcu_reader).ctr & URCU_GP_CTR_NEST_MASK));
@@ -558,6 +604,8 @@ void rcu_unregister_thread(void)
  	URCU_TLS(rcu_reader).registered = 0;
  	cds_list_del(&URCU_TLS(rcu_reader).node);
  	mutex_unlock(&rcu_registry_lock);
+
+	urcu_signal_restore();
  }

  #ifdef RCU_MEMBARRIER



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