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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZRsAvYecCOpeHvPY@google.com>
Date:   Mon, 2 Oct 2023 10:41:17 -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 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.

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