lttng-dev.lists.lttng.org archive mirror
 help / color / mirror / Atom feed
* [lttng-dev] URCU background threads vs signalfd
@ 2022-09-22  9:15 Eric Wong via lttng-dev
  2022-09-23 15:57 ` Mathieu Desnoyers via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong via lttng-dev @ 2022-09-22  9:15 UTC (permalink / raw)
  To: lttng-dev

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

There might be other places, too, I haven't looked too closely...

Anyways, I'm currently relying on this workaround to ensure
initial calls to call_rcu and cds_lfht_resize are run with
signals blocked:

/* error-checking omitted for brevity */
__attribute__((constructor)) static void my_ctor(void)
{
	sigset_t set, old;
	struct foo_hdr *h;

	sigfillset(&set);
	pthread_sigmask(SIG_SETMASK, &set, &old);

	g_tbl = cds_lfht_new(8192, 1, 0, CDS_LFHT_AUTO_RESIZE, 0);

	h = malloc(sizeof(struct foo_hdr)));
	if (h) /* force call_rcu to start background thread */
		call_rcu(&h->as.dead, free_foo_hdr_rcu);

	/* start more background threads before unblocking signals */
	cds_lfht_resize(g_tbl, 16384);

	pthread_sigmask(SIG_SETMASK, &old, NULL);
}

But I suspect it's better to ensure signals are blocked in all
URCU-created threads...

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-22  9:15 [lttng-dev] URCU background threads vs signalfd Eric Wong via lttng-dev
@ 2022-09-23 15:57 ` Mathieu Desnoyers via lttng-dev
  2022-09-23 17:55   ` Eric Wong via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers via lttng-dev @ 2022-09-23 15:57 UTC (permalink / raw)
  To: Eric Wong, lttng-dev

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 ?

Thanks,

Mathieu

> 
> There might be other places, too, I haven't looked too closely...
> 
> Anyways, I'm currently relying on this workaround to ensure
> initial calls to call_rcu and cds_lfht_resize are run with
> signals blocked:
> 
> /* error-checking omitted for brevity */
> __attribute__((constructor)) static void my_ctor(void)
> {
> 	sigset_t set, old;
> 	struct foo_hdr *h;
> 
> 	sigfillset(&set);
> 	pthread_sigmask(SIG_SETMASK, &set, &old);
> 
> 	g_tbl = cds_lfht_new(8192, 1, 0, CDS_LFHT_AUTO_RESIZE, 0);
> 
> 	h = malloc(sizeof(struct foo_hdr)));
> 	if (h) /* force call_rcu to start background thread */
> 		call_rcu(&h->as.dead, free_foo_hdr_rcu);
> 
> 	/* start more background threads before unblocking signals */
> 	cds_lfht_resize(g_tbl, 16384);
> 
> 	pthread_sigmask(SIG_SETMASK, &old, NULL);
> }
> 
> But I suspect it's better to ensure signals are blocked in all
> URCU-created threads...
> 
> Thanks.
> _______________________________________________
> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-23 15:57 ` Mathieu Desnoyers via lttng-dev
@ 2022-09-23 17:55   ` Eric Wong via lttng-dev
  2022-09-23 18:24     ` Mathieu Desnoyers via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong via lttng-dev @ 2022-09-23 17:55 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev

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:

diff --git a/src/rculfhash.c b/src/rculfhash.c
index 7c0b9fb..5f455af 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);
diff --git a/src/urcu-call-rcu-impl.h b/src/urcu-call-rcu-impl.h
index e9366b4..9f85d55 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(!ret);
 }
 
 /*
diff --git a/src/urcu-defer-impl.h b/src/urcu-defer-impl.h
index b5d7926..1c96287 100644
--- a/src/urcu-defer-impl.h
+++ b/src/urcu-defer-impl.h
@@ -409,9 +409,18 @@ void defer_rcu(void (*fct)(void *p), void *p)
 static void start_defer_thread(void)
 {
 	int ret;
+	sigset_t newmask, oldmask;
+
+	ret = sigfillset(&newmask);
+	urcu_posix_assert(!ret);
+	ret = pthread_sigmask(SIG_BLOCK, &newmask, &oldmask);
+	urcu_posix_assert(!ret);
 
 	ret = pthread_create(&tid_defer, NULL, thr_defer, NULL);
 	urcu_posix_assert(!ret);
+
+	ret = pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+	urcu_posix_assert(!ret);
 }
 
 static void stop_defer_thread(void)
diff --git a/src/workqueue.c b/src/workqueue.c
index b6361ad..1039d72 100644
--- a/src/workqueue.c
+++ b/src/workqueue.c
@@ -284,6 +284,7 @@ struct urcu_workqueue *urcu_workqueue_create(unsigned long flags,
 {
 	struct urcu_workqueue *workqueue;
 	int ret;
+	sigset_t newmask, oldmask;
 
 	workqueue = malloc(sizeof(*workqueue));
 	if (workqueue == NULL)
@@ -304,10 +305,20 @@ struct urcu_workqueue *urcu_workqueue_create(unsigned long flags,
 	workqueue->cpu_affinity = cpu_affinity;
 	workqueue->loop_count = 0;
 	cmm_smp_mb();  /* Structure initialized before pointer is planted. */
