Cygwin select() issues and improvements

john hood cgull@glup.org
Sun Feb 14 08:12:00 GMT 2016


[I Originally sent this last week, but it bounced.]

Various issues with Cygwin's select() annoyed me, and I've spent some
time gnawing on them.

* With 1-byte reads, select() on Windows Console input would forget
about unread input data stored in the fhandler's readahead buffer.
Hitting F1 would send only the first ESC character, until you released
the key and another Windows event was generated.  (one-line fix, though
I'm not sure it's appropriate/correct)

* On Windows, select() timeouts have coarse 10-16ms granularity (not
fixed-- must use clock_setres(), which has its own issues)

This issue hurts Mosh pretty badly, since it often uses select() with
1-20ms timeouts. Running Mosh on localhost on Cygwin has around 80ms of
extra latency for typing, which is pretty noticeable. Using
clock_setres() mostly cures this.

Older Unix systems used to have this issue too, but modern OS X, FreeBSD
and Linux all do much better.

* select() uses old millisecond-granularity Windows timeouts-- I changed
this to microsecond granularity.  Any actual improvement from this is
small, since Windows doesn't seem to handle timers on a less than 500us
granularity, even when you do use clock_setres().  One thing this does
fix is that selects > 43 days now work, though this is not a big issue
in actual practice, or a POSIX requirement.

* Newer versions of Windows may return early on timers, causing select()
to return early. (fixed, but other timers in Cygwin still have this problem)

* The main loop in select() has some pretty tangled logic.  I improved
that some.  There's room for more improvement but that would need
changing fhandlers and related functions.

* During testing, I discovered that if the system is at 100% CPU load
from other processes, select() will wait exactly 10 seconds on its first
invocation even though your timeout is much shorter.  But curiously,
this doesn't happen with usleep(), even though Cygwin's internal code is
very similar.  This is not a new bug; my best guess is that it's an, um,
feature of the Windows process/thread scheduler-- possibly it adjusts
priorities at that 10 second mark.  It'd be nice to figure this out, I
think it may be affecting the Mosh test suite.

Windows scheduling in general seems to be rather poor for Cygwin
processes, and there are scheduling differences between processes run in
Windows console (which are seen as interactive by the scheduler) and
processes run in mintty (which are not).  There doesn't seem to be any
priority promotion for I/O as you see on most Unix schedulers.

I've attached plausible patches; they're also available on
<https://github.com/cgull/newlib-cygwin>, branch 'microsecond-select'.
I think it's all Windows XP compatible.  I've put some test programs up
at <https://github.com/cgull/cygwin-timeouts> too.

regards,

  --jh


-------------- next part --------------
From 10a89aab8a87072ed4908d274cd93dc9a2e214f6 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Sat, 30 Jan 2016 17:36:43 -0500
Subject: [PATCH 1/6] Make buffered console characters visible to select().

---
 winsup/cygwin/select.cc | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 979b0b0..e1d48a3 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -916,7 +916,7 @@ fhandler_console::select_read (select_stuff *ss)
   s->peek = peek_console;
   s->h = get_handle ();
   s->read_selected = true;
-  s->read_ready = false;
+  s->read_ready = get_readahead_valid();
   return s;
 }
 
-- 
2.6.3

-------------- next part --------------
From 5be4befdda7c8e6146001ceeef465f8dca697706 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 28 Jan 2016 17:08:39 -0500
Subject: [PATCH 2/6] Use high-resolution timebases for select().

Also make cygwin_select() a wrapper on pselect().
---
 winsup/cygwin/cygwait.h    |  27 +++++++
 winsup/cygwin/select.cc    | 184 ++++++++++++++++++++++++++++-----------------
 winsup/cygwin/select.h     |   2 +-
 winsup/testsuite/configure |   0
 4 files changed, 143 insertions(+), 70 deletions(-)
 mode change 100644 => 100755 winsup/testsuite/configure

