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

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

[Irányossy Knoblauch Artúr]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

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

[-- Attachment #2: 0004-Fix-data-race-during-lazy-initialization.patch --]
[-- Type: text/x-patch, Size: 2379 bytes --]

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

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

[john hood]

On 2/18/16 6:39 PM, Irányossy Knoblauch Artúr wrote:
> 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 have some interest in this because my work on select() may interact
with what you're doing here.

There's some information on the web discussing issues with
QueryPerformanceCounter(), for example
<http://www.virtualdub.org/blog/pivot/entry.php?id=106>.  This is mostly
an issue with CPUs available in the (I think) 2003-2006 time frame, such
as early AMD Athlons and early Intel Core iNNN and iNNNN CPUs.  Earlier
CPUs didn't have both changeable clock rates and RDTSC, and later CPUs
had RDTSC but the clock rate is constant for RDTSC.  It's also possible
that only some versions of Windows have issues in this area, maybe later
versions of Windows avoid this problem.  Does your code work properly in
this case?

regards,

  --jh

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

[ikartur]

Hi,

On Feb 19, 2016 2:35 AM, "john hood" <cgull@glup.org> wrote:

> There's some information on the web discussing issues with
> QueryPerformanceCounter()

QueryPerformanceCounter() is the officially recommended method for time interval measurements:

https://blogs.msdn.microsoft.com/oldnewthing/20140822-01/?p=163/

https://msdn.microsoft.com/en-us/library/windows/desktop/dn553408(v=vs.85).aspx

Best Regards,
Artúr

-----Original Message-----
From: john hood <cgull@glup.org>
To: cygwin-patches@cygwin.com
Sent: Fri, 19 Feb 2016 2:35
Subject: Re: [PATCH] Multiple timer issues + new [PATCH]

On 2/18/16 6:39 PM, Irányossy Knoblauch Artúr wrote:
> 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 have some interest in this because my work on select() may interact
with what you're doing here.

There's some information on the web discussing issues with
QueryPerformanceCounter(), for example
<http://www.virtualdub.org/blog/pivot/entry.php?id=106>.  This is mostly
an issue with CPUs available in the (I think) 2003-2006 time frame, such
as early AMD Athlons and early Intel Core iNNN and iNNNN CPUs.  Earlier
CPUs didn't have both changeable clock rates and RDTSC, and later CPUs
had RDTSC but the clock rate is constant for RDTSC.  It's also possible
that only some versions of Windows have issues in this area, maybe later
versions of Windows avoid this problem.  Does your code work properly in
this case?

regards,

  --jh

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1440 bytes --]

On Feb 19 00:39, Irányossy Knoblauch Artúr wrote:
> 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. :-)

It's really simple, trust me :)

> 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.

Yeah, right.  Keep in mind that the original times.cc is from pre-2000
and hires.h is from 2002.  Way back when, source comments were
unnecessary because, as we all well know, source is self-documenting...

These days, I think what we should do here is

a) ignore the names and maybe even bulk-rename variables and types
   to something more speaking (rel_timer, abs_timer or whatever)

b) add comments, comments, comments.

> 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.

Cool, thank you!


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 --]