timekeeping: Added a function to return tv_sec portion of ktime_get_ts64()

kernel/time/timekeeping.c +12 -0

+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);

Quoted 12 lines · Heena Sirwani

kernel/time/timekeeping.c +12 -0

+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);

kernel/time/timekeeping.c void ktime_get_ts64(struct timespec64 *ts) · line 672

Quoted 10 lines

kernel/time/timekeeping.c +10 -0

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

Quoted 49 lines · Thomas Gleixner

Quoted 12 lines · Heena Sirwani

kernel/time/timekeeping.c +12 -0

+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);

kernel/time/timekeeping.c void ktime_get_ts64(struct timespec64 *ts) · line 672

Quoted 10 lines

kernel/time/timekeeping.c +10 -0

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

Quoted 1 line

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.

Quoted 5 lines · 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

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

Quoted 4 lines

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.

From: Thomas Gleixner <[email protected]>
To: Arnd Bergmann <[email protected]>
CC: Heena Sirwani <[email protected]>, [email protected], [email protected]
Date: Yesterday 19:32:09

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · 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.

Quoted 8 lines

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.

Quoted 4 lines

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

Quoted 2 lines

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

Quoted 17 lines · On Saturday 25 October 2014 19:32:09 Thomas Gleixner wrote:

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · 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.

Quoted 8 lines

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

Quoted 8 lines

Probably the simplest option.

Quoted 4 lines

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

Quoted 4 lines

Quoted 2 lines

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

Quoted 35 lines · Arnd Bergmann

Quoted 17 lines · On Saturday 25 October 2014 19:32:09 Thomas Gleixner wrote:

Quoted 7 lines · Arnd Bergmann

Quoted 5 lines · 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.

Quoted 8 lines

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

Quoted 8 lines

Probably the simplest option.

Quoted 4 lines

- 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

Quoted 4 lines

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

Quoted 6 lines

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.

Quoted 3 lines

Quoted 3 lines

Quoted 2 lines

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

Quoted 5 lines

Quoted 1 line

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

Quoted 22 lines · 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;

Quoted 5 lines

#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

kernel/time/timekeeping.c +2 -2

index ec1791fae965..d72a456c6bd3 100644
@@ -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:

kernel/time/timekeeping.c +0 -0

index ec1791fae965..d4c95b815de6 100644
@@ -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