diff mbox series

[v1,1/6] kernel/time: Add system time to system counter conversion

Message ID 20231017052457.25287-2-lakshmi.sowjanya.d@intel.com
State New
Headers
Series Add support for Intel PPS Generator |

Commit Message

D, Lakshmi Sowjanya Oct. 17, 2023, 5:24 a.m. UTC 
  From: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>

Support system-clock to system-counter conversion. Intel Timed IO
hardware, using system counter as reference to schedule events.

Co-developed-by: Christopher Hall <christopher.s.hall@intel.com>
Signed-off-by: Christopher Hall <christopher.s.hall@intel.com>
Co-developed-by: Pandith N <pandith.n@intel.com>
Signed-off-by: Pandith N <pandith.n@intel.com>
Co-developed-by: Thejesh Reddy T R <thejesh.reddy.t.r@intel.com>
Signed-off-by: Thejesh Reddy T R <thejesh.reddy.t.r@intel.com>
Signed-off-by: Lakshmi Sowjanya D <lakshmi.sowjanya.d@intel.com>
Reviewed-by: Eddie Dong <eddie.dong@intel.com>
---
 include/linux/timekeeping.h |  3 +++
 kernel/time/timekeeping.c   | 54 +++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)

Comments

Thomas Gleixner Oct. 17, 2023, 8:42 a.m. UTC | #1 
Lakshmi!

On Tue, Oct 17 2023 at 10:54, lakshmi.sowjanya.d@intel.com wrote:

'kernel/time:' is not a proper subsystem prefix.

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html#patch-subject

> Support system-clock to system-counter conversion. Intel Timed IO
> hardware, using system counter as reference to schedule events.

This tells me WHAT the patch is doing but not at all WHY this is
required and that Intel Timed IO hardware reference is just not cutting
it.

> +extern int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
> +						struct system_counterval_t *ret);

The changelog talks about system-clock. The function name and the
argument tell that this is about real-time. Thats confusing at best.

> +static inline int timekeeping_ns_to_delta(const struct tk_read_base *tkr, u64 nsec,
> +					  u64 *cycles)
> +{
> +	if (BITS_TO_BYTES(fls64(nsec) + tkr->shift) > sizeof(nsec))
> +		return -ERANGE;

Without a comment you will not be able to grok that check in three
months from now.

> +	*cycles = nsec << tkr->shift;
> +	*cycles -= tkr->xtime_nsec;
> +	do_div(*cycles, tkr->mult);
> +
> +	return 0;
> +}
> +
>  static inline u64 timekeeping_delta_to_ns(const struct tk_read_base *tkr, u64 delta)
>  {
>  	u64 nsec;
> @@ -1303,6 +1316,47 @@ int get_device_system_crosststamp(int (*get_time_fn)
>  }
>  EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
>  
> +/**
> + * ktime_convert_real_to_system_counter - Convert system time to system counter

System time is _NOT_ the same as clock realtime, really. What's wrong
with consistently using clock realtime instead of making up some new
terminology?

> + * value

Pointless line break which makes this unreadable.

> + * @sys_realtime:	realtime clock value to convert

What means sys_realtime? The argument supplies an absolute time value
based on clock realtime and not on some magic system realtime.

> + * @ret:		Computed system counter value with clocksource pointer

So @ret is returning the computed value along with a clocksource pointer
or what?

> + * Converts a supplied, future realtime clock value to the corresponding
> + * system counter value.
> + *
> + * Return: 0 on success, -errno on failure.

If this really needs error codes, then please explicitly document
them. If not, then make the function return bool and be done with it.

> + */
> +int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
> +					 struct system_counterval_t *ret)
> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	ktime_t base_real;
> +	unsigned int seq;
> +	u64 ns_delta;
> +	int err;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +
> +		base_real = ktime_add(tk->tkr_mono.base,
> +				      tk_core.timekeeper.offs_real);
> +		if (ktime_compare(sys_realtime, base_real) < 0)
> +			return -EINVAL;
> +
> +		ret->cs = tk->tkr_mono.clock;
> +		ns_delta = ktime_to_ns(ktime_sub(sys_realtime, base_real));
> +		err = timekeeping_ns_to_delta(&tk->tkr_mono, ns_delta, &ret->cycles);
> +		if (err < 0)
> +			return err;

