[v2] KVM: x86: Use fast path for Xen timer delivery

  From: David Woodhouse <dwmw@amazon.co.uk>

Most of the time there's no need to kick the vCPU and deliver the timer
event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
directly from the timer callback, and only fall back to the slow path
when it's necessary to do so.

This gives a significant improvement in timer latency testing (using
nanosleep() for various periods and then measuring the actual time
elapsed).

However, there was a reason¹ the fast path was dropped when this support
was first added. The current code holds vcpu->mutex for all operations on
the kvm->arch.timer_expires field, and the fast path introduces potential
race conditions. So... ensure the hrtimer is *cancelled* before making
changes in kvm_xen_start_timer(), and also when reading the values out
for KVM_XEN_VCPU_ATTR_TYPE_TIMER.

Add some sanity checks to ensure the truth of the claim that all the
other code paths are run with the vcpu loaded. And use hrtimer_cancel()
directly from kvm_xen_destroy_vcpu() to avoid a false positive from the
check in kvm_xen_stop_timer().

¹ https://lore.kernel.org/kvm/846caa99-2e42-4443-1070-84e49d2f11d2@redhat.com/

Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
---

 • v2: Remember, and deal with, those races.

 arch/x86/kvm/xen.c | 64 +++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 58 insertions(+), 6 deletions(-)
  

On 29/09/2023 12:36, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Most of the time there's no need to kick the vCPU and deliver the timer
> event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> directly from the timer callback, and only fall back to the slow path
> when it's necessary to do so.
> 
> This gives a significant improvement in timer latency testing (using
> nanosleep() for various periods and then measuring the actual time
> elapsed).
> 
> However, there was a reason¹ the fast path was dropped when this support
> was first added. The current code holds vcpu->mutex for all operations on
> the kvm->arch.timer_expires field, and the fast path introduces potential
> race conditions. So... ensure the hrtimer is *cancelled* before making
> changes in kvm_xen_start_timer(), and also when reading the values out
> for KVM_XEN_VCPU_ATTR_TYPE_TIMER.
> 
> Add some sanity checks to ensure the truth of the claim that all the
> other code paths are run with the vcpu loaded. And use hrtimer_cancel()
> directly from kvm_xen_destroy_vcpu() to avoid a false positive from the
> check in kvm_xen_stop_timer().
> 
> ¹ https://lore.kernel.org/kvm/846caa99-2e42-4443-1070-84e49d2f11d2@redhat.com/
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> 
>   • v2: Remember, and deal with, those races.
> 
>   arch/x86/kvm/xen.c | 64 +++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 58 insertions(+), 6 deletions(-)
> 

Reviewed-by: Paul Durrant <paul@xen.org>
  

On Fri, Sep 29, 2023, David Woodhouse wrote:
> From: David Woodhouse <dwmw@amazon.co.uk>
> 
> Most of the time there's no need to kick the vCPU and deliver the timer
> event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> directly from the timer callback, and only fall back to the slow path
> when it's necessary to do so.

It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.

> This gives a significant improvement in timer latency testing (using
> nanosleep() for various periods and then measuring the actual time
> elapsed).
> 
> However, there was a reason¹ the fast path was dropped when this support

Heh, please use [1] or [*] like everyone else.  I can barely see that tiny little ¹.

> was first added. The current code holds vcpu->mutex for all operations on
> the kvm->arch.timer_expires field, and the fast path introduces potential
> race conditions. So... ensure the hrtimer is *cancelled* before making
> changes in kvm_xen_start_timer(), and also when reading the values out
> for KVM_XEN_VCPU_ATTR_TYPE_TIMER.
> 
> Add some sanity checks to ensure the truth of the claim that all the
> other code paths are run with the vcpu loaded.  And use hrtimer_cancel()
> directly from kvm_xen_destroy_vcpu() to avoid a false positive from the
> check in kvm_xen_stop_timer().

This should really be at least 2 patches, probably 3:

  1. add the assertions and the destroy_vcpu() change
  2. cancel the timer before starting a new one or reading the value from userspace
  3. use the fastpath delivery from the timer callback

