[RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Tony Luck]

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 <tony.luck@intel.com>
---

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

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Tony Luck]

On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <tony.luck@intel.com> 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

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Steven Rostedt]

On Mon, 7 Apr 2014 22:34:51 -0700
Tony Luck <tony.luck@gmail.com> wrote:

> On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <tony.luck@intel.com> 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

[PATCH] time: Provide full featured jiffies_to_nsecs() function

[Tony Luck]

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 <tony.luck@intel.com>
---
 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

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Frederic Weisbecker]

On Mon, Apr 07, 2014 at 10:34:51PM -0700, Tony Luck wrote:
> On Mon, Apr 7, 2014 at 3:25 PM, Tony Luck <tony.luck@intel.com> 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.

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Steven Rostedt]

On Tue, 8 Apr 2014 19:49:51 +0200
Frederic Weisbecker <fweisbec@gmail.com> 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 <tony.luck@intel.com> 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

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Frederic Weisbecker]

On Tue, Apr 08, 2014 at 02:15:43PM -0400, Steven Rostedt wrote:
> On Tue, 8 Apr 2014 19:49:51 +0200
> Frederic Weisbecker <fweisbec@gmail.com> 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 <tony.luck@intel.com> 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.

Re: [RFC PATCH] time: Fix truncation in jiffies_to_usecs()

[Steven Rostedt]

On Tue, 8 Apr 2014 20:56:13 +0200
Frederic Weisbecker <fweisbec@gmail.com> 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

Re: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Tony Luck]

+       if (jl < 4*HZ) {

That was lazy ... should be something like:

+       if (jl < (u64)UINT_MAX * HZ / NSEC_PER_SEC)

-Tony

[PATCH] time: Provide full featured jiffies_to_nsecs() function

[Luck, 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 <tony.luck@intel.com>
---

Looks like I flubbed on re-sending the updated version of this
after our discussion back in early April.

 include/linux/jiffies.h    |  6 +-----
 kernel/time.c              | 25 +++++++++++++++++++++++++
 kernel/timeconst.bc        | 12 ++++++++++++
 kernel/trace/trace_clock.c |  2 +-
 4 files changed, 39 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..7e69ceed12c0 100644
--- a/kernel/time.c
+++ b/kernel/time.c
@@ -259,6 +259,7 @@ EXPORT_SYMBOL(jiffies_to_msecs);
 
 unsigned int jiffies_to_usecs(const unsigned long j)
 {
+	WARN_ON_ONCE(j > UINT_MAX * HZ / USEC_PER_SEC);
 #if HZ <= USEC_PER_SEC && !(USEC_PER_SEC % HZ)
 	return (USEC_PER_SEC / HZ) * j;
 #elif HZ > USEC_PER_SEC && !(HZ % USEC_PER_SEC)
@@ -273,6 +274,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 < (u64)UINT_MAX * HZ / NSEC_PER_SEC) {
+		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

Re: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Shy Shuky]

2014-04-09 1:32 GMT+08:00 Tony Luck <tony.luck@intel.com>:
> 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.

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

Then trace_clock_jiffies() would be like bellow:

u64 notrace trace_clock_jiffies(void)
        struct timespec uptime;
        get_monotonic_boottime(&uptime);

        return uptime.tv_sec * NSEC_PER_SEC + uptime.tv_nsec;
}

--
XiuQi

>
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  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(-)
...

RE: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Luck, Tony]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1294 bytes --]

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

I don't know.  get_monotonic_boottime() had existed for many releases
at the point the uptime trace clock was added - so it was an available
choice. Maybe not noticed by

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
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Steven Rostedt]

On Fri, 16 May 2014 17:17:46 +0000
"Luck, Tony" <tony.luck@intel.com> wrote:

> 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].

No, read_seqcount_begin() is not safe in NMI context. If it interrupts
a write, it goes into an infinite spin (see __read_seqcount_begin()).

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

As I meantioned. read_seqcount_begin() is not safe for tracing, it
had to be reimplemented.

-- Steve

> 
> 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

Re: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Xie XiuQi]

On 2014/4/9 1:32, Tony Luck wrote:
> 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 <tony.luck@intel.com>
> ---
>  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/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;

Hi Tony and Steven,
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...

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

RE: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Luck, Tony]

> 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)?

-Tony

Re: [PATCH] time: Provide full featured jiffies_to_nsecs() function

[Xie XiuQi]

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