Re: Broken process startup times after suspend (regression)

Re: Broken process startup times after suspend (regression)

[Albert Cahalan]

john stultz writes:

> Indeed. The monotonic clock's behavior around suspend and resume
> is poorly defined. When we increased it, folks didn't like the
> fact that uptime would increase while a system was suspended.

The uptime really does need to increase during suspend. Otherwise,
things get really weird with devices like the OLPC XO which will be
sleeping between keystrokes. You could run the device for hours,
yet get an uptime of only a few minutes. Suspended time should get
counted as stolen time, same as when a hypervisor takes away time.

Re: Broken process startup times after suspend (regression)

[Pavel Machek]

Hi!

> >Indeed. The monotonic clock's behavior around suspend 
> >and resume
> >is poorly defined. When we increased it, folks didn't 
> >like the
> >fact that uptime would increase while a system was 
> >suspended.
> 
> The uptime really does need to increase during suspend. 
> Otherwise,
> things get really weird with devices like the OLPC XO 
> which will be
> sleeping between keystrokes. You could run the device 
> for hours,
> yet get an uptime of only a few minutes. Suspended time 
> should get
> counted as stolen time, same as when a hypervisor takes 
> away time.

Yes, please. Just revert the offending patches, we don't want another
/proc/stat field.
							Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: Broken process startup times after suspend (regression)

[Tomas Janousek]

Hi,

On Fri, May 04, 2007 at 11:18:52AM -0700, john stultz wrote:
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 6af959c..b10d9b7 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -1056,6 +1056,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> >  
> >  	p->lock_depth = -1;		/* -1 = no lock */
> >  	do_posix_clock_monotonic_gettime(&p->start_time);
> > +	bbtime_get_ts(&p->real_start_time);
> >  	p->security = NULL;
> >  	p->io_context = NULL;
> >  	p->io_wait = NULL;
> 
> Since both do_posix_clock_monotonic_gettime and bbtime_get_ts actually
> read the timekeeping hardware (which could cost around 1.5us each), we
> probably will want to optimize this down to a single read, or just avoid
> the hardware read and take lower-granularity xtime value.
> 
> However, I'm not sure how fine-grained the start_time values need to be.

We can add a function like monotonic_to_bb probably? Nothing else crossed my
mind.

> > diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> > index b74860a..cd744a1 100644
> > --- a/kernel/hrtimer.c
> > +++ b/kernel/hrtimer.c
> > @@ -128,6 +128,33 @@ void ktime_get_ts(struct timespec *ts)
> >  }
> >  EXPORT_SYMBOL_GPL(ktime_get_ts);
> >  
> > +/**
> > + * bbtime_get_ts - get the boot based clock in timespec format
> > + * @ts:		pointer to timespec variable
> > + *
> > + * The function calculates the boot based clock from the realtime
> > + * clock, the wall_to_monotonic offset and the total sleep time and
> > + * stores the result in normalized timespec format in the variable
> > + * pointed to by @ts.
> 
> You might want to clarify this as being the "monotonic boot time", or
> the time the system has been running including sleep time and that calls
> to settimeofday() will not affect this value.

I just copied the text from the ktime_get_ts, but yes, why not.

> > + */
> > +void bbtime_get_ts(struct timespec *ts)
> 
> Any reason you added this in hrtimer.c instead of timer.c? I think
> keeping the new functions closer together would make the subtle
> difference between them more clear, and would allow total_sleep_time to
> be static to one file.

The same as above, I copied the ktime_get_ts and put it under that. Will doing
the diff against -mm solve this automagically? :)

