[PATCH 0/7] Current queue for tip/timers/core

[PATCH 0/7] Current queue for tip/timers/core

[John Stultz]

Hey Thomas, Ingo,
  Just wanted to send you my current queue of items that I
have pending for tip/timers/core for 4.2

Let me know if you have any concerns or objections.

thanks
-john

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>

Badhri Jagan Sridharan (1):
  tracing: timer: Add deferrable flag to timer_start

Harald Geyer (1):
  timekeeping: Provide new API to get the current time resolution

John Stultz (1):
  time: Rework debugging variables so they aren't global

Sasha Levin (1):
  time: make sure tz_minuteswest is set to a valid value when setting
    time

Xunlei Pang (3):
  time: include math64.h in time64.h
  s390: time: Provide read_boot_clock64() and read_persistent_clock64()
  time: Remove read_boot_clock()

 arch/s390/include/asm/timex.h       |  5 +--
 arch/s390/kernel/debug.c            | 11 ++++---
 arch/s390/kernel/time.c             |  6 ++--
 include/linux/time64.h              |  1 +
 include/linux/timekeeper_internal.h | 15 +++++++++
 include/linux/timekeeping.h         |  2 +-
 include/trace/events/timer.h        | 13 +++++---
 kernel/time/time.c                  |  4 +++
 kernel/time/timekeeping.c           | 64 ++++++++++++++++++-------------------
 kernel/time/timer.c                 |  3 +-
 10 files changed, 75 insertions(+), 49 deletions(-)

-- 
1.9.1

[PATCH 1/7] time: make sure tz_minuteswest is set to a valid value when setting time

[John Stultz]

From: Sasha Levin <sasha.levin@oracle.com>