diff --git a/winsup/cygwin/cygwait.h b/winsup/cygwin/cygwait.h
index 1240f54..3e02cdd 100644
--- a/winsup/cygwin/cygwait.h
+++ b/winsup/cygwin/cygwait.h
@@ -59,3 +59,30 @@ cygwait (DWORD howlong)
 {
   return cygwait (NULL, howlong);
 }
+
+extern inline DWORD __attribute__ ((always_inline))
+cygwait_us (HANDLE h, LONGLONG howlong, unsigned mask)
+{
+  LARGE_INTEGER li_howlong;
+  PLARGE_INTEGER pli_howlong;
+  if (howlong < 0LL)
+    pli_howlong = NULL;
+  else
+    {
+      li_howlong.QuadPart = -(10LL * howlong);
+      pli_howlong = &li_howlong;
+    }
+  return cygwait (h, pli_howlong, mask);
+}
+
+static inline DWORD __attribute__ ((always_inline))
+cygwait_us (HANDLE h, LONGLONG howlong = -1)
+{
+  return cygwait_us (h, howlong, cw_cancel | cw_sig);
+}
+
+static inline DWORD __attribute__ ((always_inline))
+cygwait_us (LONGLONG howlong)
+{
+  return cygwait_us (NULL, howlong);
+}
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index e1d48a3..4f22d92 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -85,41 +85,68 @@ details. */
       return -1; \
     }
 
-static int select (int, fd_set *, fd_set *, fd_set *, DWORD);
+static int select (int, fd_set *, fd_set *, fd_set *, LONGLONG);
 
 /* The main select code.  */
 extern "C" int
-cygwin_select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
-	       struct timeval *to)
+pselect(int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+	const struct timespec *to, const sigset_t *set)
 {
-  select_printf ("select(%d, %p, %p, %p, %p)", maxfds, readfds, writefds, exceptfds, to);
+  sigset_t oldset = _my_tls.sigmask;
 
-  pthread_testcancel ();
-  int res;
-  if (maxfds < 0)
-    {
-      set_errno (EINVAL);
-      res = -1;
-    }
-  else
+  __try
     {
-      /* Convert to milliseconds or INFINITE if to == NULL */
-      DWORD ms = to ? (to->tv_sec * 1000) + (to->tv_usec / 1000) : INFINITE;
-      if (ms == 0 && to->tv_usec)
-	ms = 1;			/* At least 1 ms granularity */
+      if (set)
+	set_signal_mask (_my_tls.sigmask, *set);
 
-      if (to)
-	select_printf ("to->tv_sec %ld, to->tv_usec %ld, ms %d", to->tv_sec, to->tv_usec, ms);
+      select_printf ("pselect(%d, %p, %p, %p, %p, %p)", maxfds, readfds, writefds, exceptfds, to, set);
+
+      pthread_testcancel ();
+      int res;
+      if (maxfds < 0)
+	{
+	  set_errno (EINVAL);
+	  res = -1;
+	}
       else
-	select_printf ("to NULL, ms %x", ms);
+	{
+	  /* Convert to microseconds or -1 if to == NULL */
+	  LONGLONG us = to ? to->tv_sec * 1000000LL + (to->tv_nsec + 999) / 1000 : -1LL;
 
-      res = select (maxfds, readfds ?: allocfd_set (maxfds),
-		    writefds ?: allocfd_set (maxfds),
-		    exceptfds ?: allocfd_set (maxfds), ms);
+	  if (to)
+	    select_printf ("to->tv_sec %ld, to->tv_nsec %ld, us %lld", to->tv_sec, to->tv_nsec, us);
+	  else
+	    select_printf ("to NULL, us %lld", us);
+
+	  res = select (maxfds, readfds ?: allocfd_set (maxfds),
+			writefds ?: allocfd_set (maxfds),
+			exceptfds ?: allocfd_set (maxfds), us);
+	}
+      syscall_printf ("%R = select(%d, %p, %p, %p, %p)", res, maxfds, readfds,
+		      writefds, exceptfds, to);
+
+      if (set)
+	set_signal_mask (_my_tls.sigmask, oldset);
+      return res;
     }
-  syscall_printf ("%R = select(%d, %p, %p, %p, %p)", res, maxfds, readfds,
-		  writefds, exceptfds, to);
-  return res;
+  __except (EFAULT) {}
+  __endtry
+  return -1;
+}
+
+/* select() is just a wrapper on pselect(). */
+extern "C" int
+cygwin_select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
+	       struct timeval *to)
+{
+  struct timespec ts;
+  if (to)
+    {
+      ts.tv_sec = to->tv_sec;
+      ts.tv_nsec = to->tv_usec * 1000;
+    }
+  return pselect (maxfds, readfds, writefds, exceptfds,
+		  to ? &ts : NULL, NULL);
 }
 
 /* This function is arbitrarily split out from cygwin_select to avoid odd
@@ -127,13 +154,13 @@ cygwin_select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
    for the sel variable.  */
 static int
 select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
