[PATCH v2] timekeeping: Added a function to return tv_sec portion of ktime_get_real_ts64()

[PATCH v2] timekeeping: Added a function to return tv_sec portion of ktime_get_real_ts64()

[Heena Sirwani]

The following patch adds a function to return tv_sec portion of
ktime_get_real_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.

The function is similar to get_seconds() function except that it
includes read_seqcount_retry loop which is required for 32-bit
architectures. This is because 32-bit machines cannot access 64-bit
tk->xtime_sec variable atomically, requiring two 32-bit register loads.

Signed-off-by: Heena Sirwani <heenasirwani@gmail.com>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>
---
Changes in v2:
        - Removed the local variable nsecs as it was not required.

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

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 115d55e..91454de 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -29,6 +29,7 @@ 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 time64_t ktime_get_real_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 93fc596..c17c620 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -673,6 +673,25 @@ time64_t ktime_get_seconds(void)
 }
 EXPORT_SYMBOL_GPL(ktime_get_seconds);
 
+time64_t ktime_get_real_seconds(void)
+{
+	time64_t seconds;
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned int seq;
+
+	if (IS_ENABLED(CONFIG_64BIT))
+		return tk->xtime_sec;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		seconds = tk->xtime_sec;
+
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return seconds;
+}
+EXPORT_SYMBOL_GPL(ktime_get_real_seconds);
+
 #ifdef CONFIG_NTP_PPS
 
 /**
-- 
1.9.1

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

[Thomas Gleixner]

On Mon, 27 Oct 2014, Heena Sirwani wrote:
> The following patch adds a function to return tv_sec portion of
> ktime_get_real_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.
> 
> The function is similar to get_seconds() function except that it
> includes read_seqcount_retry loop which is required for 32-bit
> architectures. This is because 32-bit machines cannot access 64-bit
> tk->xtime_sec variable atomically, requiring two 32-bit register loads.
>  
> Signed-off-by: Heena Sirwani <heenasirwani@gmail.com>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> ---
> Changes in v2:
>         - Removed the local variable nsecs as it was not required.
> 
>  include/linux/timekeeping.h |  1 +
>  kernel/time/timekeeping.c   | 19 +++++++++++++++++++
>  2 files changed, 20 insertions(+)
> 
> diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
> index 115d55e..91454de 100644
> --- a/include/linux/timekeeping.h
> +++ b/include/linux/timekeeping.h
> @@ -29,6 +29,7 @@ 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 time64_t ktime_get_real_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 93fc596..c17c620 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -673,6 +673,25 @@ time64_t ktime_get_seconds(void)
>  }
>  EXPORT_SYMBOL_GPL(ktime_get_seconds);
>  
> +time64_t ktime_get_real_seconds(void)
> +{
> +	time64_t seconds;
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned int seq;
> +
> +	if (IS_ENABLED(CONFIG_64BIT))
> +		return tk->xtime_sec;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		seconds = tk->xtime_sec;
> +
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return seconds;
> +}
> +EXPORT_SYMBOL_GPL(ktime_get_real_seconds);

Nice and clean implementation! Though I wonder whether we should just
name it get_seconds64().

Thanks,

	tglx

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

[Arnd Bergmann]

On Tuesday 28 October 2014 16:43:42 Thomas Gleixner wrote:
> >  
> > +time64_t ktime_get_real_seconds(void)
> > +{
> > +     time64_t seconds;
> > +     struct timekeeper *tk = &tk_core.timekeeper;
> > +     unsigned int seq;
> > +
> > +     if (IS_ENABLED(CONFIG_64BIT))
> > +             return tk->xtime_sec;
> > +
> > +     do {
> > +             seq = read_seqcount_begin(&tk_core.seq);
> > +             seconds = tk->xtime_sec;
> > +
> > +     } while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > +     return seconds;
> > +}
> > +EXPORT_SYMBOL_GPL(ktime_get_real_seconds);
> 
> Nice and clean implementation! Though I wonder whether we should just
> name it get_seconds64().
> 

I don't have a strong opinion here, I suggested ktime_get_real_seconds()
for consistency with ktime_get_real_ts64(), but get_seconds64() would
make as much sense.

As I mentioned in my other reply, we have also concluded that returning
'unsigned long' from get_seconds() at the moment is actually not a
problem for y2038 because it will do the right until 2106 by returning
the unsigned lower 32-bit of the correct 64-bit number, so we might
not actually need this one.

I also don't have a strong opinion on this matter, adding it would
make it easier for developers to pick get_seconds64/ktime_get_real_ts64()
and understand that it's correct without having to know the finer
details of the time_t/ulong distinction.

	Arnd

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

[Thomas Gleixner]

On Tue, 28 Oct 2014, Arnd Bergmann wrote:
> On Tuesday 28 October 2014 16:43:42 Thomas Gleixner wrote:
> > >  
> > > +time64_t ktime_get_real_seconds(void)
> > > +{
> > > +     time64_t seconds;
> > > +     struct timekeeper *tk = &tk_core.timekeeper;
> > > +     unsigned int seq;
> > > +
> > > +     if (IS_ENABLED(CONFIG_64BIT))
> > > +             return tk->xtime_sec;
> > > +
> > > +     do {
> > > +             seq = read_seqcount_begin(&tk_core.seq);
> > > +             seconds = tk->xtime_sec;
> > > +
> > > +     } while (read_seqcount_retry(&tk_core.seq, seq));
> > > +
> > > +     return seconds;
> > > +}
> > > +EXPORT_SYMBOL_GPL(ktime_get_real_seconds);
> > 
> > Nice and clean implementation! Though I wonder whether we should just
> > name it get_seconds64().
> > 
> 
> I don't have a strong opinion here, I suggested ktime_get_real_seconds()
> for consistency with ktime_get_real_ts64(), but get_seconds64() would
> make as much sense.
> 
> As I mentioned in my other reply, we have also concluded that returning
> 'unsigned long' from get_seconds() at the moment is actually not a
> problem for y2038 because it will do the right until 2106 by returning
> the unsigned lower 32-bit of the correct 64-bit number, so we might
> not actually need this one.

Well, the issue is that some of the use cases feed it into a time_t...

I think we should convert all in kernel users to get_seconds64 and get
rid of get_seconds. The few cases which work until 2016 can do with
the truncated value.
 
> I also don't have a strong opinion on this matter, adding it would
> make it easier for developers to pick get_seconds64/ktime_get_real_ts64()
> and understand that it's correct without having to know the finer
> details of the time_t/ulong distinction.

Right. I really want to convert all kernel time interfaces to the 64
postfix and remove the old interfaces. No point in changing the names
back. That also has the advantage that for functions which are similar
in user space we have a clear distinction.

Thanks,

	tglx

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

[Arnd Bergmann]

On Tuesday 28 October 2014 18:13:09 Thomas Gleixner wrote:
> On Tue, 28 Oct 2014, Arnd Bergmann wrote:
> > On Tuesday 28 October 2014 16:43:42 Thomas Gleixner wrote:
> > > >  
> > > > +time64_t ktime_get_real_seconds(void)
> > > > +{
> > > > +     time64_t seconds;
> > > > +     struct timekeeper *tk = &tk_core.timekeeper;
> > > > +     unsigned int seq;
> > > > +
> > > > +     if (IS_ENABLED(CONFIG_64BIT))
> > > > +             return tk->xtime_sec;
> > > > +
> > > > +     do {
> > > > +             seq = read_seqcount_begin(&tk_core.seq);
> > > > +             seconds = tk->xtime_sec;
> > > > +
> > > > +     } while (read_seqcount_retry(&tk_core.seq, seq));
> > > > +
> > > > +     return seconds;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(ktime_get_real_seconds);
> > > 
> > > Nice and clean implementation! Though I wonder whether we should just
> > > name it get_seconds64().
> > > 
> > 
> > I don't have a strong opinion here, I suggested ktime_get_real_seconds()
> > for consistency with ktime_get_real_ts64(), but get_seconds64() would
> > make as much sense.
> > 
> > As I mentioned in my other reply, we have also concluded that returning
> > 'unsigned long' from get_seconds() at the moment is actually not a
> > problem for y2038 because it will do the right until 2106 by returning
> > the unsigned lower 32-bit of the correct 64-bit number, so we might
> > not actually need this one.
> 
> Well, the issue is that some of the use cases feed it into a time_t...
> 
> I think we should convert all in kernel users to get_seconds64 and get
> rid of get_seconds. The few cases which work until 2016 can do with
> the truncated value.
>
> > I also don't have a strong opinion on this matter, adding it would
> > make it easier for developers to pick get_seconds64/ktime_get_real_ts64()
> > and understand that it's correct without having to know the finer
> > details of the time_t/ulong distinction.
> 
> Right. I really want to convert all kernel time interfaces to the 64
> postfix and remove the old interfaces. No point in changing the names
> back. That also has the advantage that for functions which are similar
> in user space we have a clear distinction.

Ok, fair enough. Let's make both ktime_get_real_seconds() and
ktime_get_seconds() return a time64_t then and use those in new code.

I'd like to apply this patch to my y2038 branch and apply the
other patches that need it on top, unless you want to still submit
it for 3.18.

Regarding the names, should we then have

time64_t ktime_get_real_seconds(void);
time64_t ktime_get_seconds(void);

or

time64_t get_seconds64(void);
time64_t ktime_get_seconds64(void);

If you don't have a preference either way, I guess we can just stay
with the patch that Heena did, otherwise it would be a trivial
change.

	Arnd

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

[Thomas Gleixner]

On Tue, 28 Oct 2014, Arnd Bergmann wrote:
> On Tuesday 28 October 2014 18:13:09 Thomas Gleixner wrote:
> I'd like to apply this patch to my y2038 branch and apply the
> other patches that need it on top, unless you want to still submit
> it for 3.18.

I think I should take these patches for 3.18. We do not create users
yet, but it makes it easier for submaintaners to apply stuff which you
convert. And it lets me have the whole core lot together with other
changes in that area. I create a seperate branch which you can pull
from.

> Regarding the names, should we then have
> 
> time64_t ktime_get_real_seconds(void);
> time64_t ktime_get_seconds(void);

I like that one. Moving all the time interfaces to a ktime_ prefix and
let the default implementation as CLOCK_MONOTONIC and the others with
the added '_real', '_boot' ... describe their CLOCK_WHATEVER property
makes it pretty clear.

Thanks,

	tglx

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

[Arnd Bergmann]

On Tuesday 28 October 2014 21:31:32 Thomas Gleixner wrote:
> On Tue, 28 Oct 2014, Arnd Bergmann wrote:
> > On Tuesday 28 October 2014 18:13:09 Thomas Gleixner wrote:
> > I'd like to apply this patch to my y2038 branch and apply the
> > other patches that need it on top, unless you want to still submit
> > it for 3.18.
> 
> I think I should take these patches for 3.18. We do not create users
> yet, but it makes it easier for submaintaners to apply stuff which you
> convert. And it lets me have the whole core lot together with other
> changes in that area. I create a seperate branch which you can pull
> from.

Ok, sounds good.

> > Regarding the names, should we then have
> > 
> > time64_t ktime_get_real_seconds(void);
> > time64_t ktime_get_seconds(void);
> 
> I like that one. Moving all the time interfaces to a ktime_ prefix and
> let the default implementation as CLOCK_MONOTONIC and the others with
> the added '_real', '_boot' ... describe their CLOCK_WHATEVER property
> makes it pretty clear.

Good. So you can apply this patch as-is and Heena will get you the
other one.

Thanks!

	Arnd

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

[Thomas Gleixner]

On Tue, 28 Oct 2014, Arnd Bergmann wrote:

> On Tuesday 28 October 2014 21:31:32 Thomas Gleixner wrote:
> > On Tue, 28 Oct 2014, Arnd Bergmann wrote:
> > > On Tuesday 28 October 2014 18:13:09 Thomas Gleixner wrote:
> > > I'd like to apply this patch to my y2038 branch and apply the
> > > other patches that need it on top, unless you want to still submit
> > > it for 3.18.
> > 
> > I think I should take these patches for 3.18. We do not create users
> > yet, but it makes it easier for submaintaners to apply stuff which you
> > convert. And it lets me have the whole core lot together with other
> > changes in that area. I create a seperate branch which you can pull
> > from.
> 
> Ok, sounds good.
> 
> > > Regarding the names, should we then have
> > > 
> > > time64_t ktime_get_real_seconds(void);
> > > time64_t ktime_get_seconds(void);
> > 
> > I like that one. Moving all the time interfaces to a ktime_ prefix and
> > let the default implementation as CLOCK_MONOTONIC and the others with
> > the added '_real', '_boot' ... describe their CLOCK_WHATEVER property
> > makes it pretty clear.
> 
> Good. So you can apply this patch as-is and Heena will get you the
> other one.

Not so much.

Applying patch timekeeping-added-a-function-to-return-tv_sec-portion-of-ktime_get_real_ts64.patch
patching file include/linux/timekeeping.h
Hunk #1 FAILED at 29.
1 out of 1 hunk FAILED -- rejects in file include/linux/timekeeping.h
patching file kernel/time/timekeeping.c
Hunk #1 FAILED at 673.
1 out of 1 hunk FAILED -- rejects in file kernel/time/timekeeping.c

That one depends on the other one. So I wait for a combo of both from
Heena.

Thanks,

	tglx