linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc()
@ 2022-02-18 22:18 Pete Swain
  2022-02-18 22:18 ` [PATCH 2/2] timers: retpoline mitigation for time funcs Pete Swain
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Pete Swain @ 2022-02-18 22:18 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86
  Cc: H. Peter Anvin, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	John Stultz, Stephen Boyd, Maciej W. Rozycki, Johan Hovold,
	Feng Tang, Paul E. McKenney, Juergen Gross, linux-kernel, kvm,
	Pete Swain

Avoid clash with host driver's INDIRECT_CALLABLE_SCOPE read_tsc()

Signed-off-by: Pete Swain <swine@google.com>
---
 arch/x86/kvm/x86.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index 641044db415d..0424d77cd214 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -2632,7 +2632,7 @@ static inline void adjust_tsc_offset_host(struct kvm_vcpu *vcpu, s64 adjustment)
 
 #ifdef CONFIG_X86_64
 
-static u64 read_tsc(void)
+static u64 kvm_read_host_tsc(void)
 {
 	u64 ret = (u64)rdtsc_ordered();
 	u64 last = pvclock_gtod_data.clock.cycle_last;
@@ -2674,7 +2674,7 @@ static inline u64 vgettsc(struct pvclock_clock *clock, u64 *tsc_timestamp,
 		break;
 	case VDSO_CLOCKMODE_TSC:
 		*mode = VDSO_CLOCKMODE_TSC;
-		*tsc_timestamp = read_tsc();
+		*tsc_timestamp = kvm_read_host_tsc();
 		v = (*tsc_timestamp - clock->cycle_last) &
 			clock->mask;
 		break;
-- 
2.35.1.473.g83b2b277ed-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH 2/2] timers: retpoline mitigation for time funcs
  2022-02-18 22:18 [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Pete Swain
@ 2022-02-18 22:18 ` Pete Swain
  2022-04-10 12:14   ` Thomas Gleixner
  2022-02-18 22:49 ` [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Jim Mattson
  2022-02-19  8:40 ` Paolo Bonzini
  2 siblings, 1 reply; 7+ messages in thread
From: Pete Swain @ 2022-02-18 22:18 UTC (permalink / raw)
  To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86
  Cc: H. Peter Anvin, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	John Stultz, Stephen Boyd, Maciej W. Rozycki, Johan Hovold,
	Feng Tang, Paul E. McKenney, Juergen Gross, linux-kernel, kvm,
	Pete Swain

Adds indirect call exports for clock reads from tsc, apic,
hrtimer, via clock-event, timekeeper & posix interfaces.

Signed-off-by: Pete Swain <swine@google.com>
---
 arch/x86/kernel/apic/apic.c    |  8 +++++---
 arch/x86/kernel/tsc.c          |  3 ++-
 include/linux/hrtimer.h        | 19 ++++++++++++++++---
 kernel/time/clockevents.c      |  9 ++++++---
 kernel/time/hrtimer.c          |  3 ++-
 kernel/time/posix-cpu-timers.c |  4 ++--
 kernel/time/posix-timers.c     |  3 ++-
 kernel/time/timekeeping.c      |  2 +-
 8 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index b70344bf6600..523a569dd35e 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -463,15 +463,17 @@ EXPORT_SYMBOL_GPL(setup_APIC_eilvt);
 /*
  * Program the next event, relative to now
  */
-static int lapic_next_event(unsigned long delta,
+INDIRECT_CALLABLE_SCOPE
+int lapic_next_event(unsigned long delta,
 			    struct clock_event_device *evt)
 {
 	apic_write(APIC_TMICT, delta);
 	return 0;
 }
 
-static int lapic_next_deadline(unsigned long delta,
-			       struct clock_event_device *evt)
+INDIRECT_CALLABLE_SCOPE
+int lapic_next_deadline(unsigned long delta,
+		       struct clock_event_device *evt)
 {
 	u64 tsc;
 
diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
index a698196377be..ff2868d5ddea 100644
--- a/arch/x86/kernel/tsc.c
+++ b/arch/x86/kernel/tsc.c
@@ -1090,7 +1090,8 @@ static void tsc_resume(struct clocksource *cs)
  * checking the result of read_tsc() - cycle_last for being negative.
  * That works because CLOCKSOURCE_MASK(64) does not mask out any bit.
  */
-static u64 read_tsc(struct clocksource *cs)
+INDIRECT_CALLABLE_SCOPE
+u64 read_tsc(struct clocksource *cs)
 {
 	return (u64)rdtsc_ordered();
 }
diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
index 0ee140176f10..9d2d110f0b8c 100644
--- a/include/linux/hrtimer.h
+++ b/include/linux/hrtimer.h
@@ -20,6 +20,7 @@
 #include <linux/seqlock.h>
 #include <linux/timer.h>
 #include <linux/timerqueue.h>
+#include <linux/indirect_call_wrapper.h>
 
 struct hrtimer_clock_base;
 struct hrtimer_cpu_base;
@@ -297,14 +298,17 @@ static inline s64 hrtimer_get_expires_ns(const struct hrtimer *timer)
 	return ktime_to_ns(timer->node.expires);
 }
 
+INDIRECT_CALLABLE_DECLARE(extern ktime_t ktime_get(void));
+
 static inline ktime_t hrtimer_expires_remaining(const struct hrtimer *timer)
 {
-	return ktime_sub(timer->node.expires, timer->base->get_time());
+	return ktime_sub(timer->node.expires,
+			INDIRECT_CALL_1(timer->base->get_time, ktime_get));
 }
 
 static inline ktime_t hrtimer_cb_get_time(struct hrtimer *timer)
 {
-	return timer->base->get_time();
+	return INDIRECT_CALL_1(timer->base->get_time, ktime_get);
 }
 
 static inline int hrtimer_is_hres_active(struct hrtimer *timer)
@@ -503,7 +507,9 @@ hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
 static inline u64 hrtimer_forward_now(struct hrtimer *timer,
 				      ktime_t interval)
 {
-	return hrtimer_forward(timer, timer->base->get_time(), interval);
+	return hrtimer_forward(timer,
+			INDIRECT_CALL_1(timer->base->get_time, ktime_get),
+			interval);
 }
 
 /* Precise sleep: */
@@ -536,4 +542,11 @@ int hrtimers_dead_cpu(unsigned int cpu);
 #define hrtimers_dead_cpu	NULL
 #endif
 
+struct clock_event_device;
+INDIRECT_CALLABLE_DECLARE(extern __weak u64 read_tsc(struct clocksource *cs));
+INDIRECT_CALLABLE_DECLARE(extern int thread_cpu_clock_get(
+		const clockid_t which_clock, struct timespec64 *tp));
+INDIRECT_CALLABLE_DECLARE(extern __weak int lapic_next_deadline(
+		unsigned long delta, struct clock_event_device *evt));
+
 #endif
diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
index 003ccf338d20..ac15412e87c4 100644
--- a/kernel/time/clockevents.c
+++ b/kernel/time/clockevents.c
@@ -245,7 +245,8 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 
 		dev->retries++;
 		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-		if (dev->set_next_event((unsigned long) clc, dev) == 0)
+		if (INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
+				  (unsigned long) clc, dev) == 0)
 			return 0;
 
 		if (++i > 2) {
@@ -284,7 +285,8 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
 
 		dev->retries++;
 		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-		if (dev->set_next_event((unsigned long) clc, dev) == 0)
+		if (INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
+				  (unsigned long) clc, dev) == 0)
 			return 0;
 	}
 	return -ETIME;
@@ -331,7 +333,8 @@ int clockevents_program_event(struct clock_event_device *dev, ktime_t expires,
 	delta = max(delta, (int64_t) dev->min_delta_ns);
 
 	clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
-	rc = dev->set_next_event((unsigned long) clc, dev);
+	rc = INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
+			   (unsigned long) clc, dev);
 
 	return (rc && force) ? clockevents_program_min_delta(dev) : rc;
 }
diff --git a/kernel/time/hrtimer.c b/kernel/time/hrtimer.c
index 0ea8702eb516..e1e17fdfcd18 100644
--- a/kernel/time/hrtimer.c
+++ b/kernel/time/hrtimer.c
@@ -1241,7 +1241,8 @@ static int __hrtimer_start_range_ns(struct hrtimer *timer, ktime_t tim,
 	remove_hrtimer(timer, base, true, force_local);
 
 	if (mode & HRTIMER_MODE_REL)
-		tim = ktime_add_safe(tim, base->get_time());
+		tim = ktime_add_safe(tim,
+			INDIRECT_CALL_1(base->get_time, ktime_get));
 
 	tim = hrtimer_update_lowres(timer, tim, mode);
 
diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
index 96b4e7810426..d8bf325fa84e 100644
--- a/kernel/time/posix-cpu-timers.c
+++ b/kernel/time/posix-cpu-timers.c
@@ -1596,8 +1596,8 @@ static int thread_cpu_clock_getres(const clockid_t which_clock,
 {
 	return posix_cpu_clock_getres(THREAD_CLOCK, tp);
 }
-static int thread_cpu_clock_get(const clockid_t which_clock,
-				struct timespec64 *tp)
+INDIRECT_CALLABLE_SCOPE
+int thread_cpu_clock_get(const clockid_t which_clock, struct timespec64 *tp)
 {
 	return posix_cpu_clock_get(THREAD_CLOCK, tp);
 }
diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
index 1cd10b102c51..35eac10ee796 100644
--- a/kernel/time/posix-timers.c
+++ b/kernel/time/posix-timers.c
@@ -1089,7 +1089,8 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock,
 	if (!kc)
 		return -EINVAL;
 
-	error = kc->clock_get_timespec(which_clock, &kernel_tp);
+	error = INDIRECT_CALL_1(kc->clock_get_timespec, thread_cpu_clock_get,
+				which_clock, &kernel_tp);
 
 	if (!error && put_timespec64(&kernel_tp, tp))
 		error = -EFAULT;
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index dcdcb85121e4..2b1a3b146614 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -190,7 +190,7 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr)
 {
 	struct clocksource *clock = READ_ONCE(tkr->clock);
 
-	return clock->read(clock);
+	return INDIRECT_CALL_1(clock->read, read_tsc, clock);
 }
 
 #ifdef CONFIG_DEBUG_TIMEKEEPING
-- 
2.35.1.473.g83b2b277ed-goog


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc()
  2022-02-18 22:18 [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Pete Swain
  2022-02-18 22:18 ` [PATCH 2/2] timers: retpoline mitigation for time funcs Pete Swain
@ 2022-02-18 22:49 ` Jim Mattson
  2022-02-19  8:40 ` Paolo Bonzini
  2 siblings, 0 replies; 7+ messages in thread
From: Jim Mattson @ 2022-02-18 22:49 UTC (permalink / raw)
  To: Pete Swain
  Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
	H. Peter Anvin, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Joerg Roedel, John Stultz,
	Stephen Boyd, Maciej W. Rozycki, Johan Hovold, Feng Tang,
	Paul E. McKenney, Juergen Gross, linux-kernel, kvm

On Fri, Feb 18, 2022 at 2:18 PM Pete Swain <swine@google.com> wrote:
>
> Avoid clash with host driver's INDIRECT_CALLABLE_SCOPE read_tsc()
>
> Signed-off-by: Pete Swain <swine@google.com>
Reviewed-by: Jim Mattson <jmattson@google.com>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc()
  2022-02-18 22:18 [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Pete Swain
  2022-02-18 22:18 ` [PATCH 2/2] timers: retpoline mitigation for time funcs Pete Swain
  2022-02-18 22:49 ` [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Jim Mattson
@ 2022-02-19  8:40 ` Paolo Bonzini
  2 siblings, 0 replies; 7+ messages in thread
From: Paolo Bonzini @ 2022-02-19  8:40 UTC (permalink / raw)
  To: Pete Swain, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
	Dave Hansen, x86
  Cc: H. Peter Anvin, Sean Christopherson, Vitaly Kuznetsov,
	Wanpeng Li, Jim Mattson, Joerg Roedel, John Stultz, Stephen Boyd,
	Maciej W. Rozycki, Johan Hovold, Feng Tang, Paul E. McKenney,
	Juergen Gross, linux-kernel, kvm

On 2/18/22 23:18, Pete Swain wrote:
> Avoid clash with host driver's INDIRECT_CALLABLE_SCOPE read_tsc()
> 
> Signed-off-by: Pete Swain <swine@google.com>
> ---
>   arch/x86/kvm/x86.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> index 641044db415d..0424d77cd214 100644
> --- a/arch/x86/kvm/x86.c
> +++ b/arch/x86/kvm/x86.c
> @@ -2632,7 +2632,7 @@ static inline void adjust_tsc_offset_host(struct kvm_vcpu *vcpu, s64 adjustment)
>   
>   #ifdef CONFIG_X86_64
>   
> -static u64 read_tsc(void)
> +static u64 kvm_read_host_tsc(void)
>   {
>   	u64 ret = (u64)rdtsc_ordered();
>   	u64 last = pvclock_gtod_data.clock.cycle_last;
> @@ -2674,7 +2674,7 @@ static inline u64 vgettsc(struct pvclock_clock *clock, u64 *tsc_timestamp,
>   		break;
>   	case VDSO_CLOCKMODE_TSC:
>   		*mode = VDSO_CLOCKMODE_TSC;
> -		*tsc_timestamp = read_tsc();
> +		*tsc_timestamp = kvm_read_host_tsc();
>   		v = (*tsc_timestamp - clock->cycle_last) &
>   			clock->mask;
>   		break;

Acked-by: Paolo Bonzini <pbonzini@redhat.com>

Paolo


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] timers: retpoline mitigation for time funcs
  2022-02-18 22:18 ` [PATCH 2/2] timers: retpoline mitigation for time funcs Pete Swain
@ 2022-04-10 12:14   ` Thomas Gleixner
  2022-04-11  9:21     ` Thomas Gleixner
  2022-04-11 11:07     ` Thomas Gleixner
  0 siblings, 2 replies; 7+ messages in thread
From: Thomas Gleixner @ 2022-04-10 12:14 UTC (permalink / raw)
  To: Pete Swain, Ingo Molnar, Borislav Petkov, Dave Hansen, x86
  Cc: H. Peter Anvin, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	John Stultz, Stephen Boyd, Maciej W. Rozycki, Johan Hovold,
	Feng Tang, Paul E. McKenney, Juergen Gross, linux-kernel, kvm,
	Pete Swain

On Fri, Feb 18 2022 at 14:18, Pete Swain wrote:
> Adds indirect call exports for clock reads from tsc, apic,
> hrtimer, via clock-event, timekeeper & posix interfaces.

Sure, but why?

> Signed-off-by: Pete Swain <swine@google.com>
> ---
>  arch/x86/kernel/apic/apic.c    |  8 +++++---
>  arch/x86/kernel/tsc.c          |  3 ++-
>  include/linux/hrtimer.h        | 19 ++++++++++++++++---
>  kernel/time/clockevents.c      |  9 ++++++---
>  kernel/time/hrtimer.c          |  3 ++-
>  kernel/time/posix-cpu-timers.c |  4 ++--
>  kernel/time/posix-timers.c     |  3 ++-
>  kernel/time/timekeeping.c      |  2 +-
>  8 files changed, 36 insertions(+), 15 deletions(-)

Can this please be split up properly? 

> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index b70344bf6600..523a569dd35e 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -463,15 +463,17 @@ EXPORT_SYMBOL_GPL(setup_APIC_eilvt);
>  /*
>   * Program the next event, relative to now
>   */
> -static int lapic_next_event(unsigned long delta,
> +INDIRECT_CALLABLE_SCOPE
> +int lapic_next_event(unsigned long delta,
>  			    struct clock_event_device *evt)

So this was formatted properly:

static int lapic_next_event(unsigned long delta,
  			    struct clock_event_device *evt)

Now it turned into garbage:

INDIRECT_CALLABLE_SCOPE
int lapic_next_event(unsigned long delta,
  			    struct clock_event_device *evt)

while:

INDIRECT_CALLABLE_SCOPE int lapic_next_event(unsigned long delta,
					     struct clock_event_device *evt)
or

INDIRECT_CALLABLE_SCOPE
int lapic_next_event(unsigned long delta, struct clock_event_device *evt)

are both what a reader would expect.

Similar issues all over the place.

> diff --git a/arch/x86/kernel/tsc.c b/arch/x86/kernel/tsc.c
> index a698196377be..ff2868d5ddea 100644
> --- a/arch/x86/kernel/tsc.c
> +++ b/arch/x86/kernel/tsc.c
> @@ -1090,7 +1090,8 @@ static void tsc_resume(struct clocksource *cs)
>   * checking the result of read_tsc() - cycle_last for being negative.
>   * That works because CLOCKSOURCE_MASK(64) does not mask out any bit.
>   */
> -static u64 read_tsc(struct clocksource *cs)
> +INDIRECT_CALLABLE_SCOPE
> +u64 read_tsc(struct clocksource *cs)

What's the extra line for?

>  {
>  	return (u64)rdtsc_ordered();
>  }
> diff --git a/include/linux/hrtimer.h b/include/linux/hrtimer.h
> index 0ee140176f10..9d2d110f0b8c 100644
> --- a/include/linux/hrtimer.h
> +++ b/include/linux/hrtimer.h
> @@ -20,6 +20,7 @@
>  #include <linux/seqlock.h>
>  #include <linux/timer.h>
>  #include <linux/timerqueue.h>
> +#include <linux/indirect_call_wrapper.h>
>  
>  struct hrtimer_clock_base;
>  struct hrtimer_cpu_base;
> @@ -297,14 +298,17 @@ static inline s64 hrtimer_get_expires_ns(const struct hrtimer *timer)
>  	return ktime_to_ns(timer->node.expires);
>  }
>  
> +INDIRECT_CALLABLE_DECLARE(extern ktime_t ktime_get(void));
> +
>  static inline ktime_t hrtimer_expires_remaining(const struct hrtimer *timer)
>  {
> -	return ktime_sub(timer->node.expires, timer->base->get_time());
> +	return ktime_sub(timer->node.expires,
> +			INDIRECT_CALL_1(timer->base->get_time, ktime_get));

This wants a proper explanation in a changelog for the hrtimer check,
why this is optimized for ktime_get().

>  }
>  
>  static inline ktime_t hrtimer_cb_get_time(struct hrtimer *timer)
>  {
> -	return timer->base->get_time();
> +	return INDIRECT_CALL_1(timer->base->get_time, ktime_get);
>  }
>  
>  static inline int hrtimer_is_hres_active(struct hrtimer *timer)
> @@ -503,7 +507,9 @@ hrtimer_forward(struct hrtimer *timer, ktime_t now, ktime_t interval);
>  static inline u64 hrtimer_forward_now(struct hrtimer *timer,
>  				      ktime_t interval)
>  {
> -	return hrtimer_forward(timer, timer->base->get_time(), interval);
> +	return hrtimer_forward(timer,
> +			INDIRECT_CALL_1(timer->base->get_time, ktime_get),
> +			interval);
>  }
>  
>  /* Precise sleep: */
> @@ -536,4 +542,11 @@ int hrtimers_dead_cpu(unsigned int cpu);
>  #define hrtimers_dead_cpu	NULL
>  #endif
>  
> +struct clock_event_device;
> +INDIRECT_CALLABLE_DECLARE(extern __weak u64 read_tsc(struct clocksource *cs));
> +INDIRECT_CALLABLE_DECLARE(extern int thread_cpu_clock_get(
> +		const clockid_t which_clock, struct timespec64 *tp));
> +INDIRECT_CALLABLE_DECLARE(extern __weak int lapic_next_deadline(
> +		unsigned long delta, struct clock_event_device *evt));
> +

No. This is not how it works. This is a generic header and x86 specific
muck has no place here.

>  #endif
> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
> index 003ccf338d20..ac15412e87c4 100644
> --- a/kernel/time/clockevents.c
> +++ b/kernel/time/clockevents.c
> @@ -245,7 +245,8 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
>  
>  		dev->retries++;
>  		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
> -		if (dev->set_next_event((unsigned long) clc, dev) == 0)
> +		if (INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
> +				  (unsigned long) clc, dev) == 0)

No. We are not sprinkling x86'isms into generic code.

> diff --git a/kernel/time/posix-cpu-timers.c b/kernel/time/posix-cpu-timers.c
> index 96b4e7810426..d8bf325fa84e 100644
> --- a/kernel/time/posix-cpu-timers.c
> +++ b/kernel/time/posix-cpu-timers.c
> @@ -1596,8 +1596,8 @@ static int thread_cpu_clock_getres(const clockid_t which_clock,
>  {
>  	return posix_cpu_clock_getres(THREAD_CLOCK, tp);
>  }
> -static int thread_cpu_clock_get(const clockid_t which_clock,
> -				struct timespec64 *tp)
> +INDIRECT_CALLABLE_SCOPE
> +int thread_cpu_clock_get(const clockid_t which_clock, struct timespec64 *tp)
>  {
>  	return posix_cpu_clock_get(THREAD_CLOCK, tp);
>  }
> diff --git a/kernel/time/posix-timers.c b/kernel/time/posix-timers.c
> index 1cd10b102c51..35eac10ee796 100644
> --- a/kernel/time/posix-timers.c
> +++ b/kernel/time/posix-timers.c
> @@ -1089,7 +1089,8 @@ SYSCALL_DEFINE2(clock_gettime, const clockid_t, which_clock,
>  	if (!kc)
>  		return -EINVAL;
>  
> -	error = kc->clock_get_timespec(which_clock, &kernel_tp);
> +	error = INDIRECT_CALL_1(kc->clock_get_timespec, thread_cpu_clock_get,
> +				which_clock, &kernel_tp);

The argument for selecting thread_cpu_clock_get() as optimization target
is?

>  
>  	if (!error && put_timespec64(&kernel_tp, tp))
>  		error = -EFAULT;
> diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> index dcdcb85121e4..2b1a3b146614 100644
> --- a/kernel/time/timekeeping.c
> +++ b/kernel/time/timekeeping.c
> @@ -190,7 +190,7 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr)
>  {
>  	struct clocksource *clock = READ_ONCE(tkr->clock);
>  
> -	return clock->read(clock);
> +	return INDIRECT_CALL_1(clock->read, read_tsc, clock);

Again. No X86 muck here.

Thanks,

        tglx

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] timers: retpoline mitigation for time funcs
  2022-04-10 12:14   ` Thomas Gleixner
@ 2022-04-11  9:21     ` Thomas Gleixner
  2022-04-11 11:07     ` Thomas Gleixner
  1 sibling, 0 replies; 7+ messages in thread
From: Thomas Gleixner @ 2022-04-11  9:21 UTC (permalink / raw)
  To: Pete Swain, Ingo Molnar, Borislav Petkov, Dave Hansen, x86
  Cc: H. Peter Anvin, Paolo Bonzini, Sean Christopherson,
	Vitaly Kuznetsov, Wanpeng Li, Jim Mattson, Joerg Roedel,
	John Stultz, Stephen Boyd, Maciej W. Rozycki, Johan Hovold,
	Feng Tang, Paul E. McKenney, Juergen Gross, linux-kernel, kvm,
	Pete Swain

Pete,

On Sun, Apr 10 2022 at 14:14, Thomas Gleixner wrote:
> On Fri, Feb 18 2022 at 14:18, Pete Swain wrote:
>> diff --git a/kernel/time/clockevents.c b/kernel/time/clockevents.c
>> index 003ccf338d20..ac15412e87c4 100644
>> --- a/kernel/time/clockevents.c
>> +++ b/kernel/time/clockevents.c
>> @@ -245,7 +245,8 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
>>  
>>  		dev->retries++;
>>  		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
>> -		if (dev->set_next_event((unsigned long) clc, dev) == 0)
>> +		if (INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
>> +				  (unsigned long) clc, dev) == 0)
>
> No. We are not sprinkling x86'isms into generic code.
>
>> --- a/kernel/time/timekeeping.c
>> +++ b/kernel/time/timekeeping.c
>> @@ -190,7 +190,7 @@ static inline u64 tk_clock_read(const struct tk_read_base *tkr)
>>  {
>>  	struct clocksource *clock = READ_ONCE(tkr->clock);
>>  
>> -	return clock->read(clock);
>> +	return INDIRECT_CALL_1(clock->read, read_tsc, clock);
>
> Again. No X86 muck here.

Just for clarification. I have absolutely no interest in opening this
can of worms. The indirect call is in general more expensive on some
architectures, so when we grant this for x86, then the next architecture
comes around the corner and wants the same treatment. Guess how well
that works and how maintainable this is.

And no, you can't just go there and have a

 #define arch_read_favourite_clocksource		read_foo

because we have multiplatform kernels where the clocksource is selected
at runtime which means every platform wants to be the one defining this.

You can do that in your own frankenkernels, but for mainline this is not
going to fly. You have to come up with something smarter than just
taking your profiling results and slapping INDIRECT_CALL_*() all over
the place. INDIRECT_CALL is a hack as it leaves the conditional around
even if retpoline gets patched out.

The kernel has mechanisms to do better than that.

Let's look at tk_clock_read(). tkr->clock changes usually once maybe
twice during boot. Until shutdown it might change when e.g. TSC becomes
unstable and there are a few other cases like suspend/resume. But none
of these events are hotpath.

While we have several instances of tk_read_base, all of them have the
same clock pointer, except for the rare case of suspend/resume. It's
redundant storage for various reasons.

So with some thought this can be implemented with static_call() which is
currently supported by x86 and powerpc32, but there is no reason why
it can't be supported by other architectures. INDIRECT_CALL is a x86'ism
with a dependency on RETPOLINE, which will never gain traction outside
of x86.

Thanks,

        tglx


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 2/2] timers: retpoline mitigation for time funcs
  2022-04-10 12:14   ` Thomas Gleixner
  2022-04-11  9:21     ` Thomas Gleixner
@ 2022-04-11 11:07     ` Thomas Gleixner
  1 sibling, 0 replies; 7+ messages in thread
From: Thomas Gleixner @ 2022-04-11 11:07 UTC (permalink / raw)
  To: Pete Swain
  Cc: Ingo Molnar, Borislav Petkov, Dave Hansen, x86, H. Peter Anvin,
	Paolo Bonzini, Sean Christopherson, Vitaly Kuznetsov, Wanpeng Li,
	Jim Mattson, Joerg Roedel, John Stultz, Stephen Boyd,
	Maciej W. Rozycki, Johan Hovold, Feng Tang, Paul E. McKenney,
	Juergen Gross, linux-kernel, kvm, Pete Swain

On Sun, Apr 10 2022 at 14:14, Thomas Gleixner wrote:
> On Fri, Feb 18 2022 at 14:18, Pete Swain wrote:
>> @@ -245,7 +245,8 @@ static int clockevents_program_min_delta(struct clock_event_device *dev)
>>  
>>  		dev->retries++;
>>  		clc = ((unsigned long long) delta * dev->mult) >> dev->shift;
>> -		if (dev->set_next_event((unsigned long) clc, dev) == 0)
>> +		if (INDIRECT_CALL_1(dev->set_next_event, lapic_next_deadline,
>> +				  (unsigned long) clc, dev) == 0)
>
> No. We are not sprinkling x86'isms into generic code.

Even if we would do that, then this is completely useless.

The hotpath function is clockevents_program_event() and not
clockevents_program_min_delta().

The latter is invoked from the former if the to be programmed event is
in the past _and_ the force argument is set, i.e. in 0.1% of the
invocations of clockevents_program_event(), which itself has an indirect
call to dev->set_next_event().

If your profiles show clockevents_program_min_delta() as dominant, then
there is something massively wrong with your kernel.

Thanks,

        tglx






^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2022-04-11 11:11 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-18 22:18 [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Pete Swain
2022-02-18 22:18 ` [PATCH 2/2] timers: retpoline mitigation for time funcs Pete Swain
2022-04-10 12:14   ` Thomas Gleixner
2022-04-11  9:21     ` Thomas Gleixner
2022-04-11 11:07     ` Thomas Gleixner
2022-02-18 22:49 ` [PATCH 1/2] kvm/x86: rename kvm's read_tsc() as kvm_read_host_tsc() Jim Mattson
2022-02-19  8:40 ` Paolo Bonzini

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).