[PATCH v2 4/7] Cygwin: signal: Optimize the priority of the sig thread

[Takashi Yano]

On Wed, 27 Nov 2024 17:53:53 +0100
Corinna Vinschen wrote:
> On Nov 26 17:55, Takashi Yano wrote:
> > Previously, the sig thread ran in THREAD_PRIORITY_HIGHEST priority.
> > This causes a critical delay in the signal handling in the main thread
> > if too many signals are received rapidly and the CPU is very busy.
> > In this case, most of the CPU time is allocated to the sig thread, so
> > the main thread cannot have a chance to handle signals. With this patch,
> > the sig thread priority is set to the same priority as the main thread
> > to avoid such a situation. Furthermore, if the signal is alerted to
> > the main thread, but the main thread does not handle it yet, in order
> > to increase the chance of handling it in the main thread, reduce the
> > sig thread priority to THREAD_PRIORITY_LOWEST temporarily.
> > 
> > Addresses: https://cygwin.com/pipermail/cygwin/2024-November/256744.html
> > Fixes: 53ad6f1394aa ("(cygthread::cygthread): Use three only arguments for detached threads, and start the thread via QueueUserAPC/async_create.")
> > Reported-by: Christian Franke <Christian.Franke@t-online.de>
> > Reviewed-by:
> > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > ---
> >  winsup/cygwin/sigproc.cc | 26 ++++++++++++++++++++++++++
> >  1 file changed, 26 insertions(+)
> > 
> > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> > index b8d961a07..fc4360951 100644
> > --- a/winsup/cygwin/sigproc.cc
> > +++ b/winsup/cygwin/sigproc.cc
> > @@ -1319,6 +1319,23 @@ wait_sig (VOID *)
> >      {
> >        DWORD nb;
> >        sigpacket pack = {};
> > +      /* Follow to the main thread priority */
>                    
> Just "Follow the ..."
> 
> > +      int prio = THREAD_PRIORITY_NORMAL;
> > +      if (cygwin_finished_initializing)
> > +	{
> > +	  HANDLE h_main_thread = NULL;
> > +	  threadlist_t *tl_entry = cygheap->find_tls (_main_tls);
> > +	  if (_main_tls->thread_id)
> > +	    h_main_thread = OpenThread (THREAD_QUERY_INFORMATION,
> > +					FALSE, _main_tls->thread_id);
> 
> We already have the main thread handle globally available in hMainThread.
> 
> But there's something I don't understand here: You don't know if the
> main thread is actually the thread handling the signal.  So why should
> the priority of the main thread be the role model?

Hmmm, just setting THREAD_PRIORITY_NORMAL might be appropriate.
See v3 patch.

> The culprit of the behaviour you're seeing is the fact that *all*
> cygthread's are running with THREAD_PRIORITY_HIGHEST prio.
> 
> Maybe it's time to rethink this.  Most (none?) of the cygthreads really
> need highest priority.  This *may* have been useful when we only had a
> single CPU core, but these times have gone by, and cygthreads serve
> quite a few tasks which don't need THREAD_PRIORITY_HIGHEST.
> 
> We could try to start all threads with normal priority, and
> only threads suffering from priority problems could be moved to
> another prio.

Enough testing will be necessary for that, I think. As for this patch,
we should use just
SetThreadPriority(GetCurrentThread(), THREAD_PRIORITY_NORMAL)
at the begining of the thread function.

> > +      if (cygwin_finished_initializing)
> > +	{
> > +	  threadlist_t *tl_entry = cygheap->find_tls (_main_tls);
> > +	  if (_main_tls->current_sig)
> > +	    /* Decrease the priority in order to make main thread process
> > +	       this signal. */
> > +	    SetThreadPriority (GetCurrentThread (), THREAD_PRIORITY_LOWEST);
> > +	  cygheap->unlock_tls (tl_entry);
> > +	}
> 
> Along these lines, I really wonder if this is required.  What if
> we just stick to THREAD_PRIORITY_NORMAL here?

I also think so first, however, without this, the Christian's
test case stops in a short time as if it's stuck. Doing this
here might not be sutable.

Please see v3 patch.

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