[PATCH v2 0/2] x86/delay: Introduce TPAUSE instruction

[PATCH v2 0/2] x86/delay: Introduce TPAUSE instruction

[Kyung Min Park]

Intel processors that support the WAITPKG feature implement
the TPAUSE instruction that suspends execution in a lower power
state until the TSC (Time Stamp Counter) exceeds a certain value.

Update the udelay() function to use TPAUSE on systems where it
is available. Note that we hard code the deeper (C0.2) sleep
state because exit latency is small compared to the "microseconds"
that usleep() will delay.

ChangeLog:
- Change from v1 to v2:
  1. The patchset applies after Thomas's cleanup patch as below:
     https://lkml.org/lkml/diff/2020/3/18/893/1
  2. Change function/variable names as suggested by Thomas i.e.
     a. Change to delay_halt_fn/delay_halt_mwaitx/delay_halt_tpause from
        wait_func/mwaitx/tpause.
     b. Change variable name loops to cycles.
     c. Change back to the original name delay_fn from delay_platform.
  3. Organize comments to use full width.
  4. Add __ro_after_init for the function pointer delay_halt_fn.
  5. Change patch titles as suggested by Thomas.

Kyung Min Park (2):
  x86/delay: Refactor delay_mwaitx() for TPAUSE support
  x86/delay: Introduce TPAUSE delay

 arch/x86/include/asm/mwait.h | 17 ++++++++++
 arch/x86/lib/delay.c         | 75 +++++++++++++++++++++++++++++++++-----------
 2 files changed, 73 insertions(+), 19 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] x86/delay: Refactor delay_mwaitx() for TPAUSE support

[Kyung Min Park]

Refactor code to make it easier to add a new model specific function to
delay for a number of cycles.

No functional change.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Kyung Min Park <kyung.min.park@intel.com>
---
 arch/x86/lib/delay.c | 48 ++++++++++++++++++++++++++++++------------------
 1 file changed, 30 insertions(+), 18 deletions(-)

diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index a6376cc..e6db855 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -34,6 +34,7 @@ static void delay_loop(u64 __loops);
  * during boot.
  */
 static void (*delay_fn)(u64) __ro_after_init = delay_loop;
+static void (*delay_halt_fn)(u64 start, u64 cycles) __ro_after_init;
 
 /* simple loop based delay: */
 static void delay_loop(u64 __loops)
@@ -100,9 +101,33 @@ static void delay_tsc(u64 cycles)
  * counts with TSC frequency. The input value is the number of TSC cycles
  * to wait. MWAITX will also exit when the timer expires.
  */