> ¹ https://lore.kernel.org/kvm/846caa99-2e42-4443-1070-84e49d2f11d2@redhat.com/
> 
> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> ---
> 
>  • v2: Remember, and deal with, those races.
> 
>  arch/x86/kvm/xen.c | 64 +++++++++++++++++++++++++++++++++++++++++-----
>  1 file changed, 58 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> index fb1110b2385a..9d0d602a2466 100644
> --- a/arch/x86/kvm/xen.c
> +++ b/arch/x86/kvm/xen.c
> @@ -117,6 +117,8 @@ static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
>  
>  void kvm_xen_inject_timer_irqs(struct kvm_vcpu *vcpu)
>  {
> +	WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> +
>  	if (atomic_read(&vcpu->arch.xen.timer_pending) > 0) {
>  		struct kvm_xen_evtchn e;
>  
> @@ -136,18 +138,41 @@ static enum hrtimer_restart xen_timer_callback(struct hrtimer *timer)
>  {
>  	struct kvm_vcpu *vcpu = container_of(timer, struct kvm_vcpu,
>  					     arch.xen.timer);
> +	struct kvm_xen_evtchn e;
> +	int rc;
> +
>  	if (atomic_read(&vcpu->arch.xen.timer_pending))
>  		return HRTIMER_NORESTART;
>  
> -	atomic_inc(&vcpu->arch.xen.timer_pending);
> -	kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> -	kvm_vcpu_kick(vcpu);
> +	e.vcpu_id = vcpu->vcpu_id;
> +	e.vcpu_idx = vcpu->vcpu_idx;
> +	e.port = vcpu->arch.xen.timer_virq;
> +	e.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
> +
> +	rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
> +	if (rc == -EWOULDBLOCK) {
> +		atomic_inc(&vcpu->arch.xen.timer_pending);
> +		kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> +		kvm_vcpu_kick(vcpu);
> +	} else {
> +		vcpu->arch.xen.timer_expires = 0;

Eww.  IIUC, timer_expires isn't cleared so that the pending IRQ is captured by
kvm_xen_vcpu_get_attr(), i.e. because xen.timer_pending itself isn't migrated.

> +	}
>  
>  	return HRTIMER_NORESTART;
>  }
>  
>  static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_ns)
>  {
> +	WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> +
> +	/*
> +	 * Avoid races with the old timer firing. Checking timer_expires
> +	 * to avoid calling hrtimer_cancel() will only have false positives
> +	 * so is fine.
> +	 */
> +	if (vcpu->arch.xen.timer_expires)
> +		hrtimer_cancel(&vcpu->arch.xen.timer);
> +
>  	atomic_set(&vcpu->arch.xen.timer_pending, 0);
>  	vcpu->arch.xen.timer_expires = guest_abs;
>  
> @@ -163,6 +188,8 @@ static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_
>  
>  static void kvm_xen_stop_timer(struct kvm_vcpu *vcpu)
>  {
> +	WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> +
>  	hrtimer_cancel(&vcpu->arch.xen.timer);
>  	vcpu->arch.xen.timer_expires = 0;
>  	atomic_set(&vcpu->arch.xen.timer_pending, 0);
> @@ -1019,13 +1046,38 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
>  		r = 0;
>  		break;
>  
> -	case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> +	case KVM_XEN_VCPU_ATTR_TYPE_TIMER: {
> +		bool pending = false;
> +
> +		/*
> +		 * Ensure a consistent snapshot of state is captures, with a

s/captures/captured

> +		 * timer either being pending, or fully delivered. Not still

Kind of a nit, but IMO "fully delivered" isn't accurate, at least not without
more clarification.  I would considered "fully delivered" to mean that the IRQ
has caused the guest to start executing its IRQ handler.  Maybe "fully queued in
the event channel"?

> +		 * lurking in the timer_pending flag for deferred delivery.
> +		 */
> +		if (vcpu->arch.xen.timer_expires) {
> +			pending = hrtimer_cancel(&vcpu->arch.xen.timer);
> +			kvm_xen_inject_timer_irqs(vcpu);

If the goal is to not have pending timers, then kvm_xen_inject_timer_irqs()
should be called unconditionally after canceling the hrtimer, no?

> +		}
> +
>  		data->u.timer.port = vcpu->arch.xen.timer_virq;
>  		data->u.timer.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
>  		data->u.timer.expires_ns = vcpu->arch.xen.timer_expires;
> +
> +		/*
> +		 * The timer may be delivered immediately, while the returned
> +		 * state causes it to be set up and delivered again on the

Similar to the "fully delivered" stuff above, maybe s/timer/hrtimer to make it
a bit more clear that the host hrtimer can fire twice, but it won't ever result
in two timer IRQs being delivered from the guest's perspective.

> +		 * destination system after migration. That's fine, as the
> +		 * guest will not even have had a chance to run and process
> +		 * the interrupt by that point, so it won't even notice the
> +		 * duplicate IRQ.

Rather than say "so it won't even notice the duplicate IRQ", maybe be more explicit
and say "so the queued IRQ is guaranteed to be coalesced in the event channel
and/or guest local APIC".  Because I read the whole "delivered IRQ" stuff as there
really being two injected IRQs into the guest.

FWIW, this is all really gross, but I agree that even if the queued IRQ makes it
all the way to the guest's APIC, the IRQs will still be coalesced in the end.


> +		 */
> +		if (pending)
> +			hrtimer_start_expires(&vcpu->arch.xen.timer,
> +					      HRTIMER_MODE_ABS_HARD);
> +
>  		r = 0;
>  		break;
> -
> +	}
>  	case KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR:
>  		data->u.vector = vcpu->arch.xen.upcall_vector;
>  		r = 0;
> @@ -2085,7 +2137,7 @@ void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)
>  void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu)
>  {
>  	if (kvm_xen_timer_enabled(vcpu))

IIUC, this can more precisely be:

	if (vcpu->arch.xen.timer_expires)
		hrtimer_cancel(&vcpu->arch.xen.timer);

at which point it might make sense to add a small helper

  static void kvm_xen_cancel_timer(struct kvm_vcpu *vcpu)
  {
	if (vcpu->arch.xen.timer_expires)
		hrtimer_cancel(&vcpu->arch.xen.timer);
  }

to share code with 

> -		kvm_xen_stop_timer(vcpu);
> +		hrtimer_cancel(&vcpu->arch.xen.timer);
>  
>  	kvm_gpc_deactivate(&vcpu->arch.xen.runstate_cache);
>  	kvm_gpc_deactivate(&vcpu->arch.xen.runstate2_cache);
> -- 
> 2.40.1
> 
>
  

On Fri, 2023-09-29 at 08:16 -0700, Sean Christopherson wrote:
> On Fri, Sep 29, 2023, David Woodhouse wrote:
> > From: David Woodhouse <dwmw@amazon.co.uk>
> > 
> > Most of the time there's no need to kick the vCPU and deliver the timer
> > event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> > directly from the timer callback, and only fall back to the slow path
> > when it's necessary to do so.
> 
> It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
> only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.

That's an implementation detail. Like all of the fast path functions
that can be called from kvm_arch_set_irq_inatomic(), it has its own
criteria for why it might return -EWOULDBLOCK or not. Those are *its*
business. And in fact one of Paul's current patches is tweaking them
subtly, but that isn't relevant here. (But yes, you are broadly correct
in your understanding.)

> > This gives a significant improvement in timer latency testing (using
> > nanosleep() for various periods and then measuring the actual time
> > elapsed).
> > 
> > However, there was a reason¹ the fast path was dropped when this support
> 
> Heh, please use [1] or [*] like everyone else.  I can barely see that tiny little ¹.

Isn't that the *point*? The reference to the footnote isn't supposed to
detract from the flow of the main text. It's exactly how you'll see it
when typeset properly. I've always assumed the people using [1] or [*]
just haven't yet realised that it's the 21st century and we are no
longer limited to 7-bit ASCII. Or haven't worked out how to type
anything but ASCII.

> > was first added. The current code holds vcpu->mutex for all operations on
> > the kvm->arch.timer_expires field, and the fast path introduces potential
> > race conditions. So... ensure the hrtimer is *cancelled* before making
> > changes in kvm_xen_start_timer(), and also when reading the values out
> > for KVM_XEN_VCPU_ATTR_TYPE_TIMER.
> > 
> > Add some sanity checks to ensure the truth of the claim that all the
> > other code paths are run with the vcpu loaded.  And use hrtimer_cancel()
> > directly from kvm_xen_destroy_vcpu() to avoid a false positive from the
> > check in kvm_xen_stop_timer().
> 
> This should really be at least 2 patches, probably 3:
> 
>   1. add the assertions and the destroy_vcpu() change
>   2. cancel the timer before starting a new one or reading the value from userspace
>   3. use the fastpath delivery from the timer callback

Hm, I think that's borderline. I pondered it and came to the opposite
conclusion. Cancelling the timer wasn't needed without the fastpath
delivery; it isn't a separate fix. You could consider it a preparatory
patch I suppose... but I didn't. It's not like adding the fast path in
itself is complex enough that the other parts need to be broken out. 

> > ¹ https://lore.kernel.org/kvm/846caa99-2e42-4443-1070-84e49d2f11d2@redhat.com/
> > 
> > Signed-off-by: David Woodhouse <dwmw@amazon.co.uk>
> > ---
> > 
> >  • v2: Remember, and deal with, those races.
> > 
> >  arch/x86/kvm/xen.c | 64 +++++++++++++++++++++++++++++++++++++++++-----
> >  1 file changed, 58 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/x86/kvm/xen.c b/arch/x86/kvm/xen.c
> > index fb1110b2385a..9d0d602a2466 100644
> > --- a/arch/x86/kvm/xen.c
> > +++ b/arch/x86/kvm/xen.c
> > @@ -117,6 +117,8 @@ static int kvm_xen_shared_info_init(struct kvm *kvm, gfn_t gfn)
> >  
> >  void kvm_xen_inject_timer_irqs(struct kvm_vcpu *vcpu)
> >  {
> > +       WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> > +
> >         if (atomic_read(&vcpu->arch.xen.timer_pending) > 0) {
> >                 struct kvm_xen_evtchn e;
> >  
> > @@ -136,18 +138,41 @@ static enum hrtimer_restart xen_timer_callback(struct hrtimer *timer)
> >  {
> >         struct kvm_vcpu *vcpu = container_of(timer, struct kvm_vcpu,
> >                                              arch.xen.timer);
> > +       struct kvm_xen_evtchn e;
> > +       int rc;
> > +
> >         if (atomic_read(&vcpu->arch.xen.timer_pending))
> >                 return HRTIMER_NORESTART;
> >  
> > -       atomic_inc(&vcpu->arch.xen.timer_pending);
> > -       kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> > -       kvm_vcpu_kick(vcpu);
> > +       e.vcpu_id = vcpu->vcpu_id;
> > +       e.vcpu_idx = vcpu->vcpu_idx;
> > +       e.port = vcpu->arch.xen.timer_virq;
> > +       e.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
> > +
> > +       rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
> > +       if (rc == -EWOULDBLOCK) {
> > +               atomic_inc(&vcpu->arch.xen.timer_pending);
> > +               kvm_make_request(KVM_REQ_UNBLOCK, vcpu);
> > +               kvm_vcpu_kick(vcpu);
> > +       } else {
> > +               vcpu->arch.xen.timer_expires = 0;
> 
> Eww.  IIUC, timer_expires isn't cleared so that the pending IRQ is captured by
> kvm_xen_vcpu_get_attr(), i.e. because xen.timer_pending itself isn't migrated.

-EPARSE. Er... yes?

The timer_expires field is non-zero when there's a pending timer. When
the timer interrupt has fired, the time is no longer pending and the
timer_expires field is set to zero.

The xen.timer_pending field is indeed not migrated. We *flush* it in
kvm_xen_vcpu_get_attr(). It's now only used for the *deferral* to the
slow path.

So... yes, timer_expires *wasn't* previously cleared, because the timer
IRQ hadn't been delivered yet, and yes that was to avoid races with
kvm_xen_vcpu_get_attr(). Partly because the timer_pending field was
internal and not migrated. As far as userspace is concerned, the timer
has either fired, or it has not. Implementation details of the
timer_pending field — and the per-vcpu evtchn_pending_sel — are not
exposed.

> > +       }
> >  
> >         return HRTIMER_NORESTART;
> >  }
> >  
> >  static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_ns)
> >  {
> > +       WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> > +
> > +       /*
> > +        * Avoid races with the old timer firing. Checking timer_expires
> > +        * to avoid calling hrtimer_cancel() will only have false positives
> > +        * so is fine.
> > +        */
> > +       if (vcpu->arch.xen.timer_expires)
> > +               hrtimer_cancel(&vcpu->arch.xen.timer);
> > +
> >         atomic_set(&vcpu->arch.xen.timer_pending, 0);
> >         vcpu->arch.xen.timer_expires = guest_abs;
> >  
> > @@ -163,6 +188,8 @@ static void kvm_xen_start_timer(struct kvm_vcpu *vcpu, u64 guest_abs, s64 delta_
> >  
> >  static void kvm_xen_stop_timer(struct kvm_vcpu *vcpu)
> >  {
> > +       WARN_ON_ONCE(vcpu != kvm_get_running_vcpu());
> > +
> >         hrtimer_cancel(&vcpu->arch.xen.timer);
> >         vcpu->arch.xen.timer_expires = 0;
> >         atomic_set(&vcpu->arch.xen.timer_pending, 0);
> > @@ -1019,13 +1046,38 @@ int kvm_xen_vcpu_get_attr(struct kvm_vcpu *vcpu, struct kvm_xen_vcpu_attr *data)
> >                 r = 0;
> >                 break;
> >  
> > -       case KVM_XEN_VCPU_ATTR_TYPE_TIMER:
> > +       case KVM_XEN_VCPU_ATTR_TYPE_TIMER: {
> > +               bool pending = false;
> > +
> > +               /*
> > +                * Ensure a consistent snapshot of state is captures, with a
> 
> s/captures/captured

Ack.

> > +                * timer either being pending, or fully delivered. Not still
> 
> Kind of a nit, but IMO "fully delivered" isn't accurate, at least not without
> more clarification.  I would considered "fully delivered" to mean that the IRQ
> has caused the guest to start executing its IRQ handler.  Maybe "fully queued in
> the event channel"?

OK, I'll reword.


> > +                * lurking in the timer_pending flag for deferred delivery.
> > +                */
> > +               if (vcpu->arch.xen.timer_expires) {
> > +                       pending = hrtimer_cancel(&vcpu->arch.xen.timer);
> > +                       kvm_xen_inject_timer_irqs(vcpu);
> 
> If the goal is to not have pending timers, then kvm_xen_inject_timer_irqs()
> should be called unconditionally after canceling the hrtimer, no?

It *is* called unconditionally after cancelling the hrtimer.

Or did you mean unconditionally whether we cancel the hrtimer or not?
The comment explains the logic for not needing that. If *either* the
timer is still active, *or* it's already fired and has taken the slow
path and set the timer_pending flag, then timer_expires won't have been
zeroed yet. So the race conditions inherent in doing this conditional
on vcpu->arch.xen.timer_expires are fine because there are only false
positives (which cause us to cancel a timer, and try to inject a
pending IRQ, which wasn't running and wasn't pending respectively).

Sounds like I need to expand that comment? 

> > +               }
> > +
> >                 data->u.timer.port = vcpu->arch.xen.timer_virq;
> >                 data->u.timer.priority = KVM_IRQ_ROUTING_XEN_EVTCHN_PRIO_2LEVEL;
> >                 data->u.timer.expires_ns = vcpu->arch.xen.timer_expires;
> > +
> > +               /*
> > +                * The timer may be delivered immediately, while the returned
> > +                * state causes it to be set up and delivered again on the
> 
> Similar to the "fully delivered" stuff above, maybe s/timer/hrtimer to make it
> a bit more clear that the host hrtimer can fire twice, but it won't ever result
> in two timer IRQs being delivered from the guest's perspective.

Ack.

> > +                * destination system after migration. That's fine, as the
> > +                * guest will not even have had a chance to run and process
> > +                * the interrupt by that point, so it won't even notice the
> > +                * duplicate IRQ.
> 
> Rather than say "so it won't even notice the duplicate IRQ", maybe be more explicit
> and say "so the queued IRQ is guaranteed to be coalesced in the event channel
> and/or guest local APIC".  Because I read the whole "delivered IRQ" stuff as there
> really being two injected IRQs into the guest.
> 
> FWIW, this is all really gross, but I agree that even if the queued IRQ makes it
> all the way to the guest's APIC, the IRQs will still be coalesced in the end.
> 

As discussed before, we *could* have made fetching the timer attr also
*pause* the timer. It just seemed like extra complexity for no good
reason. The shared info page containing the event channel bitmap has to
be migrated after the vCPU state anyway.


> 
> > +                */
> > +               if (pending)
> > +                       hrtimer_start_expires(&vcpu->arch.xen.timer,
> > +                                             HRTIMER_MODE_ABS_HARD);
> > +
> >                 r = 0;
> >                 break;
> > -
> > +       }
> >         case KVM_XEN_VCPU_ATTR_TYPE_UPCALL_VECTOR:
> >                 data->u.vector = vcpu->arch.xen.upcall_vector;
> >                 r = 0;
> > @@ -2085,7 +2137,7 @@ void kvm_xen_init_vcpu(struct kvm_vcpu *vcpu)
> >  void kvm_xen_destroy_vcpu(struct kvm_vcpu *vcpu)
> >  {
> >         if (kvm_xen_timer_enabled(vcpu))
> 
> IIUC, this can more precisely be:
> 
>         if (vcpu->arch.xen.timer_expires)
>                 hrtimer_cancel(&vcpu->arch.xen.timer);
> 
> at which point it might make sense to add a small helper
> 
>   static void kvm_xen_cancel_timer(struct kvm_vcpu *vcpu)
>   {
>         if (vcpu->arch.xen.timer_expires)
>                 hrtimer_cancel(&vcpu->arch.xen.timer);
>   }
> 
> to share code with 
> 
> > -               kvm_xen_stop_timer(vcpu);
> > +               hrtimer_cancel(&vcpu->arch.xen.timer);
> >  
> >         kvm_gpc_deactivate(&vcpu->arch.xen.runstate_cache);
> >         kvm_gpc_deactivate(&vcpu->arch.xen.runstate2_cache);

In fact if I make the helper return a bool, I think I can use it
*three* times. At a cost of having a third function
kvm_xen_cancel_timer() alongside the existing kvm_xen_start_timer() and
kvm_xen_stop_timer(), and those *three* now make for a slightly
confusing set. I'll take a look and see how it makes me feel.
  

On Fri, Sep 29, 2023, David Woodhouse wrote:
> On Fri, 2023-09-29 at 08:16 -0700, Sean Christopherson wrote:
> > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > 
> > > Most of the time there's no need to kick the vCPU and deliver the timer
> > > event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> > > directly from the timer callback, and only fall back to the slow path
> > > when it's necessary to do so.
> > 
> > It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
> > only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.
> 
> That's an implementation detail.

And?  The target audience of changelogs are almost always people that care about
the implementation.

> Like all of the fast path functions that can be called from
> kvm_arch_set_irq_inatomic(), it has its own criteria for why it might return
> -EWOULDBLOCK or not. Those are *its* business.

And all of the KVM code is the business of the people who contribute to the kernel,
now and in the future.  Yeah, there's a small chance that a detailed changelog can
become stale if the patch races with some other in-flight change, but even *that*
is a useful data point.  E.g. if Paul's patches somehow broke/degraded this code,
then knowing that what the author (you) intended/observed didn't match reality when
the patch was applied would be extremely useful information for whoever encountered
the hypothetical breakage.

> And in fact one of Paul's current patches is tweaking them subtly, but that
> isn't relevant here. (But yes, you are broadly correct in your
> understanding.)
> 
> > > This gives a significant improvement in timer latency testing (using
> > > nanosleep() for various periods and then measuring the actual time
> > > elapsed).
> > > 
> > > However, there was a reason¹ the fast path was dropped when this support
> > 
> > Heh, please use [1] or [*] like everyone else.  I can barely see that tiny little ¹.
> 
> Isn't that the *point*? The reference to the footnote isn't supposed to
> detract from the flow of the main text. It's exactly how you'll see it
> when typeset properly.
 
Footnotes that are "typeset properly" have the entire footnote in a different
font+size.  A tiny number next to normal sized text just looks weird to me.

And I often do a "reverse lookup" when I get to footnotes that are links, e.g. to
gauge whether or not it's worth my time to follow the link.  Trying to find the
tiny ¹ via a quick visual scan is an exercise in frustration, at least for the
monospace font I use for reading mail, e.g. it's much more readable on my end in
an editor using a different font.

Which is a big benefit to sticking to the old and kludgly ASCII: it provides a
fairly consistent experience regardless of what client/font/etc each reader is
using.  I'm not completely against using unicode characters, e.g. for names with
characters not found in the Latin alphabet, but for code and things like this,
IMO simpler is better.

> I've always assumed the people using [1] or [*] just haven't yet realised
> that it's the 21st century and we are no longer limited to 7-bit ASCII. Or
> haven't worked out how to type anything but ASCII.

Please don't devolve into ad hominem attacks against other reviews and contributors.
If you want to argue that using footnote notation unicode is superior in some way,
then by all means, present your arguments.
  

On Mon, 2023-10-02 at 10:41 -0700, Sean Christopherson wrote:
> On Fri, Sep 29, 2023, David Woodhouse wrote:
> > On Fri, 2023-09-29 at 08:16 -0700, Sean Christopherson wrote:
> > > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > 
> > > > Most of the time there's no need to kick the vCPU and deliver the timer
> > > > event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> > > > directly from the timer callback, and only fall back to the slow path
> > > > when it's necessary to do so.
> > > 
> > > It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
> > > only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.
> > 
> > That's an implementation detail.
> 
> And?  The target audience of changelogs are almost always people that care about
> the implementation.
>
> > Like all of the fast path functions that can be called from
> > kvm_arch_set_irq_inatomic(), it has its own criteria for why it might return
> > -EWOULDBLOCK or not. Those are *its* business.
> 
> And all of the KVM code is the business of the people who contribute to the kernel,
> now and in the future.  Yeah, there's a small chance that a detailed changelog can
> become stale if the patch races with some other in-flight change, but even *that*
> is a useful data point.  E.g. if Paul's patches somehow broke/degraded this code,
> then knowing that what the author (you) intended/observed didn't match reality when
> the patch was applied would be extremely useful information for whoever encountered
> the hypothetical breakage.

Fair enough, but on this occasion it truly doesn't matter. It has
nothing to do with the implementation of *this* patch. This code makes
no assumptions and has no dependency on *when* that fast path might
return -EWOULDBLOCK. Sometimes it does, sometimes it doesn't. This code
just doesn't care one iota.

If this code had *dependencies* on the precise behaviour of
kvm_xen_set_evtchn_fast() that we needed to reason about, then sure,
I'd have written those explicitly into the commit comment *and* tried
to find some way of enforcing them with runtime warnings etc.

But it doesn't. So I am no more inclined to document the precise
behaviour of kvm_xen_set_evtchn_fast() in a patch which just happens to
call it, than I am inclined to document hrtimer_cancel() or any other
function called from the new code :)

> > And in fact one of Paul's current patches is tweaking them subtly, but that
> > isn't relevant here. (But yes, you are broadly correct in your
> > understanding.)
> > 
> > > > This gives a significant improvement in timer latency testing (using
> > > > nanosleep() for various periods and then measuring the actual time
> > > > elapsed).
> > > > 
> > > > However, there was a reason¹ the fast path was dropped when this support
> > > 
> > > Heh, please use [1] or [*] like everyone else.  I can barely see that tiny little ¹.
> > 
> > Isn't that the *point*? The reference to the footnote isn't supposed to
> > detract from the flow of the main text. It's exactly how you'll see it
> > when typeset properly.
>  
> Footnotes that are "typeset properly" have the entire footnote in a different
> font+size.  A tiny number next to normal sized text just looks weird to me.
> 
> And I often do a "reverse lookup" when I get to footnotes that are links, e.g. to
> gauge whether or not it's worth my time to follow the link.  Trying to find the
> tiny ¹ via a quick visual scan is an exercise in frustration, at least for the
> monospace font I use for reading mail, e.g. it's much more readable on my end in
> an editor using a different font.
> 
> Which is a big benefit to sticking to the old and kludgly ASCII: it provides a
> fairly consistent experience regardless of what client/font/etc each reader is
> using.  I'm not completely against using unicode characters, e.g. for names with
> characters not found in the Latin alphabet, but for code and things like this,
> IMO simpler is better.
> 
> > I've always assumed the people using [1] or [*] just haven't yet realised
> > that it's the 21st century and we are no longer limited to 7-bit ASCII. Or
> > haven't worked out how to type anything but ASCII.
> 
> Please don't devolve into ad hominem attacks against other reviews and contributors.
> If you want to argue that using footnote notation unicode is superior in some way,
> then by all means, present your arguments.

Hey, you started the logical fallacies with the ad populum when you
said "everyone else" :)

Not that that was true; there are examples of ¹ being used in the
kernel changelog going back decades.

I can just drop the footnote; there's not a huge amount of benefit in
referring back to the old mail thread anyway. The gist of it was
covered in the commit comment.
  

On Mon, Oct 02, 2023, David Woodhouse wrote:
> On Mon, 2023-10-02 at 10:41 -0700, Sean Christopherson wrote:
> > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > On Fri, 2023-09-29 at 08:16 -0700, Sean Christopherson wrote:
> > > > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > > 
> > > > > Most of the time there's no need to kick the vCPU and deliver the timer
> > > > > event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> > > > > directly from the timer callback, and only fall back to the slow path
> > > > > when it's necessary to do so.
> > > > 
> > > > It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
> > > > only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.
> > > 
> > > That's an implementation detail.
> > 
> > And?  The target audience of changelogs are almost always people that care about
> > the implementation.
> >
> > > Like all of the fast path functions that can be called from
> > > kvm_arch_set_irq_inatomic(), it has its own criteria for why it might return
> > > -EWOULDBLOCK or not. Those are *its* business.
> > 
> > And all of the KVM code is the business of the people who contribute to the kernel,
> > now and in the future.  Yeah, there's a small chance that a detailed changelog can
> > become stale if the patch races with some other in-flight change, but even *that*
> > is a useful data point.  E.g. if Paul's patches somehow broke/degraded this code,
> > then knowing that what the author (you) intended/observed didn't match reality when
> > the patch was applied would be extremely useful information for whoever encountered
> > the hypothetical breakage.
> 
> Fair enough, but on this occasion it truly doesn't matter. It has
> nothing to do with the implementation of *this* patch. This code makes
> no assumptions and has no dependency on *when* that fast path might
> return -EWOULDBLOCK. Sometimes it does, sometimes it doesn't. This code
> just doesn't care one iota.
> 
> If this code had *dependencies* on the precise behaviour of
> kvm_xen_set_evtchn_fast() that we needed to reason about, then sure,
> I'd have written those explicitly into the commit comment *and* tried
> to find some way of enforcing them with runtime warnings etc.
> 
> But it doesn't. So I am no more inclined to document the precise
> behaviour of kvm_xen_set_evtchn_fast() in a patch which just happens to
> call it, than I am inclined to document hrtimer_cancel() or any other
> function called from the new code :)

Just because some bit of code doesn't care/differentiate doesn't mean the behavior
of said code is correct.  I agree that adding a comment to explain the gory details
is unnecessary and would lead to stale code.  But changelogs essentially capture a
single point in a time, and a big role of the changelog is to help reviewers and
readers understand (a) the *intent* of the change and (b) whether or not that change
is correct.

E.g. there's an assumption that -EWOULDBLOCK is the only non-zero return code where
the correct response is to go down the slow path.

I'm not asking to spell out every single condition, I'm just asking for clarification
on what the intended behavior is, e.g.

  Use kvm_xen_set_evtchn_fast() directly from the timer callback, and fall
  back to the slow path if the event is valid but fast delivery isn't
  possible, which currently can only happen if delivery needs to block,
  e.g. because the gfn=>pfn cache is invalid or stale.

instead of simply saying "when it's necessary to do so" and leaving it up to the
reader to figure what _they_ think that means, which might not always align with
what the author actually meant.

> > > And in fact one of Paul's current patches is tweaking them subtly, but that
> > > isn't relevant here. (But yes, you are broadly correct in your
> > > understanding.)
> > > 
> > > > > This gives a significant improvement in timer latency testing (using
> > > > > nanosleep() for various periods and then measuring the actual time
> > > > > elapsed).
> > > > > 
> > > > > However, there was a reason¹ the fast path was dropped when this support
> > > > 
> > > > Heh, please use [1] or [*] like everyone else.  I can barely see that tiny little ¹.
> > > 
> > > Isn't that the *point*? The reference to the footnote isn't supposed to
> > > detract from the flow of the main text. It's exactly how you'll see it
> > > when typeset properly.
> >  
> > Footnotes that are "typeset properly" have the entire footnote in a different
> > font+size.  A tiny number next to normal sized text just looks weird to me.
> > 
> > And I often do a "reverse lookup" when I get to footnotes that are links, e.g. to
> > gauge whether or not it's worth my time to follow the link.  Trying to find the
> > tiny ¹ via a quick visual scan is an exercise in frustration, at least for the
> > monospace font I use for reading mail, e.g. it's much more readable on my end in
> > an editor using a different font.
> > 
> > Which is a big benefit to sticking to the old and kludgly ASCII: it provides a
> > fairly consistent experience regardless of what client/font/etc each reader is
> > using.  I'm not completely against using unicode characters, e.g. for names with
> > characters not found in the Latin alphabet, but for code and things like this,
> > IMO simpler is better.
> > 
> > > I've always assumed the people using [1] or [*] just haven't yet realised
> > > that it's the 21st century and we are no longer limited to 7-bit ASCII. Or
> > > haven't worked out how to type anything but ASCII.
> > 
> > Please don't devolve into ad hominem attacks against other reviews and contributors.
> > If you want to argue that using footnote notation unicode is superior in some way,
> > then by all means, present your arguments.
> 
> Hey, you started the logical fallacies with the ad populum when you
> said "everyone else" :)
>
> Not that that was true; there are examples of ¹ being used in the
> kernel changelog going back decades.

