[v6,08/11] timekeeping: Add function to convert realtime to base clock

Commit Message

D, Lakshmi Sowjanya April 10, 2024, 11:48 a.m. UTC

From: Lakshmi Sowjanya D <[e-mail]>

PPS(Pulse Per Second) generates signals in realtime, but Timed IO
hardware understands time in base clock reference. Add an interface,
ktime_real_to_base_clock() to convert realtime to base clock.

Add the helper function timekeeping_clocksource_has_base(), to check
whether the current clocksource has the same base clock. This will be
used by Timed IO device to check if the base clock is X86_ART(Always
Running Timer).

Co-developed-by: Thomas Gleixner <[e-mail]>
Signed-off-by: Thomas Gleixner <[e-mail]>
Co-developed-by: Christopher S. Hall <[e-mail]>
Signed-off-by: Christopher S. Hall <[e-mail]>
Signed-off-by: Lakshmi Sowjanya D <[e-mail]>
---
 include/linux/timekeeping.h |  4 +++
 kernel/time/timekeeping.c   | 70 +++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

Comments

Thomas Gleixner April 10, 2024, 10:15 p.m. UTC | #1

On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> From: Lakshmi Sowjanya D <[e-mail]>
>
> PPS(Pulse Per Second) generates signals in realtime, but Timed IO

... generates signals based on CLOCK_REALTIME, but ...

> hardware understands time in base clock reference.

The hardware does not understand anything.

> Add an interface,
> ktime_real_to_base_clock() to convert realtime to base clock.
>
> Add the helper function timekeeping_clocksource_has_base(), to check
> whether the current clocksource has the same base clock. This will be
> used by Timed IO device to check if the base clock is X86_ART(Always
> Running Timer).

Again this fails to explain the rationale and as this is a core change
which is hardware agnostic the whole Timed IO and ART reference is not
really helpful. Something like this:

  "PPS (Pulse Per Second) generates a hardware pulse every second based
   on CLOCK_REALTIME. This works fine when the pulse is generated in
   software from a hrtimer callback function.

   For hardware which generates the pulse by programming a timer it's
   required to convert CLOCK_REALTIME to the underlying hardware clock.

   The X86 Timed IO device is based on the Always Running Timer (ART),
   which is the base clock of the TSC, which is usually the system
   clocksource on X86.

   The core code already has functionality to convert base clock
   timestamps to system clocksource timestamps, but there is no support
   for converting the other way around.

   Provide the required functionality to support such devices in a
   generic way to avoid code duplication in drivers:

      1) ktime_real_to_base_clock() to convert a CLOCK_REALTIME
         timestamp to a base clock timestamp

      2) timekeeping_clocksource_has_base() to allow drivers to validate
         that the system clocksource is based on a particular
         clocksource ID.
  
> +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids base_id)
> +{
> +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> +	struct clocksource_base *base = cs->base;
> +
> +	/* Check whether base_id matches the base clock */
> +	if (!base || base->id != base_id)
> +		return false;
> +
> +	*cycles -= base->offset;
> +	if (!convert_clock(cycles, base->denominator, base->numerator))
> +		return false;
> +	return true;
> +}
> +
> +static u64 convert_ns_to_cs(u64 delta)
> +{
> +	struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
> +
> +	return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
> +}

> +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids base_id, u64 *cycles)

As this is a kernel API function it really wants kernel-doc comment to
explain the functionality, the arguments and the return value.

> +{
> +	struct timekeeper *tk = &tk_core.timekeeper;
> +	unsigned int seq;
> +	u64 delta;
> +
> +	do {
> +		seq = read_seqcount_begin(&tk_core.seq);
> +		if ((u64)treal < tk->tkr_mono.base_real)
> +			return false;
> +		delta = (u64)treal - tk->tkr_mono.base_real;

In the previous version you had a sanity check on delta:

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

And I told you:

>> I don't think this cutoff is valid. There is no guarantee that this is
>> linear unless:
>>
>>       Treal[last timekeeper update] <= treal < Treal[next timekeeper update]
>>
>> Look at the dance in get_device_system_crosststamp() and
>> adjust_historical_crosststamp() to see why.

So now there is not even a check anymore whether the delta conversion
can overflow.

There is zero explanation why this conversion is considered to be
correct.

> +		*cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> +		if (!convert_cs_to_base(cycles, base_id))
> +			return false;
> +	} while (read_seqcount_retry(&tk_core.seq, seq));
> +
> +	return true;
> +}

