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 :) > > 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.
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
