[PATCH v2] Cygwin: signal: Do not handle signal when __SIGFLUSHFAST is sent

[Takashi Yano]

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.yano@nifty.ne.jp>