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:
commit 26158dc3e9c20fc0488944f0c3eefdc19255e7da
Author: Corinna Vinschen <cori...@vinschen.de>
Date: Fri Nov 28 20:46:13 2014 +0000
* cygheap.cc (init_cygheap::init_tls_list): Accommodate threadlist
having a new type threadlist_t *. Convert commented out code into
an
#if 0. Create thread mutex. Explain why.
(init_cygheap::remove_tls): Drop timeout value. Always wait
infinitely
for tls_sentry. Return mutex HANDLE of just deleted threadlist
entry.
(init_cygheap::find_tls): New implementation taking tls pointer as
search parameter. Return threadlist_t *.
(init_cygheap::find_tls): Return threadlist_t *. Define ix as auto
variable. Drop exception handling since crash must be made
impossible
due to correct synchronization. Return with locked mutex.
* cygheap.h (struct threadlist_t): Define.
(struct init_cygheap): Convert threadlist to threadlist_t type.
(init_cygheap::remove_tls): Align declaration to above change.
(init_cygheap::find_tls): Ditto.
(init_cygheap::unlock_tls): Define.
* cygtls.cc (_cygtls::remove): Unlock and close mutex when
finishing.
* exceptions.cc (sigpacket::process): Lock _cygtls area of thread
before
accessing it.
* fhandler_termios.cc (fhandler_termios::bg_check): Ditto.
* sigproc.cc (sig_send): Ditto.
* thread.cc (pthread::exit): Ditto. Add comment.
(pthread::cancel): Ditto.
--
Takashi Yano <takashi.y...@nifty.ne.jp>
