[PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Heena Sirwani]

The following patch adds a function to return tv_sec portion of
ktime_get_ts64() function in order to have a function that returns
seconds as 64-bit integers instead of 32-bit integers to address the
y2038 problem.

Since we are interested only in the seconds portion of ktime_get_ts64()
and require this to be as fast as possible, we take the implementation
of ktime_get_ts64() as it is and remove everything pertaining to the
nanoseconds portion. We only add to the seconds calculation if the
calculation of nanoseconds is more than one billion nanoseconds. We
replace timekeeping_get_ns() which returns the exact nanoseconds value
with tk_xtime() that returns nanosecond value of the last timer tick
since we do not require such fine resolution for nanoseconds portion but
want the seconds to be returned as fast as possible.

Signed-off-by: Heena Sirwani <heenasirwani@gmail.com>
---
Changes in v5:
        - Made the commit message more clear.
	- Changed the type of variable ts from timespec64 to time64_t as
	  we do not use the tv_nsec portion.
	- Changed the type of nsec from s64 to s32.
	- replaced the value 1000000000 with NSEC_PER_SEC.

 include/linux/timekeeping.h |    1 +
 kernel/time/timekeeping.c   |   25 +++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 1caa6b0..115d55e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -28,6 +28,7 @@ struct timespec __current_kernel_time(void);
 struct timespec get_monotonic_coarse(void);
 extern void getrawmonotonic(struct timespec *ts);
 extern void ktime_get_ts64(struct timespec64 *ts);
+extern time64_t ktime_get_seconds(void);
 
 extern int __getnstimeofday64(struct timespec64 *tv);
 extern void getnstimeofday64(struct timespec64 *tv);
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ec1791f..93fc596 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -648,6 +648,31 @@ void ktime_get_ts64(struct timespec64 *ts)
 }
 EXPORT_SYMBOL_GPL(ktime_get_ts64);
 
