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

[Sean Christopherson]

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.

> 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.