-	DWORD ms)
+	LONGLONG us)
 {
   select_stuff::wait_states wait_state = select_stuff::select_loop;
   int ret = 0;
 
   /* Record the current time for later use. */
-  LONGLONG start_time = gtod.msecs ();
+  LONGLONG start_time = gtod.usecs ();
 
   select_stuff sel;
   sel.return_on_signal = 0;
@@ -158,7 +185,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
       /* Degenerate case.  No fds to wait for.  Just wait for time to run out
 	 or signal to arrive. */
       if (sel.start.next == NULL)
-	switch (cygwait (ms))
+	switch (cygwait_us (us))
 	  {
 	  case WAIT_SIGNALED:
 	    select_printf ("signal received");
@@ -178,12 +205,12 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	    wait_state = select_stuff::select_set_zero;
 	    break;
 	  }
-      else if (sel.always_ready || ms == 0)
+      else if (sel.always_ready || us == 0)
 	/* Catch any active fds via sel.poll() below */
 	wait_state = select_stuff::select_ok;
       else
 	/* wait for an fd to become active or time out */
-	wait_state = sel.wait (r, w, e, ms);
+	wait_state = sel.wait (r, w, e, us);
 
       select_printf ("sel.wait returns %d", wait_state);
 
@@ -209,11 +236,11 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
       sel.cleanup ();
       sel.destroy ();
       /* Recalculate time remaining to wait if we are going to be looping. */
-      if (wait_state == select_stuff::select_loop && ms != INFINITE)
+      if (wait_state == select_stuff::select_loop && us != -1)
 	{
-	  select_printf ("recalculating ms");
-	  LONGLONG now = gtod.msecs ();
-	  if (now > (start_time + ms))
+	  select_printf ("recalculating us");
+	  LONGLONG now = gtod.usecs ();
+	  if (now > (start_time + us))
 	    {
 	      select_printf ("timed out after verification");
 	      /* Set descriptor bits to zero per POSIX. */
@@ -225,9 +252,9 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	    }
 	  else
 	    {
-	      ms -= (now - start_time);
+	      us -= (now - start_time);
 	      start_time = now;
-	      select_printf ("ms now %u", ms);
+	      select_printf ("us now %lld", us);
 	    }
 	}
     }
@@ -238,33 +265,6 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   return ret;
 }
 
-extern "C" int
-pselect(int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
-	const struct timespec *ts, const sigset_t *set)
-{
-  struct timeval tv;
-  sigset_t oldset = _my_tls.sigmask;
-
-  __try
-    {
-      if (ts)
-	{
-	  tv.tv_sec = ts->tv_sec;
-	  tv.tv_usec = ts->tv_nsec / 1000;
-	}
-      if (set)
-	set_signal_mask (_my_tls.sigmask, *set);
-      int ret = cygwin_select (maxfds, readfds, writefds, exceptfds,
-			       ts ? &tv : NULL);
-      if (set)
-	set_signal_mask (_my_tls.sigmask, oldset);
-      return ret;
-    }
-  __except (EFAULT) {}
-  __endtry
-  return -1;
-}
-
 /* Call cleanup functions for all inspected fds.  Gets rid of any
    executing threads. */
 void
@@ -362,13 +362,52 @@ err:
 /* The heart of select.  Waits for an fd to do something interesting. */
 select_stuff::wait_states
 select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