LOL, fine, "almost everyone else".
  

On Mon, 2023-10-02 at 11:45 -0700, Sean Christopherson wrote:
> On Mon, Oct 02, 2023, David Woodhouse wrote:
> > On Mon, 2023-10-02 at 10:41 -0700, Sean Christopherson wrote:
> > > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > > On Fri, 2023-09-29 at 08:16 -0700, Sean Christopherson wrote:
> > > > > On Fri, Sep 29, 2023, David Woodhouse wrote:
> > > > > > From: David Woodhouse <dwmw@amazon.co.uk>
> > > > > > 
> > > > > > Most of the time there's no need to kick the vCPU and deliver the timer
> > > > > > event through kvm_xen_inject_timer_irqs(). Use kvm_xen_set_evtchn_fast()
> > > > > > directly from the timer callback, and only fall back to the slow path
> > > > > > when it's necessary to do so.
> > > > > 
> > > > > It'd be helpful for non-Xen folks to explain "when it's necessary".  IIUC, the
> > > > > only time it's necessary is if the gfn=>pfn cache isn't valid/fresh.
> > > > 
> > > > That's an implementation detail.
> > > 
> > > And?  The target audience of changelogs are almost always people that care about
> > > the implementation.
> > > 
> > > > Like all of the fast path functions that can be called from
> > > > kvm_arch_set_irq_inatomic(), it has its own criteria for why it might return
> > > > -EWOULDBLOCK or not. Those are *its* business.
> > > 
> > > And all of the KVM code is the business of the people who contribute to the kernel,
> > > now and in the future.  Yeah, there's a small chance that a detailed changelog can
> > > become stale if the patch races with some other in-flight change, but even *that*
> > > is a useful data point.  E.g. if Paul's patches somehow broke/degraded this code,
> > > then knowing that what the author (you) intended/observed didn't match reality when
> > > the patch was applied would be extremely useful information for whoever encountered
> > > the hypothetical breakage.
> > 
> > Fair enough, but on this occasion it truly doesn't matter. It has
> > nothing to do with the implementation of *this* patch. This code makes
> > no assumptions and has no dependency on *when* that fast path might
> > return -EWOULDBLOCK. Sometimes it does, sometimes it doesn't. This code
> > just doesn't care one iota.
> > 
> > If this code had *dependencies* on the precise behaviour of
> > kvm_xen_set_evtchn_fast() that we needed to reason about, then sure,
> > I'd have written those explicitly into the commit comment *and* tried
> > to find some way of enforcing them with runtime warnings etc.
> > 
> > But it doesn't. So I am no more inclined to document the precise
> > behaviour of kvm_xen_set_evtchn_fast() in a patch which just happens to
> > call it, than I am inclined to document hrtimer_cancel() or any other
> > function called from the new code :)
> 
> Just because some bit of code doesn't care/differentiate doesn't mean the behavior
> of said code is correct.  I agree that adding a comment to explain the gory details
> is unnecessary and would lead to stale code.  But changelogs essentially capture a
> single point in a time, and a big role of the changelog is to help reviewers and
> readers understand (a) the *intent* of the change and (b) whether or not that change
> is correct.
> 
> E.g. there's an assumption that -EWOULDBLOCK is the only non-zero return code where
> the correct response is to go down the slow path.
> 
> I'm not asking to spell out every single condition, I'm just asking for clarification
> on what the intended behavior is, e.g.
> 
>   Use kvm_xen_set_evtchn_fast() directly from the timer callback, and fall
>   back to the slow path if the event is valid but fast delivery isn't
>   possible, which currently can only happen if delivery needs to block,
>   e.g. because the gfn=>pfn cache is invalid or stale.
> 
> instead of simply saying "when it's necessary to do so" and leaving it up to the
> reader to figure what _they_ think that means, which might not always align with
> what the author actually meant.