Invalid values may overflow later, leading to undefined behaviour when
multiplied by 60 to get the amount of seconds.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 kernel/time/time.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/kernel/time/time.c b/kernel/time/time.c
index c42c2c3..972e3bb 100644
--- a/kernel/time/time.c
+++ b/kernel/time/time.c
@@ -173,6 +173,10 @@ int do_sys_settimeofday(const struct timespec *tv, const struct timezone *tz)
 		return error;
 
 	if (tz) {
+		/* Verify we're witin the +-15 hrs range */
+		if (tz->tz_minuteswest > 15*60 || tz->tz_minuteswest < -15*60)
+			return -EINVAL;
+
 		sys_tz = *tz;
 		update_vsyscall_tz();
 		if (firsttime) {
-- 
1.9.1

[PATCH 2/7] timekeeping: Provide new API to get the current time resolution

[John Stultz]

From: Harald Geyer <harald@ccbib.org>

This patch series introduces a new function
u32 ktime_get_resolution_ns(void)
which allows to clean up some driver code.

In particular the IIO subsystem has a function to provide timestamps for
events but no means to get their resolution. So currently the dht11 driver
tries to guess the resolution in a rather messy and convoluted way. We
can do much better with the new code.

This API is not designed to be exposed to user space.

This has been tested on i386, sunxi and mxs.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Harald Geyer <harald@ccbib.org>
[jstultz: Tweaked to make it build after upstream changes]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/timekeeping.h |  1 +
 kernel/time/timekeeping.c   | 17 +++++++++++++++++
 2 files changed, 18 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 99176af..9af5c12 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -163,6 +163,7 @@ extern ktime_t ktime_get(void);
 extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
 extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
 extern ktime_t ktime_get_raw(void);
+extern u32 ktime_get_resolution_ns(void);
 
 /**
  * ktime_get_real - get the real (wall-) time in ktime_t format
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 3365e32..85d3763 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -702,6 +702,23 @@ ktime_t ktime_get(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get);
 
+u32 ktime_get_resolution_ns(void)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned int seq;
+	u32 nsecs;
+
+	WARN_ON(timekeeping_suspended);
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		nsecs = tk->tkr_mono.mult >> tk->tkr_mono.shift;
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return nsecs;
+}
+EXPORT_SYMBOL_GPL(ktime_get_resolution_ns);
+
 static ktime_t *offsets[TK_OFFS_MAX] = {
 	[TK_OFFS_REAL]	= &tk_core.timekeeper.offs_real,
 	[TK_OFFS_BOOT]	= &tk_core.timekeeper.offs_boot,
-- 
1.9.1

Re: [PATCH 2/7] timekeeping: Provide new API to get the current time resolution

[Harald Geyer]

Hi John,

John Stultz writes:
> From: Harald Geyer <harald@ccbib.org>
> 
> This patch series introduces a new function
> u32 ktime_get_resolution_ns(void)
> which allows to clean up some driver code.

thanks for keeping track of this, but is this patch still useful?

I was thinking that the variable hrtimer_resolution, that Thomas
introduced in
https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/include/linux/hrtimer.h?h=timers/wip&id=03eeacdb07e2fdfc4ef311c2593286c92eba609c
is meant to provide the same information. I haven't looked into this
in detail yet, so I might be wrong, but it is on my todo list for
after it appears in the trees I work with...

TIA,
Harald

> In particular the IIO subsystem has a function to provide timestamps for
> events but no means to get their resolution. So currently the dht11 driver
> tries to guess the resolution in a rather messy and convoluted way. We
> can do much better with the new code.
> 
> This API is not designed to be exposed to user space.
> 
> This has been tested on i386, sunxi and mxs.
> 
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
> Signed-off-by: Harald Geyer <harald@ccbib.org>
> [jstultz: Tweaked to make it build after upstream changes]
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  include/linux/timekeeping.h |  1 +
>  kernel/time/timekeeping.c   | 17 +++++++++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 99176af..9af5c12 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -163,6 +163,7 @@ extern ktime_t ktime_get(void);
>  extern ktime_t ktime_get_with_offset(enum tk_offsets offs);
>  extern ktime_t ktime_mono_to_any(ktime_t tmono, enum tk_offsets offs);
>  extern ktime_t ktime_get_raw(void);
> +extern u32 ktime_get_resolution_ns(void);
>  
>  /**
>   * ktime_get_real - get the real (wall-) time in ktime_t format
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index 3365e32..85d3763 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -702,6 +702,23 @@ ktime_t ktime_get(void)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get);
>  
> +u32 ktime_get_resolution_ns(void)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned int seq;
> +	u32 nsecs;
> +
> +	WARN_ON(timekeeping_suspended);
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		nsecs = tk->tkr_mono.mult >> tk->tkr_mono.shift;
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return nsecs;
> +}
> +EXPORT_SYMBOL_GPL(ktime_get_resolution_ns);
> +
>  static ktime_t *offsets[TK_OFFS_MAX] = {
>  	[TK_OFFS_REAL]	= &tk_core.timekeeper.offs_real,
>  	[TK_OFFS_BOOT]	= &tk_core.timekeeper.offs_boot,
> -- 
> 1.9.1
> 

Re: [PATCH 2/7] timekeeping: Provide new API to get the current time resolution

[John Stultz]

On Wed, May 20, 2015 at 10:53 AM, Harald Geyer <harald@ccbib.org> wrote:
> Hi John,
>
> John Stultz writes:
>> From: Harald Geyer <harald@ccbib.org>
>>
>> This patch series introduces a new function
>> u32 ktime_get_resolution_ns(void)
>> which allows to clean up some driver code.
>
> thanks for keeping track of this, but is this patch still useful?
>
> I was thinking that the variable hrtimer_resolution, that Thomas
> introduced in
> https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/include/linux/hrtimer.h?h=timers/wip&id=03eeacdb07e2fdfc4ef311c2593286c92eba609c
> is meant to provide the same information. I haven't looked into this
> in detail yet, so I might be wrong, but it is on my todo list for
> after it appears in the trees I work with...

Well, I don't think the above covers the same usage, since one is the
hrtimer resolution (which we expose to userspace via the posix timers
interface) vs the timekeeping/clocksource resolution (which we don't
intend to expose to userspace).

That said, if you're not sure if this patch is still necessary, I'm
happy to drop it, since your iio code was the only potential user so
far.  :)

thanks
-john

Re: [PATCH 2/7] timekeeping: Provide new API to get the current time resolution

[Harald Geyer]

John Stultz writes:
> > I was thinking that the variable hrtimer_resolution, that Thomas
> > introduced in
> > https://git.kernel.org/cgit/linux/kernel/git/tip/tip.git/commit/include/linux/hrtimer.h?h=timers/wip&id=03eeacdb07e2fdfc4ef311c2593286c92eba609c
> > is meant to provide the same information. I haven't looked into this
> > in detail yet, so I might be wrong, but it is on my todo list for
> > after it appears in the trees I work with...
> 
> Well, I don't think the above covers the same usage, since one is the
> hrtimer resolution (which we expose to userspace via the posix timers
> interface) vs the timekeeping/clocksource resolution (which we don't
> intend to expose to userspace).

Ok, hrtimer resolution is not updated when we change clocksource.
Seems like I misinterpreted something when I read the series.

> That said, if you're not sure if this patch is still necessary, I'm
> happy to drop it, since your iio code was the only potential user so
> far.  :)

Looks like my patch is still needed... :(

Thanks,
Harald

[PATCH 3/7] time: Rework debugging variables so they aren't global

[John Stultz]

Ingo suggested that the timekeeping debugging variables
recently added should not be global, and should be tied
to the timekeeper's read_base.

Thus this patch implements that suggestion.

This version is differnet from the earlier versions
as it keeps the variables in the timekeeper structure
rather then in the tkr.

Cc: Ingo Molnar <mingo@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Prarit Bhargava <prarit@redhat.com>
Cc: Richard Cochran <richardcochran@gmail.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/timekeeper_internal.h | 15 +++++++++++++++
 kernel/time/timekeeping.c           | 33 +++++++++++----------------------
 2 files changed, 26 insertions(+), 22 deletions(-)

diff --git a/include/linux/timekeeper_internal.h b/include/linux/timekeeper_internal.h
index 6f8276a..35007b2 100644
--- a/include/linux/timekeeper_internal.h
+++ b/include/linux/timekeeper_internal.h
@@ -61,6 +61,9 @@ struct tk_read_base {
  *			shifted nano seconds.
  * @ntp_error_shift:	Shift conversion between clock shifted nano seconds and
  *			ntp shifted nano seconds.
+ * @last_warning:	Warning ratelimiter (DEBUG_TIMEKEEPING)
+ * @underflow_seen:	Underflow warning flag (DEBUG_TIMEKEEPING)
+ * @overflow_seen:	Overflow warning flag (DEBUG_TIMEKEEPING)
  *
  * Note: For timespec(64) based interfaces wall_to_monotonic is what
  * we need to add to xtime (or xtime corrected for sub jiffie times)
@@ -106,6 +109,18 @@ struct timekeeper {
 	s64			ntp_error;
 	u32			ntp_error_shift;
 	u32			ntp_err_mult;
+#ifdef CONFIG_DEBUG_TIMEKEEPING
+	long			last_warning;
+	/*
+	 * These simple flag variables are managed
+	 * without locks, which is racy, but ok since
+	 * we don't really care about being super
+	 * precise about how many events were seen,
+	 * just that a problem was observed.
+	 */
+	int			underflow_seen;
+	int			overflow_seen;
+#endif
 };
 
 #ifdef CONFIG_GENERIC_TIME_VSYSCALL
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 85d3763..2f10b65 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -118,18 +118,6 @@ static inline void tk_update_sleep_time(struct timekeeper *tk, ktime_t delta)
 
 #ifdef CONFIG_DEBUG_TIMEKEEPING
 #define WARNING_FREQ (HZ*300) /* 5 minute rate-limiting */
-/*
- * These simple flag variables are managed
- * without locks, which is racy, but ok since
- * we don't really care about being super
- * precise about how many events were seen,
- * just that a problem was observed.
- */
-static int timekeeping_underflow_seen;
-static int timekeeping_overflow_seen;
-
-/* last_warning is only modified under the timekeeping lock */
-static long timekeeping_last_warning;
 
 static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
 {
@@ -149,29 +137,30 @@ static void timekeeping_check_update(struct timekeeper *tk, cycle_t offset)
 		}
 	}
 
