[v6,01/11] x86/tsc: Add base clock properties in clocksource structure

Commit Message

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

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

Add base clock hardware abstraction in clocksource structure.

Add clocksource ID for x86 ART(Always Running Timer). The newly added
clocksource ID and conversion parameters are used to convert time in a
clocksource domain to base clock and vice versa.

Convert the base clock to the system clock using convert_base_to_cs() in
get_device_system_crosststamp().

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]>
---
 arch/x86/kernel/tsc.c           | 42 +++++++++++++++++++--------------
 include/linux/clocksource.h     | 27 +++++++++++++++++++++
 include/linux/clocksource_ids.h |  1 +
 include/linux/timekeeping.h     |  2 ++
 kernel/time/timekeeping.c       | 37 +++++++++++++++++++++++++++--
 5 files changed, 89 insertions(+), 20 deletions(-)

Comments

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

On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> From: Lakshmi Sowjanya D <[e-mail]>
>
> Add base clock hardware abstraction in clocksource structure.
>
> Add clocksource ID for x86 ART(Always Running Timer). The newly added
> clocksource ID and conversion parameters are used to convert time in a
> clocksource domain to base clock and vice versa.
>
> Convert the base clock to the system clock using convert_base_to_cs() in
> get_device_system_crosststamp().

In https://lore.kernel.org/all/875xxhi1ty.ffs@tglx I asked you to
provide a change log which explains the WHY and not the WHAT. The new
change log still fails to explain WHY this change is needed and which
problem it is trying to solve.

I further asked you to do:

    1) Add the clocksource_base struct and provide the infrastructure in
       get_device_system_crosststamp()

    2) Make TSC/ART use it

But this still does #1 and #2 in one go.

If you don't understand my review comments, then please ask. If you
disagree with them then please tell me and argue with me.

Just ignoring them is not an option.

Thanks,

        tglx

Thomas Gleixner April 10, 2024, 9:32 p.m. UTC | #2

On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> @@ -48,6 +49,7 @@ struct module;
>   * @archdata:		Optional arch-specific data
>   * @max_cycles:		Maximum safe cycle value which won't overflow on
>   *			multiplication
> + * @freq_khz:		Clocksource frequency in khz.
>   * @name:		Pointer to clocksource name
>   * @list:		List head for registration (internal)
>   * @rating:		Rating value for selection (higher is better)
> @@ -70,6 +72,8 @@ struct module;
>   *			validate the clocksource from which the snapshot was
>   *			taken.
>   * @flags:		Flags describing special properties
> + * @base:		Hardware abstraction for clock on which a clocksource
> + *			is based
>   * @enable:		Optional function to enable the clocksource
>   * @disable:		Optional function to disable the clocksource
>   * @suspend:		Optional suspend function for the clocksource
> @@ -105,12 +109,14 @@ struct clocksource {
>  	struct arch_clocksource_data archdata;
>  #endif
>  	u64			max_cycles;
> +	u32			freq_khz;

Q: Why is this a bad place to add this member?

A: Because it creates a 4 byte hole in the data structure.

>  	const char		*name;
>  	struct list_head	list;

While adding it here fills a 4 byte hole.

Hint:

  pahole -c clocksource kernel/time/clocksource.o

would have told you that.

>  	int			rating;
>  	enum clocksource_ids	id;
>  	enum vdso_clock_mode	vdso_clock_mode;
>  	unsigned long		flags;
> +	struct clocksource_base *base;

> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index b58dffc58a8f..2542cfefbdee 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -1193,6 +1193,40 @@ static bool timestamp_in_interval(u64 start, u64 end, u64 ts)
>  	return false;
>  }
>  
> +static bool convert_clock(u64 *val, u32 numerator, u32 denominator)
> +{
> +	u64 rem, res;
> +
> +	if (!numerator || !denominator)
> +		return false;
> +
> +	res = div64_u64_rem(*val, denominator, &rem) * numerator;
> +	*val = res + div_u64(rem * numerator, denominator);
> +	return true;
> +}
> +
> +static bool convert_base_to_cs(struct system_counterval_t *scv)
> +{
> +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> +	struct clocksource_base *base = cs->base;
> +	u32 num, den;
> +
> +	/* The timestamp was taken from the time keeper clock source */
> +	if (cs->id == scv->cs_id)
> +		return true;
> +
> +	/* Check whether cs_id matches the base clock */
> +	if (!base || base->id != scv->cs_id)
> +		return false;
> +
> +	num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> +	den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> +
> +	convert_clock(&scv->cycles, num, den);

Q: Why does this ignore the return value of convert_clock() ?

A: Because all drivers will correctly fill in everything.

Q: Then why does convert_clock() bother to check and have a return
   value?

A: Because drivers will fail to correctly fill in everything

Thanks,

        tglx

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

> -----Original Message-----
> From: Thomas Gleixner <[e-mail]>
> Sent: Thursday, April 11, 2024 2:51 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 01/11] x86/tsc: Add base clock properties in clocksource
> structure
> 
> On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> > From: Lakshmi Sowjanya D <[e-mail]>
> >
> > Add base clock hardware abstraction in clocksource structure.
> >
> > Add clocksource ID for x86 ART(Always Running Timer). The newly added
> > clocksource ID and conversion parameters are used to convert time in a
> > clocksource domain to base clock and vice versa.
> >
> > Convert the base clock to the system clock using convert_base_to_cs()
> > in get_device_system_crosststamp().
> 
> In https://lore.kernel.org/all/875xxhi1ty.ffs@tglx I asked you to provide a
> change log which explains the WHY and not the WHAT. The new change log still
> fails to explain WHY this change is needed and which problem it is trying to
> solve.

