[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <ZRw9gstj8TWiiBvd@google.com>
Date: Tue, 3 Oct 2023 09:12:50 -0700
From: Sean Christopherson <seanjc@...gle.com>
To: David Woodhouse <dwmw2@...radead.org>
Cc: kvm <kvm@...r.kernel.org>, Paul Durrant <paul@....org>,
Paolo Bonzini <pbonzini@...hat.com>,
Thomas Gleixner <tglx@...utronix.de>,
Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: x86: Use fast path for Xen timer delivery
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.
Powered by blists - more mailing lists