I found i386 timer_resume is updating jiffies, not jiffies_64. It
looks there is a potential overflow problem. Is this a correct fix?
Signed-off-by: Atsushi Nemoto <[email protected]>
diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..e4ed172 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -413,7 +413,7 @@ static int timer_resume(struct sys_devic
xtime.tv_sec = sec;
xtime.tv_nsec = 0;
write_sequnlock_irqrestore(&xtime_lock, flags);
- jiffies += sleep_length;
+ jiffies_64 += sleep_length;
wall_jiffies += sleep_length;
if (last_timer->resume)
last_timer->resume();
On 3/2/06, Atsushi Nemoto <[email protected]> wrote:
> I found i386 timer_resume is updating jiffies, not jiffies_64. It
> looks there is a potential overflow problem. Is this a correct fix?
>
> Signed-off-by: Atsushi Nemoto <[email protected]>
>
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..e4ed172 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -413,7 +413,7 @@ static int timer_resume(struct sys_devic
> xtime.tv_sec = sec;
> xtime.tv_nsec = 0;
> write_sequnlock_irqrestore(&xtime_lock, flags);
> - jiffies += sleep_length;
> + jiffies_64 += sleep_length;
> wall_jiffies += sleep_length;
> if (last_timer->resume)
> last_timer->resume();
The 64-bit jiffies value is not atomic. You need to hold xtime_lock to read it.
>>>>> On Thu, 2 Mar 2006 10:43:12 -0600, "Ram Gupta" <[email protected]> said:
>> I found i386 timer_resume is updating jiffies, not jiffies_64. It
>> looks there is a potential overflow problem. Is this a correct
>> fix?
ram> The 64-bit jiffies value is not atomic. You need to hold
ram> xtime_lock to read it.
OK, and I guess wall_jiffies also needs xtime_lock.
I found i386 timer_resume is updating jiffies, not jiffies_64. It
looks there is a potential overflow problem. And jiffies_64 and
wall_jiffies should be protected by xtime_lock.
Signed-off-by: Atsushi Nemoto <[email protected]>
diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..9d30747 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -412,9 +412,9 @@ static int timer_resume(struct sys_devic
write_seqlock_irqsave(&xtime_lock, flags);
xtime.tv_sec = sec;
xtime.tv_nsec = 0;
- write_sequnlock_irqrestore(&xtime_lock, flags);
- jiffies += sleep_length;
+ jiffies_64 += sleep_length;
wall_jiffies += sleep_length;
+ write_sequnlock_irqrestore(&xtime_lock, flags);
if (last_timer->resume)
last_timer->resume();
cur_timer = last_timer;
Atsushi Nemoto <[email protected]> wrote:
>
> >>>>> On Thu, 2 Mar 2006 10:43:12 -0600, "Ram Gupta" <[email protected]> said:
> >> I found i386 timer_resume is updating jiffies, not jiffies_64. It
> >> looks there is a potential overflow problem. Is this a correct
> >> fix?
>
> ram> The 64-bit jiffies value is not atomic. You need to hold
> ram> xtime_lock to read it.
>
> OK, and I guess wall_jiffies also needs xtime_lock.
>
>
> I found i386 timer_resume is updating jiffies, not jiffies_64. It
> looks there is a potential overflow problem. And jiffies_64 and
> wall_jiffies should be protected by xtime_lock.
>
> Signed-off-by: Atsushi Nemoto <[email protected]>
>
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..9d30747 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -412,9 +412,9 @@ static int timer_resume(struct sys_devic
> write_seqlock_irqsave(&xtime_lock, flags);
> xtime.tv_sec = sec;
> xtime.tv_nsec = 0;
> - write_sequnlock_irqrestore(&xtime_lock, flags);
> - jiffies += sleep_length;
> + jiffies_64 += sleep_length;
> wall_jiffies += sleep_length;
> + write_sequnlock_irqrestore(&xtime_lock, flags);
> if (last_timer->resume)
> last_timer->resume();
> cur_timer = last_timer;
Thanks, that looks like 2.6.16 material.
What happens if the machine slept for more than 49.7 days?
>>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <[email protected]> said:
akpm> Thanks, that looks like 2.6.16 material.
akpm> What happens if the machine slept for more than 49.7 days?
Well, jiffies will lose 49.7 days... Then, how about this? We can
sleep 136 years.
Signed-off-by: Atsushi Nemoto <[email protected]>
diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
index a14d594..be5d079 100644
--- a/arch/i386/kernel/time.c
+++ b/arch/i386/kernel/time.c
@@ -400,7 +400,7 @@ static int timer_resume(struct sys_devic
{
unsigned long flags;
unsigned long sec;
- unsigned long sleep_length;
+ u64 sleep_length;
#ifdef CONFIG_HPET_TIMER
if (is_hpet_enabled())
@@ -408,7 +408,7 @@ static int timer_resume(struct sys_devic
#endif
setup_pit_timer();
sec = get_cmos_time() + clock_cmos_diff;
- sleep_length = (get_cmos_time() - sleep_start) * HZ;
+ sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;
write_seqlock_irqsave(&xtime_lock, flags);
xtime.tv_sec = sec;
xtime.tv_nsec = 0;
Atsushi Nemoto <[email protected]> wrote:
>
> >>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <[email protected]> said:
> akpm> Thanks, that looks like 2.6.16 material.
>
> akpm> What happens if the machine slept for more than 49.7 days?
>
> Well, jiffies will lose 49.7 days... Then, how about this? We can
> sleep 136 years.
>
> Signed-off-by: Atsushi Nemoto <[email protected]>
>
> diff --git a/arch/i386/kernel/time.c b/arch/i386/kernel/time.c
> index a14d594..be5d079 100644
> --- a/arch/i386/kernel/time.c
> +++ b/arch/i386/kernel/time.c
> @@ -400,7 +400,7 @@ static int timer_resume(struct sys_devic
> {
> unsigned long flags;
> unsigned long sec;
> - unsigned long sleep_length;
> + u64 sleep_length;
>
> #ifdef CONFIG_HPET_TIMER
> if (is_hpet_enabled())
> @@ -408,7 +408,7 @@ static int timer_resume(struct sys_devic
> #endif
> setup_pit_timer();
> sec = get_cmos_time() + clock_cmos_diff;
> - sleep_length = (get_cmos_time() - sleep_start) * HZ;
> + sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;
> write_seqlock_irqsave(&xtime_lock, flags);
> xtime.tv_sec = sec;
> xtime.tv_nsec = 0;
but...
wall_jiffies += sleep_length;
wall_jiffies is 32-bit.
>>>>> On Thu, 2 Mar 2006 19:48:41 -0800, Andrew Morton <[email protected]> said:
>> Well, jiffies will lose 49.7 days... Then, how about this? We can
>> sleep 136 years.
akpm> but...
akpm> wall_jiffies += sleep_length;
akpm> wall_jiffies is 32-bit.
Yes ... and I think wall_jiffies can be removed completely (with
update_times() cleanup). Of course it should not be 2.6.16 material.
---
Atsushi Nemoto
On Fri, Mar 03, 2006 at 12:31:10PM +0900, Atsushi Nemoto wrote:
> >>>>> On Thu, 2 Mar 2006 18:45:02 -0800, Andrew Morton <[email protected]> said:
> sec = get_cmos_time() + clock_cmos_diff;
> - sleep_length = (get_cmos_time() - sleep_start) * HZ;
> + sleep_length = (u64)(get_cmos_time() - sleep_start) * HZ;
Calls to get_cmos_time() currently take an average of .5 seconds as it
waits for a clock edge to go by. I've got some patches queueing up to
eliminate this but it's probably worth caching the result here anyway.
--
Mathematics is the supreme nostalgia of our time.