Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Tue, Jun 19, 2018 at 5:23 PM, Pavel Tatashin
 wrote:

>> > > +   *ts = (struct timespec64){0, 0};
>>
>> I dunno if additional variable would be better for readability, like
>>
>> struct timespec64 null_ts = {0,0};
>
> I don't mind adding ts_null, but I think, as-is ok here,

Actually I meant presicely null_ts, and the reason why is that alias
is much easy to parse to get the idea, than to read entire "struct bla
bla bla".

But again, this is my personal point of view.

>
>> ...
>> *ts = null_ts;
>>
>> > > +   else
>> > > +   *ts = ns_to_timespec64(ns_now - ns_boot);
>>
>> But I'm fine as long as Thomas is okay with this code.

> Thank you for the review!

You're welcome!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Tue, Jun 19, 2018 at 5:23 PM, Pavel Tatashin
 wrote:

>> > > +   *ts = (struct timespec64){0, 0};
>>
>> I dunno if additional variable would be better for readability, like
>>
>> struct timespec64 null_ts = {0,0};
>
> I don't mind adding ts_null, but I think, as-is ok here,

Actually I meant presicely null_ts, and the reason why is that alias
is much easy to parse to get the idea, than to read entire "struct bla
bla bla".

But again, this is my personal point of view.

>
>> ...
>> *ts = null_ts;
>>
>> > > +   else
>> > > +   *ts = ns_to_timespec64(ns_now - ns_boot);
>>
>> But I'm fine as long as Thomas is okay with this code.

> Thank you for the review!

You're welcome!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

> > > +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 
> > > *ts)
> > > +{
> > > +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> > > +   bool valid_clock;
> > > +   u64 ns_now;
> > > +
> > > +   ns_now = timespec64_to_ns(now);
> > > +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> > > +   (ns_now > ns_boot);
> > > +
> >
>
>
> > > +   if (!valid_clock)
>
> Are we expecting more often clock to be non-valid?
> Perhaps change to positive conditional?

Hi Andy,

Sure, I will change to:
if (valid_clock)
...
else
...

>
> > > +   *ts = (struct timespec64){0, 0};
>
> I dunno if additional variable would be better for readability, like
>
> struct timespec64 null_ts = {0,0};

I don't mind adding ts_null, but I think, as-is ok here,

> ...
> *ts = null_ts;
>
> > > +   else
> > > +   *ts = ns_to_timespec64(ns_now - ns_boot);
>
> But I'm fine as long as Thomas is okay with this code.
>

Thank you for the review!

Pavel

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

> > > +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 
> > > *ts)
> > > +{
> > > +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> > > +   bool valid_clock;
> > > +   u64 ns_now;
> > > +
> > > +   ns_now = timespec64_to_ns(now);
> > > +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> > > +   (ns_now > ns_boot);
> > > +
> >
>
>
> > > +   if (!valid_clock)
>
> Are we expecting more often clock to be non-valid?
> Perhaps change to positive conditional?

Hi Andy,

Sure, I will change to:
if (valid_clock)
...
else
...

>
> > > +   *ts = (struct timespec64){0, 0};
>
> I dunno if additional variable would be better for readability, like
>
> struct timespec64 null_ts = {0,0};

I don't mind adding ts_null, but I think, as-is ok here,

> ...
> *ts = null_ts;
>
> > > +   else
> > > +   *ts = ns_to_timespec64(ns_now - ns_boot);
>
> But I'm fine as long as Thomas is okay with this code.
>

Thank you for the review!

Pavel

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Mon, Jun 18, 2018 at 11:42 AM Andy Shevchenko
 wrote:
>
> On Fri, Jun 15, 2018 at 8:48 PM Pavel Tatashin  
> wrote:
> >
> > read_boot_clock64() returns time of when system was started. Now, that
> > early boot clock is going to be available on x86 it is possible to
> > implement x86 specific version of read_boot_clock64() that takes advantage
> > of this new feature.
>

Oops, sorry for previous empty mail.

> > +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 
> > *ts)
> > +{
> > +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> > +   bool valid_clock;
> > +   u64 ns_now;
> > +
> > +   ns_now = timespec64_to_ns(now);
> > +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> > +   (ns_now > ns_boot);
> > +
>


> > +   if (!valid_clock)

Are we expecting more often clock to be non-valid?
Perhaps change to positive conditional?

> > +   *ts = (struct timespec64){0, 0};

I dunno if additional variable would be better for readability, like

struct timespec64 null_ts = {0,0};
...
*ts = null_ts;

> > +   else
> > +   *ts = ns_to_timespec64(ns_now - ns_boot);

But I'm fine as long as Thomas is okay with this code.

> > +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Mon, Jun 18, 2018 at 11:42 AM Andy Shevchenko
 wrote:
>
> On Fri, Jun 15, 2018 at 8:48 PM Pavel Tatashin  
> wrote:
> >
> > read_boot_clock64() returns time of when system was started. Now, that
> > early boot clock is going to be available on x86 it is possible to
> > implement x86 specific version of read_boot_clock64() that takes advantage
> > of this new feature.
>

Oops, sorry for previous empty mail.

> > +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 
> > *ts)
> > +{
> > +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> > +   bool valid_clock;
> > +   u64 ns_now;
> > +
> > +   ns_now = timespec64_to_ns(now);
> > +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> > +   (ns_now > ns_boot);
> > +
>


> > +   if (!valid_clock)

Are we expecting more often clock to be non-valid?
Perhaps change to positive conditional?

> > +   *ts = (struct timespec64){0, 0};

I dunno if additional variable would be better for readability, like

struct timespec64 null_ts = {0,0};
...
*ts = null_ts;

> > +   else
> > +   *ts = ns_to_timespec64(ns_now - ns_boot);

But I'm fine as long as Thomas is okay with this code.

> > +}


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Fri, Jun 15, 2018 at 8:48 PM Pavel Tatashin
 wrote:
>
> read_boot_clock64() returns time of when system was started. Now, that
> early boot clock is going to be available on x86 it is possible to
> implement x86 specific version of read_boot_clock64() that takes advantage
> of this new feature.

> +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
> +{
> +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> +   bool valid_clock;
> +   u64 ns_now;
> +
> +   ns_now = timespec64_to_ns(now);
> +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> +   (ns_now > ns_boot);
> +

> +   if (!valid_clock)
> +   *ts = (struct timespec64){0, 0};
> +   else
> +   *ts = ns_to_timespec64(ns_now - ns_boot);



> +}


--
With Best Regards,
Andy Shevchenko

Re: [PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Andy Shevchenko]

On Fri, Jun 15, 2018 at 8:48 PM Pavel Tatashin
 wrote:
>
> read_boot_clock64() returns time of when system was started. Now, that
> early boot clock is going to be available on x86 it is possible to
> implement x86 specific version of read_boot_clock64() that takes advantage
> of this new feature.

> +void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
> +{
> +   u64 ns_boot = sched_clock_cpu(smp_processor_id());
> +   bool valid_clock;
> +   u64 ns_now;
> +
> +   ns_now = timespec64_to_ns(now);
> +   valid_clock = ns_boot && timespec64_valid_strict(now) &&
> +   (ns_now > ns_boot);
> +

> +   if (!valid_clock)
> +   *ts = (struct timespec64){0, 0};
> +   else
> +   *ts = ns_to_timespec64(ns_now - ns_boot);



> +}


--
With Best Regards,
Andy Shevchenko

[PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

read_boot_clock64() returns time of when system was started. Now, that
early boot clock is going to be available on x86 it is possible to
implement x86 specific version of read_boot_clock64() that takes advantage
of this new feature.

Signed-off-by: Pavel Tatashin 
---
 arch/x86/kernel/time.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 774ebafa97c4..32dff35719d9 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -104,3 +105,32 @@ void __init time_init(void)
 {
late_time_init = x86_late_time_init;
 }
+
+/*
+ * Called once during to boot to initialize boot time.
+ * This function returns timestamp in timespec format which is sec/nsec from
+ * epoch of when boot started.
+ * We use sched_clock_cpu() that gives us nanoseconds from when this clock has
+ * been started and it happens quiet early during boot process. To calculate
+ * offset from epoch we use information provided in 'now' by the caller
+ *
+ * If sched_clock_cpu() is not available or if there is any kind of error
+ * i.e. time from epoch is smaller than boot time, we must return zeros in ts,
+ * and the caller will take care of the error: by assuming that the time when
+ * this function was called is the beginning of boot time.
+ */
+void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
+{
+   u64 ns_boot = sched_clock_cpu(smp_processor_id());
+   bool valid_clock;
+   u64 ns_now;
+
+   ns_now = timespec64_to_ns(now);
+   valid_clock = ns_boot && timespec64_valid_strict(now) &&
+   (ns_now > ns_boot);
+
+   if (!valid_clock)
+   *ts = (struct timespec64){0, 0};
+   else
+   *ts = ns_to_timespec64(ns_now - ns_boot);
+}
-- 
2.17.1

[PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

read_boot_clock64() returns time of when system was started. Now, that
early boot clock is going to be available on x86 it is possible to
implement x86 specific version of read_boot_clock64() that takes advantage
of this new feature.

Signed-off-by: Pavel Tatashin 
---
 arch/x86/kernel/time.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 774ebafa97c4..32dff35719d9 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -104,3 +105,32 @@ void __init time_init(void)
 {
late_time_init = x86_late_time_init;
 }
+
+/*
+ * Called once during to boot to initialize boot time.
+ * This function returns timestamp in timespec format which is sec/nsec from
+ * epoch of when boot started.
+ * We use sched_clock_cpu() that gives us nanoseconds from when this clock has
+ * been started and it happens quiet early during boot process. To calculate
+ * offset from epoch we use information provided in 'now' by the caller
+ *
+ * If sched_clock_cpu() is not available or if there is any kind of error
+ * i.e. time from epoch is smaller than boot time, we must return zeros in ts,
+ * and the caller will take care of the error: by assuming that the time when
+ * this function was called is the beginning of boot time.
+ */
+void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
+{
+   u64 ns_boot = sched_clock_cpu(smp_processor_id());
+   bool valid_clock;
+   u64 ns_now;
+
+   ns_now = timespec64_to_ns(now);
+   valid_clock = ns_boot && timespec64_valid_strict(now) &&
+   (ns_now > ns_boot);
+
+   if (!valid_clock)
+   *ts = (struct timespec64){0, 0};
+   else
+   *ts = ns_to_timespec64(ns_now - ns_boot);
+}
-- 
2.17.1

[PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

read_boot_clock64() returns time of when system was started. Now, that
early boot clock is going to be available on x86 it is possible to
implement x86 specific version of read_boot_clock64() that takes advantage
of this new feature.

Signed-off-by: Pavel Tatashin 
---
 arch/x86/kernel/time.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 774ebafa97c4..32dff35719d9 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -104,3 +105,32 @@ void __init time_init(void)
 {
late_time_init = x86_late_time_init;
 }
+
+/*
+ * Called once during to boot to initialize boot time.
+ * This function returns timestamp in timespec format which is sec/nsec from
+ * epoch of when boot started.
+ * We use sched_clock_cpu() that gives us nanoseconds from when this clock has
+ * been started and it happens quiet early during boot process. To calculate
+ * offset from epoch we use information provided in 'now' by the caller
+ *
+ * If sched_clock_cpu() is not available or if there is any kind of error
+ * i.e. time from epoch is smaller than boot time, we must return zeros in ts,
+ * and the caller will take care of the error: by assuming that the time when
+ * this function was called is the beginning of boot time.
+ */
+void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
+{
+   u64 ns_boot = sched_clock_cpu(smp_processor_id());
+   bool valid_clock;
+   u64 ns_now;
+
+   ns_now = timespec64_to_ns(now);
+   valid_clock = ns_boot && timespec64_valid_strict(now) &&
+   (ns_now > ns_boot);
+
+   if (!valid_clock)
+   *ts = (struct timespec64){0, 0};
+   else
+   *ts = ns_to_timespec64(ns_now - ns_boot);
+}
-- 
2.16.1

[PATCH v10 3/7] x86/time: read_boot_clock64() implementation

[Pavel Tatashin]

read_boot_clock64() returns time of when system was started. Now, that
early boot clock is going to be available on x86 it is possible to
implement x86 specific version of read_boot_clock64() that takes advantage
of this new feature.

Signed-off-by: Pavel Tatashin 
---
 arch/x86/kernel/time.c | 30 ++
 1 file changed, 30 insertions(+)

diff --git a/arch/x86/kernel/time.c b/arch/x86/kernel/time.c
index 774ebafa97c4..32dff35719d9 100644
--- a/arch/x86/kernel/time.c
+++ b/arch/x86/kernel/time.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -104,3 +105,32 @@ void __init time_init(void)
 {
late_time_init = x86_late_time_init;
 }
+
+/*
+ * Called once during to boot to initialize boot time.
+ * This function returns timestamp in timespec format which is sec/nsec from
+ * epoch of when boot started.
+ * We use sched_clock_cpu() that gives us nanoseconds from when this clock has
+ * been started and it happens quiet early during boot process. To calculate
+ * offset from epoch we use information provided in 'now' by the caller
+ *
+ * If sched_clock_cpu() is not available or if there is any kind of error
+ * i.e. time from epoch is smaller than boot time, we must return zeros in ts,
+ * and the caller will take care of the error: by assuming that the time when
+ * this function was called is the beginning of boot time.
+ */
+void __init read_boot_clock64(struct timespec64 *now, struct timespec64 *ts)
+{
+   u64 ns_boot = sched_clock_cpu(smp_processor_id());
+   bool valid_clock;
+   u64 ns_now;
+
+   ns_now = timespec64_to_ns(now);
+   valid_clock = ns_boot && timespec64_valid_strict(now) &&
+   (ns_now > ns_boot);
+
+   if (!valid_clock)
+   *ts = (struct timespec64){0, 0};
+   else
+   *ts = ns_to_timespec64(ns_now - ns_boot);
+}
-- 
2.16.1