> +/**
> + * timekeeping_clocksource_has_base - Check whether the current clocksource
> + *     has a base clock

s/has a base clock/is based on a given base clock

> + * @id:		The clocksource ID to check for

base clocksource ID

> + *
> + * Note:	The return value is a snapshot which can become invalid right
> + *		after the function returns.
> + *
> + * Return:	true if the timekeeper clocksource has a base clock with @id,
> + *		false otherwise
> + */

Thanks,

        tglx

D, Lakshmi Sowjanya April 16, 2024, 10:17 a.m. UTC | #2

> -----Original Message-----
> From: Thomas Gleixner <[e-mail]>
> Sent: Thursday, April 11, 2024 3:46 AM
> To: D, Lakshmi Sowjanya <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail]; linux-
> [e-mail]
> Cc: [e-mail]; [e-mail]; [e-mail]; intel-
> [e-mail]; [e-mail]; Dong, Eddie
> <[e-mail]>; Hall, Christopher S <[e-mail]>;
> Brandeburg, Jesse <[e-mail]>; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> Nguyen, Anthony L <[e-mail]>;
> [e-mail]; N, Pandith <[e-mail]>; Mohan,
> Subramanian <[e-mail]>; T R, Thejesh Reddy
> <[e-mail]>; D, Lakshmi Sowjanya
> <[e-mail]>
> Subject: Re: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
> base clock
> 
> On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> > From: Lakshmi Sowjanya D <[e-mail]>
> >
> > PPS(Pulse Per Second) generates signals in realtime, but Timed IO
> 
> ... generates signals based on CLOCK_REALTIME, but ...
> 
> > hardware understands time in base clock reference.
> 
> The hardware does not understand anything.
> 
> > Add an interface,
> > ktime_real_to_base_clock() to convert realtime to base clock.
> >
> > Add the helper function timekeeping_clocksource_has_base(), to check
> > whether the current clocksource has the same base clock. This will be
> > used by Timed IO device to check if the base clock is X86_ART(Always
> > Running Timer).
> 
> Again this fails to explain the rationale and as this is a core change which is
> hardware agnostic the whole Timed IO and ART reference is not really helpful.
> Something like this:
> 
>   "PPS (Pulse Per Second) generates a hardware pulse every second based
>    on CLOCK_REALTIME. This works fine when the pulse is generated in
>    software from a hrtimer callback function.
> 
>    For hardware which generates the pulse by programming a timer it's
>    required to convert CLOCK_REALTIME to the underlying hardware clock.
> 
>    The X86 Timed IO device is based on the Always Running Timer (ART),
>    which is the base clock of the TSC, which is usually the system
>    clocksource on X86.
> 
>    The core code already has functionality to convert base clock
>    timestamps to system clocksource timestamps, but there is no support
>    for converting the other way around.
> 
>    Provide the required functionality to support such devices in a
>    generic way to avoid code duplication in drivers:
> 
>       1) ktime_real_to_base_clock() to convert a CLOCK_REALTIME
>          timestamp to a base clock timestamp
> 
>       2) timekeeping_clocksource_has_base() to allow drivers to validate
>          that the system clocksource is based on a particular
>          clocksource ID.

Thanks for the commit message. 
I will update as suggested.

> 
> > +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids
> > +base_id) {
> > +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > +	struct clocksource_base *base = cs->base;
> > +
> > +	/* Check whether base_id matches the base clock */
> > +	if (!base || base->id != base_id)
> > +		return false;
> > +
> > +	*cycles -= base->offset;
> > +	if (!convert_clock(cycles, base->denominator, base->numerator))
> > +		return false;
> > +	return true;
> > +}
> > +
> > +static u64 convert_ns_to_cs(u64 delta) {
> > +	struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
> > +
> > +	return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
> > +}
> 
> > +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> > +base_id, u64 *cycles)
> 
> As this is a kernel API function it really wants kernel-doc comment to explain the
> functionality, the arguments and the return value.

Will add the following documentation:

" ktime_real_to_base_clock()- Convert CLOCK_REALTIME timestamp to a base clock timestamp.
@treal: 	CLOCK_REALTIME timestamp to convert.
@base_id: 	base clocksource id.
@*cycles: 	pointer to store the converted base clock timestamp.

Converts a supplied, future realtime clock value to the corresponding base clock value.

Return:  true if the conversion is successful, false otherwise."