-static void delay_mwaitx(u64 cycles)
+static void delay_halt_mwaitx(u64 unused, u64 cycles)
 {
-	u64 start, end, delay;
+	u64 delay;
+
+	delay = min_t(u64, MWAITX_MAX_WAIT_CYCLES, cycles);
+	/*
+	 * Use cpu_tss_rw as a cacheline-aligned, seldomly accessed per-cpu
+	 * variable as the monitor target.
+	 */
+	 __monitorx(raw_cpu_ptr(&cpu_tss_rw), 0, 0);
+
+	/*
+	 * AMD, like Intel, supports the EAX hint and EAX=0xf means, do not
+	 * enter any deep C-state and we use it here in delay() to minimize
+	 * wakeup latency.
+	 */
+	__mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
+}
+
+/*
+ * Call a vendor specific function to delay for a given amount of time. Because
+ * these functions may return earlier than requested, check for actual elapsed
+ * time and call again until done.
+ */
+static void delay_halt(u64 __cycles)
+{
+	u64 start, end, cycles = __cycles;
 
 	/*
 	 * Timer value of 0 causes MWAITX to wait indefinitely, unless there
@@ -114,21 +139,7 @@ static void delay_mwaitx(u64 cycles)
 	start = rdtsc_ordered();
 
 	for (;;) {
-		delay = min_t(u64, MWAITX_MAX_WAIT_CYCLES, cycles);
-
-		/*
-		 * Use cpu_tss_rw as a cacheline-aligned, seldomly
-		 * accessed per-cpu variable as the monitor target.
-		 */
-		__monitorx(raw_cpu_ptr(&cpu_tss_rw), 0, 0);
-
-		/*
-		 * AMD, like Intel's MWAIT version, supports the EAX hint and
-		 * EAX=0xf0 means, do not enter any deep C-state and we use it
-		 * here in delay() to minimize wakeup latency.
-		 */
-		__mwaitx(MWAITX_DISABLE_CSTATES, delay, MWAITX_ECX_TIMER_ENABLE);
-
+		delay_halt_fn(start, cycles);
 		end = rdtsc_ordered();
 
 		if (cycles <= end - start)
@@ -147,7 +158,8 @@ void use_tsc_delay(void)
 
 void use_mwaitx_delay(void)
 {
-	delay_fn = delay_mwaitx;
+	delay_halt_fn = delay_halt_mwaitx;
+	delay_fn = delay_halt;
 }
 
 int read_current_timer(unsigned long *timer_val)
-- 
2.7.4

[PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Kyung Min Park]

TPAUSE instructs the processor to enter an implementation-dependent
optimized state. The instruction execution wakes up when the time-stamp
counter reaches or exceeds the implicit EDX:EAX 64-bit input value.
The instruction execution also wakes up due to the expiration of
the operating system time-limit or by an external interrupt
or exceptions such as a debug exception or a machine check exception.

TPAUSE offers a choice of two lower power states:
 1. Light-weight power/performance optimized state C0.1
 2. Improved power/performance optimized state C0.2
This way, it can save power with low wake-up latency in comparison to
spinloop based delay. The selection between the two is governed by the
input register.

TPAUSE is available on processors with X86_FEATURE_WAITPKG.

Reviewed-by: Tony Luck <tony.luck@intel.com>
Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Kyung Min Park <kyung.min.park@intel.com>
---
 arch/x86/include/asm/mwait.h | 17 +++++++++++++++++
 arch/x86/lib/delay.c         | 27 ++++++++++++++++++++++++++-
 2 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
index aaf6643..fd59db0 100644
--- a/arch/x86/include/asm/mwait.h
+++ b/arch/x86/include/asm/mwait.h
@@ -22,6 +22,8 @@
 #define MWAITX_ECX_TIMER_ENABLE		BIT(1)
 #define MWAITX_MAX_WAIT_CYCLES		UINT_MAX
 #define MWAITX_DISABLE_CSTATES		0xf0
+#define TPAUSE_C01_STATE		1
+#define TPAUSE_C02_STATE		0
 
 static inline void __monitor(const void *eax, unsigned long ecx,
 			     unsigned long edx)
@@ -120,4 +122,19 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
 	current_clr_polling();
 }
 
+/*
+ * Caller can specify whether to enter C0.1 (low latency, less
+ * power saving) or C0.2 state (saves more power, but longer wakeup
+ * latency). This may be overridden by the IA32_UMWAIT_CONTROL MSR
+ * which can force requests for C0.2 to be downgraded to C0.1.
+ */
+static inline void __tpause(unsigned int ecx, unsigned int edx,
+			    unsigned int eax)
+{
+	/* "tpause %ecx, %edx, %eax;" */
+	asm volatile(".byte 0x66, 0x0f, 0xae, 0xf1\t\n"
+		     :
+		     : "c"(ecx), "d"(edx), "a"(eax));
+}
+
 #endif /* _ASM_X86_MWAIT_H */
diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
index e6db855..5f11f0a 100644
--- a/arch/x86/lib/delay.c
+++ b/arch/x86/lib/delay.c
@@ -97,6 +97,27 @@ static void delay_tsc(u64 cycles)
 }
 
 /*
+ * On Intel the TPAUSE instruction waits until any of:
+ * 1) the TSC counter exceeds the value provided in EAX:EDX
+ * 2) global timeout in IA32_UMWAIT_CONTROL is exceeded
+ * 3) an external interrupt occurs
+ */
+static void delay_halt_tpause(u64 start, u64 cycles)
+{
+	u64 until = start + cycles;
+	unsigned int eax, edx;
+
+	eax = (unsigned int)(until & 0xffffffff);
+	edx = (unsigned int)(until >> 32);
+
+	/*
+	 * Hard code the deeper (C0.2) sleep state because exit latency is
+	 * small compared to the "microseconds" that usleep() will delay.
+	 */
+	__tpause(TPAUSE_C02_STATE, edx, eax);
+}
+
+/*
  * On some AMD platforms, MWAITX has a configurable 32-bit timer, that
  * counts with TSC frequency. The input value is the number of TSC cycles
  * to wait. MWAITX will also exit when the timer expires.
@@ -152,8 +173,12 @@ static void delay_halt(u64 __cycles)
 
 void use_tsc_delay(void)
 {
-	if (delay_fn == delay_loop)
+	if (static_cpu_has(X86_FEATURE_WAITPKG)) {
+		delay_halt_fn = delay_halt_tpause;
+		delay_fn = delay_halt;
+	} else if (delay_fn == delay_loop) {
 		delay_fn = delay_tsc;
+	}
 }
 
 void use_mwaitx_delay(void)
-- 
2.7.4

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Andy Lutomirski]

On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park <kyung.min.park@intel.com> wrote:
>
> TPAUSE instructs the processor to enter an implementation-dependent
> optimized state. The instruction execution wakes up when the time-stamp
> counter reaches or exceeds the implicit EDX:EAX 64-bit input value.
> The instruction execution also wakes up due to the expiration of
> the operating system time-limit or by an external interrupt
> or exceptions such as a debug exception or a machine check exception.
>
> TPAUSE offers a choice of two lower power states:
>  1. Light-weight power/performance optimized state C0.1
>  2. Improved power/performance optimized state C0.2
> This way, it can save power with low wake-up latency in comparison to
> spinloop based delay. The selection between the two is governed by the
> input register.
>
> TPAUSE is available on processors with X86_FEATURE_WAITPKG.
>
> Reviewed-by: Tony Luck <tony.luck@intel.com>
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Kyung Min Park <kyung.min.park@intel.com>
> ---
>  arch/x86/include/asm/mwait.h | 17 +++++++++++++++++
>  arch/x86/lib/delay.c         | 27 ++++++++++++++++++++++++++-
>  2 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/include/asm/mwait.h b/arch/x86/include/asm/mwait.h
> index aaf6643..fd59db0 100644
> --- a/arch/x86/include/asm/mwait.h
> +++ b/arch/x86/include/asm/mwait.h
> @@ -22,6 +22,8 @@
>  #define MWAITX_ECX_TIMER_ENABLE                BIT(1)
>  #define MWAITX_MAX_WAIT_CYCLES         UINT_MAX
>  #define MWAITX_DISABLE_CSTATES         0xf0
> +#define TPAUSE_C01_STATE               1
> +#define TPAUSE_C02_STATE               0
>
>  static inline void __monitor(const void *eax, unsigned long ecx,
>                              unsigned long edx)
> @@ -120,4 +122,19 @@ static inline void mwait_idle_with_hints(unsigned long eax, unsigned long ecx)
>         current_clr_polling();
>  }
>
> +/*
> + * Caller can specify whether to enter C0.1 (low latency, less
> + * power saving) or C0.2 state (saves more power, but longer wakeup
> + * latency). This may be overridden by the IA32_UMWAIT_CONTROL MSR
> + * which can force requests for C0.2 to be downgraded to C0.1.
> + */
> +static inline void __tpause(unsigned int ecx, unsigned int edx,
> +                           unsigned int eax)
> +{
> +       /* "tpause %ecx, %edx, %eax;" */
> +       asm volatile(".byte 0x66, 0x0f, 0xae, 0xf1\t\n"
> +                    :
> +                    : "c"(ecx), "d"(edx), "a"(eax));
> +}
> +
>  #endif /* _ASM_X86_MWAIT_H */
> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
> index e6db855..5f11f0a 100644
> --- a/arch/x86/lib/delay.c
> +++ b/arch/x86/lib/delay.c
> @@ -97,6 +97,27 @@ static void delay_tsc(u64 cycles)
>  }
>
>  /*
> + * On Intel the TPAUSE instruction waits until any of:
> + * 1) the TSC counter exceeds the value provided in EAX:EDX
> + * 2) global timeout in IA32_UMWAIT_CONTROL is exceeded
> + * 3) an external interrupt occurs
> + */
> +static void delay_halt_tpause(u64 start, u64 cycles)
> +{
> +       u64 until = start + cycles;
> +       unsigned int eax, edx;
> +
> +       eax = (unsigned int)(until & 0xffffffff);
> +       edx = (unsigned int)(until >> 32);
> +
> +       /*
> +        * Hard code the deeper (C0.2) sleep state because exit latency is
> +        * small compared to the "microseconds" that usleep() will delay.
> +        */
> +       __tpause(TPAUSE_C02_STATE, edx, eax);
> +}
> +
> +/*
>   * On some AMD platforms, MWAITX has a configurable 32-bit timer, that
>   * counts with TSC frequency. The input value is the number of TSC cycles
>   * to wait. MWAITX will also exit when the timer expires.
> @@ -152,8 +173,12 @@ static void delay_halt(u64 __cycles)
>
>  void use_tsc_delay(void)
>  {
> -       if (delay_fn == delay_loop)
> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
> +               delay_halt_fn = delay_halt_tpause;
> +               delay_fn = delay_halt;
> +       } else if (delay_fn == delay_loop) {
>                 delay_fn = delay_tsc;
> +       }
>  }

This is an odd way to dispatch: you're using static_cpu_has(), but
you're using it once to populate a function pointer.  Why not just put
the static_cpu_has() directly into delay_halt() and open-code the
three variants?  That will also make it a lot easier to understand the
oddity with start and cycles.

--Andy

> --
> 2.7.4
>

Re: [PATCH v2 0/2] x86/delay: Introduce TPAUSE instruction

[Thomas Gleixner]

Hi!

Kyung Min Park <kyung.min.park@intel.com> writes:

> Intel processors that support the WAITPKG feature implement
> the TPAUSE instruction that suspends execution in a lower power
> state until the TSC (Time Stamp Counter) exceeds a certain value.
>
> Update the udelay() function to use TPAUSE on systems where it
> is available. Note that we hard code the deeper (C0.2) sleep
> state because exit latency is small compared to the "microseconds"
> that usleep() will delay.
>
> ChangeLog:
> - Change from v1 to v2:
>   1. The patchset applies after Thomas's cleanup patch as below:
>      https://lkml.org/lkml/diff/2020/3/18/893/1

lkml.org is horrible. Please use lore.kernel.org if at all.

Also please just add the patch to the series when posting so that people
don't have to go through loops and hoops to grab that dependency.

Thanks,

        tglx

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Thomas Gleixner]

Andy Lutomirski <luto@kernel.org> writes:
> On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park <kyung.min.park@intel.com> wrote:
>>  void use_tsc_delay(void)
>>  {
>> -       if (delay_fn == delay_loop)
>> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
>> +               delay_halt_fn = delay_halt_tpause;
>> +               delay_fn = delay_halt;
>> +       } else if (delay_fn == delay_loop) {
>>                 delay_fn = delay_tsc;
>> +       }
>>  }
>
> This is an odd way to dispatch: you're using static_cpu_has(), but
> you're using it once to populate a function pointer.  Why not just put
> the static_cpu_has() directly into delay_halt() and open-code the
> three variants?

Two: mwaitx and tpause.

> That will also make it a lot easier to understand the oddity with
> start and cycles.

Indeed. That makes sense. Should have thought about it :)

Thanks,

        tglx

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Joe Perches]

On Thu, 2020-03-19 at 21:13 -0700, Kyung Min Park wrote:
> TPAUSE instructs the processor to enter an implementation-dependent
> optimized state. The instruction execution wakes up when the time-stamp
> counter reaches or exceeds the implicit EDX:EAX 64-bit input value.
> The instruction execution also wakes up due to the expiration of
> the operating system time-limit or by an external interrupt
> or exceptions such as a debug exception or a machine check exception.
[]
> diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
[]
> @@ -97,6 +97,27 @@ static void delay_tsc(u64 cycles)
>  }
>  
>  /*
> + * On Intel the TPAUSE instruction waits until any of:
> + * 1) the TSC counter exceeds the value provided in EAX:EDX
> + * 2) global timeout in IA32_UMWAIT_CONTROL is exceeded
> + * 3) an external interrupt occurs
> + */
> +static void delay_halt_tpause(u64 start, u64 cycles)
> +{
> +	u64 until = start + cycles;
> +	unsigned int eax, edx;
> +
> +	eax = (unsigned int)(until & 0xffffffff);
> +	edx = (unsigned int)(until >> 32);

trivia:

perhaps lower_32_bits and upper_32_bits

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Andy Lutomirski]