-		    DWORD ms)
+		    LONGLONG us)
 {
   HANDLE w4[MAXIMUM_WAIT_OBJECTS];
   select_record *s = &start;
   DWORD m = 0;
 
+  /* Always wait for signals. */
   wait_signal_arrived here (w4[m++]);
+
+  /* Set a fallback millisecond timeout for WMFO. */
+  DWORD dTimeoutMs;
+  if (us >= INFINITE * 1000)
+    {
+      dTimeoutMs = INFINITE - 1;
+    }
+  else if (us < 0)
+    {
+      dTimeoutMs = INFINITE;
+    }
+  else
+    {
+      dTimeoutMs = (us + 999) / 1000;
+    }
+  /* Try to create and set a waitable timer, if a finite timeout has
+     been requested.  If successful, disable the fallback timeout. */
+  LARGE_INTEGER liTimeout;
+  HANDLE hTimeout = CreateWaitableTimer (NULL, TRUE, NULL);
+  if (NULL == hTimeout)
+    {
+      select_printf("CreateWaitableTimer failed (%d)\n", GetLastError());
+    }
+  w4[m++] = hTimeout;
+  if (NULL != hTimeout && us >= 0)
+    {
+      liTimeout.QuadPart = -us * 10;
+      int setret;
+      if ((setret = SetWaitableTimer (hTimeout, &liTimeout, 0, NULL, NULL, 0)))
+	{
+	  dTimeoutMs = INFINITE;
+	}
+      else
+	{
+	  select_printf ("SetWaitableTimer failed: %d (%08x)\n", setret, GetLastError());
+	}
+    }
+  /* Optionally wait for pthread cancellation. */
   if ((w4[m] = pthread::get_cancel_event ()) != NULL)
     m++;
 
@@ -397,21 +436,27 @@ select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 next_while:;
     }
 
-  debug_printf ("m %d, ms %u", m, ms);
+  debug_printf ("m %d, us %llu, dTimeoutMs %d", m, us, dTimeoutMs);
 
   DWORD wait_ret;
   if (!windows_used)
-    wait_ret = WaitForMultipleObjects (m, w4, FALSE, ms);
+    wait_ret = WaitForMultipleObjects (m, w4, FALSE, dTimeoutMs);
   else
     /* Using MWMO_INPUTAVAILABLE is the officially supported solution for
        the problem that the call to PeekMessage disarms the queue state
        so that a subsequent MWFMO hangs, even if there are still messages
        in the queue. */
