time: Fix truncation in jiffies_to_usecs()

c) If not this ... then what? Separate routine to convert large numbers of jiffies to usec/nsecs? Should we make the existing one barf when handed a number that overflows?

Quoted 3 lines · Tony Luck

c) If not this ... then what? Separate routine to convert large numbers of jiffies to usec/nsecs? Should we make the existing one barf when handed a number that overflows?

Having thought about this a bit more - I'm leaning towards leaving jiffies_to_usecs() alone, but using it as a model for a from-scratch implementation of:
u64 jiffies_to_nsecs(const unsigned long j)
{
}

This is what the uptime tracer actually needs - and there is only one user of jiffies_to_nsecs() to worry about.

Quoted 3 lines · Tony Luck

c) If not this ... then what? Separate routine to convert large numbers of jiffies to usec/nsecs? Should we make the existing one barf when handed a number that overflows?

Having thought about this a bit more - I'm leaning towards leaving jiffies_to_usecs() alone, but using it as a model for a from-scratch implementation of:
u64 jiffies_to_nsecs(const unsigned long j)
{
}

This is what the uptime tracer actually needs - and there is only one user of jiffies_to_nsecs() to worry about.

Quoted 13 lines · Tony Luck

Quoted 3 lines · Tony Luck

c) If not this ... then what? Separate routine to convert large numbers of jiffies to usec/nsecs? Should we make the existing one barf when handed a number that overflows?

Having thought about this a bit more - I'm leaning towards leaving jiffies_to_usecs() alone, but using it as a model for a from-scratch implementation of:
u64 jiffies_to_nsecs(const unsigned long j)
{
}

Quoted 2 lines

This is what the uptime tracer actually needs - and there is only one user of jiffies_to_nsecs() to worry about.

I'm not sure I get what you're trying to do. We already have jiffies_to_nsecs(). Anyway I'll just wait and check out the next patch :)

Quoted 14 lines · On Tue, 8 Apr 2014 19:49:51 +0200 Frederic Weisbecker <[email protected]>…

Quoted 13 lines · Tony Luck

Quoted 3 lines · Tony Luck

c) If not this ... then what? Separate routine to convert large numbers of jiffies to usec/nsecs? Should we make the existing one barf when handed a number that overflows?

Having thought about this a bit more - I'm leaning towards leaving jiffies_to_usecs() alone, but using it as a model for a from-scratch implementation of:
u64 jiffies_to_nsecs(const unsigned long j)
{
}

Quoted 5 lines

Quoted 2 lines

This is what the uptime tracer actually needs - and there is only one user of jiffies_to_nsecs() to worry about.

I'm not sure I get what you're trying to do. We already have jiffies_to_nsecs(). Anyway I'll just wait and check out the next patch :)

I believe the issue is the way it's implemented:

static inline u64 jiffies_to_nsecs(const unsigned long j)

{
	return (u64)jiffies_to_usecs(j) * NSEC_PER_USEC;
}

The problem is with jiffies_to_usecs(). Which we probably should change.

With HZ = 100,
1 second jiffies_to_usecs(100) = 1000,000.
1 minute jiffies_to_usec(6000) = 60,000,000.
1 hour jiffies_to_usecs(360000) = 3,600,000,000
1 hour 11 minutes 35 seconds -

    jiffies_to_usecs(429500) = 4,295,000,000

2^32 = 4294967296 < 4,295,000,000

Overflow!

That means after 1 hour, 11 minutes and 35 seconds, jiffies_to_usecs() will return a reset number. Time will go backwards. It doesn't matter what you typecast the return value of jiffies_to_usecs() to, the result is wrong.

Actually, I like Tony's first patch. I really think jiffies_to_usecs() should return a u64 number.

So it may indeed be a better idea to first create a standalone jiffies_to_nsecs(). It can then be used to deprecate and replace most (if not all) calls to jiffies_to_usecs() altogether. Just the conversion must be made one by one to make sure that users can handle that.

Of course a big fat comment on jiffies_to_usecs() to describe that it's unsafe and deprecated would help a bit.