> 
> > +{
> > +	struct timekeeper *tk = &tk_core.timekeeper;
> > +	unsigned int seq;
> > +	u64 delta;
> > +
> > +	do {
> > +		seq = read_seqcount_begin(&tk_core.seq);
> > +		if ((u64)treal < tk->tkr_mono.base_real)
> > +			return false;
> > +		delta = (u64)treal - tk->tkr_mono.base_real;
> 
> In the previous version you had a sanity check on delta:
> 
> >>> +		if (delta > tk->tkr_mono.clock->max_idle_ns)
> >>> +			return false;
> 
> And I told you:
> 
> >> I don't think this cutoff is valid. There is no guarantee that this
> >> is linear unless:
> >>
> >>       Treal[last timekeeper update] <= treal < Treal[next timekeeper
> >> update]
> >>
> >> Look at the dance in get_device_system_crosststamp() and
> >> adjust_historical_crosststamp() to see why.
> 
> So now there is not even a check anymore whether the delta conversion can
> overflow.
> 
> There is zero explanation why this conversion is considered to be correct.

Adding the following check for delta overflow in convert_ns_to_cs function.

	if (BITS_TO_BYTES(fls64(*delta) + tkr->shift) >= sizeof(*delta))
			return false;
					
> 
> > +		*cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
> > +		if (!convert_cs_to_base(cycles, base_id))
> > +			return false;
> > +	} while (read_seqcount_retry(&tk_core.seq, seq));
> > +
> > +	return true;
> > +}
> 
> > +/**
> > + * timekeeping_clocksource_has_base - Check whether the current
> clocksource
> > + *     has a base clock
> 
> s/has a base clock/is based on a given base clock
> 
> > + * @id:		The clocksource ID to check for
> 
> base clocksource ID
> 
> > + *
> > + * Note:	The return value is a snapshot which can become invalid right
> > + *		after the function returns.
> > + *
> > + * Return:	true if the timekeeper clocksource has a base clock with @id,
> > + *		false otherwise
> > + */
> 
> Thanks,
> 
>         tglx

Patch

diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index b2ee182d891e..fc12a9ba2c88 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -318,6 +318,10 @@  struct system_counterval_t {
 	bool			use_nsecs;
 };
 
+extern bool ktime_real_to_base_clock(ktime_t treal,
+				     enum clocksource_ids base_id, u64 *cycles);
+extern bool timekeeping_clocksource_has_base(enum clocksource_ids id);
+
 /*
  * Get cross timestamp between system clock and device clock
  */
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 2542cfefbdee..e4ecce722969 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1227,6 +1227,48 @@  static bool convert_base_to_cs(struct system_counterval_t *scv)
 	return true;
 }
 
+static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids base_id)
+{
+	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
+	struct clocksource_base *base = cs->base;
+
+	/* Check whether base_id matches the base clock */
+	if (!base || base->id != base_id)
+		return false;
+
+	*cycles -= base->offset;
+	if (!convert_clock(cycles, base->denominator, base->numerator))
+		return false;
+	return true;
+}
+
+static u64 convert_ns_to_cs(u64 delta)
+{
+	struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
+
+	return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
+}
+
+bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids base_id, u64 *cycles)
+{
+	struct timekeeper *tk = &tk_core.timekeeper;
+	unsigned int seq;
+	u64 delta;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		if ((u64)treal < tk->tkr_mono.base_real)
+			return false;
+		delta = (u64)treal - tk->tkr_mono.base_real;
+		*cycles = tk->tkr_mono.cycle_last + convert_ns_to_cs(delta);
+		if (!convert_cs_to_base(cycles, base_id))
+			return false;
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return true;
+}
+EXPORT_SYMBOL_GPL(ktime_real_to_base_clock);
+
 /**
  * get_device_system_crosststamp - Synchronously capture system/device timestamp
  * @get_time_fn:	Callback to get simultaneous device time and
@@ -1337,6 +1379,34 @@  int get_device_system_crosststamp(int (*get_time_fn)
 }
 EXPORT_SYMBOL_GPL(get_device_system_crosststamp);
 
+/**
+ * timekeeping_clocksource_has_base - Check whether the current clocksource
+ *     has a base clock
+ * @id:		The clocksource ID to check for
+ *
+ * Note:	The return value is a snapshot which can become invalid right
+ *		after the function returns.
+ *
+ * Return:	true if the timekeeper clocksource has a base clock with @id,
+ *		false otherwise
+ */
+bool timekeeping_clocksource_has_base(enum clocksource_ids id)
+{
+	unsigned int seq;
+	bool ret;
+
+	do {
+		seq = read_seqcount_begin(&tk_core.seq);
+		if (tk_core.timekeeper.tkr_mono.clock->base)
+			ret = (tk_core.timekeeper.tkr_mono.clock->base->id == id);
+		else
+			ret = false;
+	} while (read_seqcount_retry(&tk_core.seq, seq));
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(timekeeping_clocksource_has_base);
+
 /**
  * do_settimeofday64 - Sets the time of day.
  * @ts:     pointer to the timespec64 variable containing the new time