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 <[email protected]> > > > Reviewed-by: Corinna Vinschen <[email protected]>, Christian Franke > > > <[email protected]> > > > Signed-off-by: Takashi Yano <[email protected]> > > > --- > > > 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
