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.
