[PATCH v3 1/6] Cygwin: signal: Redesign signal queue handling

[Corinna Vinschen]

On Mar 13 00:08, Takashi Yano wrote:
> On Wed, 12 Mar 2025 12:16:12 +0100
> Corinna Vinschen wrote:
> > Hi Takashi,
> > 
> > On Mar 12 12:27, Takashi Yano wrote:
> > > The previous implementation of the signal queue behaves as:
> > > 1) Signals in the queue are processed in a disordered manner.
> > > 2) If the same signal is already in the queue, new signal is discarded.
> > > 
> > > Strictly speaking, these behaviours do not violate POSIX. However,
> > > these could be a cause of unexpected behaviour in some software. In
> > > Linux, some important signals such as SIGSTOP/SIGCONT do not seem to
> > > behave like that.
> > > This patch prevents SIGKILL, SIGSTOP, SIGCONT, and SIGRT* from that
> > > issue. Moreover, if SA_SIGINFO is set in sa_flags, the signal is
> > > treated almost as the same.
> > > 
> > > Addresses: https://cygwin.com/pipermail/cygwin/2025-March/257582.html
> > > Fixes: 7ac6173643b1 ("(pending_signals): New class.")
> > > Reported by: Christian Franke <Christian.Franke@t-online.de>
> > > Reviewed-by: Corinna Vinschen <corinna@vinschen.de>, Christian Franke <Christian.Franke@t-online.de>
> > > Signed-off-by: Takashi Yano <takashi.yano@nifty.ne.jp>
> > > ---
> > >  winsup/cygwin/sigproc.cc | 128 ++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 106 insertions(+), 22 deletions(-)
> > > 
> > > diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
> > > index 8739f18f5..ab3acfd24 100644
> > > --- a/winsup/cygwin/sigproc.cc
> > > +++ b/winsup/cygwin/sigproc.cc
> > > @@ -21,6 +21,7 @@ details. */
> > >  #include "cygtls.h"
> > >  #include "ntdll.h"
> > >  #include "exception.h"
> > > +#include <assert.h>
> > >  
> > >  /*
> > >   * Convenience defines
> > > @@ -28,6 +29,10 @@ details. */
> > >  #define WSSC		  60000	// Wait for signal completion
> > >  #define WPSP		  40000	// Wait for proc_subproc mutex
> > >  
> > > +#define PIPE_DEPTH _NSIG /* Historically, the pipe size is _NSIG packet */
> > > +#define SIGQ_ROOM 4
> > > +#define SIGQ_DEPTH (PIPE_DEPTH + SIGQ_ROOM)
> > 
> > I'm missing a comment here.  Why adding SIGQ_ROOM?
> 
> First, I thought signal queue length must be larger than pipe size
> to send __SIGFLUSHFAST safely. However, on second thought, it is
> not enough for some cases while it is not necessary for other cases.
> [PATCH v3 4/6] Cygwin: signal: Do not send __SIGFLUSHFAST if the pipe/queue is full
> ensure the safety for sending __SIGFLUSHFAST.
> 
> > Other than that, LGTM.
> 
> Thanks for reviewing.
> I would be happy if you could review also [PATCH v3 2/6]-[PATCH v3 6/6].

I did, sorry.  They all LGTM, too.  I'm a bit put off by having to add
two events and one mutex to accomplish a safe __SIGFLUSHFAST (rather
hoping a single semaphore would do the trick), but I see how you use the
objects and it makes sense to me.

> > In terms of the queue size, I wonder if we really have to restrict the
> > queue to a small number of queued signals, 69 right now.  The pipe used
> > for communication will take 64K, one allocation granularity slot, anyway.
> > Linux, for instance, queues more than 60K signals.
> > 
> > So, wouldn't it make sense to raise the queu depth to some higher
> > value and the pipe size so that it it's <= 64K?
> > 
> > While looking into your patch, it occured to me that we have a
> > long-standing bug: We never changed __SIGQUEUE_MAX/SIGQUEUE_MAX in
> > include/cygwin/limits.h when we started to support 64 signals (we only
> > did that for 64 bit Cygwin).
> > 
> > We can't change that for existing binaries actually referring the
> > SIGQUEUE_MAX macro, but we should change this, so that
> > sysconf( _SC_SIGQUEUE_MAX) returns the right value, isn't it?
> 
> I don't understand what you are concious of. If we change the value
> of SIGQUEUE_MAX, what happens in terms of the binary compatibility?

I don't think we get a problem in terms of binary compat.  Old apps
get a too small value, but I'm not really sure this is a problem at
all.  I was just stumbling over this and found that this should have
been changed to 64 (or 65) already ages ago.


Corinna