+time64_t ktime_get_seconds(void)
+{
+	time64_t ts;
+	struct timekeeper *tk = &tk_core.timekeeper;
+	struct timespec64 tomono;
+	s32 nsec;
+	unsigned int seq;
+
+	WARN_ON(timekeeping_suspended);
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		ts = tk->xtime_sec;
+		nsec = (long)(tk->tkr.xtime_nsec >> tk->tkr.shift);
+		tomono = tk->wall_to_monotonic;
+
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	ts += tomono.tv_sec;
+	if (nsec + tomono.tv_nsec >= NSEC_PER_SEC)
+		ts += 1;
+	return ts;
+}
+EXPORT_SYMBOL_GPL(ktime_get_seconds);
+
 #ifdef CONFIG_NTP_PPS
 
 /**
-- 
1.7.9.5

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Thomas Gleixner]

On Sat, 25 Oct 2014, Heena Sirwani wrote:
> +time64_t ktime_get_seconds(void)
> +{
> +	time64_t ts;
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	struct timespec64 tomono;
> +	s32 nsec;
> +	unsigned int seq;
> +
> +	WARN_ON(timekeeping_suspended);
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		ts = tk->xtime_sec;
> +		nsec = (long)(tk->tkr.xtime_nsec >> tk->tkr.shift);
> +		tomono = tk->wall_to_monotonic;
> +
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	ts += tomono.tv_sec;
> +	if (nsec + tomono.tv_nsec >= NSEC_PER_SEC)
> +		ts += 1;
> +	return ts;

I'd rather have an extra field in the timekeeper

    u64 xtime_sec;
+   u64 ktime_sec;

and update this in tk_update_ktime_data() so the readout function
boils down to

time64_t ktime_get_seconds(void)
{
#if BITS_PER_LONG < 64
	u64 sec;
	int seq;

	do {
		seq = read_seqcount_begin(&tk_core.seq);
		sec = tk->ktime_sec;
	} while (read_seqcount_retry(&tk_core.seq, seq));

	return sec;
#else
	return tk->ktime_sec;
#endif
}

So 64bit can do w/o the seqcount and 32bit avoids all extra math, right?

Thanks,

	tglx

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Thomas Gleixner]

On Sat, 25 Oct 2014, Thomas Gleixner wrote:
> On Sat, 25 Oct 2014, Heena Sirwani wrote:
> > +time64_t ktime_get_seconds(void)
> > +{
> > +	time64_t ts;
> > +	struct timekeeper *tk = &tk_core.timekeeper;
> > +	struct timespec64 tomono;
> > +	s32 nsec;
> > +	unsigned int seq;
> > +
> > +	WARN_ON(timekeeping_suspended);
> > +
> > +	do {
> > +		seq = read_seqcount_begin(&tk_core.seq);
> > +		ts = tk->xtime_sec;
> > +		nsec = (long)(tk->tkr.xtime_nsec >> tk->tkr.shift);
> > +		tomono = tk->wall_to_monotonic;
> > +
> > +	} while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > +	ts += tomono.tv_sec;
> > +	if (nsec + tomono.tv_nsec >= NSEC_PER_SEC)
> > +		ts += 1;
> > +	return ts;
> 
> I'd rather have an extra field in the timekeeper
> 
>     u64 xtime_sec;
> +   u64 ktime_sec;
> 
> and update this in tk_update_ktime_data() so the readout function
> boils down to
> 
> time64_t ktime_get_seconds(void)
> {
> #if BITS_PER_LONG < 64
> 	u64 sec;
> 	int seq;
> 
> 	do {
> 		seq = read_seqcount_begin(&tk_core.seq);
> 		sec = tk->ktime_sec;
> 	} while (read_seqcount_retry(&tk_core.seq, seq));
> 
> 	return sec;
> #else
> 	return tk->ktime_sec;
> #endif
> }
> 
> So 64bit can do w/o the seqcount and 32bit avoids all extra math, right?

Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
32bit value is plenty enough unless we care about systems with more
than 136 years uptime. So if we calculate the seconds value of ktime,
i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
32 and 64bit w/o the seqcount loop.

Where we really need the above readout mechanism is get_seconds() as
that will break in 2038 on 32bit. So there you need to change the
return value from unsigned long to time64_t and change the
implementation as above just xtime_sec instead of ktime_sec.

Thanks,

	tglx

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Arnd Bergmann]

On Saturday 25 October 2014 17:22:23 Thomas Gleixner wrote:
> On Sat, 25 Oct 2014, Thomas Gleixner wrote:
> > On Sat, 25 Oct 2014, Heena Sirwani wrote:
> > > +time64_t ktime_get_seconds(void)
> > > +{
> > > +   time64_t ts;
> > > +   struct timekeeper *tk = &tk_core.timekeeper;
> > > +   struct timespec64 tomono;
> > > +   s32 nsec;
> > > +   unsigned int seq;
> > > +
> > > +   WARN_ON(timekeeping_suspended);
> > > +
> > > +   do {
> > > +           seq = read_seqcount_begin(&tk_core.seq);
> > > +           ts = tk->xtime_sec;
> > > +           nsec = (long)(tk->tkr.xtime_nsec >> tk->tkr.shift);
> > > +           tomono = tk->wall_to_monotonic;
> > > +
> > > +   } while (read_seqcount_retry(&tk_core.seq, seq));
> > > +
> > > +   ts += tomono.tv_sec;
> > > +   if (nsec + tomono.tv_nsec >= NSEC_PER_SEC)
> > > +           ts += 1;
> > > +   return ts;
> > 
> > I'd rather have an extra field in the timekeeper
> > 
> >     u64 xtime_sec;
> > +   u64 ktime_sec;
> > 
> > and update this in tk_update_ktime_data() so the readout function
> > boils down to
> > 
> > time64_t ktime_get_seconds(void)
> > {
> > #if BITS_PER_LONG < 64
> >       u64 sec;
> >       int seq;
> > 
> >       do {
> >               seq = read_seqcount_begin(&tk_core.seq);
> >               sec = tk->ktime_sec;
> >       } while (read_seqcount_retry(&tk_core.seq, seq));
> > 
> >       return sec;
> > #else
> >       return tk->ktime_sec;
> > #endif
> > }
> > 
> > So 64bit can do w/o the seqcount and 32bit avoids all extra math, right?
> 
> Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
> 32bit value is plenty enough unless we care about systems with more
> than 136 years uptime. So if we calculate the seconds value of ktime,
> i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
> 32 and 64bit w/o the seqcount loop.

Ah, very good point. That opens the question which type that function
should return. I really want to remove all uses of time_t from the
kernel, mostly so we know when we're done with this. However as you
say we know that we only need a 32-bit value here. Some possible
ideas:

- use time64_t here anyway and accept the slight inefficiency in return
  for clarity
- introduce a monotonic_time_t (we probably also want a struct
  monotonic_timespec if we do that) which is basically the old time_t
  but is known to be y2038 safe because we only ever use it to store
  monotonic times.
- return u32 and use the same type in the callers instead of
  time_t/time64_t/monotonic_time_t.

> Where we really need the above readout mechanism is get_seconds() as
> that will break in 2038 on 32bit. So there you need to change the
> return value from unsigned long to time64_t and change the
> implementation as above just xtime_sec instead of ktime_sec.

Heena already posted a first draft of that patch to the opw internal
mailing list, I found a small issue that needs to be resolved and
then she can post the new version to you for review.

	Arnd

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Thomas Gleixner]

On Sat, 25 Oct 2014, Arnd Bergmann wrote:
> On Saturday 25 October 2014 17:22:23 Thomas Gleixner wrote:
> > Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
> > 32bit value is plenty enough unless we care about systems with more
> > than 136 years uptime. So if we calculate the seconds value of ktime,
> > i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
> > 32 and 64bit w/o the seqcount loop.
> 
> Ah, very good point. That opens the question which type that function
> should return. I really want to remove all uses of time_t from the
> kernel, mostly so we know when we're done with this. However as you
> say we know that we only need a 32-bit value here. Some possible
> ideas:
> 
> - use time64_t here anyway and accept the slight inefficiency in return
>   for clarity

Probably the simplest option.

> - introduce a monotonic_time_t (we probably also want a struct
>   monotonic_timespec if we do that) which is basically the old time_t
>   but is known to be y2038 safe because we only ever use it to store
>   monotonic times.

Not sure whether its worth the trouble.

> - return u32 and use the same type in the callers instead of
>   time_t/time64_t/monotonic_time_t.

Works as well. I have no immediate preference.
 
> > Where we really need the above readout mechanism is get_seconds() as
> > that will break in 2038 on 32bit. So there you need to change the
> > return value from unsigned long to time64_t and change the
> > implementation as above just xtime_sec instead of ktime_sec.
> 
> Heena already posted a first draft of that patch to the opw internal
> mailing list, I found a small issue that needs to be resolved and
> then she can post the new version to you for review.

Great!

Thanks,

	tglx

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Arnd Bergmann]

On Saturday 25 October 2014 19:32:09 Thomas Gleixner wrote:
> From:	Thomas Gleixner <tglx@linutronix.de>
> To:	Arnd Bergmann <arnd@arndb.de>
> CC:	Heena Sirwani <heenasirwani@gmail.com>, linux-kernel@vger.kernel.org, john.stultz@linaro.org
> Date:	Yesterday 19:32:09
> On Sat, 25 Oct 2014, Arnd Bergmann wrote:
> > On Saturday 25 October 2014 17:22:23 Thomas Gleixner wrote:
> > > Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
> > > 32bit value is plenty enough unless we care about systems with more
> > > than 136 years uptime. So if we calculate the seconds value of ktime,
> > > i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
> > > 32 and 64bit w/o the seqcount loop.
> > 
> > Ah, very good point. That opens the question which type that function
> > should return. I really want to remove all uses of time_t from the
> > kernel, mostly so we know when we're done with this. However as you
> > say we know that we only need a 32-bit value here. Some possible
> > ideas:
> > 
> > - use time64_t here anyway and accept the slight inefficiency in return
> >   for clarity
> 
> Probably the simplest option.
> 
> > - introduce a monotonic_time_t (we probably also want a struct
> >   monotonic_timespec if we do that) which is basically the old time_t
> >   but is known to be y2038 safe because we only ever use it to store
> >   monotonic times.
> 
> Not sure whether its worth the trouble.

We have around 20 drivers using ktime_get_ts() or getrawmonotonic().
If we had a 'struct monotonic_timespec', we could trivially convert
them, otherwise we have to look at each one individually to figure
out what they should use, in particular if they would have noticeably
worse performance by moving to ktime_get_ts64 or getrawmonotonic64.

The way to do that would be to add 

#define monotonic_timespec timespec

now, and rename timespec to monotonic_timespec after we are done
converting all other in-kernel users of timespec. There are probably
a few drivers that today use do_gettimeofday or getnstimeofday that
could be converted to use ktime_get_ts using a 32-bit
monotonic_timespec.

The alternative to that would be to make them all use ktime_t, which
might be more efficient but also more work to do.

> > - return u32 and use the same type in the callers instead of
> >   time_t/time64_t/monotonic_time_t.
> 
> Works as well. I have no immediate preference.

I think I like the u32 approach better than ktime_t, but it's a
very mild preference. I'll wait for your reply on the monotonic_time_t/
monotonic_timespec comments above. Maybe John has an opinion as well.

Heena, could you prepare a new patch using u32 once we have worked out
the ktime_sec implementation?

	Arnd

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Arnd Bergmann]

On Saturday 25 October 2014 17:12:48 Thomas Gleixner wrote:
> I'd rather have an extra field in the timekeeper
> 
>     u64 xtime_sec;
> +   u64 ktime_sec;
> 
> and update this in tk_update_ktime_data() so the readout function
> boils down to
> 
> time64_t ktime_get_seconds(void)
> {
> #if BITS_PER_LONG < 64
>         u64 sec;
>         int seq;
> 
>         do {
>                 seq = read_seqcount_begin(&tk_core.seq);
>                 sec = tk->ktime_sec;
>         } while (read_seqcount_retry(&tk_core.seq, seq));
> 
>         return sec;
> #else
>         return tk->ktime_sec;
> #endif
> }
> 
> So 64bit can do w/o the seqcount and 32bit avoids all extra math, right?
> 

But where would you set ktime_sec? My first thought was tk_update_ktime_data(),
but would then either be slightly wrong because it doesn't take the current
xtime_nsec + tk->wall_to_monotonic.tv_nsec into account, if we do

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ec1791fae965..d72a456c6bd3 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -426,8 +426,8 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
 	 *	nsec = base_mono + now();
 	 * ==> base_mono = (xtime_sec + wtm_sec) * 1e9 + wtm_nsec
 	 */
