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 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@xxxxxxxxxxxx>
> > > > > 
> > > > > 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".