-    wait_ret = MsgWaitForMultipleObjectsEx (m, w4, ms,
+    wait_ret = MsgWaitForMultipleObjectsEx (m, w4, dTimeoutMs,
 					    QS_ALLINPUT | QS_ALLPOSTMESSAGE,
 					    MWMO_INPUTAVAILABLE);
   select_printf ("wait_ret %d, m = %d.  verifying", wait_ret, m);
 
+  if (dTimeoutMs == INFINITE)
+    {
+      CancelWaitableTimer (hTimeout);
+      CloseHandle (hTimeout);
+    }
+
   wait_states res;
   switch (wait_ret)
     {
@@ -434,12 +479,13 @@ next_while:;
       s->set_select_errno ();
       res = select_error;
       break;
+    case WAIT_OBJECT_0 + 1:
     case WAIT_TIMEOUT:
       select_printf ("timed out");
       res = select_set_zero;
       break;
-    case WAIT_OBJECT_0 + 1:
-      if (startfds > 1)
+    case WAIT_OBJECT_0 + 2:
+      if (startfds > 2)
 	{
 	  cleanup ();
 	  destroy ();
diff --git a/winsup/cygwin/select.h b/winsup/cygwin/select.h
index 0035820..581ee4e 100644
--- a/winsup/cygwin/select.h
+++ b/winsup/cygwin/select.h
@@ -96,7 +96,7 @@ public:
 
   bool test_and_set (int, fd_set *, fd_set *, fd_set *);
   int poll (fd_set *, fd_set *, fd_set *);
-  wait_states wait (fd_set *, fd_set *, fd_set *, DWORD);
+  wait_states wait (fd_set *, fd_set *, fd_set *, LONGLONG);
   void cleanup ();
   void destroy ();
 
diff --git a/winsup/testsuite/configure b/winsup/testsuite/configure
old mode 100644
new mode 100755
-- 
2.6.3

-------------- next part --------------
From dc475ad54abcf1690e1dfc51c0f4a9319b664d6b Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Sat, 30 Jan 2016 17:33:36 -0500
Subject: [PATCH 3/6] Move get_nonascii_key into fhandler_console.

---
 winsup/cygwin/fhandler.h          | 1 +
 winsup/cygwin/fhandler_console.cc | 4 +---
 winsup/cygwin/select.cc           | 3 +--
 3 files changed, 3 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/fhandler.h b/winsup/cygwin/fhandler.h
index d94f38d..ad86330 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1458,6 +1458,7 @@ private:
   bool set_unit ();
   static bool need_invisible ();
   static void free_console ();
+  static const char *get_nonascii_key (INPUT_RECORD& input_rec, char *);
 
   fhandler_console (void *) {}
 
diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index a52449e..3ed1fe8 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -46,8 +46,6 @@ details. */
 #define srTop (con.b.srWindow.Top + con.scroll_region.Top)
 #define srBottom ((con.scroll_region.Bottom < 0) ? con.b.srWindow.Bottom : con.b.srWindow.Top + con.scroll_region.Bottom)
 
-const char *get_nonascii_key (INPUT_RECORD&, char *);
-
 const unsigned fhandler_console::MAX_WRITE_CHARS = 16384;
 
 fhandler_console::console_state NO_COPY *fhandler_console::shared_console_info;
@@ -2377,7 +2375,7 @@ static const struct {
 };
 
 const char *
-get_nonascii_key (INPUT_RECORD& input_rec, char *tmp)
+fhandler_console::get_nonascii_key (INPUT_RECORD& input_rec, char *tmp)
 {
 #define NORMAL  0
 #define SHIFT	1
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 4f22d92..08f6fc2 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -885,7 +885,6 @@ fhandler_fifo::select_except (select_stuff *ss)
 static int
 peek_console (select_record *me, bool)
 {
-  extern const char * get_nonascii_key (INPUT_RECORD& input_rec, char *);
   fhandler_console *fh = (fhandler_console *) me->fh;
 
   if (!me->read_selected)
@@ -921,7 +920,7 @@ peek_console (select_record *me, bool)
 	  {
 	    if (irec.Event.KeyEvent.bKeyDown
 		&& (irec.Event.KeyEvent.uChar.AsciiChar
-		    || get_nonascii_key (irec, tmpbuf)))
+		    || fhandler_console::get_nonascii_key (irec, tmpbuf)))
 	      return me->read_ready = true;
 	  }
 	else
-- 
2.6.3

-------------- next part --------------
From 16c1cded01efa9b15cc5b127d948981bf319dbf3 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Sat, 30 Jan 2016 17:37:33 -0500
Subject: [PATCH 4/6] Debug printfs

---
 winsup/cygwin/fhandler.cc         |  1 +
 winsup/cygwin/fhandler_console.cc |  9 ++++++++-
 winsup/cygwin/select.cc           | 12 +++++++++++-
 3 files changed, 20 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 7e4d996..4c9df73 100644
--- a/winsup/cygwin/fhandler.cc
+++ b/winsup/cygwin/fhandler.cc
@@ -90,6 +90,7 @@ fhandler_base::get_readahead ()
   /* FIXME - not thread safe */
   if (raixget >= ralen)
     raixget = raixput = ralen = 0;
+  debug_printf("available: %d", chret > -1);
   return chret;
 }
 
diff --git a/winsup/cygwin/fhandler_console.cc b/winsup/cygwin/fhandler_console.cc
index 3ed1fe8..3cf2f41 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -308,7 +308,9 @@ fhandler_console::read (void *pv, size_t& buflen)
   int ch;
   set_input_state ();
 
+  debug_printf("requested buflen %d", buflen);
   int copied_chars = get_readahead_into_buffer (buf, buflen);
+  debug_printf("copied_chars %d", copied_chars);
 
   if (copied_chars)
     {
@@ -686,9 +688,11 @@ fhandler_console::read (void *pv, size_t& buflen)
 	  continue;
 	}
 
+      debug_printf("toadd = %p, nread = %d", toadd, nread);
       if (toadd)
 	{
-	  line_edit_status res = line_edit (toadd, nread, ti);
+	  ssize_t bytes_read;
+	  line_edit_status res = line_edit (toadd, nread, ti, &bytes_read);
 	  if (res == line_edit_signalled)
 	    goto sig_exit;
 	  else if (res == line_edit_input_done)
@@ -696,6 +700,8 @@ fhandler_console::read (void *pv, size_t& buflen)
 	}
     }
 
+  debug_printf("ralen = %d, bytes = %d", ralen, ralen - raixget);
+
   while (buflen)
     if ((ch = get_readahead ()) < 0)
       break;
@@ -707,6 +713,7 @@ fhandler_console::read (void *pv, size_t& buflen)
 #undef buf
 
   buflen = copied_chars;
+  debug_printf("buflen set to %d", buflen);
   return;
 
 err:
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 08f6fc2..839622f 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -921,18 +921,28 @@ peek_console (select_record *me, bool)
 	    if (irec.Event.KeyEvent.bKeyDown
 		&& (irec.Event.KeyEvent.uChar.AsciiChar
 		    || fhandler_console::get_nonascii_key (irec, tmpbuf)))
-	      return me->read_ready = true;
+	      {
+		debug_printf("peeked KEY_EVENT");
+		return me->read_ready = true;
+	      }
 	  }
 	else
 	  {
 	    if (irec.EventType == MOUSE_EVENT
 		&& fh->mouse_aware (irec.Event.MouseEvent))
+	      {
+		debug_printf("peeked MOUSE_EVENT");
 		return me->read_ready = true;
+	      }
 	    if (irec.EventType == FOCUS_EVENT && fh->focus_aware ())
+	      {
+		debug_printf("peeked FOCUS_EVENT");
 		return me->read_ready = true;
+	      }
 	  }
 
 	/* Read and discard the event */
