jiffies_to_usecs() returns an "int" so it can only handle times up to
2^32 microseconds (1 hour 11 minutes 34 seconds) before truncation
occurs. This is a problem for the "uptime" trace clock added in
commit 8aacf017b065a805d27467843490c976835eb4a5
tracing: Add "uptime" trace clock that uses jiffies
since it converts jiffies since boot into microseconds (and then
on to nanoseconds).
Change return type from "int" to "u64" to avoid trucncation.
Also have the trace clock code use jiffies_to_nsecs() directly.
Signed-off-by: Tony Luck <[email protected]>
---
This is a RFC because:
a) Not sure it is fair to all the existing users of jiffies_to_usecs()
to start handing them a u64 when their usage needs are met by "int"
(mostly they only pass in small arguments, like 10).
b) If this is OK - it needs to touch a few more files where people do
printk("yada yada yada %d blah blah blah\n", jiffies_to_nsecs(something));
to change that format to %lld
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?
include/linux/jiffies.h | 4 ++--
kernel/time.c | 2 +-
kernel/trace/trace_clock.c | 2 +-
3 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 1f44466c1e9d..4b2621af705a 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -293,11 +293,11 @@ extern unsigned long preset_lpj;
* Convert various time units to each other:
*/
extern unsigned int jiffies_to_msecs(const unsigned long j);
-extern unsigned int jiffies_to_usecs(const unsigned long j);
+extern u64 jiffies_to_usecs(const unsigned long j);
static inline u64 jiffies_to_nsecs(const unsigned long j)
{
- return (u64)jiffies_to_usecs(j) * NSEC_PER_USEC;
+ return jiffies_to_usecs(j) * NSEC_PER_USEC;
}
extern unsigned long msecs_to_jiffies(const unsigned int m);
diff --git a/kernel/time.c b/kernel/time.c
index 7c7964c33ae7..e0dfc77f60ae 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -257,7 +257,7 @@ unsigned int jiffies_to_msecs(const unsigned long j)
}
EXPORT_SYMBOL(jiffies_to_msecs);
-unsigned int jiffies_to_usecs(const unsigned long j)
+u64 jiffies_to_usecs(const unsigned long j)
{
#if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
return (USEC_PER_SEC / HZ) * j;
diff --git a/kernel/trace/trace_clock.c b/kernel/trace/trace_clock.c
index 26dc348332b7..52470fba1d26 100644
--- a/kernel/trace/trace_clock.c
+++ b/kernel/trace/trace_clock.c
@@ -65,7 +65,7 @@ u64 notrace trace_clock_jiffies(void)
u64 jiffy = jiffies - INITIAL_JIFFIES;
/* Return nsecs */
- return (u64)jiffies_to_usecs(jiffy) * 1000ULL;
+ return jiffies_to_nsecs(jiffy);
}
/*
--
1.8.4.1
On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <[email protected]> wrote:
> 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.
-Tony
On Mon, 7 Apr 2014 22:34:51 -0700
Tony Luck <[email protected]> wrote:
> On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <[email protected]> wrote:
>
> > 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.
Sounds good to me.
-- Steve
On Mon, Apr 07, 2014 at 10:34:51PM -0700, Tony Luck wrote:
> On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <[email protected]> wrote:
>
> > 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.
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 :)
Thanks.
On Tue, 8 Apr 2014 19:49:51 +0200
Frederic Weisbecker <[email protected]> wrote:
> On Mon, Apr 07, 2014 at 10:34:51PM -0700, Tony Luck wrote:
> > On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <[email protected]> wrote:
> >
> > > 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.
>
> 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.
-- Steve
On Tue, Apr 08, 2014 at 02:15:43PM -0400, Steven Rostedt wrote:
> On Tue, 8 Apr 2014 19:49:51 +0200
> Frederic Weisbecker <[email protected]> wrote:
>
> > On Mon, Apr 07, 2014 at 10:34:51PM -0700, Tony Luck wrote:
> > > On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <[email protected]> wrote:
> > >
> > > > 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.
> >
> > 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.
Ah! Ok got it now.
> Actually, I like Tony's first patch. I really think jiffies_to_usecs()
> should return a u64 number.
Agreed it's way too error-prone. OTOH there are too many users to allow such a blind
broad conversion of its return type:
$ git grep jiffies_to_usecs | cut -f1 | wc -l
52
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.
On Tue, 8 Apr 2014 20:56:13 +0200
Frederic Weisbecker <[email protected]> wrote:
> 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.
Sure, sounds like a plan.
-- Steve
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(-)
diff --git a/include/linux/jiffies.h b/include/linux/jiffies.h
index 1f44466c1e9d..3cf44401055f 100644
--- a/include/linux/jiffies.h
+++ b/include/linux/jiffies.h
@@ -294,11 +294,7 @@ extern unsigned long preset_lpj;
*/
extern unsigned int jiffies_to_msecs(const unsigned long j);
extern unsigned int jiffies_to_usecs(const unsigned long j);
-
-static inline u64 jiffies_to_nsecs(const unsigned long j)
-{
- return (u64)jiffies_to_usecs(j) * NSEC_PER_USEC;
-}
+extern u64 jiffies_to_nsecs(const u64 j);
extern unsigned long msecs_to_jiffies(const unsigned int m);
extern unsigned long usecs_to_jiffies(const unsigned int u);
diff --git a/kernel/time.c b/kernel/time.c
index 7c7964c33ae7..bbd18b0e5698 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -273,6 +273,30 @@ unsigned int jiffies_to_usecs(const unsigned long j)
}
EXPORT_SYMBOL(jiffies_to_usecs);
+u64 jiffies_to_nsecs(const u64 jl)
+{
+ /* can we do this with 32-bit math? */
+ if (jl < 4*HZ) {
+ unsigned long j = jl;
+#if !(NSEC_PER_SEC % HZ)
+ return (NSEC_PER_SEC / HZ) * j;
+#else
+# if BITS_PER_LONG == 32
+ return (HZ_TO_NSEC_MUL32 * j) >> HZ_TO_NSEC_SHR32;
+# else
+ return (j * HZ_TO_NSEC_NUM) / HZ_TO_NSEC_DEN;
+# endif
+#endif
+ } else {
+#if !(NSEC_PER_SEC % HZ)
+ return (NSEC_PER_SEC / HZ) * jl;
+#else
+ return (jl * HZ_TO_NSEC_NUM) / HZ_TO_NSEC_DEN;
+#endif
+ }
+}
+EXPORT_SYMBOL(jiffies_to_nsecs);
+
/**
* timespec_trunc - Truncate timespec to a granularity
* @t: Timespec
diff --git a/kernel/timeconst.bc b/kernel/timeconst.bc
index 511bdf2cafda..6f6e8b285c2b 100644
--- a/kernel/timeconst.bc
+++ b/kernel/timeconst.bc
@@ -100,6 +100,18 @@ define timeconst(hz) {
print "#define USEC_TO_HZ_DEN\t\t", 1000000/cd, "\n"
print "\n"
+ s=fmuls(32,1000000000,hz)
+ obase=16
+ print "#define HZ_TO_NSEC_MUL32\tU64_C(0x", fmul(s,1000000000,hz), ")\n"
+ obase=10
+ print "#define HZ_TO_NSEC_SHR32\t", s, "\n"
+
+ obase=10
+ cd=gcd(hz,1000000000)
+ print "#define HZ_TO_NSEC_NUM\t\t", 1000000000/cd, "\n"
+ print "#define HZ_TO_NSEC_DEN\t\t", hz/cd, "\n"
+ print "\n"
+
print "#endif /* KERNEL_TIMECONST_H */\n"
}
halt
diff --git a/kernel/trace/trace_clock.c b/kernel/trace/trace_clock.c
index 26dc348332b7..52470fba1d26 100644
--- a/kernel/trace/trace_clock.c
+++ b/kernel/trace/trace_clock.c
@@ -65,7 +65,7 @@ u64 notrace trace_clock_jiffies(void)
u64 jiffy = jiffies - INITIAL_JIFFIES;
/* Return nsecs */
- return (u64)jiffies_to_usecs(jiffy) * 1000ULL;
+ return jiffies_to_nsecs(jiffy);
}
/*
--
1.8.4.1
+ if (jl < 4*HZ) {
That was lazy ... should be something like:
+ if (jl < (u64)UINT_MAX * HZ / NSEC_PER_SEC)
-Tony
On 2014/5/20 6:35, Luck, Tony wrote:
>> 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)?
Hi Tony & Steven,
I've sent a patch to solve this problem, please help to review, thanks.
http://lkml.org/lkml/2014/6/28/41
tracing: fix uptime overflow problem
Thanks,
XiuQi