Rephrased the commit message to:
" Add base clock hardware abstraction in clocksource structure.

The core code has a mechanism to convert the ART base clock to
the corresponding tsc value. 
Provide the generic functionality in convert_base_to_cs() to convert
base clock timestamps to system clocksource without requiring
architecture specific parameters.

Add the infrastructure in get_device_system_crosststamp()."

> 
> I further asked you to do:
> 
>     1) Add the clocksource_base struct and provide the infrastructure in
>        get_device_system_crosststamp()
> 
>     2) Make TSC/ART use it
> 
> But this still does #1 and #2 in one go.
> 
> If you don't understand my review comments, then please ask. If you disagree
> with them then please tell me and argue with me.
> 
> Just ignoring them is not an option.

Sorry Thomas, that was a mis-understanding. We had split only realtime as separate patch.
We will split the first patch as suggested. 
	1. Timekeeping part(convert_base_to_cs() and infrastructure in get_device_system_crosststamp())
	2. x86(TSC/ART values update into the structure)

Thanks,
Sowjanya

> 
> Thanks,
> 
>         tglx

D, Lakshmi Sowjanya April 16, 2024, 10:11 a.m. UTC | #4

> -----Original Message-----
> From: Thomas Gleixner <[e-mail]>
> Sent: Thursday, April 11, 2024 3:03 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 01/11] x86/tsc: Add base clock properties in clocksource
> structure
> 
> On Wed, Apr 10 2024 at 17:18, [e-mail] wrote:
> > @@ -48,6 +49,7 @@ struct module;
> >   * @archdata:		Optional arch-specific data
> >   * @max_cycles:		Maximum safe cycle value which won't
> overflow on
> >   *			multiplication
> > + * @freq_khz:		Clocksource frequency in khz.
> >   * @name:		Pointer to clocksource name
> >   * @list:		List head for registration (internal)
> >   * @rating:		Rating value for selection (higher is better)
> > @@ -70,6 +72,8 @@ struct module;
> >   *			validate the clocksource from which the snapshot was
> >   *			taken.
> >   * @flags:		Flags describing special properties
> > + * @base:		Hardware abstraction for clock on which a clocksource
> > + *			is based
> >   * @enable:		Optional function to enable the clocksource
> >   * @disable:		Optional function to disable the clocksource
> >   * @suspend:		Optional suspend function for the clocksource
> > @@ -105,12 +109,14 @@ struct clocksource {
> >  	struct arch_clocksource_data archdata;  #endif
> >  	u64			max_cycles;
> > +	u32			freq_khz;
> 
> Q: Why is this a bad place to add this member?
> 
> A: Because it creates a 4 byte hole in the data structure.
> 
> >  	const char		*name;
> >  	struct list_head	list;
> 
> While adding it here fills a 4 byte hole.
> 
> Hint:
> 
>   pahole -c clocksource kernel/time/clocksource.o
> 
> would have told you that.
> 
> >  	int			rating;
> >  	enum clocksource_ids	id;
> >  	enum vdso_clock_mode	vdso_clock_mode;
> >  	unsigned long		flags;
> > +	struct clocksource_base *base;
> 
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index b58dffc58a8f..2542cfefbdee 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1193,6 +1193,40 @@ static bool timestamp_in_interval(u64 start, u64
> end, u64 ts)
> >  	return false;
> >  }
> >
> > +static bool convert_clock(u64 *val, u32 numerator, u32 denominator) {
> > +	u64 rem, res;
> > +
> > +	if (!numerator || !denominator)
> > +		return false;
> > +
> > +	res = div64_u64_rem(*val, denominator, &rem) * numerator;
> > +	*val = res + div_u64(rem * numerator, denominator);
> > +	return true;
> > +}
> > +
> > +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> > +	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > +	struct clocksource_base *base = cs->base;
> > +	u32 num, den;
> > +
> > +	/* The timestamp was taken from the time keeper clock source */
> > +	if (cs->id == scv->cs_id)
> > +		return true;
> > +
> > +	/* Check whether cs_id matches the base clock */
> > +	if (!base || base->id != scv->cs_id)
> > +		return false;
> > +
> > +	num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> > +	den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> > +
> > +	convert_clock(&scv->cycles, num, den);
> 
> Q: Why does this ignore the return value of convert_clock() ?
> 
> A: Because all drivers will correctly fill in everything.
> 
> Q: Then why does convert_clock() bother to check and have a return
>    value?
> 
> A: Because drivers will fail to correctly fill in everything