-	nsec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
-	nsec *= NSEC_PER_SEC;
+	tk->ktime_sec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
+	nsec = tk->ktime_sec * NSEC_PER_SEC;
 	nsec += tk->wall_to_monotonic.tv_nsec;
 	tk->tkr.base_mono = ns_to_ktime(nsec);
 

or we'd have to add an expensive timekeeping_get_ns() call:

diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index ec1791fae965..d4c95b815de6 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -426,9 +426,11 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
 	 *	nsec = base_mono + now();
 	 * ==> base_mono = (xtime_sec + wtm_sec) * 1e9 + wtm_nsec
 	 */
-	nsec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
-	nsec *= NSEC_PER_SEC;
-	nsec += tk->wall_to_monotonic.tv_nsec;
+	tk->ktime_sec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
+	nsec = tk->wall_to_monotonic.tv_nsec;
+	if ((nsec + timekeeping_get_ns()) > NSEC_PER_SEC)
+		tk->ktime_sec += 1;
+	nsec += tk->ktime_sec * NSEC_PER_SEC;
 	tk->tkr.base_mono = ns_to_ktime(nsec);
 
 	/* Update the monotonic raw base */

	Arnd

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Thomas Gleixner]

On Sun, 26 Oct 2014, Arnd Bergmann wrote:
> On Saturday 25 October 2014 17:12:48 Thomas Gleixner wrote:
> > I'd rather have an extra field in the timekeeper
> > 
> >     u64 xtime_sec;
> > +   u64 ktime_sec;
> > 
> > and update this in tk_update_ktime_data() so the readout function
> > boils down to
> > 
> > time64_t ktime_get_seconds(void)
> > {
> > #if BITS_PER_LONG < 64
> >         u64 sec;
> >         int seq;
> > 
> >         do {
> >                 seq = read_seqcount_begin(&tk_core.seq);
> >                 sec = tk->ktime_sec;
> >         } while (read_seqcount_retry(&tk_core.seq, seq));
> > 
> >         return sec;
> > #else
> >         return tk->ktime_sec;
> > #endif
> > }
> > 
> > So 64bit can do w/o the seqcount and 32bit avoids all extra math, right?
> > 
> 
> But where would you set ktime_sec? My first thought was tk_update_ktime_data(),
> but would then either be slightly wrong because it doesn't take the current
> xtime_nsec + tk->wall_to_monotonic.tv_nsec into account, if we do
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index ec1791fae965..d72a456c6bd3 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -426,8 +426,8 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
>  	 *	nsec = base_mono + now();
>  	 * ==> base_mono = (xtime_sec + wtm_sec) * 1e9 + wtm_nsec
>  	 */
> -	nsec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
> -	nsec *= NSEC_PER_SEC;
> +	tk->ktime_sec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
> +	nsec = tk->ktime_sec * NSEC_PER_SEC;
>  	nsec += tk->wall_to_monotonic.tv_nsec;
>  	tk->tkr.base_mono = ns_to_ktime(nsec);
>  
> 
> or we'd have to add an expensive timekeeping_get_ns() call:
> 
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index ec1791fae965..d4c95b815de6 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -426,9 +426,11 @@ static inline void tk_update_ktime_data(struct timekeeper *tk)
>  	 *	nsec = base_mono + now();
>  	 * ==> base_mono = (xtime_sec + wtm_sec) * 1e9 + wtm_nsec
>  	 */
> -	nsec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
> -	nsec *= NSEC_PER_SEC;
> -	nsec += tk->wall_to_monotonic.tv_nsec;
> +	tk->ktime_sec = (s64)(tk->xtime_sec + tk->wall_to_monotonic.tv_sec);
> +	nsec = tk->wall_to_monotonic.tv_nsec;
> +	if ((nsec + timekeeping_get_ns()) > NSEC_PER_SEC)
> +		tk->ktime_sec += 1;
> +	nsec += tk->ktime_sec * NSEC_PER_SEC;
>  	tk->tkr.base_mono = ns_to_ktime(nsec);