+	debug_printf("discarded other event");
 	ReadConsoleInput (h, &irec, 1, &events_read);
       }
 
-- 
2.6.3

-------------- next part --------------
From e5c129b73fda9c96b3c14fdcac82cb15ca695e4d Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 4 Feb 2016 00:44:56 -0500
Subject: [PATCH 5/6] Improve select implementation.

This simplifies timeout handling and makes it slightly more correct.
It ensures that select() never returns early, even with sloppy timers
on newer versions of Windows, and handles uninteresting Windows events
cleanly.
---
 winsup/cygwin/cygwait.h | 27 ---------------------
 winsup/cygwin/select.cc | 63 ++++++++++++-------------------------------------
 winsup/cygwin/select.h  |  1 -
 3 files changed, 15 insertions(+), 76 deletions(-)

diff --git a/winsup/cygwin/cygwait.h b/winsup/cygwin/cygwait.h
index 3e02cdd..1240f54 100644
--- a/winsup/cygwin/cygwait.h
+++ b/winsup/cygwin/cygwait.h
@@ -59,30 +59,3 @@ cygwait (DWORD howlong)
 {
   return cygwait (NULL, howlong);
 }
-
-extern inline DWORD __attribute__ ((always_inline))
-cygwait_us (HANDLE h, LONGLONG howlong, unsigned mask)
-{
-  LARGE_INTEGER li_howlong;
-  PLARGE_INTEGER pli_howlong;
-  if (howlong < 0LL)
-    pli_howlong = NULL;
-  else
-    {
-      li_howlong.QuadPart = -(10LL * howlong);
-      pli_howlong = &li_howlong;
-    }
-  return cygwait (h, pli_howlong, mask);
-}
-
-static inline DWORD __attribute__ ((always_inline))
-cygwait_us (HANDLE h, LONGLONG howlong = -1)
-{
-  return cygwait_us (h, howlong, cw_cancel | cw_sig);
-}
-
-static inline DWORD __attribute__ ((always_inline))
-cygwait_us (LONGLONG howlong)
-{
-  return cygwait_us (NULL, howlong);
-}
diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 839622f..872fc85 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -32,7 +32,6 @@ details. */
 #include "pinfo.h"
 #include "sigproc.h"
 #include "cygtls.h"