The "uptime" tracer added in:
commit 8aacf017b065a805d27467843490c976835eb4a5
tracing: Add "uptime" trace clock that uses jiffies
has wraparound problems when the system has been up more than 1 hour 11 minutes and 34 seconds. It converts jiffies to nanoseconds using:

        (u64)jiffies_to_usecs(jiffy) * 1000ULL

but since jiffies_to_usecs() only returns a 32-bit value, it truncates at 2^32 microseconds. An additional problem on 32-bit systems is that the argument is "unsigned long", so fixing the return value only helps until 2^32 jiffies (49.7 days on a HZ=1000 system).

So we provide a full featured jiffies_to_nsec() function that takes a "u64" argument and provides a "u64" result. To avoid cries of rage from the other user of this: scheduler_tick_max_deferment() we check whether the argument is small enough that we can do the calculations in 32-bit operations.

Signed-off-by Tony Luck <[email protected]>

---
 include/linux/jiffies.h    |  6 +-----
 kernel/time.c              | 24 ++++++++++++++++++++++++
 kernel/timeconst.bc        | 12 ++++++++++++
 kernel/trace/trace_clock.c |  2 +-
 4 files changed, 38 insertions(+), 6 deletions(-)

How about get uptime by get_monotonic_boottime() directly, which's the same as /proc/uptime.

Is this function safe to call in every context (including NMI & machine check)? [it uses read_seqcount_begin/read_seqcount_retry ... which I *think* is safe ... but this stuff is tricky, so I'd like some reassurance].

Mauro, Steven: Did we just do math on jiffies because we wanted less overhead in a tracepoint?

Bigger question (mostly for Mauro) ... what was the motivation for the "uptime" tracer to begin with? The rasdaemon code that is using it converts the times from traces into absolute times (by adding an offset it computes by comparing uptime and gettimeofday() when it starts). But this would seem to be fraught with problems:
1) Do we get this right for events that happen in daylight saving time shift windows?
2) Is there a "drift" problem for systems that stay up for months and rely on ntp to keep wall clock time in line with reality?

-Tony

The "uptime" tracer added in:
commit 8aacf017b065a805d27467843490c976835eb4a5
tracing: Add "uptime" trace clock that uses jiffies
has wraparound problems when the system has been up more than 1 hour 11 minutes and 34 seconds. It converts jiffies to nanoseconds using:

	(u64)jiffies_to_usecs(jiffy) * 1000ULL

but since jiffies_to_usecs() only returns a 32-bit value, it truncates at 2^32 microseconds. An additional problem on 32-bit systems is that the argument is "unsigned long", so fixing the return value only helps until 2^32 jiffies (49.7 days on a HZ=1000 system).

So we provide a full featured jiffies_to_nsec() function that takes a "u64" argument and provides a "u64" result. To avoid cries of rage from the other user of this: scheduler_tick_max_deferment() we check whether the argument is small enough that we can do the calculations in 32-bit operations.

Signed-off-by Tony Luck <[email protected]>

---
 include/linux/jiffies.h    |  6 +-----
 kernel/time.c              | 24 ++++++++++++++++++++++++
 kernel/timeconst.bc        | 12 ++++++++++++
 kernel/trace/trace_clock.c |  2 +-
 4 files changed, 38 insertions(+), 6 deletions(-)

kernel/trace/trace_clock.c +1 -1

 	/* Return nsecs */
-	return (u64)jiffies_to_usecs(jiffy) * 1000ULL;
+	return jiffies_to_nsecs(jiffy);
 }
 
 /*
 

Another problem, maybe we should use get_jiffies_64() instead of jiffies directly here OR we'll meet wraparound problems on 32bit system. But a same problem is that the jiffies_lock is not safe in NMI context...

Quoted 3 lines

Another problem, maybe we should use get_jiffies_64() instead of jiffies directly here OR we'll meet wraparound problems on 32bit system. But a same problem is that the jiffies_lock is not safe in NMI context...

To quote Winnie the Pooh - "Bother".

Can someone think of a clever way to get a sane 64 bit jiffie value on 32-bit systems (even when we get an NMI in the middle of updating the 64-bit jiffie)?