No. Why would you do that? Look at the math you implemented and just
do the same thing in the update function.

Thanks,

	tglx

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Thomas Gleixner]

On Sun, 26 Oct 2014, Arnd Bergmann wrote:
> On Saturday 25 October 2014 19:32:09 Thomas Gleixner wrote:
> > On Sat, 25 Oct 2014, Arnd Bergmann wrote:
> > > On Saturday 25 October 2014 17:22:23 Thomas Gleixner wrote:
> > > > Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
> > > > 32bit value is plenty enough unless we care about systems with more
> > > > than 136 years uptime. So if we calculate the seconds value of ktime,
> > > > i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
> > > > 32 and 64bit w/o the seqcount loop.
> > > 
> > > Ah, very good point. That opens the question which type that function
> > > should return. I really want to remove all uses of time_t from the
> > > kernel, mostly so we know when we're done with this. However as you
> > > say we know that we only need a 32-bit value here. Some possible
> > > ideas:
> > > 
> > > - use time64_t here anyway and accept the slight inefficiency in return
> > >   for clarity
> > 
> > Probably the simplest option.
> > 
> > > - introduce a monotonic_time_t (we probably also want a struct
> > >   monotonic_timespec if we do that) which is basically the old time_t
> > >   but is known to be y2038 safe because we only ever use it to store
> > >   monotonic times.
> > 
> > Not sure whether its worth the trouble.
> 
> We have around 20 drivers using ktime_get_ts() or getrawmonotonic().
> If we had a 'struct monotonic_timespec', we could trivially convert
> them, otherwise we have to look at each one individually to figure
> out what they should use, in particular if they would have noticeably
> worse performance by moving to ktime_get_ts64 or getrawmonotonic64.
> 
> The way to do that would be to add 
> 
> #define monotonic_timespec timespec
> 
> now, and rename timespec to monotonic_timespec after we are done
> converting all other in-kernel users of timespec. There are probably

