* Cygwin select() issues and improvements
@ 2016-02-14 8:12 john hood
2016-02-14 12:29 ` Thomas Wolff
2016-02-15 12:57 ` Corinna Vinschen
0 siblings, 2 replies; 19+ messages in thread
From: john hood @ 2016-02-14 8:12 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]
[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
[-- Attachment #2: 0001-Make-buffered-console-characters-visible-to-select.patch --]
[-- Type: text/plain, Size: 701 bytes --]
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
[-- Attachment #3: 0002-Use-high-resolution-timebases-for-select.patch --]
[-- Type: text/plain, Size: 11235 bytes --]
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
[-- Attachment #4: 0003-Move-get_nonascii_key-into-fhandler_console.patch --]
[-- Type: text/plain, Size: 2365 bytes --]
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
[-- Attachment #5: 0004-Debug-printfs.patch --]
[-- Type: text/plain, Size: 3055 bytes --]
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
[-- Attachment #6: 0005-Improve-select-implementation.patch --]
[-- Type: text/plain, Size: 6235 bytes --]
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
[-- Attachment #7: 0006-Regress-to-undocumented-Nt-Timer-calls.patch --]
[-- Type: text/plain, Size: 2429 bytes --]
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-14 8:12 Cygwin select() issues and improvements john hood
@ 2016-02-14 12:29 ` Thomas Wolff
2016-02-14 18:03 ` John Hood
2016-02-15 12:57 ` Corinna Vinschen
1 sibling, 1 reply; 19+ messages in thread
From: Thomas Wolff @ 2016-02-14 12:29 UTC (permalink / raw)
To: cygwin-patches
Am 14.02.2016 um 09:09 schrieb john hood:
> [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)
>
> * ...
If that also solves the UTF-8 byte splitting problem
https://cygwin.com/ml/cygwin/2014-12/msg00118.html that would be great,
see test program attached there.
Thomas
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-14 12:29 ` Thomas Wolff
@ 2016-02-14 18:03 ` John Hood
0 siblings, 0 replies; 19+ messages in thread
From: John Hood @ 2016-02-14 18:03 UTC (permalink / raw)
To: Thomas Wolff; +Cc: cygwin-patches
I hadn't checked UTF-8 input before, but yes, it's the same problem. Your test program and mine are very similar.
Regards,
--jh
> On Feb 14, 2016, at 7:29 AM, Thomas Wolff <towo@towo.net> wrote:
>
>> Am 14.02.2016 um 09:09 schrieb john hood:
>> [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)
>>
>> * ...
> If that also solves the UTF-8 byte splitting problem https://cygwin.com/ml/cygwin/2014-12/msg00118.html that would be great, see test program attached there.
> Thomas
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-14 8:12 Cygwin select() issues and improvements john hood
2016-02-14 12:29 ` Thomas Wolff
@ 2016-02-15 12:57 ` Corinna Vinschen
2016-02-19 1:20 ` john hood
2016-03-20 13:09 ` Corinna Vinschen
1 sibling, 2 replies; 19+ messages in thread
From: Corinna Vinschen @ 2016-02-15 12:57 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 2589 bytes --]
On Feb 14 03:09, john hood wrote:
> [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)
I think so, yes. I applied this patch, thank you.
With the other patches I have two problems.
One of them is that they are not trivial enough to be acceptable without
copyright assignment (except patch 3, but see below). Please have a
look at https://cygwin.com/contrib.html, the "Before you get started"
section. There's a link to an "assign.txt" file with instructions.
The other one is just this: Can you please describe each change in the
accompanying patch comment so that it's accessible from the git log?
> * Newer versions of Windows may return early on timers, causing select()
> to return early. (fixed, but other timers in Cygwin still have this problem)
It would be nice if we could discuss this a bit more in detail. I wasn't
aware of this change.
> * The main loop in select() has some pretty tangled logic. I improved
> that some. There's room for more improvement
Definitely.
> but that would need
> changing fhandlers and related functions.
If it makes sense, sure.
> 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'm not aware of such a feature.
> 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.
XP/2K3 compatibility is not required anymore. If you think you need
a feature which is only available on Vista or later, feel free. We just
have to inform the Cygwin mailing list, as promised.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-15 12:57 ` Corinna Vinschen
@ 2016-02-19 1:20 ` john hood
2016-02-19 10:46 ` Corinna Vinschen
2016-03-20 13:09 ` Corinna Vinschen
1 sibling, 1 reply; 19+ messages in thread
From: john hood @ 2016-02-19 1:20 UTC (permalink / raw)
To: cygwin-patches
On 2/15/16 7:57 AM, Corinna Vinschen wrote:
> On Feb 14 03:09, john hood wrote:
>> [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)
>
> I think so, yes. I applied this patch, thank you.
>
> With the other patches I have two problems.
>
> One of them is that they are not trivial enough to be acceptable without
> copyright assignment (except patch 3, but see below). Please have a
> look at https://cygwin.com/contrib.html, the "Before you get started"
> section. There's a link to an "assign.txt" file with instructions.
>
> The other one is just this: Can you please describe each change in the
> accompanying patch comment so that it's accessible from the git log?
Sorry for the slow response here. I have a bad cold and I'm not getting
to things quickly.
I'll do those two things; I wanted to see if there was interest first.
>> * Newer versions of Windows may return early on timers, causing select()
>> to return early. (fixed, but other timers in Cygwin still have this problem)
>
> It would be nice if we could discuss this a bit more in detail. I wasn't
> aware of this change.
Microsoft official documentation:
<https://msdn.microsoft.com/en-us/library/windows/desktop/ms687069%28v=vs.85%29.aspx#waitfunctionsandtime-outintervals>
<https://msdn.microsoft.com/en-us/library/windows/hardware/jj602805%28v=vs.85%29.aspx>
Try running my socket-t program in
<https://github.com/cgull/cygwin-timeouts> as 'socket-t 10000'; it will
report the actual time waited. On Windows 10, you will see lots of
variation in timeouts, with some of them shorter than the requested
time. My ancient Vista laptop has much less variation and is never
shorter. Win7 is similar.
The same behavior applies to all other uses of waitable timers in
Cygwin, I think, and perhaps all timing-related code. It's possible
the recent patches from Artúr <ikartur@gmail.com> are related to this
problem, I haven't checked.
>> * The main loop in select() has some pretty tangled logic. I improved
>> that some. There's room for more improvement
>
> Definitely.
>
>> but that would need
>> changing fhandlers and related functions.
>
> If it makes sense, sure.
The thing that I think should happen there is that fhandlers'
select_{read,write,except}() functions should go away, and an fhandler
should only have a poll() function that indicates what's available, and
a get_waitable_object() function, that gives sel.wait() something to
sleep on. The select_{read,write,except}() functions, and the
always_ready state variables, partially implement both of these pieces
of functionality, and really complicate the implementation for select().
I'm not sure I'll ever get to it, these Cygwin issues are very much a
side project for me.
>> 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'm not aware of such a feature.
>
>> 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.
>
> XP/2K3 compatibility is not required anymore. If you think you need
> a feature which is only available on Vista or later, feel free. We just
> have to inform the Cygwin mailing list, as promised.
I don't think we need any post-XP compatibility here.
The last patch in my series reverts from the documented
CreateWaitableTimer() interfaces to the ancient, undocumented
NTCreateTimer() interfaces only for consistency with the rest of the
Cygwin codebase, which only uses NTCreateTimer(). The documented
interfaces are all present in XP. The undocumented interfaces have all
the functionality this code needs.
I'm on #cygwin and #cygwin-dev, ask questions there if you want.
regards,
--jh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-19 1:20 ` john hood
@ 2016-02-19 10:46 ` Corinna Vinschen
2016-03-04 8:58 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-02-19 10:46 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 3453 bytes --]
On Feb 18 20:20, john hood wrote:
> On 2/15/16 7:57 AM, Corinna Vinschen wrote:
> > On Feb 14 03:09, john hood wrote:
> >> Various issues with Cygwin's select() annoyed me, and I've spent some
> >> time gnawing on them.
> > One of them is that they are not trivial enough to be acceptable without
> > copyright assignment (except patch 3, but see below). Please have a
> > look at https://cygwin.com/contrib.html, the "Before you get started"
> > section. There's a link to an "assign.txt" file with instructions.
> >
> > The other one is just this: Can you please describe each change in the
> > accompanying patch comment so that it's accessible from the git log?
>
> Sorry for the slow response here. I have a bad cold and I'm not getting
> to things quickly.
I know what you mean. I'm still coughing badly from the flu I catched
lately.
> Microsoft official documentation:
>
> <https://msdn.microsoft.com/en-us/library/windows/desktop/ms687069%28v=vs.85%29.aspx#waitfunctionsandtime-outintervals>
>
> <https://msdn.microsoft.com/en-us/library/windows/hardware/jj602805%28v=vs.85%29.aspx>
>
> Try running my socket-t program in
> <https://github.com/cgull/cygwin-timeouts> as 'socket-t 10000'; it will
> report the actual time waited. On Windows 10, you will see lots of
> variation in timeouts, with some of them shorter than the requested
> time. My ancient Vista laptop has much less variation and is never
> shorter. Win7 is similar.
In the second link it sounds like a change in W8 might causing this.
> The thing that I think should happen there is that fhandlers'
> select_{read,write,except}() functions should go away, and an fhandler
> should only have a poll() function that indicates what's available, and
> a get_waitable_object() function, that gives sel.wait() something to
> sleep on. The select_{read,write,except}() functions, and the
> always_ready state variables, partially implement both of these pieces
> of functionality, and really complicate the implementation for select().
>
> I'm not sure I'll ever get to it, these Cygwin issues are very much a
> side project for me.
That's ok, but the idea is nice. It would be cool if we could improve
select. From my POV it has at least three downsides. It's pretty slow,
the code is complicated, and it's badly commented. Also, IIRC, the number
of descriptors is restricted to 63 due to WFMO restricted to this number
of handles. This is not a restriction for sockets since sockets are
using threads per each 63 objects, but the other objects are not doing
that. So, yeah, there's a lot to improve on select alone.
> The last patch in my series reverts from the documented
> CreateWaitableTimer() interfaces to the ancient, undocumented
> NTCreateTimer() interfaces only for consistency with the rest of the
> Cygwin codebase, which only uses NTCreateTimer(). The documented
> interfaces are all present in XP. The undocumented interfaces have all
> the functionality this code needs.
Using NtCreateTimer is perfectly fine and I think the API is cleaner
than the CreateWaitableTimer API.
> I'm on #cygwin and #cygwin-dev, ask questions there if you want.
Not ATM, but feel free to contact me on the dev channel.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-19 10:46 ` Corinna Vinschen
@ 2016-03-04 8:58 ` Corinna Vinschen
2016-03-13 21:37 ` [PATCH] " john hood
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-03-04 8:58 UTC (permalink / raw)
To: cygwin-patches; +Cc: john hood
[-- Attachment #1: Type: text/plain, Size: 3933 bytes --]
John,
Ping? I'd be interested to get your patches into Cygwin. select
really needs some kicking :)
Thanks,
Corinna
On Feb 19 11:46, Corinna Vinschen wrote:
> On Feb 18 20:20, john hood wrote:
> > On 2/15/16 7:57 AM, Corinna Vinschen wrote:
> > > On Feb 14 03:09, john hood wrote:
> > >> Various issues with Cygwin's select() annoyed me, and I've spent some
> > >> time gnawing on them.
> > > One of them is that they are not trivial enough to be acceptable without
> > > copyright assignment (except patch 3, but see below). Please have a
> > > look at https://cygwin.com/contrib.html, the "Before you get started"
> > > section. There's a link to an "assign.txt" file with instructions.
> > >
> > > The other one is just this: Can you please describe each change in the
> > > accompanying patch comment so that it's accessible from the git log?
> >
> > Sorry for the slow response here. I have a bad cold and I'm not getting
> > to things quickly.
>
> I know what you mean. I'm still coughing badly from the flu I catched
> lately.
>
> > Microsoft official documentation:
> >
> > <https://msdn.microsoft.com/en-us/library/windows/desktop/ms687069%28v=vs.85%29.aspx#waitfunctionsandtime-outintervals>
> >
> > <https://msdn.microsoft.com/en-us/library/windows/hardware/jj602805%28v=vs.85%29.aspx>
> >
> > Try running my socket-t program in
> > <https://github.com/cgull/cygwin-timeouts> as 'socket-t 10000'; it will
> > report the actual time waited. On Windows 10, you will see lots of
> > variation in timeouts, with some of them shorter than the requested
> > time. My ancient Vista laptop has much less variation and is never
> > shorter. Win7 is similar.
>
> In the second link it sounds like a change in W8 might causing this.
>
> > The thing that I think should happen there is that fhandlers'
> > select_{read,write,except}() functions should go away, and an fhandler
> > should only have a poll() function that indicates what's available, and
> > a get_waitable_object() function, that gives sel.wait() something to
> > sleep on. The select_{read,write,except}() functions, and the
> > always_ready state variables, partially implement both of these pieces
> > of functionality, and really complicate the implementation for select().
> >
> > I'm not sure I'll ever get to it, these Cygwin issues are very much a
> > side project for me.
>
> That's ok, but the idea is nice. It would be cool if we could improve
> select. From my POV it has at least three downsides. It's pretty slow,
> the code is complicated, and it's badly commented. Also, IIRC, the number
> of descriptors is restricted to 63 due to WFMO restricted to this number
> of handles. This is not a restriction for sockets since sockets are
> using threads per each 63 objects, but the other objects are not doing
> that. So, yeah, there's a lot to improve on select alone.
>
> > The last patch in my series reverts from the documented
> > CreateWaitableTimer() interfaces to the ancient, undocumented
> > NTCreateTimer() interfaces only for consistency with the rest of the
> > Cygwin codebase, which only uses NTCreateTimer(). The documented
> > interfaces are all present in XP. The undocumented interfaces have all
> > the functionality this code needs.
>
> Using NtCreateTimer is perfectly fine and I think the API is cleaner
> than the CreateWaitableTimer API.
>
> > I'm on #cygwin and #cygwin-dev, ask questions there if you want.
>
> Not ATM, but feel free to contact me on the dev channel.
>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen Please, send mails regarding Cygwin to
> Cygwin Maintainer cygwin AT cygwin DOT com
> Red Hat
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH] Re: Cygwin select() issues and improvements
2016-03-04 8:58 ` Corinna Vinschen
@ 2016-03-13 21:37 ` john hood
2016-03-14 10:13 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: john hood @ 2016-03-13 21:37 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 556 bytes --]
On 3/4/16 3:58 AM, Corinna Vinschen wrote:
> John,
>
>
> Ping? I'd be interested to get your patches into Cygwin. select
> really needs some kicking :)
Sorry to be so slow responding. Here's a rebased, squashed,
changelog-ified patch, and I've sent an assignment in to Red Hat.
Aside: Why maintain Git comments in ChangeLog format? That made sense
up into the CVS era, but now version control systems and viewers provide
most of the details that a ChangeLog needed to list, and newlib/Cygwin
has stopped maintaining ChangeLogs.
regards,
--jh
[-- Attachment #2: 0001-Improve-Cygwin-select-implementation.patch --]
[-- Type: text/plain, Size: 17712 bytes --]
From 0e407e4206746d8a6d1ee93d680daefa8ac2f00b Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 28 Jan 2016 17:08:39 -0500
Subject: [PATCH] Improve Cygwin select() implementation.
* fhandler.h: (fhandler_console) Move get_nonascii_key() from
select.c into this class.
* fhandler.cc (fhandler_base::get_readahead): Add debug code.
* fhandler_console.cc (fhandler_console::read): Add debug code.
* select.h: Eliminate redundant select_stuff::select_loop state.
Change prototype for select_stuff::wait() for larger microsecond
timeouts.
* select.cc (pselect): Convert from old cygwin_select().
Implement microsecond timeouts. Add debug code.
(cygwin_select): Rewrite as a wrapper on pselect().
(select): Implement microsecond timeouts. Eliminate redundant
select_stuff::select_loop state. Eliminate redundant code for
zero timeout. Do not return early on early timer return.
(select_stuff::wait): Implement microsecond timeouts with a timer
object. Eliminate redundant select_stuff::select_loop state.
(peek_console): Move get_nonascii_key() into fhandler_console
class. Add debug code.
---
winsup/cygwin/fhandler.cc | 1 +
winsup/cygwin/fhandler.h | 1 +
winsup/cygwin/fhandler_console.cc | 13 +-
winsup/cygwin/select.cc | 254 ++++++++++++++++++++------------------
winsup/cygwin/select.h | 3 +-
winsup/testsuite/configure | 0
6 files changed, 149 insertions(+), 123 deletions(-)
mode change 100644 => 100755 winsup/testsuite/configure
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 33743d4..86f77c3 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.h b/winsup/cygwin/fhandler.h
index 134fd71..bad63f2 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1463,6 +1463,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..3cf2f41 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;
@@ -310,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)
{
@@ -688,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)
@@ -698,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;
@@ -709,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:
@@ -2377,7 +2382,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 e1d48a3..4bfc484 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
@@ -85,41 +84,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);
+
+ select_printf ("pselect(%d, %p, %p, %p, %p, %p)", maxfds, readfds, writefds, exceptfds, to, set);
- if (to)
- select_printf ("to->tv_sec %ld, to->tv_usec %ld, ms %d", to->tv_sec, to->tv_usec, ms);
+ 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 +153,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;
+ select_stuff::wait_states wait_state = select_stuff::select_set_zero;
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;
@@ -155,65 +181,37 @@ 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 (ms))
- {
- 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 || ms == 0)
+ 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);
- 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 && ms != INFINITE)
+ /* Check and recalculate timeout. */
+ if (us != -1LL && wait_state == select_stuff::select_set_zero)
{
- 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,46 +223,19 @@ 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);
}
}
}
- while (wait_state == select_stuff::select_loop);
+ while (wait_state == select_stuff::select_set_zero);
if (wait_state < select_stuff::select_ok)
ret = -1;
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 +333,50 @@ 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 timeout, or not, for WMFO. */
+ DWORD dTimeoutMs;
+ if (us == 0)
+ {
+ dTimeoutMs = 0;
+ }
+ else
+ {
+ dTimeoutMs = INFINITE;
+ }
+ /* Create and set a waitable timer, if a finite timeout has been
+ requested. */
+ LARGE_INTEGER liTimeout;
+ HANDLE hTimeout;
+ NTSTATUS status;
+ status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
+ if (!NT_SUCCESS (status))
+ {
+ select_printf("NtCreateTimer failed (%d)\n", GetLastError());
+ return select_error;
+ }
+ w4[m++] = hTimeout;
+ if (us >= 0)
+ {
+ liTimeout.QuadPart = -us * 10;
+ int setret;
+ status = NtSetTimer (hTimeout, &liTimeout, NULL, NULL, FALSE, 0, NULL);
+ if (!NT_SUCCESS(status))
+ {
+ 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++;
@@ -397,21 +405,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 +448,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 ();
@@ -450,7 +465,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
@@ -464,13 +479,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:
@@ -839,7 +850,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)
@@ -875,19 +885,29 @@ peek_console (select_record *me, bool)
{
if (irec.Event.KeyEvent.bKeyDown
&& (irec.Event.KeyEvent.uChar.AsciiChar
- || get_nonascii_key (irec, tmpbuf)))
- return me->read_ready = true;
+ || fhandler_console::get_nonascii_key (irec, tmpbuf)))
+ {
+ 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);
}
diff --git a/winsup/cygwin/select.h b/winsup/cygwin/select.h
index 0035820..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
@@ -96,7 +95,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.7.2
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-13 21:37 ` [PATCH] " john hood
@ 2016-03-14 10:13 ` Corinna Vinschen
2016-03-17 9:29 ` John Hood
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-03-14 10:13 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]
Hi John,
On Mar 13 17:37, john hood wrote:
> On 3/4/16 3:58 AM, Corinna Vinschen wrote:
> > John,
> >
> >
> > Ping? I'd be interested to get your patches into Cygwin. select
> > really needs some kicking :)
>
> Sorry to be so slow responding. Here's a rebased, squashed,
> changelog-ified patch,
Thank you. Uhm... I just don't understand why you squashed them into a
single big patch. Multiple independent smaller patches are better to
handle, especially when looking for potential bugs later.
Would you mind terribly to split them again?
> and I've sent an assignment in to Red Hat.
Cool, thanks! This might take a few days.
> Aside: Why maintain Git comments in ChangeLog format? That made sense
> up into the CVS era, but now version control systems and viewers provide
> most of the details that a ChangeLog needed to list, and newlib/Cygwin
> has stopped maintaining ChangeLogs.
It's probably the "get off my lawn" mentality of old, die-hard CVS users.
Both, Jeff and I, the maintainers of newlib and Cygwin are such specimen.
I'm in the process of reconsidering it for Cygwin-related logs. I'm still
mostly sticking to these ChangeLog entries, but I'm moving over to adding
textual descriptions to the git log. See my latest commits for examples.
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-14 10:13 ` Corinna Vinschen
@ 2016-03-17 9:29 ` John Hood
2016-03-17 16:37 ` Eric Blake
0 siblings, 1 reply; 19+ messages in thread
From: John Hood @ 2016-03-17 9:29 UTC (permalink / raw)
To: cygwin-patches
On 3/14/2016 6:12 AM, Corinna Vinschen wrote:
> Hi John,
>
> On Mar 13 17:37, john hood wrote:
>> On 3/4/16 3:58 AM, Corinna Vinschen wrote:
>>> John,
>>>
>>>
>>> Ping? I'd be interested to get your patches into Cygwin. select
>>> really needs some kicking :)
>> Sorry to be so slow responding. Here's a rebased, squashed,
>> changelog-ified patch,
> Thank you. Uhm... I just don't understand why you squashed them into a
> single big patch. Multiple independent smaller patches are better to
> handle, especially when looking for potential bugs later.
>
> Would you mind terribly to split them again?
i just looked at this, but I'm going to leave the patch as a single
patch. The patches in the original series are not completely
independent of each other, it has a bug or two in the middle, and also
some reversed edits. The endpoint is known tested and working, but some
of the intermediate commits aren't that well tested. It *is* too big as
a single commit-- but I think that's better than the original patch
series from my development work, which I never intended to submit as-is
anyway.
Regards,
--jh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-17 9:29 ` John Hood
@ 2016-03-17 16:37 ` Eric Blake
2016-03-19 22:44 ` john hood
0 siblings, 1 reply; 19+ messages in thread
From: Eric Blake @ 2016-03-17 16:37 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 1633 bytes --]
On 03/17/2016 03:29 AM, John Hood wrote:
> On 3/14/2016 6:12 AM, Corinna Vinschen wrote:
>> Hi John,
>>
>> On Mar 13 17:37, john hood wrote:
>>> On 3/4/16 3:58 AM, Corinna Vinschen wrote:
>>>> John,
>>>>
>>>>
>>>> Ping? I'd be interested to get your patches into Cygwin. select
>>>> really needs some kicking :)
>>> Sorry to be so slow responding. Here's a rebased, squashed,
>>> changelog-ified patch,
>> Thank you. Uhm... I just don't understand why you squashed them into a
>> single big patch. Multiple independent smaller patches are better to
>> handle, especially when looking for potential bugs later.
>>
>> Would you mind terribly to split them again?
> i just looked at this, but I'm going to leave the patch as a single
> patch. The patches in the original series are not completely
> independent of each other, it has a bug or two in the middle, and also
> some reversed edits. The endpoint is known tested and working, but some
> of the intermediate commits aren't that well tested. It *is* too big as
> a single commit-- but I think that's better than the original patch
> series from my development work, which I never intended to submit as-is
> anyway.
But that's where 'git rebase' is your friend. Just because your original
series wasn't perfect doesn't mean you can avoid cleaning things up and
posting an improved version. The goal of patch submissions is to make
the reviewer's job easier, even if it makes it longer for you to post
the perfect patch series.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-17 16:37 ` Eric Blake
@ 2016-03-19 22:44 ` john hood
2016-03-20 15:00 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: john hood @ 2016-03-19 22:44 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1.1.1: Type: text/plain, Size: 567 bytes --]
On 3/17/16 12:37 PM, Eric Blake wrote:
> But that's where 'git rebase' is your friend. Just because your original
> series wasn't perfect doesn't mean you can avoid cleaning things up and
> posting an improved version. The goal of patch submissions is to make
> the reviewer's job easier, even if it makes it longer for you to post
> the perfect patch series.
I was coming down sick when I wrote that first email and my logic was
not impeccable...
Here's a patch series with hopefully-reasonable changelog-style git
comments.
regards,
--jh
[-- Attachment #1.1.2: 0001-Use-high-resolution-timebases-for-select.patch --]
[-- Type: text/plain, Size: 11286 bytes --]
From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 28 Jan 2016 17:08:39 -0500
Subject: [PATCH 1/5] Use high-resolution timebases for select().
* cygwait.h: Add cygwait_us() methods.
* select.h: Change prototype for select_stuff::wait() for larger
microsecond timeouts.
* select.cc (pselect): Convert from old cygwin_select().
Implement microsecond timeouts.
(cygwin_select): Rewrite as a wrapper on pselect().
(select): Implement microsecond timeouts.
(select_stuff::wait): Implement microsecond timeouts with a timer
object.
---
winsup/cygwin/cygwait.h | 27 +++++++
winsup/cygwin/select.cc | 182 ++++++++++++++++++++++++++++++------------------
winsup/cygwin/select.h | 2 +-
3 files changed, 141 insertions(+), 70 deletions(-)
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 1f3276c..8f759a4 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);
+
+ select_printf ("pselect(%d, %p, %p, %p, %p, %p)", maxfds, readfds, writefds, exceptfds, to, set);
- if (to)
- select_printf ("to->tv_sec %ld, to->tv_usec %ld, ms %d", to->tv_sec, to->tv_usec, ms);
+ 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;
+
+ 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);
- res = select (maxfds, readfds ?: allocfd_set (maxfds),
- writefds ?: allocfd_set (maxfds),
- exceptfds ?: allocfd_set (maxfds), ms);
+ 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,50 @@ 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 timeout, or not, for WMFO. */
+ DWORD dTimeoutMs;
+ if (us == 0)
+ {
+ dTimeoutMs = 0;
+ }
+ else
+ {
+ dTimeoutMs = INFINITE;
+ }
+ /* Create and set a waitable timer, if a finite timeout has been
+ requested. */
+ LARGE_INTEGER liTimeout;
+ HANDLE hTimeout;
+ NTSTATUS status;
+ status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
+ if (!NT_SUCCESS (status))
+ {
+ select_printf("NtCreateTimer failed (%d)\n", GetLastError());
+ return select_error;
+ }
+ w4[m++] = hTimeout;
+ if (us >= 0)
+ {
+ liTimeout.QuadPart = -us * 10;
+ int setret;
+ status = NtSetTimer (hTimeout, &liTimeout, NULL, NULL, FALSE, 0, NULL);
+ if (!NT_SUCCESS(status))
+ {
+ 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++;
@@ -397,21 +434,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 +477,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 ();
--
2.7.2
[-- Attachment #1.1.3: 0002-Move-get_nonascii_key-into-fhandler_console.patch --]
[-- Type: text/plain, Size: 2538 bytes --]
From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Sat, 30 Jan 2016 17:33:36 -0500
Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
* fhandler.h (fhandler_console): Move get_nonascii_key() from
select.c into this class.
* select.cc (peek_console): Move get_nonascii_key() into
fhandler_console class.
---
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 1c1862b..2dbe377 100644
--- a/winsup/cygwin/fhandler.h
+++ b/winsup/cygwin/fhandler.h
@@ -1469,6 +1469,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 ef2d1c5..f683e90 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;
@@ -2391,7 +2389,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 8f759a4..10205a0 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -883,7 +883,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)
@@ -925,7 +924,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.7.2
[-- Attachment #1.1.4: 0003-Debug-printfs.patch --]
[-- Type: text/plain, Size: 3459 bytes --]
From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Sat, 30 Jan 2016 17:37:33 -0500
Subject: [PATCH 3/5] Debug printfs.
* fhandler.cc (fhandler_base::get_readahead): Add debug code.
* fhandler_console.cc (fhandler_console::read): Add debug code.
* select.cc (pselect): Add debug code.
(peek_console): Add debug code.
---
winsup/cygwin/fhandler.cc | 1 +
winsup/cygwin/fhandler_console.cc | 10 +++++++++-
winsup/cygwin/select.cc | 12 +++++++++++-
3 files changed, 21 insertions(+), 2 deletions(-)
diff --git a/winsup/cygwin/fhandler.cc b/winsup/cygwin/fhandler.cc
index 33743d4..86f77c3 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 f683e90..5dd071c 100644
--- a/winsup/cygwin/fhandler_console.cc
+++ b/winsup/cygwin/fhandler_console.cc
@@ -309,6 +309,8 @@ fhandler_console::read (void *pv, size_t& buflen)
int ch;
set_input_state ();
+ debug_printf("requested buflen %d", buflen);
+
/* Check console read-ahead buffer filled from terminal requests */
if (con.cons_rapoi && *con.cons_rapoi)
{
@@ -318,6 +320,7 @@ fhandler_console::read (void *pv, size_t& buflen)
}
int copied_chars = get_readahead_into_buffer (buf, buflen);
+ debug_printf("copied_chars %d", copied_chars);
if (copied_chars)
{
@@ -695,9 +698,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)
@@ -705,6 +710,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;
@@ -716,6 +723,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 10205a0..25b0c5a 100644
--- a/winsup/cygwin/select.cc
+++ b/winsup/cygwin/select.cc
@@ -925,18 +925,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.7.2
[-- Attachment #1.1.5: 0004-Improve-and-simplify-select.patch --]
[-- Type: text/plain, Size: 6390 bytes --]
From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Thu, 4 Feb 2016 00:44:56 -0500
Subject: [PATCH 4/5] Improve and simplify select().
* cygwait.h (cygwait_us) Remove; this reverts previous changes.
* select.h: Eliminate redundant select_stuff::select_loop state.
* select.cc (select): Eliminate redundant
select_stuff::select_loop state. Eliminate redundant code for
zero timeout. Do not return early on early timer return.
(select_stuff::wait): Eliminate redundant
select_stuff::select_loop state.
---
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 25b0c5a..556a79f 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;
@@ -494,7 +465,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
@@ -508,13 +479,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.7.2
[-- Attachment #1.1.6: 0005-winsup-testsuite-configure-chmod-a-x.patch --]
[-- Type: text/plain, Size: 460 bytes --]
From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
From: John Hood <cgull@glup.org>
Date: Fri, 18 Mar 2016 04:31:16 -0400
Subject: [PATCH 5/5] * winsup/testsuite/configure: chmod a+x
---
winsup/testsuite/configure | 0
1 file changed, 0 insertions(+), 0 deletions(-)
mode change 100644 => 100755 winsup/testsuite/configure
diff --git a/winsup/testsuite/configure b/winsup/testsuite/configure
old mode 100644
new mode 100755
--
2.7.2
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: Cygwin select() issues and improvements
2016-02-15 12:57 ` Corinna Vinschen
2016-02-19 1:20 ` john hood
@ 2016-03-20 13:09 ` Corinna Vinschen
1 sibling, 0 replies; 19+ messages in thread
From: Corinna Vinschen @ 2016-03-20 13:09 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 947 bytes --]
Sidenote from the past:
On Feb 15 13:57, Corinna Vinschen wrote:
> On Feb 14 03:09, john hood wrote:
> > 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'm not aware of such a feature.
On a hunch, do you see a difference when swiching the machine's process
scheduling from "Adjust for best performance of Programs" to "Adjust for
best performance of Background services"?
You can do this in the performance options in the advanced system
settings.
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-19 22:44 ` john hood
@ 2016-03-20 15:00 ` Corinna Vinschen
2016-03-29 12:49 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-03-20 15:00 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 6593 bytes --]
Hi John,
On Mar 19 18:43, john hood wrote:
> From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Thu, 28 Jan 2016 17:08:39 -0500
> Subject: [PATCH 1/5] Use high-resolution timebases for select().
>
> * cygwait.h: Add cygwait_us() methods.
> * select.h: Change prototype for select_stuff::wait() for larger
> microsecond timeouts.
> * select.cc (pselect): Convert from old cygwin_select().
> Implement microsecond timeouts.
> (cygwin_select): Rewrite as a wrapper on pselect().
> (select): Implement microsecond timeouts.
> (select_stuff::wait): Implement microsecond timeouts with a timer
> object.
I have a bit of a problem with patch 1 and 4. In the same patchset
you add cygwait_us and remove it again. That doesn't really look
useful to me. Can you create the patches so that this part is skipped,
please? The rest of the patch should work as is with the existing version
of cygwait.
Two general style issues:
- Please don't use Microsofts variable naming convention. It's used in
kernel.cc to use the same names as in the documentation and there are
a few rare cases where class members are using this convention, but
other than that we usually use lowercase and underscores only. Please
use that as well.
- Always prepend a space to an opening bracket in function or macro calls,
gnu-style. There are a couple of cases where you missed that. If you find
such cases prior to your patch, pleae change them while you're at it.
Btw., it would be helpful to get a patch series the way git format-patch/
send-email generates them. It allows easier review to have every patch
in a single mail. I changed the text on https://cygwin.com/contrib.html
to be a bit more clear about this. Well, hopefully a bit more clear.
> @@ -362,13 +362,50 @@ 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 timeout, or not, for WMFO. */
> + DWORD dTimeoutMs;
> + if (us == 0)
> + {
> + dTimeoutMs = 0;
> + }
> + else
> + {
> + dTimeoutMs = INFINITE;
> + }
Please, no braces for oneliners. Also, this entire statement can be
folded into a oneliner:
ms = us ? INFINITE : 0;
> + status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
Does it really make sense to build up and break down a timer per each
call to select_stuff::wait? This function is called in a loop. What
about creating the timer in the caller and only arm and disarm it in the
wait call?
> + if (dTimeoutMs == INFINITE)
> + {
> + CancelWaitableTimer (hTimeout);
> + CloseHandle (hTimeout);
> + }
For clarity, since the timer has been created and armed using native
functions, please use NtCancelTimer and NtClose here.
> From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Sat, 30 Jan 2016 17:33:36 -0500
> Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
>
> * fhandler.h (fhandler_console): Move get_nonascii_key() from
> select.c into this class.
> * select.cc (peek_console): Move get_nonascii_key() into
> fhandler_console class.
Patch applied.
> From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Sat, 30 Jan 2016 17:37:33 -0500
> Subject: [PATCH 3/5] Debug printfs.
>
> * fhandler.cc (fhandler_base::get_readahead): Add debug code.
> * fhandler_console.cc (fhandler_console::read): Add debug code.
> * select.cc (pselect): Add debug code.
> (peek_console): Add debug code.
Why? It's a lot of additional debug output. Was that only required for
developing or does it serve a real purpose for debugging user bug reports
in future? If so, I wouldn't mind to have a bit of additional description
in the git log to explain the debug statements...
> From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Thu, 4 Feb 2016 00:44:56 -0500
> Subject: [PATCH 4/5] Improve and simplify select().
>
> * cygwait.h (cygwait_us) Remove; this reverts previous changes.
> * select.h: Eliminate redundant select_stuff::select_loop state.
> * select.cc (select): Eliminate redundant
See above.
> @@ -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)
This obviously allows to fold everything into select_stuff::wait, but
the more it seems like a good idea to move the timer creation into the
caller for this case, doesn't it?
Alternatively, we could add a per-thread timer handle to the cygtls
area. It could be created on first use and deleted when the thread
exits. But that's just an idea for a future improvement, never
mind for now.
> From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
> From: John Hood <cgull@glup.org>
> Date: Fri, 18 Mar 2016 04:31:16 -0400
> Subject: [PATCH 5/5] * winsup/testsuite/configure: chmod a+x
Applied.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-20 15:00 ` Corinna Vinschen
@ 2016-03-29 12:49 ` Corinna Vinschen
2016-05-08 20:43 ` john hood
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-03-29 12:49 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 7144 bytes --]
John, ping?
On Mar 20 16:00, Corinna Vinschen wrote:
> On Mar 19 18:43, john hood wrote:
> > From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Thu, 28 Jan 2016 17:08:39 -0500
> > Subject: [PATCH 1/5] Use high-resolution timebases for select().
> >
> > * cygwait.h: Add cygwait_us() methods.
> > * select.h: Change prototype for select_stuff::wait() for larger
> > microsecond timeouts.
> > * select.cc (pselect): Convert from old cygwin_select().
> > Implement microsecond timeouts.
> > (cygwin_select): Rewrite as a wrapper on pselect().
> > (select): Implement microsecond timeouts.
> > (select_stuff::wait): Implement microsecond timeouts with a timer
> > object.
>
> I have a bit of a problem with patch 1 and 4. In the same patchset
> you add cygwait_us and remove it again. That doesn't really look
> useful to me. Can you create the patches so that this part is skipped,
> please? The rest of the patch should work as is with the existing version
> of cygwait.
>
> Two general style issues:
>
> - Please don't use Microsofts variable naming convention. It's used in
> kernel.cc to use the same names as in the documentation and there are
> a few rare cases where class members are using this convention, but
> other than that we usually use lowercase and underscores only. Please
> use that as well.
>
> - Always prepend a space to an opening bracket in function or macro calls,
> gnu-style. There are a couple of cases where you missed that. If you find
> such cases prior to your patch, pleae change them while you're at it.
>
> Btw., it would be helpful to get a patch series the way git format-patch/
> send-email generates them. It allows easier review to have every patch
> in a single mail. I changed the text on https://cygwin.com/contrib.html
> to be a bit more clear about this. Well, hopefully a bit more clear.
>
> > @@ -362,13 +362,50 @@ 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 timeout, or not, for WMFO. */
> > + DWORD dTimeoutMs;
> > + if (us == 0)
> > + {
> > + dTimeoutMs = 0;
> > + }
> > + else
> > + {
> > + dTimeoutMs = INFINITE;
> > + }
>
> Please, no braces for oneliners. Also, this entire statement can be
> folded into a oneliner:
>
> ms = us ? INFINITE : 0;
>
> > + status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
>
> Does it really make sense to build up and break down a timer per each
> call to select_stuff::wait? This function is called in a loop. What
> about creating the timer in the caller and only arm and disarm it in the
> wait call?
>
> > + if (dTimeoutMs == INFINITE)
> > + {
> > + CancelWaitableTimer (hTimeout);
> > + CloseHandle (hTimeout);
> > + }
>
> For clarity, since the timer has been created and armed using native
> functions, please use NtCancelTimer and NtClose here.
>
> > From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Sat, 30 Jan 2016 17:33:36 -0500
> > Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
> >
> > * fhandler.h (fhandler_console): Move get_nonascii_key() from
> > select.c into this class.
> > * select.cc (peek_console): Move get_nonascii_key() into
> > fhandler_console class.
>
> Patch applied.
>
> > From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Sat, 30 Jan 2016 17:37:33 -0500
> > Subject: [PATCH 3/5] Debug printfs.
> >
> > * fhandler.cc (fhandler_base::get_readahead): Add debug code.
> > * fhandler_console.cc (fhandler_console::read): Add debug code.
> > * select.cc (pselect): Add debug code.
> > (peek_console): Add debug code.
>
> Why? It's a lot of additional debug output. Was that only required for
> developing or does it serve a real purpose for debugging user bug reports
> in future? If so, I wouldn't mind to have a bit of additional description
> in the git log to explain the debug statements...
>
> > From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Thu, 4 Feb 2016 00:44:56 -0500
> > Subject: [PATCH 4/5] Improve and simplify select().
> >
> > * cygwait.h (cygwait_us) Remove; this reverts previous changes.
> > * select.h: Eliminate redundant select_stuff::select_loop state.
> > * select.cc (select): Eliminate redundant
>
> See above.
>
> > @@ -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)
>
> This obviously allows to fold everything into select_stuff::wait, but
> the more it seems like a good idea to move the timer creation into the
> caller for this case, doesn't it?
>
> Alternatively, we could add a per-thread timer handle to the cygtls
> area. It could be created on first use and deleted when the thread
> exits. But that's just an idea for a future improvement, never
> mind for now.
>
> > From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
> > From: John Hood <cgull@glup.org>
> > Date: Fri, 18 Mar 2016 04:31:16 -0400
> > Subject: [PATCH 5/5] * winsup/testsuite/configure: chmod a+x
>
> Applied.
>
>
> Thanks,
> Corinna
>
> --
> Corinna Vinschen Please, send mails regarding Cygwin to
> Cygwin Maintainer cygwin AT cygwin DOT com
> Red Hat
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-03-29 12:49 ` Corinna Vinschen
@ 2016-05-08 20:43 ` john hood
2016-05-18 19:23 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: john hood @ 2016-05-08 20:43 UTC (permalink / raw)
To: cygwin-patches
On 3/29/16 8:49 AM, Corinna Vinschen wrote:
> John, ping?
Sorry it took so long to reply, but I finally got around to cleaning up
the patchset, I've mailed it separately. I was pretty frustrated at my
slow Windows machine and the friction in dealing with the project, and
also sick for a while.
> On Mar 20 16:00, Corinna Vinschen wrote:
>> On Mar 19 18:43, john hood wrote:
>>> From c805552cdc9e673ef2330388ddb8b7a0da741766 Mon Sep 17 00:00:00 2001
>>> From: John Hood <cgull@glup.org>
>>> Date: Thu, 28 Jan 2016 17:08:39 -0500
>>> Subject: [PATCH 1/5] Use high-resolution timebases for select().
>>>
>>> * cygwait.h: Add cygwait_us() methods.
>>> * select.h: Change prototype for select_stuff::wait() for larger
>>> microsecond timeouts.
>>> * select.cc (pselect): Convert from old cygwin_select().
>>> Implement microsecond timeouts.
>>> (cygwin_select): Rewrite as a wrapper on pselect().
>>> (select): Implement microsecond timeouts.
>>> (select_stuff::wait): Implement microsecond timeouts with a timer
>>> object.
>>
>> I have a bit of a problem with patch 1 and 4. In the same patchset
>> you add cygwait_us and remove it again. That doesn't really look
>> useful to me. Can you create the patches so that this part is skipped,
>> please? The rest of the patch should work as is with the existing version
>> of cygwait.
The issue here is that patch 1 changed the structure of the code and
added the requirement for a cygwait_us(), but patch 4 couldn't be moved
above patch 1 without writing/testing new code to get a good patch
series. I avoided the issue by simply changing 'cygwait_us(...,
microsecond_timeout)' to 'cygwait(..., microsecond_timeout * 1000)',
which makes patch 1 actually not microsecond resolution anymore, but did
allow me to get rid of the add/revert of 'cygwait_us()'. The final code
is still the same and has the intended high time resolution capability.
>> Two general style issues:
>>
>> - Please don't use Microsofts variable naming convention. It's used in
>> kernel.cc to use the same names as in the documentation and there are
>> a few rare cases where class members are using this convention, but
>> other than that we usually use lowercase and underscores only. Please
>> use that as well.
Fixed.
>> - Always prepend a space to an opening bracket in function or macro calls,
>> gnu-style. There are a couple of cases where you missed that. If you find
>> such cases prior to your patch, pleae change them while you're at it.
Fixed.
>> Btw., it would be helpful to get a patch series the way git format-patch/
>> send-email generates them. It allows easier review to have every patch
>> in a single mail. I changed the text on https://cygwin.com/contrib.html
>> to be a bit more clear about this. Well, hopefully a bit more clear.
>>
>>> @@ -362,13 +362,50 @@ 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 timeout, or not, for WMFO. */
>>> + DWORD dTimeoutMs;
>>> + if (us == 0)
>>> + {
>>> + dTimeoutMs = 0;
>>> + }
>>> + else
>>> + {
>>> + dTimeoutMs = INFINITE;
>>> + }
>>
>> Please, no braces for oneliners. Also, this entire statement can be
>> folded into a oneliner:
>>
>> ms = us ? INFINITE : 0;
In my experience brace-less conditional blocks are dangerous, but this
does collapse fine into a ternary conditional.
>>> + status = NtCreateTimer(&hTimeout, TIMER_ALL_ACCESS, NULL, NotificationTimer);
>>
>> Does it really make sense to build up and break down a timer per each
>> call to select_stuff::wait? This function is called in a loop. What
>> about creating the timer in the caller and only arm and disarm it in the
>> wait call?
Not fixed. Managing the resource gets more complicated if you hoist it
up to the caller with its various exit conditions. Plus, doing that
doesn't make the minimum or typical cost of select any less, it only
makes the handling of events that select ignores slightly more
efficient. As I see it, ignored mouse events on the console are the
only case where we're likely to see many ignored events, correct me if
I'm wrong.
>>> + if (dTimeoutMs == INFINITE)
>>> + {
>>> + CancelWaitableTimer (hTimeout);
>>> + CloseHandle (hTimeout);
>>> + }
>>
>> For clarity, since the timer has been created and armed using native
>> functions, please use NtCancelTimer and NtClose here.
Fixed.
>>> From 225f852594d9ff6a1231063ece3d529b9cc1bf7f Mon Sep 17 00:00:00 2001
>>> From: John Hood <cgull@glup.org>
>>> Date: Sat, 30 Jan 2016 17:33:36 -0500
>>> Subject: [PATCH 2/5] Move get_nonascii_key into fhandler_console.
>>>
>>> * fhandler.h (fhandler_console): Move get_nonascii_key() from
>>> select.c into this class.
>>> * select.cc (peek_console): Move get_nonascii_key() into
>>> fhandler_console class.
>>
>> Patch applied.
>>
>>> From b2e2b5ac1d6b62610c51a66113e5ab97b1d43750 Mon Sep 17 00:00:00 2001
>>> From: John Hood <cgull@glup.org>
>>> Date: Sat, 30 Jan 2016 17:37:33 -0500
>>> Subject: [PATCH 3/5] Debug printfs.
>>>
>>> * fhandler.cc (fhandler_base::get_readahead): Add debug code.
>>> * fhandler_console.cc (fhandler_console::read): Add debug code.
>>> * select.cc (pselect): Add debug code.
>>> (peek_console): Add debug code.
>>
>> Why? It's a lot of additional debug output. Was that only required for
>> developing or does it serve a real purpose for debugging user bug reports
>> in future? If so, I wouldn't mind to have a bit of additional description
>> in the git log to explain the debug statements...
Split out into a separate patch, apply or discard as you please. I
think it's useful for user bugs (when select is breaking, it really
helps to see what's going on inside) but I don't feel strongly about it.
>>> From cf2db014fefd4a8488316cf9313325b79e25518d Mon Sep 17 00:00:00 2001
>>> From: John Hood <cgull@glup.org>
>>> Date: Thu, 4 Feb 2016 00:44:56 -0500
>>> Subject: [PATCH 4/5] Improve and simplify select().
>>>
>>> * cygwait.h (cygwait_us) Remove; this reverts previous changes.
>>> * select.h: Eliminate redundant select_stuff::select_loop state.
>>> * select.cc (select): Eliminate redundant
>>
>> See above.
>>
>>> @@ -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)
>>
>> This obviously allows to fold everything into select_stuff::wait, but
>> the more it seems like a good idea to move the timer creation into the
>> caller for this case, doesn't it?
>>
>> Alternatively, we could add a per-thread timer handle to the cygtls
>> area. It could be created on first use and deleted when the thread
>> exits. But that's just an idea for a future improvement, never
>> mind for now.
These are things that can be fixed/improved with more effort, but I have
other things I'd rather be working on.
>>> From 3f3f7112f948d70c15046641cf3cc898a9ca4c71 Mon Sep 17 00:00:00 2001
>>> From: John Hood <cgull@glup.org>
>>> Date: Fri, 18 Mar 2016 04:31:16 -0400
>>> Subject: [PATCH 5/5] * winsup/testsuite/configure: chmod a+x
>>
>> Applied.
regards,
--jh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-05-08 20:43 ` john hood
@ 2016-05-18 19:23 ` Corinna Vinschen
2016-05-19 0:02 ` john hood
0 siblings, 1 reply; 19+ messages in thread
From: Corinna Vinschen @ 2016-05-18 19:23 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]
Hi John,
On May 8 16:43, john hood wrote:
> On 3/29/16 8:49 AM, Corinna Vinschen wrote:
> > John, ping?
>
> Sorry it took so long to reply, but I finally got around to cleaning up
> the patchset, I've mailed it separately.
I don't see the patchset anywhere. Did I miss your mail or did it
fail to make it to this list?!?
> I was pretty frustrated at my
> slow Windows machine and the friction in dealing with the project,
What friction? Was there anything I or others did to alienate you?
If there's some problem, please also feel free to discuss on the
#cygwin-developers IRC channel @Freenode. You're apparently lurking
anyway.
> also sick for a while.
My sympathy.
And no worries if anything takes longer than anticipated. We're not
on some tight schedule here :)
> > On Mar 20 16:00, Corinna Vinschen wrote:
> >> Does it really make sense to build up and break down a timer per each
> >> call to select_stuff::wait? This function is called in a loop. What
> >> about creating the timer in the caller and only arm and disarm it in the
> >> wait call?
>
> Not fixed. Managing the resource gets more complicated if you hoist it
> up to the caller with its various exit conditions. Plus, doing that
> doesn't make the minimum or typical cost of select any less, it only
> makes the handling of events that select ignores slightly more
> efficient. As I see it, ignored mouse events on the console are the
> only case where we're likely to see many ignored events, correct me if
> I'm wrong.
I don't know ATM. I'll have another look when I review the patch but
actually we can also just keep it in mind for some later optimization,
*iff* it makes sense.
> >> Why? It's a lot of additional debug output. Was that only required for
> >> developing or does it serve a real purpose for debugging user bug reports
> >> in future? If so, I wouldn't mind to have a bit of additional description
> >> in the git log to explain the debug statements...
>
> Split out into a separate patch, apply or discard as you please. I
> think it's useful for user bugs (when select is breaking, it really
> helps to see what's going on inside) but I don't feel strongly about it.
Ok.
Thanks,
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-05-18 19:23 ` Corinna Vinschen
@ 2016-05-19 0:02 ` john hood
2016-05-20 9:16 ` Corinna Vinschen
0 siblings, 1 reply; 19+ messages in thread
From: john hood @ 2016-05-19 0:02 UTC (permalink / raw)
To: cygwin-patches
On 5/18/16 3:23 PM, Corinna Vinschen wrote:
> Hi John,
>
> On May 8 16:43, john hood wrote:
>> On 3/29/16 8:49 AM, Corinna Vinschen wrote:
>>> John, ping?
>>
>> Sorry it took so long to reply, but I finally got around to cleaning up
>> the patchset, I've mailed it separately.
>
> I don't see the patchset anywhere. Did I miss your mail or did it
> fail to make it to this list?!?
It never made it to the list. Some aspect of my network's email config
(I still haven't figured out what) caused email sent directly by my
workstation to be dropped by the sourceware.org mail server. I changed
it to use my mail server instead and that worked.
>> I was pretty frustrated at my
>> slow Windows machine and the friction in dealing with the project,
> What friction? Was there anything I or others did to alienate you?
> If there's some problem, please also feel free to discuss on the
> #cygwin-developers IRC channel @Freenode. You're apparently lurking
> anyway.
The slow Windows machine is probably the larger part of that. It's a
netbook-class machine. It's very slow; development on it was rather
unpleasant. I just set up a VM with a Windows 10 preview on a fast
machine-- it's around four times faster.
But also, working with the email-and-patches style of work that you have
here is really considerably more work than working with projects on
GitHub (I'm a maintainer of a project there) or GitLab. The email
configuration issue above is a good example. It really is easier for
both contributors and maintainers. Assuming you have an account,
contributing a change is something like this: push a branch ("fork") to
the website's shared repo, go to its web view, enter your message for
the pull request, click send, and done. I know Cygwin/Redhat are
unlikely to move to GitHub, but GitLab offers the same basic workflow,
and I would guess 90% of the functionality of GitHub. I don't want to
over-emphasize this, but it is significant.
regards,
--jh
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH] Re: Cygwin select() issues and improvements
2016-05-19 0:02 ` john hood
@ 2016-05-20 9:16 ` Corinna Vinschen
0 siblings, 0 replies; 19+ messages in thread
From: Corinna Vinschen @ 2016-05-20 9:16 UTC (permalink / raw)
To: cygwin-patches
[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]
On May 18 20:02, john hood wrote:
> On 5/18/16 3:23 PM, Corinna Vinschen wrote:
> > Hi John,
> >
> > On May 8 16:43, john hood wrote:
> >> On 3/29/16 8:49 AM, Corinna Vinschen wrote:
> >>> John, ping?
> >>
> >> Sorry it took so long to reply, but I finally got around to cleaning up
> >> the patchset, I've mailed it separately.
> >
> > I don't see the patchset anywhere. Did I miss your mail or did it
> > fail to make it to this list?!?
>
> It never made it to the list. Some aspect of my network's email config
> (I still haven't figured out what) caused email sent directly by my
> workstation to be dropped by the sourceware.org mail server. I changed
> it to use my mail server instead and that worked.
Probably some trusted server test. I don't remember how the method is
called but it's used by quite a lot of mail servers (and equally quite
a lot don't...)
> >> I was pretty frustrated at my
> >> slow Windows machine and the friction in dealing with the project,
>
> > What friction? Was there anything I or others did to alienate you?
> > If there's some problem, please also feel free to discuss on the
> > #cygwin-developers IRC channel @Freenode. You're apparently lurking
> > anyway.
>
> The slow Windows machine is probably the larger part of that. It's a
> netbook-class machine. It's very slow; development on it was rather
> unpleasant. I just set up a VM with a Windows 10 preview on a fast
> machine-- it's around four times faster.
>
> But also, working with the email-and-patches style of work that you have
> here is really considerably more work than working with projects on
> GitHub (I'm a maintainer of a project there) or GitLab.
Linux kernel development works the same way and it works pretty well.
Also, git makes it very easy to create and send patches and patchsets
via git format-patch and git send-email. It's an improvement on an
almost excessive scale compared to CVS 'til early 2015 :)
Corinna
--
Corinna Vinschen Please, send mails regarding Cygwin to
Cygwin Maintainer cygwin AT cygwin DOT com
Red Hat
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2016-05-20 9:16 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-14 8:12 Cygwin select() issues and improvements john hood
2016-02-14 12:29 ` Thomas Wolff
2016-02-14 18:03 ` John Hood
2016-02-15 12:57 ` Corinna Vinschen
2016-02-19 1:20 ` john hood
2016-02-19 10:46 ` Corinna Vinschen
2016-03-04 8:58 ` Corinna Vinschen
2016-03-13 21:37 ` [PATCH] " john hood
2016-03-14 10:13 ` Corinna Vinschen
2016-03-17 9:29 ` John Hood
2016-03-17 16:37 ` Eric Blake
2016-03-19 22:44 ` john hood
2016-03-20 15:00 ` Corinna Vinschen
2016-03-29 12:49 ` Corinna Vinschen
2016-05-08 20:43 ` john hood
2016-05-18 19:23 ` Corinna Vinschen
2016-05-19 0:02 ` john hood
2016-05-20 9:16 ` Corinna Vinschen
2016-03-20 13:09 ` Corinna Vinschen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).