On Fri, Mar 20, 2020 at 3:00 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Andy Lutomirski <luto@kernel.org> writes:
> > On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park <kyung.min.park@intel.com> wrote:
> >>  void use_tsc_delay(void)
> >>  {
> >> -       if (delay_fn == delay_loop)
> >> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
> >> +               delay_halt_fn = delay_halt_tpause;
> >> +               delay_fn = delay_halt;
> >> +       } else if (delay_fn == delay_loop) {
> >>                 delay_fn = delay_tsc;
> >> +       }
> >>  }
> >
> > This is an odd way to dispatch: you're using static_cpu_has(), but
> > you're using it once to populate a function pointer.  Why not just put
> > the static_cpu_has() directly into delay_halt() and open-code the
> > three variants?
>
> Two: mwaitx and tpause.

I was imagining there would also be a variant for systems with neither feature.

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Thomas Gleixner]

Andy Lutomirski <luto@kernel.org> writes:

> On Fri, Mar 20, 2020 at 3:00 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> Andy Lutomirski <luto@kernel.org> writes:
>> > On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park <kyung.min.park@intel.com> wrote:
>> >>  void use_tsc_delay(void)
>> >>  {
>> >> -       if (delay_fn == delay_loop)
>> >> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
>> >> +               delay_halt_fn = delay_halt_tpause;
>> >> +               delay_fn = delay_halt;
>> >> +       } else if (delay_fn == delay_loop) {
>> >>                 delay_fn = delay_tsc;
>> >> +       }
>> >>  }
>> >
>> > This is an odd way to dispatch: you're using static_cpu_has(), but
>> > you're using it once to populate a function pointer.  Why not just put
>> > the static_cpu_has() directly into delay_halt() and open-code the
>> > three variants?
>>
>> Two: mwaitx and tpause.
>
> I was imagining there would also be a variant for systems with neither feature.