You can't do that because timespec will have to stay for the "compat"
syscalls.

> a few drivers that today use do_gettimeofday or getnstimeofday that
> could be converted to use ktime_get_ts using a 32-bit
> monotonic_timespec.
> 
> The alternative to that would be to make them all use ktime_t, which
> might be more efficient but also more work to do.

If you touch a file anyway then you better chose the best
solution. The only reason why you want to use the less work option is
if you can do a conversion scripted w/o actually analyzing each
changed file deeply.

> > > - return u32 and use the same type in the callers instead of
> > >   time_t/time64_t/monotonic_time_t.
> > 
> > Works as well. I have no immediate preference.
> 
> I think I like the u32 approach better than ktime_t, but it's a
> very mild preference. I'll wait for your reply on the monotonic_time_t/
> monotonic_timespec comments above. Maybe John has an opinion as well.

You really should look at the call sites and judge based on the
requirements of those.

Thanks,

	tglx

Re: [PATCH v5] timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

[Arnd Bergmann]

On Tuesday 28 October 2014 12:26:15 Thomas Gleixner wrote:
> On Sun, 26 Oct 2014, Arnd Bergmann wrote:
> > On Saturday 25 October 2014 19:32:09 Thomas Gleixner wrote:
> > > On Sat, 25 Oct 2014, Arnd Bergmann wrote:
> > > > On Saturday 25 October 2014 17:22:23 Thomas Gleixner wrote:
> > > > > Hmm. Thinking more about it. That's actually overkill. For ktime_sec a
> > > > > 32bit value is plenty enough unless we care about systems with more
> > > > > than 136 years uptime. So if we calculate the seconds value of ktime,
> > > > > i.e. CLOCK_MONOTONIC, in the update function, we can read it on both
> > > > > 32 and 64bit w/o the seqcount loop.
> > > > 
> > > > Ah, very good point. That opens the question which type that function
> > > > should return. I really want to remove all uses of time_t from the
> > > > kernel, mostly so we know when we're done with this. However as you
> > > > say we know that we only need a 32-bit value here. Some possible
> > > > ideas:
> > > > 
> > > > - use time64_t here anyway and accept the slight inefficiency in return
> > > >   for clarity
> > > 
> > > Probably the simplest option.
> > > 
> > > > - introduce a monotonic_time_t (we probably also want a struct
> > > >   monotonic_timespec if we do that) which is basically the old time_t
> > > >   but is known to be y2038 safe because we only ever use it to store
> > > >   monotonic times.
> > > 
> > > Not sure whether its worth the trouble.
> > 
> > We have around 20 drivers using ktime_get_ts() or getrawmonotonic().
> > If we had a 'struct monotonic_timespec', we could trivially convert
> > them, otherwise we have to look at each one individually to figure
> > out what they should use, in particular if they would have noticeably
> > worse performance by moving to ktime_get_ts64 or getrawmonotonic64.
> > 
> > The way to do that would be to add 
> > 
> > #define monotonic_timespec timespec
> > 
> > now, and rename timespec to monotonic_timespec after we are done
> > converting all other in-kernel users of timespec. There are probably
> 
> You can't do that because timespec will have to stay for the "compat"
> syscalls.

