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

[Sean Christopherson]

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.