Re: [PATCH] Multiple timer issues + new [PATCH]

[IrÃnyossy Knoblauch ArtÃr <ikartur at gmail dot com>]

Hi,

On Thu, Feb 18, 2016 at 12:28 PM, Corinna Vinschen
<corinna-cygwin@cygwin.com> wrote:

> Would you mind terribly to send a copyright assignment per
> https://cygwin.com/contrib.html?  If you send it as PDF by mail it takes
> usually just a few days to be countersigned.

OK, I will try my best. :-)

> I would apply patch 2 immediately, but as far as I can see it relies
> on patch 1.  Without patch 1, exchanging gtod with ntod will not change
> anything since it's still a non-monotonic timer.  Or am I missing
> something?

Exchanging 'gtod' with 'ntod' in select() could solve the issue if
'ntod' had its msecs() method implemented. So you are correct, the
patches are dependent on each other.

The important thing to note is that gtod (type hires_ms) is getting
its time using GetSystemTimeAsFileTime(), which is the system time
(UTC). Thus, the time returned by gtod is not adequate for measuring
time intervals, because a system time adjustment could interfere with
the measurement.

The ntod timer (type hires_ns), however, is getting its time value
from QueryPerformanceCounter(), which, according to the MSDN
documentation, will provide a "time stamp that can be used for
time-interval measurements" -- that is just what the doctor ordered.
:-)

I could not find any information regarding what the names 'gtod' and
'ntod' are supposed to mean, and their type names, hires_ms and
hires_ns, respectively, aren't conveying that 'ntod' is monotonic
while 'gtod' isn't.

Also, as I have been writing this mail, I have noticed that there is
still a data race left in the prime() function, so I have made a patch
for that, too.

Best Regards,
ArtÃr

From 904534671453b8ca16e010ea35220ec8a2c2dc28 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Ir=C3=A1nyossy=20Knoblauch=20Art=C3=BAr?=
 <ikartur@gmail.com>
Date: Thu, 18 Feb 2016 23:48:47 +0100
Subject: [PATCH 4/4] Fix data race during lazy initialization

* Move initialization to the constructor of hires_ns.

  In a multithreaded environment, unsynchronized lazy initialization can
  cause race condition errors.

* Cleaned up reset() function which had no effect.
---
 winsup/cygwin/hires.h  | 15 ++++-----------
 winsup/cygwin/times.cc | 11 ++---------
 2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/winsup/cygwin/hires.h b/winsup/cygwin/hires.h
index 8311ad1..8a32b67 100644
--- a/winsup/cygwin/hires.h
+++ b/winsup/cygwin/hires.h
@@ -34,26 +34,19 @@ details. */
 /* # of 100ns intervals per second. */
 #define NSPERSEC 10000000LL
 
-class hires_base
-{
- protected:
-  int inited;
- public:
-  void reset() {inited = false;}
-};
-
-class hires_ns : public hires_base
+class hires_ns
 {
   double freq;
-  void prime ();
+  bool inited;
  public:
+  hires_ns();
   LONGLONG nsecs ();
   LONGLONG usecs () {return nsecs () / 1000LL;}
   LONGLONG msecs () {return nsecs () / 1000000LL;}
   LONGLONG resolution();
 };
 
-class hires_ms : public hires_base
+class hires_ms
 {
  public:
   LONGLONG nsecs ();
diff --git a/winsup/cygwin/times.cc b/winsup/cygwin/times.cc
index cb498d7..2365193 100644
--- a/winsup/cygwin/times.cc
+++ b/winsup/cygwin/times.cc
@@ -142,7 +142,6 @@ settimeofday (const struct timeval *tv, const struct timezone *tz)
       st.wMilliseconds = tv->tv_usec / 1000;
 
       res = -!SetSystemTime (&st);
-      gtod.reset ();
 
       if (res)
 	set_errno (EPERM);
@@ -472,14 +471,12 @@ ftime (struct timeb *tp)
   return 0;
 }
 
-#define stupid_printf if (cygwin_finished_initializing) debug_printf
-void
-hires_ns::prime ()
+hires_ns::hires_ns ()
 {
   LARGE_INTEGER ifreq;
   if (!QueryPerformanceFrequency (&ifreq))
     {
-      inited = -1;
+      inited = false;
       return;
     }
 
@@ -491,8 +488,6 @@ LONGLONG
 hires_ns::nsecs ()
 {
   if (!inited)
-    prime ();
-  if (inited < 0)
     {
       set_errno (ENOSYS);
       return (LONGLONG) -1;
@@ -643,8 +638,6 @@ LONGLONG
 hires_ns::resolution ()
 {
   if (!inited)
-    prime ();
-  if (inited < 0)
     {
       set_errno (ENOSYS);
       return (LONGLONG) -1;
-- 
1.9.1