This is completely uncomprehensible especially with the hidden range
check in the conversion function. All of this can be simplified to:

bool ktime_real_to_system_counter(ktime_t treal, struct system_counterval_t *scv)
{
	struct timekeeper *tk = &tk_core.timekeeper;
	unsigned int seq;
	u64 delta;

	do {
		seq = read_seqcount_begin(&tk_core.seq);

	        delta = (u64)treal - tk->tkr_mono.base_real;

		if (delta > tk->tkr_mono.clock->max_idle_ns)
                	return false;

                scv->cycles = timekeeping_ns_to_delta(&tk->tkr_mono, delta);
                scv->cycles += tk->tkr_mono.cycle_last;

		scv->cs = tk->tkr_mono.clock;
	} while (read_seqcount_retry(&tk_core.seq, seq));

	return true;
}

That gets rid of this hideous range check in the conversion function and
makes the code simple and straight forward, no?

Related question: What guarantee that the supplied value is not in the
past?

Nothing. It only guarantees that it is not before the realtime base
value, which can be up to one second in the past.

Maybe it does not matter, but then the 'future realtime clock value' in
the function documentation is wishful thinking.

Thanks,

        tglx
diff mbox series

Patch

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index fe1e467ba046..e5eb6699d691 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -289,6 +289,9 @@  extern int get_device_system_crosststamp(
 			struct system_time_snapshot *history,
 			struct system_device_crosststamp *xtstamp);
 
+extern int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
+						struct system_counterval_t *ret);
+
 /*
  * Simultaneously snapshot realtime and monotonic raw clocks
  */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 266d02809dbb..ff6a4c7387ee 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -371,6 +371,19 @@  static void tk_setup_internals(struct timekeeper *tk, struct clocksource *clock)
 
 /* Timekeeper helper functions. */
 
+static inline int timekeeping_ns_to_delta(const struct tk_read_base *tkr, u64 nsec,
+					  u64 *cycles)
+{
+	if (BITS_TO_BYTES(fls64(nsec) + tkr->shift) > sizeof(nsec))
+		return -ERANGE;
+
+	*cycles = nsec << tkr->shift;
+	*cycles -= tkr->xtime_nsec;
+	do_div(*cycles, tkr->mult);
+
+	return 0;
+}
+
 static inline u64 timekeeping_delta_to_ns(const struct tk_read_base *tkr, u64 delta)
 {
 	u64 nsec;
@@ -1303,6 +1316,47 @@  int get_device_system_crosststamp(int (*get_time_fn)
 }
 EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
 
+/**
+ * ktime_convert_real_to_system_counter - Convert system time to system counter
+ * value
+ * @sys_realtime:	realtime clock value to convert
+ * @ret:		Computed system counter value with clocksource pointer
+ *
+ * Converts a supplied, future realtime clock value to the corresponding
+ * system counter value.
+ *
+ * Return: 0 on success, -errno on failure.
+ */
+int ktime_convert_real_to_system_counter(ktime_t sys_realtime,
+					 struct system_counterval_t *ret)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	ktime_t base_real;
+	unsigned int seq;
+	u64 ns_delta;
+	int err;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+
+		base_real = ktime_add(tk->tkr_mono.base,
+				      tk_core.timekeeper.offs_real);
+		if (ktime_compare(sys_realtime, base_real) < 0)
+			return -EINVAL;
+
+		ret->cs = tk->tkr_mono.clock;
+		ns_delta = ktime_to_ns(ktime_sub(sys_realtime, base_real));
+		err = timekeeping_ns_to_delta(&tk->tkr_mono, ns_delta, &ret->cycles);
+		if (err < 0)
+			return err;
+
+		ret->cycles += tk->tkr_mono.cycle_last;
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(ktime_convert_real_to_system_counter);
+
 /**
  * do_settimeofday64 - Sets the time of day.
  * @ts:     pointer to the timespec64 variable containing the new time