-#include "cygwait.h"
 
 /*
  * All these defines below should be in sys/types.h
@@ -156,7 +155,7 @@ static int
 select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	LONGLONG us)
 {
-  select_stuff::wait_states wait_state = select_stuff::select_loop;
+  select_stuff::wait_states wait_state = select_stuff::select_set_zero;
   int ret = 0;
 
   /* Record the current time for later use. */
@@ -182,30 +181,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	  }
       select_printf ("sel.always_ready %d", sel.always_ready);
 
-      /* Degenerate case.  No fds to wait for.  Just wait for time to run out
-	 or signal to arrive. */
-      if (sel.start.next == NULL)
-	switch (cygwait_us (us))
-	  {
-	  case WAIT_SIGNALED:
-	    select_printf ("signal received");
-	    /* select() is always interrupted by a signal so set EINTR,
-	       unconditionally, ignoring any SA_RESTART detection by
-	       call_signal_handler().  */
-	    _my_tls.call_signal_handler ();
-	    set_sig_errno (EINTR);
-	    wait_state = select_stuff::select_signalled;
-	    break;
-	  case WAIT_CANCELED:
-	    sel.destroy ();
-	    pthread::static_cancel_self ();
-	    /*NOTREACHED*/
-	  default:
-	    /* Set wait_state to zero below. */
-	    wait_state = select_stuff::select_set_zero;
-	    break;
-	  }
-      else if (sel.always_ready || us == 0)
+      if (sel.always_ready || us == 0)
 	/* Catch any active fds via sel.poll() below */
 	wait_state = select_stuff::select_ok;
       else
@@ -214,29 +190,24 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 
       select_printf ("sel.wait returns %d", wait_state);
 
-      if (wait_state >= select_stuff::select_ok)
+      if (wait_state == select_stuff::select_ok)
 	{
 	  UNIX_FD_ZERO (readfds, maxfds);
 	  UNIX_FD_ZERO (writefds, maxfds);
 	  UNIX_FD_ZERO (exceptfds, maxfds);
-	  if (wait_state == select_stuff::select_set_zero)
-	    ret = 0;
-	  else
-	    {
-	      /* Set bit mask from sel records.  This also sets ret to the
-		 right value >= 0, matching the number of bits set in the
-		 fds records.  if ret is 0, continue to loop. */
-	      ret = sel.poll (readfds, writefds, exceptfds);
-	      if (!ret)
-		wait_state = select_stuff::select_loop;
-	    }
+	  /* Set bit mask from sel records.  This also sets ret to the
+	     right value >= 0, matching the number of bits set in the
+	     fds records.  if ret is 0, continue to loop. */
+	  ret = sel.poll (readfds, writefds, exceptfds);
+	  if (!ret)
+	    wait_state = select_stuff::select_set_zero;
 	}
       /* Always clean up everything here.  If we're looping then build it
 	 all up again.  */
       sel.cleanup ();
       sel.destroy ();
-      /* Recalculate time remaining to wait if we are going to be looping. */
-      if (wait_state == select_stuff::select_loop && us != -1)
+      /* Check and recalculate timeout. */
+      if (us != -1LL && wait_state == select_stuff::select_set_zero)
 	{
 	  select_printf ("recalculating us");
 	  LONGLONG now = gtod.usecs ();
@@ -258,7 +229,7 @@ select (int maxfds, fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
 	    }
 	}
     }
-  while (wait_state == select_stuff::select_loop);
+  while (wait_state == select_stuff::select_set_zero);
 
   if (wait_state < select_stuff::select_ok)
     ret = -1;
@@ -496,7 +467,7 @@ next_while:;
 	 to wait for.  */
     default:
       s = &start;