Oh I see, you want to get rid of both function pointers. That's tricky.

The boot time function is delay_loop() which is using the magic (1 << 12)
boot time value until calibration in one way or the other happens and
something calls use_tsc_delay() or use_mwaitx_delay(). Yes, that's all
horrible but X86_FEATURE_TSC is unusable for this.

Let me think about it.

Thanks,

        tglx

RE: [PATCH v2 0/2] x86/delay: Introduce TPAUSE instruction

[Park, Kyung Min]

Hi Thomas,

> -----Original Message-----
> From: Thomas Gleixner <tglx@linutronix.de>
> Sent: Friday, March 20, 2020 2:57 AM
> To: Park, Kyung Min <kyung.min.park@intel.com>; x86@kernel.org; linux-
> kernel@vger.kernel.org
> Cc: mingo@redhat.com; hpa@zytor.com; gregkh@linuxfoundation.org;
> ak@linux.intel.com; Luck, Tony <tony.luck@intel.com>; Raj, Ashok
> <ashok.raj@intel.com>; Shankar, Ravi V <ravi.v.shankar@intel.com>; Yu,
> Fenghua <fenghua.yu@intel.com>; Park, Kyung Min
> <kyung.min.park@intel.com>
> Subject: Re: [PATCH v2 0/2] x86/delay: Introduce TPAUSE instruction
> 
> Hi!
> 
> Kyung Min Park <kyung.min.park@intel.com> writes:
> 
> > Intel processors that support the WAITPKG feature implement the TPAUSE
> > instruction that suspends execution in a lower power state until the
> > TSC (Time Stamp Counter) exceeds a certain value.
> >
> > Update the udelay() function to use TPAUSE on systems where it is
> > available. Note that we hard code the deeper (C0.2) sleep state
> > because exit latency is small compared to the "microseconds"
> > that usleep() will delay.
> >
> > ChangeLog:
> > - Change from v1 to v2:
> >   1. The patchset applies after Thomas's cleanup patch as below:
> >      https://lkml.org/lkml/diff/2020/3/18/893/1
> 
> lkml.org is horrible. Please use lore.kernel.org if at all.