> > @@ -832,6 +836,21 @@ void getnstimeofday(struct timespec *ts)
> >  EXPORT_SYMBOL(getnstimeofday);
> >  
> >  /**
> > + * getboottime - Return the real time of system boot.
> > + * @ts: 	pointer to the timespec to be set
> 
> Might want to clarify this as being the "boot time" based on the current
> CLOCK_REALTIME clock, noting that calls to settimeofday() would affect
> this value.

Ok.

> The only other gotcha is that this patch conflicts with a 2.6.22 queued
> patch in -mm that moves the majority of timekeeping code in
> kernel/timer.c to kernel/time/timekeeping.c
> 
> So it might be easiest to re-diff this against -mm and send it to Andrew
> for inclusion along with the cleanups. However, since this is a fix, it
> should have priority over cleanups. I just don't want to upset Andrew's
> tree too much :)
> 
> Your thoughts?

I'll look at that and will see.

Thanks for the comments,
-- 
TJ. (Brno, CZ), BaseOS, Red Hat

Re: Broken process startup times after suspend (regression)

[john stultz]

On Fri, 2007-05-04 at 18:13 +0200, Tomas Janousek wrote:
> Hi,
> 
> On Thu, May 03, 2007 at 12:59:20PM -0700, john stultz wrote:
> > Indeed. The monotonic clock's behavior around suspend and resume is
> > poorly defined. When we increased it, folks didn't like the fact that
> > uptime would increase while a system was suspended. 
> 
> Seems like your change also made accounting and OOM calculations behave more
> correctly. And my previous patch broke that completely, because they would use
> "your" uptime and "my" process start time, resulting in negative numbers when
> calculating the process uptime. Therefore, I had to create another start_time
> in the task_struct.
> 
> > > I was thinking about a solution and I am posting the least intrusive one I
> > > found. I add a total_sleep_time variable which is to be added to
> > > wall_to_monotonic when one wants the proper boot time. I do by no means say
> > > it's the best one, I expect comments.
> > 
> > While I'd prefer wrapping it in a interface and keeping it in
> > timekeeping (rather then exporting a collection of values that should be
> > combined in varying ways), I think this approach is probably the best.
> > Rather then forcing one behavior or the other we can provide both smooth
> > monotonic and monotonic w/ sleep.
> 
> Ok, thanks for the comment, I did so.

Looks great! Just a few minor comments.


> And here's new patch:
> 
> 
> From: Tomas Janousek <tjanouse@redhat.com>
> Subject: [PATCH] Fix boot time and process startup times after suspend
> 
> The commits
>   411187fb05cd11676b0979d9fbf3291db69dbce2 (GTOD: persistent clock support)
>   c1d370e167d66b10bca3b602d3740405469383de (i386: use GTOD persistent clock
>     support)
> changed the monotonic time so that it no longer jumps after resume, but it's
> not possible to use it for boot time and process start time calculations then.
> 
> I add a getboottime function to get the real boot time and a boot-based time
> concept. Because we need a process start time corresponding to the monotonic
> time, I add real_start_time to the task_struct which is then used in
> /proc/pid/stat. The /proc/stat's btime uses getboottime instead of the
> monotonic time offset as well.
> 
> Signed-off-by: Tomas Janousek <tjanouse@redhat.com>
> Cc: Tomas Smetana <tsmetana@redhat.com>
> ---
>  fs/proc/array.c       |    5 +++--
>  fs/proc/proc_misc.c   |    8 +++++---
>  include/linux/ktime.h |    2 ++
>  include/linux/sched.h |    2 +-
>  include/linux/time.h  |    2 ++
>  kernel/fork.c         |    1 +
>  kernel/hrtimer.c      |   27 +++++++++++++++++++++++++++
>  kernel/timer.c        |   21 +++++++++++++++++++++
>  8 files changed, 62 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/proc/array.c b/fs/proc/array.c
> index 07c9cdb..831734c 100644
> --- a/fs/proc/array.c
> +++ b/fs/proc/array.c
> @@ -405,8 +405,9 @@ static int do_task_stat(struct task_struct *task, char * buffer, int whole)
>  
>  	/* Temporary variable needed for gcc-2.96 */
>  	/* convert timespec -> nsec*/
> -	start_time = (unsigned long long)task->start_time.tv_sec * NSEC_PER_SEC
> -				+ task->start_time.tv_nsec;
> +	start_time =
> +		(unsigned long long)task->real_start_time.tv_sec * NSEC_PER_SEC
> +				+ task->real_start_time.tv_nsec;
>  	/* convert nsec -> ticks */
>  	start_time = nsec_to_clock_t(start_time);
>  
> diff --git a/fs/proc/proc_misc.c b/fs/proc/proc_misc.c
> index e2c4c0a..b1791a0 100644
> --- a/fs/proc/proc_misc.c
> +++ b/fs/proc/proc_misc.c
> @@ -453,12 +453,14 @@ static int show_stat(struct seq_file *p, void *v)
>  	unsigned long jif;
>  	cputime64_t user, nice, system, idle, iowait, irq, softirq, steal;
>  	u64 sum = 0;
> +	struct timespec boottime;
>  
>  	user = nice = system = idle = iowait =
>  		irq = softirq = steal = cputime64_zero;
> -	jif = - wall_to_monotonic.tv_sec;
> -	if (wall_to_monotonic.tv_nsec)
> -		--jif;
> +	getboottime(&boottime);
> +	jif = boottime.tv_sec;
> +	if (boottime.tv_nsec)
> +		++jif;
>  
>  	for_each_possible_cpu(i) {
>  		int j;
> diff --git a/include/linux/ktime.h b/include/linux/ktime.h
> index 248305b..346c1e1 100644
> --- a/include/linux/ktime.h
> +++ b/include/linux/ktime.h
> @@ -269,6 +269,8 @@ static inline s64 ktime_to_ns(const ktime_t kt)
>  
>  /* Get the monotonic time in timespec format: */
>  extern void ktime_get_ts(struct timespec *ts);
> +/* Get the boot based time in timespec format: */
> +extern void bbtime_get_ts(struct timespec *ts);
>  
>  /* Get the real (wall-) time in timespec format: */
>  #define ktime_get_real_ts(ts)	getnstimeofday(ts)
> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 49fe299..0845bde 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -884,7 +884,7 @@ struct task_struct {
>  	unsigned long rt_priority;
>  	cputime_t utime, stime;
>  	unsigned long nvcsw, nivcsw; /* context switch counts */
> -	struct timespec start_time;
> +	struct timespec start_time, real_start_time;
>  /* mm fault and swap info: this can arguably be seen as either mm-specific or thread-specific */
>  	unsigned long min_flt, maj_flt;
>  
> diff --git a/include/linux/time.h b/include/linux/time.h
> index 8ea8dea..a52f481 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -90,6 +90,7 @@ static inline struct timespec timespec_sub(struct timespec lhs,
>  
>  extern struct timespec xtime;
>  extern struct timespec wall_to_monotonic;
> +extern unsigned long total_sleep_time;
>  extern seqlock_t xtime_lock __attribute__((weak));
>  
>  extern unsigned long read_persistent_clock(void);
> @@ -116,6 +117,7 @@ extern int do_setitimer(int which, struct itimerval *value,
>  extern unsigned int alarm_setitimer(unsigned int seconds);
>  extern int do_getitimer(int which, struct itimerval *value);
>  extern void getnstimeofday(struct timespec *tv);
> +extern void getboottime(struct timespec *ts);
>  
>  extern struct timespec timespec_trunc(struct timespec t, unsigned gran);
>  extern int timekeeping_is_continuous(void);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6af959c..b10d9b7 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1056,6 +1056,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
>  
>  	p->lock_depth = -1;		/* -1 = no lock */
>  	do_posix_clock_monotonic_gettime(&p->start_time);
> +	bbtime_get_ts(&p->real_start_time);
>  	p->security = NULL;
>  	p->io_context = NULL;
>  	p->io_wait = NULL;

Since both do_posix_clock_monotonic_gettime and bbtime_get_ts actually
read the timekeeping hardware (which could cost around 1.5us each), we
probably will want to optimize this down to a single read, or just avoid
the hardware read and take lower-granularity xtime value.

However, I'm not sure how fine-grained the start_time values need to be.

> diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
> index b74860a..cd744a1 100644
> --- a/kernel/hrtimer.c
> +++ b/kernel/hrtimer.c
> @@ -128,6 +128,33 @@ void ktime_get_ts(struct timespec *ts)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_ts);
>  
> +/**
> + * bbtime_get_ts - get the boot based clock in timespec format
> + * @ts:		pointer to timespec variable
> + *
> + * The function calculates the boot based clock from the realtime
> + * clock, the wall_to_monotonic offset and the total sleep time and
> + * stores the result in normalized timespec format in the variable
> + * pointed to by @ts.

You might want to clarify this as being the "monotonic boot time", or
the time the system has been running including sleep time and that calls
to settimeofday() will not affect this value.

> + */
> +void bbtime_get_ts(struct timespec *ts)

Any reason you added this in hrtimer.c instead of timer.c? I think
keeping the new functions closer together would make the subtle
difference between them more clear, and would allow total_sleep_time to
be static to one file.

> +{
> +	struct timespec tobb;
> +	unsigned long seq;
> +
> +	do {
> +		seq = read_seqbegin(&xtime_lock);
> +		getnstimeofday(ts);
> +		tobb = wall_to_monotonic;
> +		tobb.tv_sec += total_sleep_time;
> +
> +	} while (read_seqretry(&xtime_lock, seq));
> +
> +	set_normalized_timespec(ts, ts->tv_sec + tobb.tv_sec,
> +				ts->tv_nsec + tobb.tv_nsec);
> +}
> +EXPORT_SYMBOL_GPL(bbtime_get_ts);
> +
>  /*
>   * Get the coarse grained time at the softirq based on xtime and
>   * wall_to_monotonic.
> diff --git a/kernel/timer.c b/kernel/timer.c
> index dd6c2c1..a1a8ccb 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -759,9 +759,13 @@ unsigned long next_timer_interrupt(void)
>   * at zero at system boot time, so wall_to_monotonic will be negative,
>   * however, we will ALWAYS keep the tv_nsec part positive so we can use
>   * the usual normalization.
> + *
> + * We need to add total_sleep_time to wall_to_monotonic to get the real boot
> + * time.
>   */
>  struct timespec xtime __attribute__ ((aligned (16)));
>  struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
> +unsigned long total_sleep_time;
>  
>  EXPORT_SYMBOL(xtime);
>  
> @@ -832,6 +836,21 @@ void getnstimeofday(struct timespec *ts)
>  EXPORT_SYMBOL(getnstimeofday);
>  
>  /**
> + * getboottime - Return the real time of system boot.
> + * @ts: 	pointer to the timespec to be set

Might want to clarify this as being the "boot time" based on the current
CLOCK_REALTIME clock, noting that calls to settimeofday() would affect
this value.

> + * Returns the time of day in a timespec.
> + */
> +void getboottime(struct timespec *ts)
> +{
> +	set_normalized_timespec(ts,
> +		- (wall_to_monotonic.tv_sec + total_sleep_time),
> +		- wall_to_monotonic.tv_nsec);
> +}
> +
> +EXPORT_SYMBOL(getboottime);
> +
> +/**
>   * do_gettimeofday - Returns the time of day in a timeval
>   * @tv:		pointer to the timeval to be set
>   *
> @@ -975,6 +994,7 @@ void __init timekeeping_init(void)
>  	xtime.tv_nsec = 0;
>  	set_normalized_timespec(&wall_to_monotonic,
>  		-xtime.tv_sec, -xtime.tv_nsec);
> +	total_sleep_time = 0;
>  
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  }
> @@ -1004,6 +1024,7 @@ static int timekeeping_resume(struct sys_device *dev)
>  
>  		xtime.tv_sec += sleep_length;
>  		wall_to_monotonic.tv_sec -= sleep_length;
> +		total_sleep_time += sleep_length;
>  	}
>  	/* re-base the last cycle value */
>  	clock->cycle_last = clocksource_read(clock);


The only other gotcha is that this patch conflicts with a 2.6.22 queued
patch in -mm that moves the majority of timekeeping code in
kernel/timer.c to kernel/time/timekeeping.c

So it might be easiest to re-diff this against -mm and send it to Andrew
for inclusion along with the cleanups. However, since this is a fix, it
should have priority over cleanups. I just don't want to upset Andrew's
tree too much :)

Your thoughts?
-john

Re: Broken process startup times after suspend (regression)

[Tomas Janousek]

Hi,

On Thu, May 03, 2007 at 12:59:20PM -0700, john stultz wrote:
> Indeed. The monotonic clock's behavior around suspend and resume is
> poorly defined. When we increased it, folks didn't like the fact that
> uptime would increase while a system was suspended. 

Seems like your change also made accounting and OOM calculations behave more
correctly. And my previous patch broke that completely, because they would use
"your" uptime and "my" process start time, resulting in negative numbers when
calculating the process uptime. Therefore, I had to create another start_time
in the task_struct.

> > I was thinking about a solution and I am posting the least intrusive one I
> > found. I add a total_sleep_time variable which is to be added to
> > wall_to_monotonic when one wants the proper boot time. I do by no means say
> > it's the best one, I expect comments.
> 
> While I'd prefer wrapping it in a interface and keeping it in
> timekeeping (rather then exporting a collection of values that should be
> combined in varying ways), I think this approach is probably the best.
> Rather then forcing one behavior or the other we can provide both smooth
> monotonic and monotonic w/ sleep.

Ok, thanks for the comment, I did so.

And here's new patch:


From: Tomas Janousek <tjanouse@redhat.com>
Subject: [PATCH] Fix boot time and process startup times after suspend

The commits
  411187fb05cd11676b0979d9fbf3291db69dbce2 (GTOD: persistent clock support)
  c1d370e167d66b10bca3b602d3740405469383de (i386: use GTOD persistent clock
    support)
changed the monotonic time so that it no longer jumps after resume, but it's
not possible to use it for boot time and process start time calculations then.

I add a getboottime function to get the real boot time and a boot-based time
concept. Because we need a process start time corresponding to the monotonic
time, I add real_start_time to the task_struct which is then used in
/proc/pid/stat. The /proc/stat's btime uses getboottime instead of the
monotonic time offset as well.

Signed-off-by: Tomas Janousek <tjanouse@redhat.com>
Cc: Tomas Smetana <tsmetana@redhat.com>
---
 fs/proc/array.c       |    5 +++--
 fs/proc/proc_misc.c   |    8 +++++---
 include/linux/ktime.h |    2 ++
 include/linux/sched.h |    2 +-
 include/linux/time.h  |    2 ++
 kernel/fork.c         |    1 +
 kernel/hrtimer.c      |   27 +++++++++++++++++++++++++++
 kernel/timer.c        |   21 +++++++++++++++++++++
 8 files changed, 62 insertions(+), 6 deletions(-)

diff --git a/fs/proc/array.c b/fs/proc/array.c
index 07c9cdb..831734c 100644
--- a/fs/proc/array.c
+++ b/fs/proc/array.c
@@ -405,8 +405,9 @@ static int do_task_stat(struct task_struct *task, char * buffer, int whole)
 
 	/* Temporary variable needed for gcc-2.96 */
 	/* convert timespec -> nsec*/
-	start_time = (unsigned long long)task->start_time.tv_sec * NSEC_PER_SEC
-				+ task->start_time.tv_nsec;
+	start_time =
+		(unsigned long long)task->real_start_time.tv_sec * NSEC_PER_SEC
+				+ task->real_start_time.tv_nsec;
 	/* convert nsec -> ticks */
 	start_time = nsec_to_clock_t(start_time);
 
diff --git a/fs/proc/proc_misc.c b/fs/proc/proc_misc.c
index e2c4c0a..b1791a0 100644
--- a/fs/proc/proc_misc.c
+++ b/fs/proc/proc_misc.c
@@ -453,12 +453,14 @@ static int show_stat(struct seq_file *p, void *v)
 	unsigned long jif;
 	cputime64_t user, nice, system, idle, iowait, irq, softirq, steal;
 	u64 sum = 0;
+	struct timespec boottime;
 
 	user = nice = system = idle = iowait =
 		irq = softirq = steal = cputime64_zero;
-	jif = - wall_to_monotonic.tv_sec;
-	if (wall_to_monotonic.tv_nsec)
-		--jif;
+	getboottime(&boottime);
+	jif = boottime.tv_sec;
+	if (boottime.tv_nsec)
+		++jif;
 
 	for_each_possible_cpu(i) {
 		int j;
diff --git a/include/linux/ktime.h b/include/linux/ktime.h
index 248305b..346c1e1 100644
--- a/include/linux/ktime.h
+++ b/include/linux/ktime.h
@@ -269,6 +269,8 @@ static inline s64 ktime_to_ns(const ktime_t kt)
 
 /* Get the monotonic time in timespec format: */
 extern void ktime_get_ts(struct timespec *ts);
+/* Get the boot based time in timespec format: */
+extern void bbtime_get_ts(struct timespec *ts);
 
 /* Get the real (wall-) time in timespec format: */
 #define ktime_get_real_ts(ts)	getnstimeofday(ts)
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 49fe299..0845bde 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -884,7 +884,7 @@ struct task_struct {
 	unsigned long rt_priority;
 	cputime_t utime, stime;
 	unsigned long nvcsw, nivcsw; /* context switch counts */
-	struct timespec start_time;
+	struct timespec start_time, real_start_time;
 /* mm fault and swap info: this can arguably be seen as either mm-specific or thread-specific */
 	unsigned long min_flt, maj_flt;
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 8ea8dea..a52f481 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -90,6 +90,7 @@ static inline struct timespec timespec_sub(struct timespec lhs,
 
 extern struct timespec xtime;
 extern struct timespec wall_to_monotonic;
+extern unsigned long total_sleep_time;
 extern seqlock_t xtime_lock __attribute__((weak));
 
 extern unsigned long read_persistent_clock(void);
@@ -116,6 +117,7 @@ extern int do_setitimer(int which, struct itimerval *value,
 extern unsigned int alarm_setitimer(unsigned int seconds);
 extern int do_getitimer(int which, struct itimerval *value);
 extern void getnstimeofday(struct timespec *tv);
+extern void getboottime(struct timespec *ts);
 
 extern struct timespec timespec_trunc(struct timespec t, unsigned gran);
 extern int timekeeping_is_continuous(void);
diff --git a/kernel/fork.c b/kernel/fork.c
index 6af959c..b10d9b7 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1056,6 +1056,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 
 	p->lock_depth = -1;		/* -1 = no lock */
 	do_posix_clock_monotonic_gettime(&p->start_time);
+	bbtime_get_ts(&p->real_start_time);
 	p->security = NULL;
 	p->io_context = NULL;
 	p->io_wait = NULL;
diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c
index b74860a..cd744a1 100644
--- a/kernel/hrtimer.c
+++ b/kernel/hrtimer.c
@@ -128,6 +128,33 @@ void ktime_get_ts(struct timespec *ts)
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts);
 
+/**
+ * bbtime_get_ts - get the boot based clock in timespec format
+ * @ts:		pointer to timespec variable
+ *
+ * The function calculates the boot based clock from the realtime
+ * clock, the wall_to_monotonic offset and the total sleep time and
+ * stores the result in normalized timespec format in the variable
+ * pointed to by @ts.
+ */
+void bbtime_get_ts(struct timespec *ts)
+{
+	struct timespec tobb;
+	unsigned long seq;
+
+	do {
+		seq = read_seqbegin(&xtime_lock);
+		getnstimeofday(ts);
+		tobb = wall_to_monotonic;
+		tobb.tv_sec += total_sleep_time;
+
+	} while (read_seqretry(&xtime_lock, seq));
+
+	set_normalized_timespec(ts, ts->tv_sec + tobb.tv_sec,
+				ts->tv_nsec + tobb.tv_nsec);
+}
+EXPORT_SYMBOL_GPL(bbtime_get_ts);
+
 /*
  * Get the coarse grained time at the softirq based on xtime and
  * wall_to_monotonic.
diff --git a/kernel/timer.c b/kernel/timer.c
index dd6c2c1..a1a8ccb 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -759,9 +759,13 @@ unsigned long next_timer_interrupt(void)
  * at zero at system boot time, so wall_to_monotonic will be negative,
  * however, we will ALWAYS keep the tv_nsec part positive so we can use
  * the usual normalization.
+ *
+ * We need to add total_sleep_time to wall_to_monotonic to get the real boot
+ * time.
  */
 struct timespec xtime __attribute__ ((aligned (16)));
 struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
+unsigned long total_sleep_time;
 
 EXPORT_SYMBOL(xtime);
 
@@ -832,6 +836,21 @@ void getnstimeofday(struct timespec *ts)
 EXPORT_SYMBOL(getnstimeofday);
 
 /**
+ * getboottime - Return the real time of system boot.
+ * @ts: 	pointer to the timespec to be set
+ *
+ * Returns the time of day in a timespec.
+ */
+void getboottime(struct timespec *ts)
+{
+	set_normalized_timespec(ts,
+		- (wall_to_monotonic.tv_sec + total_sleep_time),
+		- wall_to_monotonic.tv_nsec);
+}
+
+EXPORT_SYMBOL(getboottime);
+
+/**
  * do_gettimeofday - Returns the time of day in a timeval
  * @tv:		pointer to the timeval to be set
  *
@@ -975,6 +994,7 @@ void __init timekeeping_init(void)
 	xtime.tv_nsec = 0;
 	set_normalized_timespec(&wall_to_monotonic,
 		-xtime.tv_sec, -xtime.tv_nsec);
+	total_sleep_time = 0;
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
@@ -1004,6 +1024,7 @@ static int timekeeping_resume(struct sys_device *dev)
 
 		xtime.tv_sec += sleep_length;
 		wall_to_monotonic.tv_sec -= sleep_length;
+		total_sleep_time += sleep_length;
 	}
 	/* re-base the last cycle value */
 	clock->cycle_last = clocksource_read(clock);

-- 
TJ. (Brno, CZ), BaseOS, Red Hat

Re: Broken process startup times after suspend (regression)

[john stultz]

On Thu, 2007-05-03 at 17:03 +0200, Tomas Janousek wrote:
> Hi,
> 
> the commits
>   411187fb05cd11676b0979d9fbf3291db69dbce2 (GTOD: persistent clock support)
>   c1d370e167d66b10bca3b602d3740405469383de (i386: use GTOD persistent clock
>     support)
> caused a change in the way uptime is measured and introduced a regression such
> that it's no longer possible to obtain a correct process start time or the
> system boot time.
> 
> These two commits make the monotonic time not jump by hudreds of seconds after
> the kernel resumes from suspend, but they achieve it by moving the
> wall_to_monotonic offset, which is used for all boot time / uptime
> calculations. It's not possible to read the real boot time / process start
> time then.

Indeed. The monotonic clock's behavior around suspend and resume is
poorly defined. When we increased it, folks didn't like the fact that
uptime would increase while a system was suspended. 

> I was thinking about a solution and I am posting the least intrusive one I
> found. I add a total_sleep_time variable which is to be added to
> wall_to_monotonic when one wants the proper boot time. I do by no means say
> it's the best one, I expect comments.

While I'd prefer wrapping it in a interface and keeping it in
timekeeping (rather then exporting a collection of values that should be
combined in varying ways), I think this approach is probably the best.
Rather then forcing one behavior or the other we can provide both smooth
monotonic and monotonic w/ sleep.

> This requires a patch to procps that makes it use /proc/stat's btime instead
> of "now - uptime". This was written by Tomas Smetana and I'm attaching it as
> well. (It was posted upstream as it fixes another bug in ps as well.) Or we
> could just make the /proc/uptime contain "now - btime" like it used to before
> those two commits.
> 
> ---
>  fs/proc/proc_misc.c  |    2 +-
>  include/linux/time.h |    1 +
>  kernel/fork.c        |    1 +
>  kernel/timer.c       |    6 ++++++
>  4 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/proc/proc_misc.c b/fs/proc/proc_misc.c
> index e2c4c0a..a29309e 100644
> --- a/fs/proc/proc_misc.c
> +++ b/fs/proc/proc_misc.c
> @@ -456,7 +456,7 @@ static int show_stat(struct seq_file *p, void *v)
> 
>  	user = nice = system = idle = iowait =
>  		irq = softirq = steal = cputime64_zero;
> -	jif = - wall_to_monotonic.tv_sec;
> +	jif = - (wall_to_monotonic.tv_sec + total_sleep_time);
>  	if (wall_to_monotonic.tv_nsec)
>  		--jif;

As I said, could we wrap this in a function? 

Maybe something like get_monotonicwithsleep_time() (feel free to come up
with a better name :).

> diff --git a/include/linux/time.h b/include/linux/time.h
> index 8ea8dea..cb87616 100644
> --- a/include/linux/time.h
> +++ b/include/linux/time.h
> @@ -90,6 +90,7 @@ static inline struct timespec timespec_sub(struct timespec lhs,
> 
>  extern struct timespec xtime;
>  extern struct timespec wall_to_monotonic;
> +extern unsigned long total_sleep_time;
>  extern seqlock_t xtime_lock __attribute__((weak));
> 
>  extern unsigned long read_persistent_clock(void);
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 6af959c..c051f17 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -1056,6 +1056,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
> 
>  	p->lock_depth = -1;		/* -1 = no lock */
>  	do_posix_clock_monotonic_gettime(&p->start_time);
> +	p->start_time.tv_sec += total_sleep_time;
>  	p->security = NULL;
>  	p->io_context = NULL;
>  	p->io_wait = NULL;
> diff --git a/kernel/timer.c b/kernel/timer.c
> index dd6c2c1..13eb201 100644
> --- a/kernel/timer.c
> +++ b/kernel/timer.c
> @@ -759,9 +759,13 @@ unsigned long next_timer_interrupt(void)
>   * at zero at system boot time, so wall_to_monotonic will be negative,
>   * however, we will ALWAYS keep the tv_nsec part positive so we can use
>   * the usual normalization.
> + *
> + * One needs to add total_sleep_time to wall_to_monotonic to get the real boot
> + * time.
>   */
>  struct timespec xtime __attribute__ ((aligned (16)));
>  struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
> +unsigned long total_sleep_time;
> 
>  EXPORT_SYMBOL(xtime);
> 
> @@ -975,6 +979,7 @@ void __init timekeeping_init(void)
>  	xtime.tv_nsec = 0;
>  	set_normalized_timespec(&wall_to_monotonic,
>  		-xtime.tv_sec, -xtime.tv_nsec);
> +	total_sleep_time = 0;
> 
>  	write_sequnlock_irqrestore(&xtime_lock, flags);
>  }
> @@ -1004,6 +1009,7 @@ static int timekeeping_resume(struct sys_device *dev)
> 
>  		xtime.tv_sec += sleep_length;
>  		wall_to_monotonic.tv_sec -= sleep_length;
> +		total_sleep_time += sleep_length;
>  	}
>  	/* re-base the last cycle value */
>  	clock->cycle_last = clocksource_read(clock);

Otherwise it looks good.

Thanks for bringing this up!
-john

Broken process startup times after suspend (regression)

[Tomas Janousek]

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

Hi,

the commits
  411187fb05cd11676b0979d9fbf3291db69dbce2 (GTOD: persistent clock support)
  c1d370e167d66b10bca3b602d3740405469383de (i386: use GTOD persistent clock
    support)
caused a change in the way uptime is measured and introduced a regression such
that it's no longer possible to obtain a correct process start time or the
system boot time.

These two commits make the monotonic time not jump by hudreds of seconds after
the kernel resumes from suspend, but they achieve it by moving the
wall_to_monotonic offset, which is used for all boot time / uptime
calculations. It's not possible to read the real boot time / process start
time then.

I was thinking about a solution and I am posting the least intrusive one I
found. I add a total_sleep_time variable which is to be added to
wall_to_monotonic when one wants the proper boot time. I do by no means say
it's the best one, I expect comments.

This requires a patch to procps that makes it use /proc/stat's btime instead
of "now - uptime". This was written by Tomas Smetana and I'm attaching it as
well. (It was posted upstream as it fixes another bug in ps as well.) Or we
could just make the /proc/uptime contain "now - btime" like it used to before
those two commits.

---
 fs/proc/proc_misc.c  |    2 +-
 include/linux/time.h |    1 +
 kernel/fork.c        |    1 +
 kernel/timer.c       |    6 ++++++
 4 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/fs/proc/proc_misc.c b/fs/proc/proc_misc.c
index e2c4c0a..a29309e 100644
--- a/fs/proc/proc_misc.c
+++ b/fs/proc/proc_misc.c
@@ -456,7 +456,7 @@ static int show_stat(struct seq_file *p, void *v)
 
 	user = nice = system = idle = iowait =
 		irq = softirq = steal = cputime64_zero;
-	jif = - wall_to_monotonic.tv_sec;
+	jif = - (wall_to_monotonic.tv_sec + total_sleep_time);
 	if (wall_to_monotonic.tv_nsec)
 		--jif;
 
diff --git a/include/linux/time.h b/include/linux/time.h
index 8ea8dea..cb87616 100644
--- a/include/linux/time.h
+++ b/include/linux/time.h
@@ -90,6 +90,7 @@ static inline struct timespec timespec_sub(struct timespec lhs,
 
 extern struct timespec xtime;
 extern struct timespec wall_to_monotonic;
+extern unsigned long total_sleep_time;
 extern seqlock_t xtime_lock __attribute__((weak));
 
 extern unsigned long read_persistent_clock(void);
diff --git a/kernel/fork.c b/kernel/fork.c
index 6af959c..c051f17 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1056,6 +1056,7 @@ static struct task_struct *copy_process(unsigned long clone_flags,
 
 	p->lock_depth = -1;		/* -1 = no lock */
 	do_posix_clock_monotonic_gettime(&p->start_time);
+	p->start_time.tv_sec += total_sleep_time;
 	p->security = NULL;
 	p->io_context = NULL;
 	p->io_wait = NULL;
diff --git a/kernel/timer.c b/kernel/timer.c
index dd6c2c1..13eb201 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -759,9 +759,13 @@ unsigned long next_timer_interrupt(void)
  * at zero at system boot time, so wall_to_monotonic will be negative,
  * however, we will ALWAYS keep the tv_nsec part positive so we can use
  * the usual normalization.
+ *
+ * One needs to add total_sleep_time to wall_to_monotonic to get the real boot
+ * time.
  */
 struct timespec xtime __attribute__ ((aligned (16)));
 struct timespec wall_to_monotonic __attribute__ ((aligned (16)));
+unsigned long total_sleep_time;
 
 EXPORT_SYMBOL(xtime);
 
@@ -975,6 +979,7 @@ void __init timekeeping_init(void)
 	xtime.tv_nsec = 0;
 	set_normalized_timespec(&wall_to_monotonic,
 		-xtime.tv_sec, -xtime.tv_nsec);
+	total_sleep_time = 0;
 
 	write_sequnlock_irqrestore(&xtime_lock, flags);
 }
@@ -1004,6 +1009,7 @@ static int timekeeping_resume(struct sys_device *dev)
 
 		xtime.tv_sec += sleep_length;
 		wall_to_monotonic.tv_sec -= sleep_length;
+		total_sleep_time += sleep_length;
 	}
 	/* re-base the last cycle value */
 	clock->cycle_last = clocksource_read(clock);

Regards,
-- 
Tomas Janousek, SW Engineer, Red Hat, Inc.

[-- Attachment #2: procps-3.2.7-ps-stime.patch --]
[-- Type: text/plain, Size: 2279 bytes --]

--- procps-3.2.7/ps/output.c.jitter	2007-04-26 13:15:47.000000000 +0200
+++ procps-3.2.7/ps/output.c	2007-04-26 13:31:24.000000000 +0200
@@ -77,7 +77,6 @@
 static int wide_signals;  /* true if we have room */
 
 static unsigned long seconds_since_1970;
-static unsigned long time_of_boot;
 static unsigned long page_shift;
 
 
@@ -1952,7 +1951,6 @@
   // available space:  page_size*outbuf_pages-SPACE_AMOUNT
 
   seconds_since_1970 = time(NULL);
-  time_of_boot = seconds_since_1970 - seconds_since_boot;
 
   meminfo();
 
--- procps-3.2.7/ps/common.h.jitter	2005-01-27 04:43:22.000000000 +0100
+++ procps-3.2.7/ps/common.h	2007-04-26 12:44:01.000000000 +0200
@@ -302,6 +302,7 @@
 extern int             screen_cols;
 extern int             screen_rows;
 extern unsigned long   seconds_since_boot;
+extern unsigned long   time_of_boot;
 extern selection_node *selection_list;
 extern unsigned        simple_select;
 extern sort_node      *sort_list;
--- procps-3.2.7/ps/global.c.jitter	2005-10-30 01:43:34.000000000 +0200
+++ procps-3.2.7/ps/global.c	2007-04-26 13:26:38.000000000 +0200
@@ -70,6 +70,7 @@
 int             screen_cols = -1;
 int             screen_rows = -1;
 unsigned long   seconds_since_boot = -1;
+unsigned long   time_of_boot = -1;
 selection_node *selection_list = (selection_node *)0xdeadbeef;
 unsigned        simple_select = 0xffffffff;
 sort_node      *sort_list = (sort_node *)0xdeadbeef; /* ready-to-use sort list */
@@ -361,7 +362,23 @@
   look_up_our_self(&p);
   set_screen_size();
   set_personality();
-  
+  int fd;
+  char buf[BUFFSIZE];
+  const char *b;
+
+  /* get boot time from /proc/stat */
+  fd = open("/proc/stat", O_RDONLY, 0);
+  if (fd != -1) {
+    buf[BUFFSIZE-1] = 0;
+    read(fd, buf, BUFFSIZE-1);
+    b = strstr(buf, "btime ");
+    if (b) {
+      sscanf(b, "btime %lu", &time_of_boot);
+      seconds_since_boot = time(0) - time_of_boot;
+    }  
+    close(fd);
+  }
+    
   all_processes         = 0;
   bsd_c_option          = 0;
   bsd_e_option          = 0;
@@ -380,7 +397,6 @@
   negate_selection      = 0;
   page_size             = getpagesize();
   running_only          = 0;
-  seconds_since_boot    = uptime(0,0);
   selection_list        = NULL;
   simple_select         = 0;
   sort_list             = NULL;