[PATCH] gettimeofday time travels V2

[PATCH] gettimeofday time travels V2

[Philip Aston]

[-- Attachment #1: message body text --]
[-- Type: text/plain, Size: 10895 bytes --]


Patch and changelog below, two new files attached (sorry couldn't
figure out -N for cvs diff).


Chat for the interested:

Philip Aston writes:
 > Short of some unexpected wParam values, which I'll track down, I
 > now have this working.

This turned out to be an incorrect value in
winsup/w32api/include/pbt.h. It amused me that its exactly the value I
needed to trap.

Christopher Faylor writes:
 > The correct solution is to resync after events which cause the
 > clock to stop.

Done.

The power events don't let us trap the resume before time jumps
forward. Here's a trace of suspend and resume whilst printing out the
results of gettimeofday:

 D:\cygwin\cygdebug>gettimeofday-bug.exe
 gettimeofday() returns 1021395726 857121
 gettimeofday() returns 1021395728 860774
       6 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=0
  118770 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=4
  126448 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=4
 1309042 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=10
 1309336 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=10
 gettimeofday() returns 1021395730 873752
 gettimeofday() returns 1021431988 503017
 gettimeofday() returns 1021431999 871955
 1913257134 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=18
 1913288971 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=18
1 913290613 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=7
     177 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=7
gettimeofday() returns 1021395754 329588
  326676 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=10
  328531 [win] gettimeofday-bug 5908 hires::wm_event: PWRBRDCST, wParam=10
 gettimeofday() returns 1021395756 343782


Personally, I'm not too concerned about this. Applications that assume
that time never goes backwards are broken. AFAICS, the only problem is
if gettimeofday is used to periodically determine whether a certain
time has elapsed, in which case things could be incorrectly triggered
after a resume. A reasonable defence would be to apply my original
patch. Chris, that original patch is complementary, and can be applied
as well as the attached - up to you.

 > > 2. <snip> [incremental or generic?]

Robert Collins writes:
 > Incremental is best. Lower risk. Structure what you put in so that it
 > should be easily refactored, but that's all.

But cgf says:
 > General is always good.

I compromised. Generic wm_listener and wm_listener_list types, and a
register_power_event_listener() method.

 > > 3. <snip> [how to implement thread safe list?]

Robert Collins writes:
 > Mutex's are the usual solution. Also there's a thread safe list in
 > thread.h. (It uses InterLocked calls).

The list in thread.h is specific to a type, and intrusive. I used a
mutex to create my own thread-safe, non-intrusive list. I used pthread
mutexes instead of mutos since mutos must be allocated statically.

Christopher Faylor writes:
 > It may make sense to just make all of the members of the hires
 > class static since they are just maintaining global state.

Agree. Personally I'd use a singleton object rather than static
members which would also allow hires to continue to be a wm_listener,
but it amounts to the same thing. However, I didn't do this as I
consider it outside of the scope of the fix.

- Phil


w32api changes:

2002-05-15  Philip Aston  <phlipa@mail.com>

	* include/pbt.h (PBT_APMRESUMESUSPEND): Correct value is 7.



cygwin changes:

2002-05-15  Philip Aston  <philipa@mail.com>

	* Makefile.in (DLL_OFILES): Add wm_listener.o.
	* hires.h (class hires): Implement wm_listener.
	* strace.cc (strace::microseconds): Register hires as a
	power_event_listener.
	* times.cc (gettimeofday): New gtod constructor.
	(hires::hires): Register hires as a power_event_listener.
	(hires::wm_event): Mark hires for reprime after resume events.
	* window.cc (WndProc): Listen for WM_POWERBROADCAST.
	(register_power_event_listener): Register a power event wm_listener.
	* winsup.h: Declare register_power_event_listener().
	* wm_listener.h: New file. Windows event listener interface.
	* wm_listener.cc: New file. Windows event listener interface.



Index: cygwin/Makefile.in
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/Makefile.in,v
retrieving revision 1.86
diff -u -p -u -r1.86 Makefile.in
--- cygwin/Makefile.in	2 May 2002 04:13:43 -0000	1.86
+++ cygwin/Makefile.in	15 May 2002 20:18:43 -0000
@@ -134,6 +134,7 @@ DLL_OFILES:=assert.o autoload.o cygheap.
 	signal.o sigproc.o smallprint.o spawn.o strace.o strsep.o sync.o \
 	syscalls.o sysconf.o syslog.o termios.o thread.o times.o tty.o uinfo.o \
 	uname.o v8_regexp.o v8_regerror.o v8_regsub.o wait.o wincap.o window.o \
+	wm_listener.o \
 	$(EXTRA_DLL_OFILES) $(EXTRA_OFILES) $(MALLOC_OFILES) $(MT_SAFE_OBJECTS)
 
 GMON_OFILES:=gmon.o mcount.o profil.o
@@ -203,7 +204,7 @@ install-man:
 
 install_target: cygserver.exe
 	$(INSTALL_PROGRAM) cygserver.exe $(bindir)/cygserver.exe
-	
+
 install_host:
 
 
Index: cygwin/hires.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/hires.h,v
retrieving revision 1.2
diff -u -p -u -r1.2 hires.h
--- cygwin/hires.h	15 Feb 2002 17:06:40 -0000	1.2
+++ cygwin/hires.h	15 May 2002 20:18:43 -0000
@@ -11,7 +11,9 @@ details. */
 #ifndef __HIRES_H__
 #define __HIRES_H__
 
-class hires
+#include "wm_listener.h"
+
+class hires : public wm_listener
 {
   int inited;
   LARGE_INTEGER primed_ft;
@@ -19,6 +21,9 @@ class hires
   double freq;
   void prime () __attribute__ ((regparm (1)));
  public:
+  hires (bool register_now);
   LONGLONG usecs (bool justdelta) __attribute__ ((regparm (2)));
+
+  void wm_event (UINT uMsg, WPARAM wParam, LPARAM lParam);
 };
 #endif /*__HIRES_H__*/
Index: cygwin/strace.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/strace.cc,v
retrieving revision 1.26
diff -u -p -u -r1.26 strace.cc
--- cygwin/strace.cc	15 Apr 2002 19:23:27 -0000	1.26
+++ cygwin/strace.cc	15 May 2002 20:18:43 -0000
@@ -61,7 +61,14 @@ strace::hello()
 int
 strace::microseconds ()
 {
-  static hires now;
+  static hires now(false);
+  static bool registered = false;
+
+  if (cygwin_finished_initializing && !registered) {
+    registered = true;		/* or recurse */
+    register_power_event_listener(now);
+  }
+
   return (int) now.usecs (true);
 }
 
Index: cygwin/times.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/times.cc,v
retrieving revision 1.31
diff -u -p -u -r1.31 times.cc
--- cygwin/times.cc	12 May 2002 01:50:38 -0000	1.31
+++ cygwin/times.cc	15 May 2002 20:18:47 -0000
@@ -16,6 +16,9 @@ details. */
 #include <stdio.h>
 #include <stdlib.h>
 #include <errno.h>
+#include <pbt.h>
+#include <wingdi.h>
+#include <winuser.h>
 #include "cygerrno.h"
 #include "perprocess.h"
 #include "security.h"
@@ -154,7 +157,8 @@ totimeval (struct timeval *dst, FILETIME
 extern "C" int
 gettimeofday(struct timeval *tv, struct timezone *tz)
 {
-  static hires gtod;
+  static hires gtod (true);
+
   static bool tzflag;
   LONGLONG now = gtod.usecs (false);
   if (now == (LONGLONG) -1)
@@ -548,6 +552,17 @@ cygwin_tzset ()
 {
 }
 
+hires::hires (bool register_now)
+  : inited(0)
+{
+  /* Some things (strace) need to create a hireses early on with
+     register_now=false and call register_power_event_listener later
+     on when malloc will work. */
+  if (register_now) {
+    register_power_event_listener (*this);
+  }
+}
+
 void
 hires::prime ()
 {
@@ -600,5 +615,20 @@ hires::usecs (bool justdelta)
 
   // FIXME: Use round() here?
   now.QuadPart = (LONGLONG) (freq * (double) (now.QuadPart - primed_pc.QuadPart));
+
   return justdelta ? now.QuadPart : primed_ft.QuadPart + now.QuadPart;
+}
+
+/* Performance counters don't increment whilst suspended. Trap resume
+   events and force hires counter to reinitialise. */
+void
+hires::wm_event (UINT uMsg, WPARAM wParam, LPARAM lParam)
+{
+  if (uMsg == WM_POWERBROADCAST) {
+    if (wParam == PBT_APMRESUMECRITICAL ||
+	wParam == PBT_APMRESUMESUSPEND ||
+	wParam == PBT_APMRESUMESTANDBY) {
+      inited = 0;
+    }
+  }
 }
Index: cygwin/window.cc
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/window.cc,v
retrieving revision 1.17
diff -u -p -u -r1.17 window.cc
--- cygwin/window.cc	6 Jan 2002 09:28:13 -0000	1.17
+++ cygwin/window.cc	15 May 2002 20:18:47 -0000
@@ -24,12 +24,15 @@ details. */
 #include "cygerrno.h"
 #include "perprocess.h"
 #include "security.h"
+#include "wm_listener.h"
 
 static NO_COPY UINT timer_active = 0;
 static NO_COPY struct itimerval itv;
 static NO_COPY DWORD start_time;
 static NO_COPY HWND ourhwnd = NULL;
 
+static wm_listener_list power_event_listener_list;
+
 static LRESULT CALLBACK
 WndProc (HWND hwnd, UINT uMsg, WPARAM wParam, LPARAM lParam)
 {
@@ -68,6 +71,9 @@ WndProc (HWND hwnd, UINT uMsg, WPARAM wP
       else
 	raise (SIGIO);
       return 0;
+    case WM_POWERBROADCAST:
+      power_event_listener_list.wm_event(uMsg, wParam, lParam);
+      return 0;
     default:
       return DefWindowProc (hwnd, uMsg, wParam, lParam);
     }
@@ -263,3 +269,10 @@ ualarm (useconds_t value, useconds_t int
   return (otimer.it_value.tv_sec * 1000000) + otimer.it_value.tv_usec;
 }
 
+void __stdcall
+register_power_event_listener(wm_listener& listener) 
+{
+  /* Ensure hidden window exists. */
+  gethwnd ();
+  power_event_listener_list.add (listener);
+}
Index: cygwin/winsup.h
===================================================================
RCS file: /cvs/src/src/winsup/cygwin/winsup.h,v
retrieving revision 1.89
diff -u -p -u -r1.89 winsup.h
--- cygwin/winsup.h	2 May 2002 04:13:48 -0000	1.89
+++ cygwin/winsup.h	15 May 2002 20:18:47 -0000
@@ -161,6 +161,8 @@ BOOL __stdcall check_pty_fds (void);
 /* Invisible window initialization/termination. */
 HWND __stdcall gethwnd (void);
 void __stdcall window_terminate (void);
+class wm_listener;
+void __stdcall register_power_event_listener (wm_listener& l);
 
 /* Globals that handle initialization of winsock in a child process. */
 extern HANDLE wsock32_handle;
Index: w32api/include/pbt.h
===================================================================
RCS file: /cvs/src/src/winsup/w32api/include/pbt.h,v
retrieving revision 1.2
diff -u -p -u -r1.2 pbt.h
--- w32api/include/pbt.h	9 Mar 2002 09:04:09 -0000	1.2
+++ w32api/include/pbt.h	15 May 2002 20:18:47 -0000
@@ -11,7 +11,7 @@
 #define PBT_APMSUSPEND 4
 #define PBT_APMSTANDBY 5
 #define PBT_APMRESUMECRITICAL 6
-#define PBT_APMRESUMESUSPEND 8
+#define PBT_APMRESUMESUSPEND 7
 #define PBT_APMRESUMESTANDBY 8
 #define PBTF_APMRESUMEFROMFAILURE 1
 #define PBT_APMBATTERYLOW 9


[-- Attachment #2: wm_listener.cc --]
[-- Type: text/plain, Size: 1198 bytes --]

/* wm_listener.cc: Windows event listener callback interface.

   Copyright 2002 Red Hat, Inc.

   Written by Philip Aston <philipa@mail.com>

This file is part of Cygwin.

This software is a copyrighted work licensed under the terms of the
Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
details. */

#include "winsup.h"
#include "wm_listener.h"

class lock
{
  pthread_mutex_t& m_mutex;

public:
  lock (pthread_mutex_t& mutex)
    : m_mutex(mutex)
  {
    pthread_mutex_lock (&m_mutex);
  }

  ~lock ()
  {
    pthread_mutex_unlock (&m_mutex);
  }
};

wm_listener_list::wm_listener_list ()
  : m_head (0),
    m_mutex ((pthread_mutex_t) PTHREAD_MUTEX_INITIALIZER)
{
}

void
wm_listener_list::add (wm_listener& listener)
{
  /* Can't malloc until cygwin_finished_initializing == 1 */
  /*    if (!cygwin_finished_initializing) {
    system_printf ("Cannot add until cygwin_finished_initializing == 1");
    return;
  }
  */
  lock m(m_mutex);
  m_head = new node(listener, m_head);
}

void
wm_listener_list::wm_event (UINT uMsg, WPARAM wParam, LPARAM lParam)
{
  lock m(m_mutex);

  for (node* p=m_head; p!=0; p=p->m_next) {
    p->m_listener.wm_event (uMsg, wParam, lParam);
  }
}

[-- Attachment #3: wm_listener.h --]
[-- Type: text/plain, Size: 1006 bytes --]

/* wm_listener.h: Windows event listener callback interface.

   Copyright 2002 Red Hat, Inc.

   Written by Philip Aston <philipa@mail.com>

This file is part of Cygwin.

This software is a copyrighted work licensed under the terms of the
Cygwin license.  Please consult the file "CYGWIN_LICENSE" for
details. */

#ifndef __WM_LISTENER_H__
#define __WM_LISTENER_H__

#include <pthread.h>
#include <windef.h>

class wm_listener
{
 protected:
  virtual ~wm_listener() {};

 public:
  virtual void wm_event (UINT uMsg, WPARAM wParam, LPARAM lParam) = 0;
};

/* Non-intrusive, thread-safe list of wm_listeners. */
class wm_listener_list
{
  struct node
  {
    wm_listener& m_listener;
    node* const m_next;

    node (wm_listener& listener, node* next)
      : m_listener (listener), m_next (next) {}
  };

  node* m_head;

  pthread_mutex_t m_mutex;

 public:
  wm_listener_list ();
  void add (wm_listener& l);
  void wm_event (UINT uMsg, WPARAM wParam, LPARAM lParam);
};

#endif /*__WM_LISTENER_H__*/


[-- Attachment #4: Type: text/plain, Size: 214 bytes --]

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: [PATCH] gettimeofday time travels V2

[Philip Aston]

Philip Aston writes:
    * From: "Philip Aston" <paston at bea dot com>
    * To: cygwin at cygwin dot com
    * Date: Sat, 6 Jul 2002 11:55:17 +0100
    * Subject: [PATCH] gettimeofday time travels V2

Hmm... ironically my mailer wasn't running against the patched DLL :-)

- Phil


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: [PATCH] gettimeofday time travels V2

[Christopher Faylor]

On Sat, Jul 06, 2002 at 11:55:17AM +0100, Philip Aston wrote:
>The list in thread.h is specific to a type, and intrusive. I used a
>mutex to create my own thread-safe, non-intrusive list. I used pthread
>mutexes instead of mutos since mutos must be allocated statically.

What is wrong with allocating a muto statically?  You only need one for
this application in cygwin.

>Christopher Faylor writes:
> > It may make sense to just make all of the members of the hires
> > class static since they are just maintaining global state.
>
>Agree. Personally I'd use a singleton object rather than static
>members which would also allow hires to continue to be a wm_listener,
>but it amounts to the same thing. However, I didn't do this as I
>consider it outside of the scope of the fix.

This looks like nice work but I am still concerned about the necessity
of starting up the windows thread and using the windows event loop as I
mentioned in previous email.  I'd rather not have that overhead if it
can be avoided.

For the cygwin part, the patch is also big enough that it requires an
assignment.  Hopefully you've looked at http://cygwin.com/contrib.html
by now...

The w32api part should be posted separately to cygwin-patches so that
Danny will see it and apply it.

cgf

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: [PATCH] gettimeofday time travels V2

[Philip Aston]

Christopher Faylor writes:
 > On Sat, Jul 06, 2002 at 11:55:17AM +0100, Philip Aston wrote:
 > >The list in thread.h is specific to a type, and intrusive. I used a
 > >mutex to create my own thread-safe, non-intrusive list. I used pthread
 > >mutexes instead of mutos since mutos must be allocated statically.
 > 
 > What is wrong with allocating a muto statically?  You only need one for
 > this application in cygwin.

The mutex is part of my generic event listener list. Whilst I allocate
my event listener list statically, others may wish to do otherwise.

 > >Christopher Faylor writes:
 > > > It may make sense to just make all of the members of the hires
 > > > class static since they are just maintaining global state.
 > >
 > >Agree. Personally I'd use a singleton object rather than static
 > >members which would also allow hires to continue to be a wm_listener,
 > >but it amounts to the same thing. However, I didn't do this as I
 > >consider it outside of the scope of the fix.
 > 
 > This looks like nice work but I am still concerned about the necessity
 > of starting up the windows thread and using the windows event loop as I
 > mentioned in previous email.  I'd rather not have that overhead if it
 > can be avoided.

I can't think of anyway to detect the power events other than to run
an event loop.

Note, my implementation will not start the windows thread unless a
hires is used. Unfortunately a hires is used by the strace/printf
methods, so its hard to avoid.

 > For the cygwin part, the patch is also big enough that it requires
 > an assignment. Hopefully you've looked at
 > http://cygwin.com/contrib.html by now...

Yes. I have put the wheels in motion to get a disclaimer from my
employer.

 > The w32api part should be posted separately to cygwin-patches so
 > that Danny will see it and apply it.

Done.


- Phil


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: [PATCH] gettimeofday time travels V2

[Christopher Faylor]

I hate to say this, but after reading various references on the
QueryPerformance* functions, I don't think that we should be using
them.

There was a clamor to implement things this way a few months ago, and I
implemented this to provide improved precision.  However, it looks like
I sacrificed accuracy for precision.  Others have already pointed this
out and after a google/msdn search I am now convinced.

I think the best we can probably do is to use the multimedia timer
functions to get 1ms accuracy and I'll be redoing gettimeofday to use
those in the next couple of days.

I'm *very* sorry for making you jump through hoops.

If someone wants to step forward to convince me that I'm wrong in now
thinking that the QueryPerformance* functions are bad news, then please
feel free.

Otherwise, 1.3.11 will have YA implementation of gettimeofday.

cgf

On Sat, May 18, 2002 at 02:02:25PM +0100, Philip Aston wrote:
>Christopher Faylor writes:
> > On Sat, Jul 06, 2002 at 11:55:17AM +0100, Philip Aston wrote:
> > >The list in thread.h is specific to a type, and intrusive. I used a
> > >mutex to create my own thread-safe, non-intrusive list. I used pthread
> > >mutexes instead of mutos since mutos must be allocated statically.
> > 
> > What is wrong with allocating a muto statically?  You only need one for
> > this application in cygwin.
>
>The mutex is part of my generic event listener list. Whilst I allocate
>my event listener list statically, others may wish to do otherwise.
>
> > >Christopher Faylor writes:
> > > > It may make sense to just make all of the members of the hires
> > > > class static since they are just maintaining global state.
> > >
> > >Agree. Personally I'd use a singleton object rather than static
> > >members which would also allow hires to continue to be a wm_listener,
> > >but it amounts to the same thing. However, I didn't do this as I
> > >consider it outside of the scope of the fix.
> > 
> > This looks like nice work but I am still concerned about the necessity
> > of starting up the windows thread and using the windows event loop as I
> > mentioned in previous email.  I'd rather not have that overhead if it
> > can be avoided.
>
>I can't think of anyway to detect the power events other than to run
>an event loop.
>
>Note, my implementation will not start the windows thread unless a
>hires is used. Unfortunately a hires is used by the strace/printf
>methods, so its hard to avoid.
>
> > For the cygwin part, the patch is also big enough that it requires
> > an assignment. Hopefully you've looked at
> > http://cygwin.com/contrib.html by now...
>
>Yes. I have put the wheels in motion to get a disclaimer from my
>employer.
>
> > The w32api part should be posted separately to cygwin-patches so
> > that Danny will see it and apply it.
>
>Done.
>
>
>- Phil
>
>
>--
>Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
>Bug reporting:         http://cygwin.com/bugs.html
>Documentation:         http://cygwin.com/docs.html
>FAQ:                   http://cygwin.com/faq/

--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/

Re: [PATCH] gettimeofday time travels V2

[Philip Aston]

Christopher Faylor writes:
 > I hate to say this, but after reading various references on the
 > QueryPerformance* functions, I don't think that we should be using
 > them.

I'm tired and probably hysterical, but this makes me smile.

 > There was a clamor to implement things this way a few months ago,
 > and I implemented this to provide improved precision. However, it
 > looks like I sacrificed accuracy for precision. Others have already
 > pointed this out and after a google/msdn search I am now convinced.
 > 
 > I think the best we can probably do is to use the multimedia timer
 > functions to get 1ms accuracy and I'll be redoing gettimeofday to
 > use those in the next couple of days.
 > 
 > I'm *very* sorry for making you jump through hoops.
 > 
 > If someone wants to step forward to convince me that I'm wrong in
 > now thinking that the QueryPerformance* functions are bad news,
 > then please feel free.
 > 
 > Otherwise, 1.3.11 will have YA implementation of gettimeofday.

No worries, it kills me daily - I just want it fixed.

Now if there's any other requirements for windows event listeners,
I've got just the code...

- Phil


--
Unsubscribe info:      http://cygwin.com/ml/#unsubscribe-simple
Bug reporting:         http://cygwin.com/bugs.html
Documentation:         http://cygwin.com/docs.html
FAQ:                   http://cygwin.com/faq/