-      bool gotone = false;
+      res = select_set_zero;
       /* Some types of objects (e.g., consoles) wake up on "inappropriate"
 	 events like mouse movements.  The verify function will detect these
 	 situations.  If it returns false, then this wakeup was a false alarm
@@ -510,13 +481,9 @@ next_while:;
 	  }
 	else if ((((wait_ret >= m && s->windows_handle) || s->h == w4[wait_ret]))
 		 && s->verify (s, readfds, writefds, exceptfds))
-	  gotone = true;
+	  res = select_ok;
 
-      if (!gotone)
-	res = select_loop;
-      else
-	res = select_ok;
-      select_printf ("gotone %d", gotone);
+      select_printf ("res after verify %d", res);
       break;
     }
 out:
diff --git a/winsup/cygwin/select.h b/winsup/cygwin/select.h
index 581ee4e..3c749ad 100644
--- a/winsup/cygwin/select.h
+++ b/winsup/cygwin/select.h
@@ -78,7 +78,6 @@ public:
   enum wait_states
   {
     select_signalled = -3,
-    select_loop = -2,
     select_error = -1,
     select_ok = 0,
     select_set_zero = 1
-- 
2.6.3

-------------- next part --------------
From 95ce79ed5a07278399008d22747a662b09d3c3ae Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 4 Feb 2016 03:46:34 -0500
Subject: [PATCH 6/6] Regress to undocumented Nt*Timer calls.

---
 winsup/cygwin/select.cc | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/winsup/cygwin/select.cc b/winsup/cygwin/select.cc
index 872fc85..4bfc484 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -342,42 +342,40 @@ select_stuff::wait (fd_set *readfds, fd_set *writefds, fd_set *exceptfds,
   /* Always wait for signals. */
   wait_signal_arrived here (w4[m++]);
 
-  /* Set a fallback millisecond timeout for WMFO. */
+  /* Set a timeout, or not, for WMFO. */
   DWORD dTimeoutMs;
-  if (us >= INFINITE * 1000)
+  if (us == 0)
     {
-      dTimeoutMs = INFINITE - 1;
-    }
-  else if (us < 0)
-    {
-      dTimeoutMs = INFINITE;
+      dTimeoutMs = 0;
     }
   else
     {
-      dTimeoutMs = (us + 999) / 1000;
+      dTimeoutMs = INFINITE;
     }
-  /* Try to create and set a waitable timer, if a finite timeout has
-     been requested.  If successful, disable the fallback timeout. */
+  /* Create and set a waitable timer, if a finite timeout has been
+     requested. */
   LARGE_INTEGER liTimeout;
-  HANDLE hTimeout = CreateWaitableTimer (NULL, TRUE, NULL);
-  if (NULL == hTimeout)
+  HANDLE hTimeout;
+  NTSTATUS status;
+  status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
+  if (!NT_SUCCESS (status))
     {
-      select_printf("CreateWaitableTimer failed (%d)\n", GetLastError());
+      select_printf("NtCreateTimer failed (%d)\n", GetLastError());
+      return select_error;
     }
   w4[m++] = hTimeout;
-  if (NULL != hTimeout && us >= 0)
+  if (us >= 0)
     {
       liTimeout.QuadPart = -us * 10;
       int setret;
-      if ((setret = SetWaitableTimer (hTimeout, &liTimeout, 0, NULL, NULL, 0)))
+      status = NtSetTimer (hTimeout, &liTimeout, NULL, NULL, FALSE, 0, NULL);
+      if (!NT_SUCCESS(status))
 	{
-	  dTimeoutMs = INFINITE;
-	}
-      else
-	{
-	  select_printf ("SetWaitableTimer failed: %d (%08x)\n", setret, GetLastError());
+	  select_printf ("NtSetTimer failed: %d (%08x)\n", setret, GetLastError());
+	  return select_error;
 	}
     }
+
   /* Optionally wait for pthread cancellation. */
   if ((w4[m] = pthread::get_cancel_event ()) != NULL)
     m++;
-- 
2.6.3



More information about the Cygwin-patches mailing list