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.fra...@t-online.de>
> > Reviewed-by:
> > Signed-off-by: Takashi Yano <takashi.y...@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.y...@nifty.ne.jp>
