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

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

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