Re: Broken process startup times after suspend (regression)
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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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(>start_time); > > + bbtime_get_ts(>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 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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 <[EMAIL PROTECTED]> > 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 <[EMAIL PROTECTED]> > Cc: Tomas Smetana <[EMAIL PROTECTED]> > --- > 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(); > + 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,
Re: Broken process startup times after suspend (regression)
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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> Cc: Tomas Smetana <[EMAIL PROTECTED]> --- 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(); + 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,
Re: Broken process startup times after suspend (regression)
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Cc: Tomas Smetana [EMAIL PROTECTED] --- 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
Re: Broken process startup times after suspend (regression)
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 [EMAIL PROTECTED] 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 [EMAIL PROTECTED] Cc: Tomas Smetana [EMAIL PROTECTED] --- 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,
Re: Broken process startup times after suspend (regression)
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: Broken process startup times after suspend (regression)
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(>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(_to_monotonic, > -xtime.tv_sec, -xtime.tv_nsec); > + total_sleep_time = 0; > > write_sequnlock_irqrestore(_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 - To unsubscribe from this list:
Broken process startup times after suspend (regression)
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(>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(_to_monotonic, -xtime.tv_sec, -xtime.tv_nsec); + total_sleep_time = 0; write_sequnlock_irqrestore(_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. --- procps-3.2.7/ps/output.c.jitter 2007-04-26 13:15:47.0 +0200 +++ procps-3.2.7/ps/output.c2007-04-26 13:31:24.0 +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.0 +0100 +++ procps-3.2.7/ps/common.h2007-04-26 12:44:01.0 +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
Broken process startup times after suspend (regression)
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. --- procps-3.2.7/ps/output.c.jitter 2007-04-26 13:15:47.0 +0200 +++ procps-3.2.7/ps/output.c2007-04-26 13:31:24.0 +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.0 +0100 +++ procps-3.2.7/ps/common.h2007-04-26 12:44:01.0 +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
Re: Broken process startup times after suspend (regression)
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 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info