Re: [PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock differences inline
On Wed, Jan 27, 2021 at 12:23:43PM -0800, Michael Kelley wrote: > While the Hyper-V Reference TSC code is architecture neutral, the > pv_ops.time.sched_clock() function is implemented for x86/x64, but not > for ARM64. Current code calls a utility function under arch/x86 (and > coming, under arch/arm64) to handle the difference. > > Change this approach to handle the difference inline based on whether > GENERIC_SCHED_CLOCK is present. The new approach removes code under > arch/* since the difference is tied more to the specifics of the Linux > implementation than to the architecture. > > No functional change. > > Signed-off-by: Michael Kelley Reviewed-by: Boqun Feng Regards, Boqun > --- > arch/x86/include/asm/mshyperv.h| 11 --- > drivers/clocksource/hyperv_timer.c | 21 + > 2 files changed, 21 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h > index ed9dc56..5ccbba8 100644 > --- a/arch/x86/include/asm/mshyperv.h > +++ b/arch/x86/include/asm/mshyperv.h > @@ -29,17 +29,6 @@ static inline u64 hv_get_register(unsigned int reg) > > #define hv_get_raw_timer() rdtsc_ordered() > > -/* > - * Reference to pv_ops must be inline so objtool > - * detection of noinstr violations can work correctly. > - */ > -static __always_inline void hv_setup_sched_clock(void *sched_clock) > -{ > -#ifdef CONFIG_PARAVIRT > - pv_ops.time.sched_clock = sched_clock; > -#endif > -} > - > void hyperv_vector_handler(struct pt_regs *regs); > > static inline void hv_enable_stimer0_percpu_irq(int irq) {} > diff --git a/drivers/clocksource/hyperv_timer.c > b/drivers/clocksource/hyperv_timer.c > index 9cee6db..a2bee50 100644 > --- a/drivers/clocksource/hyperv_timer.c > +++ b/drivers/clocksource/hyperv_timer.c > @@ -423,6 +423,27 @@ static u64 notrace read_hv_sched_clock_msr(void) > .flags = CLOCK_SOURCE_IS_CONTINUOUS, > }; > > +/* > + * Reference to pv_ops must be inline so objtool > + * detection of noinstr violations can work correctly. > + */ > +static __always_inline void hv_setup_sched_clock(void *sched_clock) > +{ > +#ifdef CONFIG_GENERIC_SCHED_CLOCK > + /* > + * We're on an architecture with generic sched clock (not x86/x64). > + * The Hyper-V sched clock read function returns nanoseconds, not > + * the normal 100ns units of the Hyper-V synthetic clock. > + */ > + sched_clock_register(sched_clock, 64, NSEC_PER_SEC); > +#else > +#ifdef CONFIG_PARAVIRT > + /* We're on x86/x64 *and* using PV ops */ > + pv_ops.time.sched_clock = sched_clock; > +#endif > +#endif > +} > + > static bool __init hv_init_tsc_clocksource(void) > { > u64 tsc_msr; > -- > 1.8.3.1 >
Re: [PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock differences inline
On Thu, Feb 04, 2021 at 04:28:38PM +, Michael Kelley wrote: > From: Wei Liu Sent: Monday, February 1, 2021 10:55 AM > > > > On Wed, Jan 27, 2021 at 12:23:43PM -0800, Michael Kelley wrote: > > [...] > > > +/* > > > + * Reference to pv_ops must be inline so objtool > > > + * detection of noinstr violations can work correctly. > > > + */ > > > +static __always_inline void hv_setup_sched_clock(void *sched_clock) > > > > sched_clock_register is not trivial. Having __always_inline here is > > going to make the compiled object bloated. > > > > Given this is a static function, I don't think we need to specify any > > inline keyword. The compiler should be able to determine whether this > > function should be inlined all by itself. > > > > Wei. > > There was an explicit request from Peter Zijlstra and Thomas Gleixner > to force it inline. See https://lore.kernel.org/patchwork/patch/1283635/ and > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/include/asm/mshyperv.h?id=b9d8cf2eb3ceecdee3434b87763492aee9e28845 Oh. noinstr. I failed to notice the comment. Sorry for the noise. Wei.
RE: [PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock differences inline
From: Wei Liu Sent: Monday, February 1, 2021 10:55 AM > > On Wed, Jan 27, 2021 at 12:23:43PM -0800, Michael Kelley wrote: > [...] > > +/* > > + * Reference to pv_ops must be inline so objtool > > + * detection of noinstr violations can work correctly. > > + */ > > +static __always_inline void hv_setup_sched_clock(void *sched_clock) > > sched_clock_register is not trivial. Having __always_inline here is > going to make the compiled object bloated. > > Given this is a static function, I don't think we need to specify any > inline keyword. The compiler should be able to determine whether this > function should be inlined all by itself. > > Wei. There was an explicit request from Peter Zijlstra and Thomas Gleixner to force it inline. See https://lore.kernel.org/patchwork/patch/1283635/ and https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/arch/x86/include/asm/mshyperv.h?id=b9d8cf2eb3ceecdee3434b87763492aee9e28845 Michael > > > +{ > > +#ifdef CONFIG_GENERIC_SCHED_CLOCK > > + /* > > +* We're on an architecture with generic sched clock (not x86/x64). > > +* The Hyper-V sched clock read function returns nanoseconds, not > > +* the normal 100ns units of the Hyper-V synthetic clock. > > +*/ > > + sched_clock_register(sched_clock, 64, NSEC_PER_SEC); > > +#else > > +#ifdef CONFIG_PARAVIRT > > + /* We're on x86/x64 *and* using PV ops */ > > + pv_ops.time.sched_clock = sched_clock; > > +#endif > > +#endif > > +} > > + > > static bool __init hv_init_tsc_clocksource(void) > > { > > u64 tsc_msr; > > -- > > 1.8.3.1 > >
Re: [PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock differences inline
On Wed, Jan 27, 2021 at 12:23:43PM -0800, Michael Kelley wrote: [...] > +/* > + * Reference to pv_ops must be inline so objtool > + * detection of noinstr violations can work correctly. > + */ > +static __always_inline void hv_setup_sched_clock(void *sched_clock) sched_clock_register is not trivial. Having __always_inline here is going to make the compiled object bloated. Given this is a static function, I don't think we need to specify any inline keyword. The compiler should be able to determine whether this function should be inlined all by itself. Wei. > +{ > +#ifdef CONFIG_GENERIC_SCHED_CLOCK > + /* > + * We're on an architecture with generic sched clock (not x86/x64). > + * The Hyper-V sched clock read function returns nanoseconds, not > + * the normal 100ns units of the Hyper-V synthetic clock. > + */ > + sched_clock_register(sched_clock, 64, NSEC_PER_SEC); > +#else > +#ifdef CONFIG_PARAVIRT > + /* We're on x86/x64 *and* using PV ops */ > + pv_ops.time.sched_clock = sched_clock; > +#endif > +#endif > +} > + > static bool __init hv_init_tsc_clocksource(void) > { > u64 tsc_msr; > -- > 1.8.3.1 >
[PATCH 08/10] clocksource/drivers/hyper-v: Handle sched_clock differences inline
While the Hyper-V Reference TSC code is architecture neutral, the pv_ops.time.sched_clock() function is implemented for x86/x64, but not for ARM64. Current code calls a utility function under arch/x86 (and coming, under arch/arm64) to handle the difference. Change this approach to handle the difference inline based on whether GENERIC_SCHED_CLOCK is present. The new approach removes code under arch/* since the difference is tied more to the specifics of the Linux implementation than to the architecture. No functional change. Signed-off-by: Michael Kelley --- arch/x86/include/asm/mshyperv.h| 11 --- drivers/clocksource/hyperv_timer.c | 21 + 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/arch/x86/include/asm/mshyperv.h b/arch/x86/include/asm/mshyperv.h index ed9dc56..5ccbba8 100644 --- a/arch/x86/include/asm/mshyperv.h +++ b/arch/x86/include/asm/mshyperv.h @@ -29,17 +29,6 @@ static inline u64 hv_get_register(unsigned int reg) #define hv_get_raw_timer() rdtsc_ordered() -/* - * Reference to pv_ops must be inline so objtool - * detection of noinstr violations can work correctly. - */ -static __always_inline void hv_setup_sched_clock(void *sched_clock) -{ -#ifdef CONFIG_PARAVIRT - pv_ops.time.sched_clock = sched_clock; -#endif -} - void hyperv_vector_handler(struct pt_regs *regs); static inline void hv_enable_stimer0_percpu_irq(int irq) {} diff --git a/drivers/clocksource/hyperv_timer.c b/drivers/clocksource/hyperv_timer.c index 9cee6db..a2bee50 100644 --- a/drivers/clocksource/hyperv_timer.c +++ b/drivers/clocksource/hyperv_timer.c @@ -423,6 +423,27 @@ static u64 notrace read_hv_sched_clock_msr(void) .flags = CLOCK_SOURCE_IS_CONTINUOUS, }; +/* + * Reference to pv_ops must be inline so objtool + * detection of noinstr violations can work correctly. + */ +static __always_inline void hv_setup_sched_clock(void *sched_clock) +{ +#ifdef CONFIG_GENERIC_SCHED_CLOCK + /* +* We're on an architecture with generic sched clock (not x86/x64). +* The Hyper-V sched clock read function returns nanoseconds, not +* the normal 100ns units of the Hyper-V synthetic clock. +*/ + sched_clock_register(sched_clock, 64, NSEC_PER_SEC); +#else +#ifdef CONFIG_PARAVIRT + /* We're on x86/x64 *and* using PV ops */ + pv_ops.time.sched_clock = sched_clock; +#endif +#endif +} + static bool __init hv_init_tsc_clocksource(void) { u64 tsc_msr; -- 1.8.3.1