Let me change in the next patch.

> Also please just add the patch to the series when posting so that people don't
> have to go through loops and hoops to grab that dependency.

Sure, Let me add this patch to the series.

> Thanks,
> 
>         tglx
> 

Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Andy Lutomirski]

On Fri, Mar 20, 2020 at 4:23 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> Andy Lutomirski <luto@kernel.org> writes:
>
> > On Fri, Mar 20, 2020 at 3:00 AM Thomas Gleixner <tglx@linutronix.de> wrote:
> >>
> >> Andy Lutomirski <luto@kernel.org> writes:
> >> > On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park <kyung.min.park@intel.com> wrote:
> >> >>  void use_tsc_delay(void)
> >> >>  {
> >> >> -       if (delay_fn == delay_loop)
> >> >> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
> >> >> +               delay_halt_fn = delay_halt_tpause;
> >> >> +               delay_fn = delay_halt;
> >> >> +       } else if (delay_fn == delay_loop) {
> >> >>                 delay_fn = delay_tsc;
> >> >> +       }
> >> >>  }
> >> >
> >> > This is an odd way to dispatch: you're using static_cpu_has(), but
> >> > you're using it once to populate a function pointer.  Why not just put
> >> > the static_cpu_has() directly into delay_halt() and open-code the
> >> > three variants?
> >>
> >> Two: mwaitx and tpause.
> >
> > I was imagining there would also be a variant for systems with neither feature.
>
> Oh I see, you want to get rid of both function pointers. That's tricky.
>
> The boot time function is delay_loop() which is using the magic (1 << 12)
> boot time value until calibration in one way or the other happens and
> something calls use_tsc_delay() or use_mwaitx_delay(). Yes, that's all
> horrible but X86_FEATURE_TSC is unusable for this.
>
> Let me think about it.

This is definitely not worth overoptimizing.  It's a *delay* function
-- the retpoline isn't going to kill us :)

>
> Thanks,
>
>         tglx
>
>
>
>
>
>
>
>

RE: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Park, Kyung Min]

Hi Joe,