Fair enough. There's certainly scope for something along the lines of


+	rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
+	if (rc != -EWOULDBLOCK) {

   /*
    * If kvm_xen_set_evtchn_fast() returned -EWOULDBLOCK, then set the
    * timer_pending flag and kick the vCPU, to defer delivery of the 
    * event channel to a context which can sleep. If it fails for any
    * other reasons, just let it fail silently. The slow path fails 
    * silently too; a warning in that case may be guest triggerable,
    * should never happen anyway, and guests are generally going to
    * *notice* timers going missing.
    */

+		vcpu->arch.xen.timer_expires = 0;
+		return HRTIMER_NORESTART;
+	}

That's documenting *this* code, not the function it happens to call.
It's more verbose than I would normally have bothered to be, but I'm
all for improving the level of commenting in our code as long as it's
adding value.
  

On Mon, Oct 02, 2023, David Woodhouse wrote:
> On Mon, 2023-10-02 at 11:45 -0700, Sean Christopherson wrote:
> > E.g. there's an assumption that -EWOULDBLOCK is the only non-zero return code where
> > the correct response is to go down the slow path.
> > 
> > I'm not asking to spell out every single condition, I'm just asking for clarification
> > on what the intended behavior is, e.g.
> > 
> >   Use kvm_xen_set_evtchn_fast() directly from the timer callback, and fall
> >   back to the slow path if the event is valid but fast delivery isn't
> >   possible, which currently can only happen if delivery needs to block,
> >   e.g. because the gfn=>pfn cache is invalid or stale.
> > 
> > instead of simply saying "when it's necessary to do so" and leaving it up to the
> > reader to figure what _they_ think that means, which might not always align with
> > what the author actually meant.
> 
> 
> Fair enough. There's certainly scope for something along the lines of
> 
> 
> +	rc = kvm_xen_set_evtchn_fast(&e, vcpu->kvm);
> +	if (rc != -EWOULDBLOCK) {
> 
>    /*
>     * If kvm_xen_set_evtchn_fast() returned -EWOULDBLOCK, then set the
>     * timer_pending flag and kick the vCPU, to defer delivery of the 
>     * event channel to a context which can sleep. If it fails for any
>     * other reasons, just let it fail silently. The slow path fails 
>     * silently too; a warning in that case may be guest triggerable,
>     * should never happen anyway, and guests are generally going to
>     * *notice* timers going missing.
>     */
> 
> +		vcpu->arch.xen.timer_expires = 0;
> +		return HRTIMER_NORESTART;
> +	}
> 
> That's documenting *this* code, not the function it happens to call.
> It's more verbose than I would normally have bothered to be, but I'm
> all for improving the level of commenting in our code as long as it's
> adding value. 

I'm completely ok with no comment, I just want something in the changelog.  I'm
also not opposed to a comment, but I don't think it's necessary.

I don't have a problem with digging around code to understand the subtleties, or
even the high level "what" in many cases.  What I don't like is encountering code
where *nothing* explains the author's intent.  All too often I've encountered
historical code in KVM where it's not at all obvious if code does what the author
intended, e.g. if a bug was a simple goof or a completely misguided design choice.

Holler if you plan on sending a v4 with the comment.  I'm a-ok applying v3 with a
massaged changelog to fold in the gist of the comment.
  

On Tue, 2023-10-03 at 09:12 -0700, Sean Christopherson wrote:
> 
> Holler if you plan on sending a v4 with the comment.  I'm a-ok applying v3 with a
> massaged changelog to fold in the gist of the comment.

Yes please. I think you have a far better understanding of precisely
what it is that you're looking for in said comment.

I'm all for good comments, but on this occasion it seems I can't see
the wood for the trees, and I can't quite work out what it is that's
non-obvious.

Thanks!