-	if (timekeeping_underflow_seen) {
-		if (jiffies - timekeeping_last_warning > WARNING_FREQ) {
+	if (tk->underflow_seen) {
+		if (jiffies - tk->last_warning > WARNING_FREQ) {
 			printk_deferred("WARNING: Underflow in clocksource '%s' observed, time update ignored.\n", name);
 			printk_deferred("         Please report this, consider using a different clocksource, if possible.\n");
 			printk_deferred("         Your kernel is probably still fine.\n");
-			timekeeping_last_warning = jiffies;
+			tk->last_warning = jiffies;
 		}
-		timekeeping_underflow_seen = 0;
+		tk->underflow_seen = 0;
 	}
 
-	if (timekeeping_overflow_seen) {
-		if (jiffies - timekeeping_last_warning > WARNING_FREQ) {
+	if (tk->overflow_seen) {
+		if (jiffies - tk->last_warning > WARNING_FREQ) {
 			printk_deferred("WARNING: Overflow in clocksource '%s' observed, time update capped.\n", name);
 			printk_deferred("         Please report this, consider using a different clocksource, if possible.\n");
 			printk_deferred("         Your kernel is probably still fine.\n");
-			timekeeping_last_warning = jiffies;
+			tk->last_warning = jiffies;
 		}
-		timekeeping_overflow_seen = 0;
+		tk->overflow_seen = 0;
 	}
 }
 
 static inline cycle_t timekeeping_get_delta(struct tk_read_base *tkr)
 {
+	struct timekeeper *tk = &tk_core.timekeeper;
 	cycle_t now, last, mask, max, delta;
 	unsigned int seq;
 
@@ -197,13 +186,13 @@ static inline cycle_t timekeeping_get_delta(struct tk_read_base *tkr)
 	 * mask-relative negative values.
 	 */
 	if (unlikely((~delta & mask) < (mask >> 3))) {
-		timekeeping_underflow_seen = 1;
+		tk->underflow_seen = 1;
 		delta = 0;
 	}
 
 	/* Cap delta value to the max_cycles values to avoid mult overflows */
 	if (unlikely(delta > max)) {
-		timekeeping_overflow_seen = 1;
+		tk->overflow_seen = 1;
 		delta = tkr->clock->max_cycles;
 	}
 
-- 
1.9.1

Re: [PATCH 3/7] time: Rework debugging variables so they aren't global

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> Ingo suggested that the timekeeping debugging variables
> recently added should not be global, and should be tied
> to the timekeeper's read_base.
> 
> Thus this patch implements that suggestion.
> 
> This version is differnet from the earlier versions

s/differnet/
  different

> as it keeps the variables in the timekeeper structure
> rather then in the tkr.

> + * @last_warning:	Warning ratelimiter (DEBUG_TIMEKEEPING)
> + * @underflow_seen:	Underflow warning flag (DEBUG_TIMEKEEPING)
> + * @overflow_seen:	Overflow warning flag (DEBUG_TIMEKEEPING)
>   *
>   * Note: For timespec(64) based interfaces wall_to_monotonic is what
>   * we need to add to xtime (or xtime corrected for sub jiffie times)
> @@ -106,6 +109,18 @@ struct timekeeper {
>  	s64			ntp_error;
>  	u32			ntp_error_shift;
>  	u32			ntp_err_mult;
> +#ifdef CONFIG_DEBUG_TIMEKEEPING
> +	long			last_warning;
> +	/*
> +	 * These simple flag variables are managed
> +	 * without locks, which is racy, but ok since
> +	 * we don't really care about being super
> +	 * precise about how many events were seen,
> +	 * just that a problem was observed.

s/but ok since/
  but they are ok since

Thanks,

	Ingo

[PATCH 4/7] tracing: timer: Add deferrable flag to timer_start

[John Stultz]

From: Badhri Jagan Sridharan <badhri@google.com>

The timer_start event now shows whether the timer is
deferrable in case of a low-res timer. The debug_activate
function now includes deferrable flag while calling
trace_timer_start event.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Acked-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Badhri Jagan Sridharan <Badhri@google.com>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/trace/events/timer.h | 13 +++++++++----
 kernel/time/timer.c          |  3 ++-
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/timer.h b/include/trace/events/timer.h
index 68c2c20..d7abef1 100644
--- a/include/trace/events/timer.h
+++ b/include/trace/events/timer.h
@@ -43,15 +43,18 @@ DEFINE_EVENT(timer_class, timer_init,
  */
 TRACE_EVENT(timer_start,
 
-	TP_PROTO(struct timer_list *timer, unsigned long expires),
+	TP_PROTO(struct timer_list *timer,
+		unsigned long expires,
+		unsigned int deferrable),
 
-	TP_ARGS(timer, expires),
+	TP_ARGS(timer, expires, deferrable),
 
 	TP_STRUCT__entry(
 		__field( void *,	timer		)
 		__field( void *,	function	)
 		__field( unsigned long,	expires		)
 		__field( unsigned long,	now		)
+		__field( unsigned int,	deferrable	)
 	),
 
 	TP_fast_assign(
@@ -59,11 +62,13 @@ TRACE_EVENT(timer_start,
 		__entry->function	= timer->function;
 		__entry->expires	= expires;
 		__entry->now		= jiffies;
+		__entry->deferrable     = deferrable;
 	),
 
-	TP_printk("timer=%p function=%pf expires=%lu [timeout=%ld]",
+	TP_printk("timer=%p function=%pf expires=%lu [timeout=%ld] defer=%c",
 		  __entry->timer, __entry->function, __entry->expires,
-		  (long)__entry->expires - __entry->now)
+		  (long)__entry->expires - __entry->now,
+		  __entry->deferrable > 0 ? 'y':'n')
 );
 
 /**
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index d4af7c5..ed75ed5 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -650,7 +650,8 @@ static inline void
 debug_activate(struct timer_list *timer, unsigned long expires)
 {
 	debug_timer_activate(timer);
-	trace_timer_start(timer, expires);
+	trace_timer_start(timer, expires,
+		tbase_get_deferrable(timer->base));
 }
 
 static inline void debug_deactivate(struct timer_list *timer)
-- 
1.9.1

Re: [PATCH 4/7] tracing: timer: Add deferrable flag to timer_start

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> From: Badhri Jagan Sridharan <badhri@google.com>
> 
> The timer_start event now shows whether the timer is
> deferrable in case of a low-res timer. The debug_activate
> function now includes deferrable flag while calling
> trace_timer_start event.

s/now includes deferrable flag/
  now includes a deferrable flag

s/calling trace_timer_start event/
  calling the trace_timer_start event

>  TRACE_EVENT(timer_start,
>  
> -	TP_PROTO(struct timer_list *timer, unsigned long expires),
> +	TP_PROTO(struct timer_list *timer,
> +		unsigned long expires,

This isn't compat safe, should any tooling rely on this.

I see it's a mistake in prior code:

> +		unsigned int deferrable),
>  
> -	TP_ARGS(timer, expires),
> +	TP_ARGS(timer, expires, deferrable),
>  
>  	TP_STRUCT__entry(
>  		__field( void *,	timer		)
>  		__field( void *,	function	)
>  		__field( unsigned long,	expires		)
>  		__field( unsigned long,	now		)

which should probably be fixed as well.

> @@ -650,7 +650,8 @@ static inline void
>  debug_activate(struct timer_list *timer, unsigned long expires)
>  {
>  	debug_timer_activate(timer);
> -	trace_timer_start(timer, expires);
> +	trace_timer_start(timer, expires,
> +		tbase_get_deferrable(timer->base));

why is this line broken? If you put it into a single line it's still 
below 80 cols, so there's really no reason for it.

Thanks,

	Ingo

Re: [PATCH 4/7] tracing: timer: Add deferrable flag to timer_start

[Thomas Gleixner]

On Thu, 21 May 2015, Ingo Molnar wrote:
> * John Stultz <john.stultz@linaro.org> wrote:
> > -	TP_PROTO(struct timer_list *timer, unsigned long expires),
> > +	TP_PROTO(struct timer_list *timer,
> > +		unsigned long expires,
> 
> This isn't compat safe, should any tooling rely on this.

I can't see how that matters. The binary trace tells you from which
machine (32 or 64 bit) it comes. So the field size is available for
the tool. If the tool blindly applies the format string, it's hardly a
fault of the kernel. And there is no point to bloat 32bit tracing with
64bit entries just because some random tool might be stupid.

Just for the record: We have 539 'unsigned long' and 62 'long' event
fields in include/trace/events/.

Thanks,

	tglx

Re: [PATCH 4/7] tracing: timer: Add deferrable flag to timer_start

[Ingo Molnar]

* Thomas Gleixner <tglx@linutronix.de> wrote:

> On Thu, 21 May 2015, Ingo Molnar wrote:
> > * John Stultz <john.stultz@linaro.org> wrote:
> > > -	TP_PROTO(struct timer_list *timer, unsigned long expires),
> > > +	TP_PROTO(struct timer_list *timer,
> > > +		unsigned long expires,
> > 
> > This isn't compat safe, should any tooling rely on this.
> 
> I can't see how that matters. [...]

It's good practice for any user facing ABI to not be bit depende. We 
it for (almost) all the syscall ABIs.

> [...] The binary trace tells you from which machine (32 or 64 bit) 
> it comes. [...]

Yeah, tooling can almost always fix up a crappy interface, but that's 
no good reason to not do it right in the first place. That's one of 
the reasons why (most) network protocols and most filesystems don't 
have a 'bitness' nor an 'endianness' field in their formats. It's a 
fundamentally fragile concept ...

> [...] So the field size is available for the tool. If the tool 
> blindly applies the format string, it's hardly a fault of the 
> kernel. And there is no point to bloat 32bit tracing with 64bit 
> entries just because some random tool might be stupid.

Yeah, in a perfect world and so we could define arbitrarily complex 
interfaces and expect tooling to follow it.

In a not so perfect world we are conservative in defining our ABIs to 
be as simple as possible and are liberal in accepting them, not the 
other way around.

And it's not just about tooling - just to give a simple technical 
example: parsing a partly corrupted file is a lot easier if there's no 
dependency on some header area defining a 'bitness value' - you can 
just take any part of the stream and eventually have robust decoding 
even if you lost part of the stream.

Thanks,

	Ingo

Re: [PATCH 4/7] tracing: timer: Add deferrable flag to timer_start

[Ingo Molnar]

* Ingo Molnar <mingo@kernel.org> wrote:

> > [...] So the field size is available for the tool. If the tool 
> > blindly applies the format string, it's hardly a fault of the 
> > kernel. And there is no point to bloat 32bit tracing with 64bit 
> > entries just because some random tool might be stupid.
> 
> Yeah, in a perfect world and so we could define arbitrarily complex 
> interfaces and expect tooling to follow it.

OTOH, on a second thought, libtraceevent handles this correctly, so 
the chance for tooling to get this wrong is much smaller than for 
other ABIs.

Plus the advantages on 32-bit systems you mentioned are real as well - 
so I concur with you.

Thanks,

	Ingo

[PATCH 5/7] time: include math64.h in time64.h

[John Stultz]

From: Xunlei Pang <pang.xunlei@linaro.org>

On 32-bit systems, timespec64_add_ns() calls __iter_div_u64_rem()
which needs match64.h, and we want to include time64.h in some
cases.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/time64.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/time64.h b/include/linux/time64.h
index a383147..12d4e82 100644
--- a/include/linux/time64.h
+++ b/include/linux/time64.h
@@ -2,6 +2,7 @@
 #define _LINUX_TIME64_H
 
 #include <uapi/linux/time.h>
+#include <linux/math64.h>
 
 typedef __s64 time64_t;
 
-- 
1.9.1

Re: [PATCH 5/7] time: include math64.h in time64.h

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> On 32-bit systems, timespec64_add_ns() calls __iter_div_u64_rem()
> which needs match64.h, and we want to include time64.h in some

s/match64.h
  math64.h

Also, missing comma.

Thanks,

	Ingo

[PATCH 6/7] s390: time: Provide read_boot_clock64() and read_persistent_clock64()

[John Stultz]

From: Xunlei Pang <pang.xunlei@linaro.org>

As part of addressing "y2038 problem" for in-kernel uses, this
patch converts read_boot_clock() to read_boot_clock64() and
read_persistent_clock() to read_persistent_clock64() using
timespec64.

Rename some timespec to timespec64 in time.c and related references.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Cc: Heiko Carstens <heiko.carstens@de.ibm.com>
Cc: linux390@de.ibm.com
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 arch/s390/include/asm/timex.h |  5 +++--
 arch/s390/kernel/debug.c      | 11 ++++++-----
 arch/s390/kernel/time.c       |  6 +++---
 3 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/arch/s390/include/asm/timex.h b/arch/s390/include/asm/timex.h
index 98eb2a5..f39220f 100644
--- a/arch/s390/include/asm/timex.h
+++ b/arch/s390/include/asm/timex.h
@@ -10,6 +10,7 @@
 #define _ASM_S390_TIMEX_H
 
 #include <asm/lowcore.h>
+#include <linux/time64.h>
 
 /* The value of the TOD clock for 1.1.1970. */
 #define TOD_UNIX_EPOCH 0x7d91048bca000000ULL
@@ -108,10 +109,10 @@ int get_sync_clock(unsigned long long *clock);
 void init_cpu_timer(void);
 unsigned long long monotonic_clock(void);
 
-void tod_to_timeval(__u64, struct timespec *);
+void tod_to_timeval(__u64, struct timespec64 *);
 
 static inline
-void stck_to_timespec(unsigned long long stck, struct timespec *ts)
+void stck_to_timespec64(unsigned long long stck, struct timespec64 *ts)
 {
 	tod_to_timeval(stck - TOD_UNIX_EPOCH, ts);
 }
diff --git a/arch/s390/kernel/debug.c b/arch/s390/kernel/debug.c
index c1f21ac..4f2d11d 100644
--- a/arch/s390/kernel/debug.c
+++ b/arch/s390/kernel/debug.c
@@ -1457,23 +1457,24 @@ int
 debug_dflt_header_fn(debug_info_t * id, struct debug_view *view,
 			 int area, debug_entry_t * entry, char *out_buf)
 {
-	struct timespec time_spec;
+	struct timespec64 time_spec;
 	char *except_str;
 	unsigned long caller;
 	int rc = 0;
 	unsigned int level;
 
 	level = entry->id.fields.level;
-	stck_to_timespec(entry->id.stck, &time_spec);
+	stck_to_timespec64(entry->id.stck, &time_spec);
 
 	if (entry->id.fields.exception)
 		except_str = "*";
 	else
 		except_str = "-";
 	caller = ((unsigned long) entry->caller) & PSW_ADDR_INSN;
-	rc += sprintf(out_buf, "%02i %011lu:%06lu %1u %1s %02i %p  ",
-		      area, time_spec.tv_sec, time_spec.tv_nsec / 1000, level,
-		      except_str, entry->id.fields.cpuid, (void *) caller);
+	rc += sprintf(out_buf, "%02i %011lld:%06lu %1u %1s %02i %p  ",
+		      area, (long long) time_spec.tv_sec,
+		      time_spec.tv_nsec / 1000, level, except_str,
+		      entry->id.fields.cpuid, (void *) caller);
 	return rc;
 }
 EXPORT_SYMBOL(debug_dflt_header_fn);
diff --git a/arch/s390/kernel/time.c b/arch/s390/kernel/time.c
index 170ddd2..9e733d9 100644
--- a/arch/s390/kernel/time.c
+++ b/arch/s390/kernel/time.c
@@ -76,7 +76,7 @@ unsigned long long monotonic_clock(void)
 }
 EXPORT_SYMBOL(monotonic_clock);
 
-void tod_to_timeval(__u64 todval, struct timespec *xt)
+void tod_to_timeval(__u64 todval, struct timespec64 *xt)
 {
 	unsigned long long sec;
 
@@ -181,12 +181,12 @@ static void timing_alert_interrupt(struct ext_code ext_code,
 static void etr_reset(void);
 static void stp_reset(void);
 
-void read_persistent_clock(struct timespec *ts)
+void read_persistent_clock64(struct timespec64 *ts)
 {
 	tod_to_timeval(get_tod_clock() - TOD_UNIX_EPOCH, ts);
 }
 
-void read_boot_clock(struct timespec *ts)
+void read_boot_clock64(struct timespec64 *ts)
 {
 	tod_to_timeval(sched_clock_base_cc - TOD_UNIX_EPOCH, ts);
 }
-- 
1.9.1

Re: [PATCH 6/7] s390: time: Provide read_boot_clock64() and read_persistent_clock64()

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> As part of addressing "y2038 problem" for in-kernel uses, this
> patch converts read_boot_clock() to read_boot_clock64() and
> read_persistent_clock() to read_persistent_clock64() using
> timespec64.

s/addressing "y2037 problem"/
  addressing the "y2037 problem"

> Rename some timespec to timespec64 in time.c and related references.

This sentence does not parse. Did you want to say:

  Rename some instances of 'timespec' to 'timespec64' in time.c and 
  related references.

?

> @@ -108,10 +109,10 @@ int get_sync_clock(unsigned long long *clock);
>  void init_cpu_timer(void);
>  unsigned long long monotonic_clock(void);
>  
> -void tod_to_timeval(__u64, struct timespec *);
> +void tod_to_timeval(__u64, struct timespec64 *);

Please use proper prototypes with parameters spelled out as well, so 
that people grepping for APIs don't have to guess too much.

> -	rc += sprintf(out_buf, "%02i %011lu:%06lu %1u %1s %02i %p  ",
> -		      area, time_spec.tv_sec, time_spec.tv_nsec / 1000, level,
> -		      except_str, entry->id.fields.cpuid, (void *) caller);
> +	rc += sprintf(out_buf, "%02i %011lld:%06lu %1u %1s %02i %p  ",
> +		      area, (long long) time_spec.tv_sec,
> +		      time_spec.tv_nsec / 1000, level, except_str,
> +		      entry->id.fields.cpuid, (void *) caller);

Unnecessary space before the casts.

Thanks,

	Ingo

[PATCH 7/7] time: Remove read_boot_clock()

[John Stultz]

From: Xunlei Pang <pang.xunlei@linaro.org>

Now we have all the read_boot_clock64() for all implementations,
it's time to remove read_boot_clock() completely from the kernel.

Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Xunlei Pang <pang.xunlei@linaro.org>
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 include/linux/timekeeping.h |  1 -
 kernel/time/timekeeping.c   | 14 +++-----------
 2 files changed, 3 insertions(+), 12 deletions(-)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 9af5c12..3aa72e6 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -267,7 +267,6 @@ extern int persistent_clock_is_local;
 
 extern void read_persistent_clock(struct timespec *ts);
 extern void read_persistent_clock64(struct timespec64 *ts);
-extern void read_boot_clock(struct timespec *ts);
 extern void read_boot_clock64(struct timespec64 *ts);
 extern int update_persistent_clock(struct timespec now);
 extern int update_persistent_clock64(struct timespec64 now);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2f10b65..90ed5db 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1188,28 +1188,20 @@ void __weak read_persistent_clock64(struct timespec64 *ts64)
 }
 
 /**
- * read_boot_clock -  Return time of the system start.
+ * read_boot_clock64 -  Return time of the system start.
  *
  * Weak dummy function for arches that do not yet support it.
  * Function to read the exact time the system has been started.
- * Returns a timespec with tv_sec=0 and tv_nsec=0 if unsupported.
+ * Returns a timespec64 with tv_sec=0 and tv_nsec=0 if unsupported.
  *
  *  XXX - Do be sure to remove it once all arches implement it.
  */
-void __weak read_boot_clock(struct timespec *ts)
+void __weak read_boot_clock64(struct timespec64 *ts)
 {
 	ts->tv_sec = 0;
 	ts->tv_nsec = 0;
 }
 
-void __weak read_boot_clock64(struct timespec64 *ts64)
-{
-	struct timespec ts;
-
-	read_boot_clock(&ts);
-	*ts64 = timespec_to_timespec64(ts);
-}
-
 /* Flag for if timekeeping_resume() has injected sleeptime */
 static bool sleeptime_injected;
 
-- 
1.9.1

Re: [PATCH 7/7] time: Remove read_boot_clock()

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> From: Xunlei Pang <pang.xunlei@linaro.org>
> 
> Now we have all the read_boot_clock64() for all implementations,
> it's time to remove read_boot_clock() completely from the kernel.

This sentence does not parse for me. Did you want to say:

  Now that we have a read_boot_clock64() function available on every
  architecture, and converted all the users to it, it's time to remove 
  the (now unused) read_boot_clock() completely from the kernel.

?

Thanks,

	Ingo

Re: [PATCH 0/7] Current queue for tip/timers/core

[John Stultz]

On Wed, May 20, 2015 at 10:19 AM, John Stultz <john.stultz@linaro.org> wrote:
> Hey Thomas, Ingo,
>   Just wanted to send you my current queue of items that I
> have pending for tip/timers/core for 4.2
>
> Let me know if you have any concerns or objections.
>
> thanks
> -john
>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Ingo Molnar <mingo@kernel.org>
>
> Badhri Jagan Sridharan (1):
>   tracing: timer: Add deferrable flag to timer_start
>
> Harald Geyer (1):
>   timekeeping: Provide new API to get the current time resolution
>
> John Stultz (1):
>   time: Rework debugging variables so they aren't global
>
> Sasha Levin (1):
>   time: make sure tz_minuteswest is set to a valid value when setting
>     time
>
> Xunlei Pang (3):
>   time: include math64.h in time64.h
>   s390: time: Provide read_boot_clock64() and read_persistent_clock64()
>   time: Remove read_boot_clock()
>
>  arch/s390/include/asm/timex.h       |  5 +--
>  arch/s390/kernel/debug.c            | 11 ++++---
>  arch/s390/kernel/time.c             |  6 ++--
>  include/linux/time64.h              |  1 +
>  include/linux/timekeeper_internal.h | 15 +++++++++
>  include/linux/timekeeping.h         |  2 +-
>  include/trace/events/timer.h        | 13 +++++---
>  kernel/time/time.c                  |  4 +++
>  kernel/time/timekeeping.c           | 64 ++++++++++++++++++-------------------
>  kernel/time/timer.c                 |  3 +-
>  10 files changed, 75 insertions(+), 49 deletions(-)
>
> --
> 1.9.1
>

And if desired as a pull-request:
The following changes since commit 4e3d9cb0134fea035e6eb1707e5e7d8aaffa186d:

  jiffies: Remove the extra indentation level (2015-05-19 17:17:34 +0200)

are available in the git repository at:

  https://git.linaro.org/people/john.stultz/linux.git fortglx/4.2/time

for you to fetch changes up to e84e65281f6225f89923d8572ba13492bb41a353:

  time: Remove read_boot_clock() (2015-05-20 10:18:17 -0700)


thanks
-john

Re: [PATCH 0/7] Current queue for tip/timers/core

[Ingo Molnar]

* John Stultz <john.stultz@linaro.org> wrote:

> Hey Thomas, Ingo,
>   Just wanted to send you my current queue of items that I
> have pending for tip/timers/core for 4.2
> 
> Let me know if you have any concerns or objections.
> 
> thanks
> -john
> 
>   tracing: timer: Add deferrable flag to timer_start
>   timekeeping: Provide new API to get the current time resolution
>   time: Rework debugging variables so they aren't global
>   time: make sure tz_minuteswest is set to a valid value when setting time
>   time: include math64.h in time64.h
>   s390: time: Provide read_boot_clock64() and read_persistent_clock64()
>   time: Remove read_boot_clock()

Please use consistent capitalization in patch titles, i.e.:

   s/make sure/
     Make sure

   s/include/
     Include

Thanks,

	Ingo