My plan is to change those to use compat_timespec even on 32-bit systems,
so we have a shared implementation for 32-bit time syscalls on both 32-bit
native compat-time and 64-bit compat compat-time ABIs.

> > a few drivers that today use do_gettimeofday or getnstimeofday that
> > could be converted to use ktime_get_ts using a 32-bit
> > monotonic_timespec.
> > 
> > The alternative to that would be to make them all use ktime_t, which
> > might be more efficient but also more work to do.
> 
> If you touch a file anyway then you better chose the best
> solution. The only reason why you want to use the less work option is
> if you can do a conversion scripted w/o actually analyzing each
> changed file deeply.
>
> > > > - return u32 and use the same type in the callers instead of
> > > >   time_t/time64_t/monotonic_time_t.
> > > 
> > > Works as well. I have no immediate preference.
> > 
> > I think I like the u32 approach better than ktime_t, but it's a
> > very mild preference. I'll wait for your reply on the monotonic_time_t/
> > monotonic_timespec comments above. Maybe John has an opinion as well.
> 
> You really should look at the call sites and judge based on the
> requirements of those.


I looked at most users of do_gettimeofday and getnstimeofday now. About
half of them actually want wall-clock and need 64-bit time_t.

The others seem to all be in the category that should be converted to
ktime_t anyway. Let's forget about monotonic_timespec then.

One interface that we may want to add however is something that either
provides the difference between two ktime_t in milliseconds/microseconds/
nanoseconds, or provides the elapsed time since a previous ktime_t.

One or more of these

static inline s64 ktime_sub_divns(ktime_t new, ktime_t old, unsigned long unit)
{
	return ktime_divns(ktime_sub(new, old), unit);
}

static inline s64 ktime_diff_ms(ktime_t new, ktime_t old)
{
	return ktime_divns(ktime_sub(new, old), MSEC_PER_NSEC);
}

static inline s64 ktime_diff_us(ktime_t new, ktime_t old)
{
	return ktime_divns(ktime_sub(new, old), USEC_PER_NSEC);
}

static inline s64 ktime_expired_us(ktime_t old)
{
	return ktime_divns(ktime_sub(ktime_get(), old), USEC_PER_NSEC);
}

After some more discussion with Heena, I actually realized that we
might not need a new get_seconds() replacement at all: get_seconds()
returns an "unsigned long" number of seconds, and the current definition
means that it's good until 2106, not just 2038.

So my current line of thinking now is that we should not even bother
introducing ktime_get_real_seconds() but instead only audit the users
of get_seconds() to ensure they don't assign the returned value to
a time_t but use either time64_t or unsigned long as the local variable.

For consistency, ktime_get_seconds() should then also return
'unsigned long'.

	Arnd