> -----Original Message-----
> From: Joe Perches <joe@perches.com>
> Sent: Friday, March 20, 2020 3:07 AM
> To: Park, Kyung Min <kyung.min.park@intel.com>; x86@kernel.org; linux-
> kernel@vger.kernel.org
> Cc: tglx@linutronix.de; mingo@redhat.com; hpa@zytor.com;
> gregkh@linuxfoundation.org; ak@linux.intel.com; Luck, Tony
> <tony.luck@intel.com>; Raj, Ashok <ashok.raj@intel.com>; Shankar, Ravi V
> <ravi.v.shankar@intel.com>; Yu, Fenghua <fenghua.yu@intel.com>
> Subject: Re: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay
> 
> On Thu, 2020-03-19 at 21:13 -0700, Kyung Min Park wrote:
> > TPAUSE instructs the processor to enter an implementation-dependent
> > optimized state. The instruction execution wakes up when the
> > time-stamp counter reaches or exceeds the implicit EDX:EAX 64-bit input value.
> > The instruction execution also wakes up due to the expiration of the
> > operating system time-limit or by an external interrupt or exceptions
> > such as a debug exception or a machine check exception.
> []
> > diff --git a/arch/x86/lib/delay.c b/arch/x86/lib/delay.c
> []
> > @@ -97,6 +97,27 @@ static void delay_tsc(u64 cycles)  }
> >
> >  /*
> > + * On Intel the TPAUSE instruction waits until any of:
> > + * 1) the TSC counter exceeds the value provided in EAX:EDX
> > + * 2) global timeout in IA32_UMWAIT_CONTROL is exceeded
> > + * 3) an external interrupt occurs
> > + */
> > +static void delay_halt_tpause(u64 start, u64 cycles) {
> > +	u64 until = start + cycles;
> > +	unsigned int eax, edx;
> > +
> > +	eax = (unsigned int)(until & 0xffffffff);
> > +	edx = (unsigned int)(until >> 32);
> 
> trivia:
> 
> perhaps lower_32_bits and upper_32_bits

Thank you for your comment. I'll update in the next patch.

RE: [PATCH v2 2/2] x86/delay: Introduce TPAUSE delay

[Park, Kyung Min]

Hi Andy/Thomas,

 On Fri, Mar 20, 2020 at 4:23 PM Thomas Gleixner <tglx@linutronix.de> wrote:
> >
> > Andy Lutomirski <luto@kernel.org> writes:
> >
> > > On Fri, Mar 20, 2020 at 3:00 AM Thomas Gleixner <tglx@linutronix.de>
> wrote:
> > >>
> > >> Andy Lutomirski <luto@kernel.org> writes:
> > >> > On Thu, Mar 19, 2020 at 9:13 PM Kyung Min Park
> <kyung.min.park@intel.com> wrote:
> > >> >>  void use_tsc_delay(void)
> > >> >>  {
> > >> >> -       if (delay_fn == delay_loop)
> > >> >> +       if (static_cpu_has(X86_FEATURE_WAITPKG)) {
> > >> >> +               delay_halt_fn = delay_halt_tpause;
> > >> >> +               delay_fn = delay_halt;
> > >> >> +       } else if (delay_fn == delay_loop) {
> > >> >>                 delay_fn = delay_tsc;
> > >> >> +       }
> > >> >>  }
> > >> >
> > >> > This is an odd way to dispatch: you're using static_cpu_has(),
> > >> > but you're using it once to populate a function pointer.  Why not
> > >> > just put the static_cpu_has() directly into delay_halt() and
> > >> > open-code the three variants?
> > >>
> > >> Two: mwaitx and tpause.
> > >
> > > I was imagining there would also be a variant for systems with neither
> feature.
> >
> > Oh I see, you want to get rid of both function pointers. That's tricky.
> >
> > The boot time function is delay_loop() which is using the magic (1 <<
> > 12) boot time value until calibration in one way or the other happens
> > and something calls use_tsc_delay() or use_mwaitx_delay(). Yes, that's
> > all horrible but X86_FEATURE_TSC is unusable for this.
> >
> > Let me think about it.
> 
> This is definitely not worth overoptimizing.  It's a *delay* function
> -- the retpoline isn't going to kill us :)

Since the use_tsc_delay() is used just once in __init tsc_init(), 
how about adding "__init" to the use_tsc_delay() and keep these function pointers?