Agreed.
Will add a check for error case:

	if (!convert_clock(&scv->cycles, num, den))
             		return false;

Thanks,
Sowjanya

> 
> Thanks,
> 
>         tglx

Patch

diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index 5a69a49acc96..45bf2f6d0ffa 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -50,9 +50,9 @@  int tsc_clocksource_reliable;
 
 static int __read_mostly tsc_force_recalibrate;
 
-static u32 art_to_tsc_numerator;
-static u32 art_to_tsc_denominator;
-static u64 art_to_tsc_offset;
+static struct clocksource_base art_base_clk = {
+	.id    = CSID_X86_ART,
+};
 static bool have_art;
 
 struct cyc2ns {
@@ -1074,7 +1074,7 @@  core_initcall(cpufreq_register_tsc_scaling);
  */
 static void __init detect_art(void)
 {
-	unsigned int unused[2];
+	unsigned int unused;
 
 	if (boot_cpu_data.cpuid_level < ART_CPUID_LEAF)
 		return;
@@ -1089,13 +1089,14 @@  static void __init detect_art(void)
 	    tsc_async_resets)
 		return;
 
-	cpuid(ART_CPUID_LEAF, &art_to_tsc_denominator,
-	      &art_to_tsc_numerator, unused, unused+1);
+	cpuid(ART_CPUID_LEAF, &art_base_clk.denominator,
+	      &art_base_clk.numerator, &art_base_clk.freq_khz, &unused);
 
-	if (art_to_tsc_denominator < ART_MIN_DENOMINATOR)
+	art_base_clk.freq_khz /= KHZ;
+	if (art_base_clk.denominator < ART_MIN_DENOMINATOR)
 		return;
 
-	rdmsrl(MSR_IA32_TSC_ADJUST, art_to_tsc_offset);
+	rdmsrl(MSR_IA32_TSC_ADJUST, art_base_clk.offset);
 
 	/* Make this sticky over multiple CPU init calls */
 	setup_force_cpu_cap(X86_FEATURE_ART);
@@ -1303,13 +1304,13 @@  struct system_counterval_t convert_art_to_tsc(u64 art)
 {
 	u64 tmp, res, rem;
 
-	rem = do_div(art, art_to_tsc_denominator);
+	rem = do_div(art, art_base_clk.denominator);
 
-	res = art * art_to_tsc_numerator;
-	tmp = rem * art_to_tsc_numerator;
+	res = art * art_base_clk.numerator;
+	tmp = rem * art_base_clk.numerator;
 
-	do_div(tmp, art_to_tsc_denominator);
-	res += tmp + art_to_tsc_offset;
+	do_div(tmp, art_base_clk.denominator);
+	res += tmp + art_base_clk.offset;
 
 	return (struct system_counterval_t) {
 		.cs_id	= have_art ? CSID_X86_TSC : CSID_GENERIC,
@@ -1356,7 +1357,6 @@  struct system_counterval_t convert_art_ns_to_tsc(u64 art_ns)
 }
 EXPORT_SYMBOL(convert_art_ns_to_tsc);
 
-
 static void tsc_refine_calibration_work(struct work_struct *work);
 static DECLARE_DELAYED_WORK(tsc_irqwork, tsc_refine_calibration_work);
 /**
@@ -1458,8 +1458,10 @@  static void tsc_refine_calibration_work(struct work_struct *work)
 	if (tsc_unstable)
 		goto unreg;
 
-	if (boot_cpu_has(X86_FEATURE_ART))
+	if (boot_cpu_has(X86_FEATURE_ART)) {
 		have_art = true;
+		clocksource_tsc.base = &art_base_clk;
+	}
 	clocksource_register_khz(&clocksource_tsc, tsc_khz);
 unreg:
 	clocksource_unregister(&clocksource_tsc_early);
@@ -1484,8 +1486,10 @@  static int __init init_tsc_clocksource(void)
 	 * the refined calibration and directly register it as a clocksource.
 	 */
 	if (boot_cpu_has(X86_FEATURE_TSC_KNOWN_FREQ)) {
-		if (boot_cpu_has(X86_FEATURE_ART))
+		if (boot_cpu_has(X86_FEATURE_ART)) {
 			have_art = true;
+			clocksource_tsc.base = &art_base_clk;
+		}
 		clocksource_register_khz(&clocksource_tsc, tsc_khz);
 		clocksource_unregister(&clocksource_tsc_early);
 
@@ -1509,10 +1513,12 @@  static bool __init determine_cpu_tsc_frequencies(bool early)
 
 	if (early) {
 		cpu_khz = x86_platform.calibrate_cpu();
-		if (tsc_early_khz)
+		if (tsc_early_khz) {
 			tsc_khz = tsc_early_khz;
-		else
+		} else {
 			tsc_khz = x86_platform.calibrate_tsc();
+			clocksource_tsc.freq_khz = tsc_khz;
+		}
 	} else {
 		/* We should not be here with non-native cpu calibration */
 		WARN_ON(x86_platform.calibrate_cpu != native_calibrate_cpu);
diff --git a/include/linux/clocksource.h b/include/linux/clocksource.h
index 0ad8b550bb4b..66a033bff17c 100644
--- a/include/linux/clocksource.h
+++ b/include/linux/clocksource.h
@@ -21,6 +21,7 @@ 
 #include <asm/div64.h>
 #include <asm/io.h>
 
+struct clocksource_base;
 struct clocksource;
 struct module;
 
@@ -48,6 +49,7 @@  struct module;
  * @archdata:		Optional arch-specific data
  * @max_cycles:		Maximum safe cycle value which won't overflow on
  *			multiplication
+ * @freq_khz:		Clocksource frequency in khz.
  * @name:		Pointer to clocksource name
  * @list:		List head for registration (internal)
  * @rating:		Rating value for selection (higher is better)
@@ -70,6 +72,8 @@  struct module;
  *			validate the clocksource from which the snapshot was
  *			taken.
  * @flags:		Flags describing special properties
+ * @base:		Hardware abstraction for clock on which a clocksource
+ *			is based
  * @enable:		Optional function to enable the clocksource
  * @disable:		Optional function to disable the clocksource
  * @suspend:		Optional suspend function for the clocksource
@@ -105,12 +109,14 @@  struct clocksource {
 	struct arch_clocksource_data archdata;
 #endif
 	u64			max_cycles;
+	u32			freq_khz;
 	const char		*name;
 	struct list_head	list;
 	int			rating;
 	enum clocksource_ids	id;
 	enum vdso_clock_mode	vdso_clock_mode;
 	unsigned long		flags;
+	struct clocksource_base *base;
 
 	int			(*enable)(struct clocksource *cs);
 	void			(*disable)(struct clocksource *cs);
@@ -306,4 +312,25 @@  static inline unsigned int clocksource_get_max_watchdog_retry(void)
 
 void clocksource_verify_percpu(struct clocksource *cs);
 
+/**
+ * struct clocksource_base - hardware abstraction for clock on which a clocksource
+ *			is based
+ * @id:			Defaults to CSID_GENERIC. The id value is used for conversion
+ *			functions which require that the current clocksource is based
+ *			on a clocksource_base with a particular ID in certain snapshot
+ *			functions to allow callers to validate the clocksource from
+ *			which the snapshot was taken.
+ * @freq_khz:		Nominal frequency of the base clock in kHz
+ * @offset:		Offset between the base clock and the clocksource
+ * @numerator:		Numerator of the clock ratio between base clock and the clocksource
+ * @denominator:	Denominator of the clock ratio between base clock and the clocksource
+ */
+struct clocksource_base {
+	enum clocksource_ids	id;
+	u32			freq_khz;
+	u64			offset;
+	u32			numerator;
+	u32			denominator;
+};
+
 #endif /* _LINUX_CLOCKSOURCE_H */
diff --git a/include/linux/clocksource_ids.h b/include/linux/clocksource_ids.h
index a4fa3436940c..2bb4d8c2f1b0 100644
--- a/include/linux/clocksource_ids.h
+++ b/include/linux/clocksource_ids.h
@@ -9,6 +9,7 @@  enum clocksource_ids {
 	CSID_X86_TSC_EARLY,
 	CSID_X86_TSC,
 	CSID_X86_KVM_CLK,
+	CSID_X86_ART,
 	CSID_MAX,
 };
 
diff --git a/include/linux/timekeeping.h b/include/linux/timekeeping.h
index 0ea7823b7f31..b2ee182d891e 100644
--- a/include/linux/timekeeping.h
+++ b/include/linux/timekeeping.h
@@ -310,10 +310,12 @@  struct system_device_crosststamp {
  *		timekeeping code to verify comparability of two cycle values.
  *		The default ID, CSID_GENERIC, does not identify a specific
  *		clocksource.
+ * @use_nsecs:	@cycles is in nanoseconds.
  */
 struct system_counterval_t {
 	u64			cycles;
 	enum clocksource_ids	cs_id;
+	bool			use_nsecs;
 };
 
 /*
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index b58dffc58a8f..2542cfefbdee 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -1193,6 +1193,40 @@  static bool timestamp_in_interval(u64 start, u64 end, u64 ts)
 	return false;
 }
 
+static bool convert_clock(u64 *val, u32 numerator, u32 denominator)
+{
+	u64 rem, res;
+
+	if (!numerator || !denominator)
+		return false;
+
+	res = div64_u64_rem(*val, denominator, &rem) * numerator;
+	*val = res + div_u64(rem * numerator, denominator);
+	return true;
+}
+
+static bool convert_base_to_cs(struct system_counterval_t *scv)
+{
+	struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
+	struct clocksource_base *base = cs->base;
+	u32 num, den;
+
+	/* The timestamp was taken from the time keeper clock source */
+	if (cs->id == scv->cs_id)
+		return true;
+
+	/* Check whether cs_id matches the base clock */
+	if (!base || base->id != scv->cs_id)
+		return false;
+
+	num = scv->use_nsecs ? cs->freq_khz : base->numerator;
+	den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
+
+	convert_clock(&scv->cycles, num, den);
+	scv->cycles += base->offset;
+	return true;
+}
+
 /**
  * get_device_system_crosststamp - Synchronously capture system/device timestamp
  * @get_time_fn:	Callback to get simultaneous device time and
@@ -1238,8 +1272,7 @@  int get_device_system_crosststamp(int (*get_time_fn)
 		 * system counter value is the same as for the currently
 		 * installed timekeeper clocksource
 		 */
-		if (system_counterval.cs_id == CSID_GENERIC ||
-		    tk->tkr_mono.clock->id != system_counterval.cs_id)
+		if (!convert_base_to_cs(&system_counterval))
 			return -ENODEV;
 		cycles = system_counterval.cycles;