Why do we need this? Is there a function that uses pshared and required to be async signal safe?

If indeed we need this (I doubt), then it is better to use THR_CRITICAL_ENTER() around.

In D61215#1403613, @kib wrote:

Why do we need this? Is there a function that uses pshared and required to be async signal safe?

If indeed we need this (I doubt), then it is better to use THR_CRITICAL_ENTER() around.

This is found by @markj's AI bot. And yes, we need to be signal safe I think. A signal can contend with process itself over pshared_cache_insert.

I try THR_CRITICAL_ENTER in the first version, the enter itself is fine actually.

But _thr_ast in THR_CRITICAL_EXIT makes each read extremely slower when we have lock A -> B -> A case. It is even slower than baseline (without cache version).

In D61215#1403966, @aokblast wrote:

In D61215#1403613, @kib wrote:

Why do we need this? Is there a function that uses pshared and required to be async signal safe?

If indeed we need this (I doubt), then it is better to use THR_CRITICAL_ENTER() around.

This is found by @markj's AI bot. And yes, we need to be signal safe I think. A signal can contend with process itself over pshared_cache_insert.

Still, why is it needed? What are the async-signal-safe functions that could call into pshared?

In D61215#1404148, @kib wrote:

In D61215#1403966, @aokblast wrote:

In D61215#1403613, @kib wrote:

Why do we need this? Is there a function that uses pshared and required to be async signal safe?

If indeed we need this (I doubt), then it is better to use THR_CRITICAL_ENTER() around.

This is found by @markj's AI bot. And yes, we need to be signal safe I think. A signal can contend with process itself over pshared_cache_insert.

Still, why is it needed? What are the async-signal-safe functions that could call into pshared?

I ask AI to write the test toedemo this case. Of course, on POSIX spec it is an undefined behavoir. But I think it is worth to do as _thr_ast have already guaranteed pthread internal is async signal safe for pthread interl state.

Using pthread locking primitives in signal handler is UB for very good reason. We do not block signals around locks. Then, signal delivery might interrupt either locked region (causing app to recurse on its locks) or the locking code itself (causing libthr to operate on the inconsistent lock state). The pshared component is just a part of the lock state, and it is not async signal safe.

So trying to add the safety for single bit of it does not seem useful.

In D61215#1404928, @kib wrote:

Using pthread locking primitives in signal handler is UB for very good reason. We do not block signals around locks. Then, signal delivery might interrupt either locked region (causing app to recurse on its locks) or the locking code itself (causing libthr to operate on the inconsistent lock state). The pshared component is just a part of the lock state, and it is not async signal safe.

So trying to add the safety for single bit of it does not seem useful.

Oh ok. Thanks for the explanation. I will close this PR if @markj also think it is fine.

I am fine with dropping the patch. I believe the LLM's reasoning was something like, before commit 7806832f23fb the pshared hash lookup was signal-safe since it ran under pshared_lock, and this property was lost with that commit, so it may be a regression.

In D61215#1406108, @markj wrote:

I am fine with dropping the patch. I believe the LLM's reasoning was something like, before commit 7806832f23fb the pshared hash lookup was signal-safe since it ran under pshared_lock, and this property was lost with that commit, so it may be a regression.

pshared_lock does not block signals. Only rtld locks enter critical sections around.

In D61215#1406150, @kib wrote:

In D61215#1406108, @markj wrote:

I am fine with dropping the patch. I believe the LLM's reasoning was something like, before commit 7806832f23fb the pshared hash lookup was signal-safe since it ran under pshared_lock, and this property was lost with that commit, so it may be a regression.

pshared_lock does not block signals. Only rtld locks enter critical sections around.

But THR_IN_CRITICAL is true if td->locklevel > 0, and pshared_*lock() bumps that counter.

In D61215#1406156, @markj wrote:

In D61215#1406150, @kib wrote:

In D61215#1406108, @markj wrote:

I am fine with dropping the patch. I believe the LLM's reasoning was something like, before commit 7806832f23fb the pshared hash lookup was signal-safe since it ran under pshared_lock, and this property was lost with that commit, so it may be a regression.

pshared_lock does not block signals. Only rtld locks enter critical sections around.

But THR_IN_CRITICAL is true if td->locklevel > 0, and pshared_*lock() bumps that counter.

D61343