Message-ID: <ac097a26e96ded73e19200066b9063354096a8fd.camel@infradead.org>
Date:   Mon, 02 Oct 2023 19:09:18 +0100
From:   David Woodhouse <dwmw2@...radead.org>
To:     Sean Christopherson <seanjc@...gle.com>
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, 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@...zon.co.uk>
> > > > 
> > > > 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 :)

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

I can just drop the footnote; there's not a huge amount of benefit in
referring back to the old mail thread anyway. The gist of it was
covered in the commit comment.

Download attachment "smime.p7s" of type "application/pkcs7-signature" (5965 bytes)

Powered by blists - more mailing lists