RE: [patch 19/48] clockevents: Provide support for clocksource coupled comparators

[Michael Kelley]

From: mhklkml@xxxxxxxxxxxx <mhklkml@xxxxxxxxxxxx> Sent: Monday, March 23, 2026 5:22 PM
> 
> From: Thomas Gleixner <tglx@xxxxxxxxxx> Sent: Monday, March 23, 2026 2:37 PM
> >
> > On Mon, Mar 23 2026 at 04:24, Michael Kelley wrote:
> > >> > +static inline bool clockevent_set_next_coupled(struct clock_event_device *dev, ktime_t expires)
> > >> > +{
> > >> > +	u64 cycles;
> > >> > +
> > >> > +	if (unlikely(!(dev->features & CLOCK_EVT_FEAT_CLOCKSOURCE_COUPLED)))
> > >> > +		return false;
> > >> > +
> > >> > +	if (unlikely(!ktime_expiry_to_cycles(dev->cs_id, expires, &cycles)))
> > >> > +		return false;
> > >> > +
> > >> > +	if (IS_ENABLED(CONFIG_GENERIC_CLOCKEVENTS_COUPLED_INLINE))
> > >>
> > >> Since COUPLED_INLINE is always selected for x64, there's no way to add the Hyper-V
> > >> clockevent that is coupled but not inline. Adding the machinery to allow a second
> > >> inline clockevent type may not be worth it, but adding a second coupled but not
> > >> inline clockevent type on x64 should be supported. Thoughts?
> > >>
> > >> After fixing the u64 typo, and temporarily not always selecting COUPLED_INLINE in
> > >> arch/x86/Kconfig, the coupled Hyper-V TSC page clocksource and timer seem to work
> > >> correctly, though I'm still doing some testing. I'm also working on counting the number
> > >> of time reads to confirm the expected benefit.
> > >
> > > Gentle ping.  Any thoughts on this?  (And on Peter Zijlstra's "deliciously insane"
> > > follow-up?)
> >
> > Sure, we should be able to support that and I think Peter's suggestion
> > is pretty clever. Did you get it working?
> >
> 
> I got the coupling working with the Hyper-V clocksource and timer in
> non-inlined form, and observed the expected reduction in time reads.
> That was straightforward.
> 
> But I did not try Peter's suggestion, just to keep things simple. The
> Hyper-V timer can be loaded with a single wrmsrq just like the TSC
> deadline timer, but it's a different MSR that's synthetic and always
> traps to the hypervisor. Given the unavoidable overhead of trapping
> to the hypervisor, the relative gain of inlining would be small.
> 
> Michael
> 

Another thought occurred to me. Since the Hyper-V timer "set next event"
does wrmsrq just like the TSC deadline timer, add an "msr" field to struct
clock_event_device, and require clock events that specify COUPLED to also
specify the MSR. Here's a diff of the core changes:

diff --git a/arch/x86/include/asm/clock_inlined.h b/arch/x86/include/asm/clock_inlined.h
index b2dee8db2fb9..a3a3c3670ad4 100644
--- a/arch/x86/include/asm/clock_inlined.h
+++ b/arch/x86/include/asm/clock_inlined.h
@@ -16,7 +16,7 @@ struct clock_event_device;
 static __always_inline void
 arch_inlined_clockevent_set_next_coupled(u64 cycles, struct clock_event_device *evt)
 {
-	native_wrmsrq(MSR_IA32_TSC_DEADLINE, cycles);
+	native_wrmsrq(evt->msr, cycles);
 }
 
 #endif
diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
index 60cab20b7901..42326c7d3f41 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -593,6 +593,7 @@ static void setup_APIC_timer(void)
 		levt->name = "lapic-deadline";
 		levt->features &= ~(CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_DUMMY);
 		levt->features |= CLOCK_EVT_FEAT_CLOCKSOURCE_COUPLED;
+		levt->msr = MSR_IA32_TSC_DEADLINE;
 		levt->cs_id = CSID_X86_TSC;
 		levt->set_next_event = lapic_next_deadline;
 		clockevents_config_and_register(levt, tsc_khz * (1000 / TSC_DIVISOR), 0xF, ~0UL);
diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h
index 92d90220c0d4..deea69580db0 100644
--- a/include/linux/clockchips.h
+++ b/include/linux/clockchips.h
@@ -110,6 +110,7 @@ struct clock_event_device {
 	enum clock_event_state	state_use_accessors;
 	unsigned int		features;
 	enum clocksource_ids	cs_id;
+	u32			msr;
 	unsigned long		retries;
 
 	int			(*set_state_periodic)(struct clock_event_device *);

This approach is not as general as Peter's, but it covers the Hyper-V timer
case, and is simpler. The cost is an extra memory reference in
arch_inlined_clockevent_set_next_coupled(). arch/x86/Kconfig can continue
to select GENERIC_CLOCKEVENTS_COUPLED_INLINE without preventing
coupling of the Hyper-V clocksource and timer. I've built and run this with
a coupled Hyper-V clocksource and timer, and a basic smoke test works.
So something to consider ...  

If you like this approach, I'm happy to submit this as a patch. It would
be a prerequisite to my patch for the Hyper-V clocksource/timer changes
to enable coupling.

Michael