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

[Takashi Yano]

On Mon, 20 Jan 2025 12:38:24 +0100
Corinna Vinschen wrote:
> On Jan 20 18:08, Takashi Yano wrote:
> > On Mon, 20 Jan 2025 00:33:26 +0900
> > Takashi Yano wrote:
> > > On Sun, 19 Jan 2025 09:40:14 +0900
> > > Takashi Yano wrote:
> > > > However, I wonder if cw_timer is re-set by NtSetTimer() in the
> > > > cygwait(), it will be set to WSSC (60 sec) (or 10msec) in the
> > > > sig_send(), so the hang should end with in at most 60 sec unlike
> > > > the the hang Jeremy reported.
> > > > 
> > > > I should still overlook something.
> > > 
> > > Yes, I did.
> > > cygwait() calls NtCancelTimer() on return. So, cw_timer will be
> > > never signalled after recursive cygwait() call. Therefore,
> > > L358:         waitret = cygwait (select_sem, select_sem_timeout);
> > > will return only when select_sem is signalled though it is expected
> > > that cygwait() at L358 spends 1msec at most. This is most likely
> > > the reason of the hang at L358 in fhandler_pipe::raw_read().
> > > 
> > > The conclusion is:
> > > Do not use cygwait() in sig_send().
> > > 0003-Cygwin-signal-Do-not-handle-signal-when-__SIGFLUSHFA.patch
> > > is the right thing while
> > > ng-0003-Cygwin-signal-Do-not-handle-signal-when-__SIGFLUSHFA.patch
> > > and previous v2 __SIGFLUSHFAST patch
> > > are not.
> > 
> > I tried adding NtSetTimer() immediately after call_signal_handler()
> > in cygwait() to easily make it reentrant. The result is,
> > the hang in repeated cygwin1.dll build no longer happen even with
> > v2 __SIGFLUSHFAST patch.
> > 
> > It seems that making cygwait() reentrant is an alternative idea.
> 
> The TLS cw_timer was introduced in f0968c1e7eda ("* cygtls.h (struct
> _local_storage): Add cw_timer member.") to avoid having to create and
> destroy a timer on each invocation of cygwait, which would occur pretty
> often in some scenarios.
> 
> cw_timer was then recycled for select() by commit 7186b657e74b ("Improve
> timer handling in select.") for the same reason. Select may even be the
> worse problem.
> 
> And the problem doesn't go away of course, so it would be nice if we
> could keep using cw_timer in most cases in cygwait() and only fall back
> to another timer in case we use it in signal handling reentrantly.
> 
> It might have been a good idea to use different timers in cygwait()
> and select(), which could use it's own "sel_timer" or something,
> but that doesn't fix the signal handler reentrancy thingy.
> 
> So what about redefining cygwait to take a fourth parameter and
> use that if it's != NULL?  The caller can then just create its own
> timer.  Kind of like this:
> 
> diff --git a/winsup/cygwin/cygwait.cc b/winsup/cygwin/cygwait.cc
> index 80c0e971c77d..b54d6ae6f8a4 100644
> --- a/winsup/cygwin/cygwait.cc
> +++ b/winsup/cygwin/cygwait.cc
> @@ -24,10 +24,11 @@
>  LARGE_INTEGER cw_nowait_storage;
>  
>  DWORD
> -cygwait (HANDLE object, PLARGE_INTEGER timeout, unsigned mask)
> +cygwait (HANDLE object, PLARGE_INTEGER timeout, unsigned mask, HANDLE timer)
>  {
>    DWORD res;
>    DWORD num = 0;
> +  HANDLE &wait_timer = timer ? timer : _my_tls.locals.cw_timer;
>    HANDLE wait_objects[4];
>    pthread_t thread = pthread::self ();
>  
>   [use wait_timer in place of _my_tls.locals.cw_timer throughout cygwait]
> diff --git a/winsup/cygwin/local_includes/cygwait.h b/winsup/cygwin/local_includes/cygwait.h
> index 6212c334e1b9..4eda290782c0 100644
> --- a/winsup/cygwin/local_includes/cygwait.h
> +++ b/winsup/cygwin/local_includes/cygwait.h
> @@ -27,8 +27,8 @@ extern LARGE_INTEGER cw_nowait_storage;
>  
>  const unsigned cw_std_mask = cw_cancel | cw_cancel_self | cw_sig;
>  
> -DWORD cygwait (HANDLE, PLARGE_INTEGER timeout,
> -		       unsigned = cw_std_mask);
> +DWORD cygwait (HANDLE, PLARGE_INTEGER,
> +		       unsigned = cw_std_mask, HANDLE = NULL);
>  
>  extern inline DWORD __attribute__ ((always_inline))
>  cygwait (HANDLE h, DWORD howlong, unsigned mask)

Thanks for the nice idea.
I'll submit v4 seriese patch. Please review.

-- 
Takashi Yano <takashi.yano@nifty.ne.jp>