On Mon, 20 Jan 2025 12:43:07 +0100 Corinna Vinschen wrote: > On Jan 19 19:42, Takashi Yano wrote: > > Hi Corinna, > > > > On Sun, 19 Jan 2025 11:49:58 +0900 > > Takashi Yano wrote: > > > On Sat, 18 Jan 2025 17:06:50 -0800 (PST) > > > Jeremy Drake wrote: > > > > On Sat, 18 Jan 2025, Takashi Yano wrote: > > > > > > > > > While debugging this problem, I encountered another hang issue, > > > > > which is fixed by: > > > > > 0001-Cygwin-signal-Avoid-frequent-tls-lock-unlock-for-SIG.patch > > > > > > > > I'm concerned about this patch. There's a window where current_sig > > > > could > > > > be changed after exiting the while, before the lock is acquired by > > > > cygheap->find_tls (_main_tls); Should current_sig be rechecked after > > > > the > > > > lock is acquired to make sure that hasn't happened? Also, does > > > > current_sig need to be volatile, or is yield a sufficient fence for the > > > > compiler to know other threads may have changed the value? > > > > > > Thanks for pointing out this. You are right if othre threads may > > > set current_sig to non-zero value. Current cygwin sets current_sig > > > to non-zero only in > > > _cygtls::interrupt_setup() > > > and > > > _cygtls::handle_SIGCONT() > > > both are called from sigpacket::process() as follows. > > > > > > wait_sig()-> > > > sigpacket::process() +-> sigpacket::setup_handler() -> > > > _cygtls::interrupt_setup() > > > \-> _cygtls::handle_SIGCONT() > > > > > > wait_sig() is a thread which handle received signals, so other > > > threads than wait_sig() thread do not set the current_sig to non-zero. > > > That is, other threads set current_sig only to zero. Therefore, > > > I think we don't have to guard checking current_sig value by lock. > > > The only thing we shoud guard is the following case. > > > > > > [wait_sig()] [another thread] > > > current_sig = SIGCONT; > > > current_sig = 0; > > > set_signal_arrived(); > > > > > > So, we should place current_sig = SIGCONT and set_signal_arrived() > > > inside the lock. > > > > I think the lock necessary here is _cygtls::lock(), isn't it? > > Because the _cygtls::call_signal_handler() uses _cygtls::locl(). > > I'm asking you because you introduced the find_tls() lock first > > in the commit: > > Yeah, _cygtls::lock() of the target thread should be right. > The mutex in find_tls was for guarding threadlist_t, not the > thread's _cygtls.
Thanks! I'll submit a v2 patch. Please review. -- Takashi Yano <takashi.y...@nifty.ne.jp>