+
+	ret = sigfillset(&newmask);
+	urcu_posix_assert(!ret);
+	ret = pthread_sigmask(SIG_BLOCK, &newmask, &oldmask);
+	urcu_posix_assert(!ret);
+
 	ret = pthread_create(&workqueue->tid, NULL, workqueue_thread, workqueue);
 	if (ret) {
 		urcu_die(ret);
 	}
+
+	ret = pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+	urcu_posix_assert(!ret);
+
 	return workqueue;
 }
 
@@ -464,13 +475,23 @@ void urcu_workqueue_resume_worker(struct urcu_workqueue *workqueue)
 void urcu_workqueue_create_worker(struct urcu_workqueue *workqueue)
 {
 	int ret;
+	sigset_t newmask, oldmask;
 
 	/* Clear workqueue state from parent. */
 	workqueue->flags &= ~URCU_WORKQUEUE_PAUSED;
 	workqueue->flags &= ~URCU_WORKQUEUE_PAUSE;
 	workqueue->tid = 0;
+
+	ret = sigfillset(&newmask);
+	urcu_posix_assert(!ret);
+	ret = pthread_sigmask(SIG_BLOCK, &newmask, &oldmask);
+	urcu_posix_assert(!ret);
+
 	ret = pthread_create(&workqueue->tid, NULL, workqueue_thread, workqueue);
 	if (ret) {
 		urcu_die(ret);
 	}
+
+	ret = pthread_sigmask(SIG_SETMASK, &oldmask, NULL);
+	urcu_posix_assert(!ret);
 }
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-23 17:55   ` Eric Wong via lttng-dev
@ 2022-09-23 18:24     ` Mathieu Desnoyers via lttng-dev
  2022-09-23 22:05       ` Eric Wong via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers via lttng-dev @ 2022-09-23 18:24 UTC (permalink / raw)
  To: Eric Wong; +Cc: lttng-dev

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-23 18:24     ` Mathieu Desnoyers via lttng-dev
@ 2022-09-23 22:05       ` Eric Wong via lttng-dev
  2022-09-26 12:51         ` Mathieu Desnoyers via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong via lttng-dev @ 2022-09-23 22:05 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:
> 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)

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...
_______________________________________________
lttng-dev mailing list
lttng-dev@lists.lttng.org
https://lists.lttng.org/cgi-bin/mailman/listinfo/lttng-dev

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-23 22:05       ` Eric Wong via lttng-dev
@ 2022-09-26 12:51         ` Mathieu Desnoyers via lttng-dev
  2022-09-26 19:58           ` Eric Wong via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Mathieu Desnoyers via lttng-dev @ 2022-09-26 12:51 UTC (permalink / raw)
  To: Eric Wong; +Cc: lttng-dev

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-26 12:51         ` Mathieu Desnoyers via lttng-dev
@ 2022-09-26 19:58           ` Eric Wong via lttng-dev
  2022-09-26 20:12             ` Mathieu Desnoyers via lttng-dev
  0 siblings, 1 reply; 8+ messages in thread
From: Eric Wong via lttng-dev @ 2022-09-26 19:58 UTC (permalink / raw)
  To: Mathieu Desnoyers; +Cc: lttng-dev

Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [lttng-dev] URCU background threads vs signalfd
  2022-09-26 19:58           ` Eric Wong via lttng-dev
@ 2022-09-26 20:12             ` Mathieu Desnoyers via lttng-dev
  0 siblings, 0 replies; 8+ messages in thread
From: Mathieu Desnoyers via lttng-dev @ 2022-09-26 20:12 UTC (permalink / raw)
  To: Eric Wong; +Cc: lttng-dev, paulmck

On 2022-09-26 15:58, Eric Wong wrote:
> Mathieu Desnoyers <mathieu.desnoyers@efficios.com> 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-26 20:12 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-22  9:15 [lttng-dev] URCU background threads vs signalfd Eric Wong via lttng-dev
2022-09-23 15:57 ` Mathieu Desnoyers via lttng-dev
2022-09-23 17:55   ` Eric Wong via lttng-dev
2022-09-23 18:24     ` Mathieu Desnoyers via lttng-dev
2022-09-23 22:05       ` Eric Wong via lttng-dev
2022-09-26 12:51         ` Mathieu Desnoyers via lttng-dev
2022-09-26 19:58           ` Eric Wong via lttng-dev
2022-09-26 20:12             ` Mathieu Desnoyers via lttng-dev

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).