Message-ID: <YurKx+gFAWPvj35L@google.com>
Date:   Wed, 3 Aug 2022 19:21:43 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Oliver Upton <oliver.upton@...ux.dev>
Cc:     Mingwei Zhang <mizhang@...gle.com>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/5] selftests: KVM/x86: Fix vcpu_{save,load}_state() by
 adding APIC state into kvm_x86_state

KVM: selftests: for the shortlog.

On Wed, Aug 03, 2022, Oliver Upton wrote:
> Hi Mingwei,
> 
> On Tue, Aug 02, 2022 at 11:07:15PM +0000, Mingwei Zhang wrote:
> > Fix vcpu_{save,load}_state() by adding APIC state into kvm_x86_state and
> > properly save/restore it in vcpu_{save,load}_state(). When vcpu resets,
> > APIC state become software disabled in kernel and thus the corresponding
> > vCPU is not able to receive posted interrupts [1].  So, add APIC
> > save/restore in userspace in selftest library code.
> 
> Of course, there are no hard rules around it but IMO a changelog is
> easier to grok if it first describes the what/why of the problem, then
> afterwards how it is fixed by the commit.

I strongly disagree.  :-)  To some extent, it's a personal preference, e.g. I
find it easier to understand the details (why something is a problem) if I have
the extra context of how a problem is fixed (or: what code was broken).

But beyond personal preference, there are less subjective reasons for stating
what a patch does before diving into details.  First and foremost, what code is
actually being changed is the most important information, and so that information
should be easy to find.  Changelogs that bury the "what's actually changing" in a
one-liner after 3+ paragraphs of background make it very hard to find that information.

Maybe for initial review one could argue that "what's the bug" is more important,
but for skimming logs and git archeology, the gory details matter less and less.
E.g. when doing a series of "git blame", the details of each change along the way
are useless, the details only matter for the culprit; I just want to quickly
determine whether or not a commit might be of interest.

Another argument for stating "what's changing" first is that it's almost always
possible to state "what's changing" in a single sentence.  Conversely, all but the
most simple bugs require multiple sentences or paragraphs to fully describe the
problem.  If both the "what's changing" and "what's the bug" are super short then
the order doesn't matter.  But if one is shorter (almost always the "what's changing),
then covering the shorter one first is advantageous because it's less of an
inconvenience for readers/reviewers that have a strict ordering preference.  E.g.
having to skip one sentence to get to the stuff you care about is less painful than
me having to skip three paragraphs to get to the stuff that I care about.

I think the underlying problem with this changelog (and the shortlog) is that it's
too literal about what is being fixed.  Shortlogs and changelogs shouldn't be
play-by-play descriptions of the code changes, they should be abstractions of the
problem and the fix.  E.g. 

  KVM: selftests: Save/restore vAPIC state in "migration" tests
  
  Save/restore vAPIC state as part of vCPU save/load so that it's preserved
  across VM "migration".  This will allow testing that posted interrupts
  are properly handled across VM migration.

With that, the first sentence covers both the "what's changing" and provides a
high-level description of the "bug" it's fixing.  And the second sentence covers
(a) "why do we want this patch", (b) "why wasn't this a problem before", and (c)
"what's the urgency of this patch".

Powered by blists - more mailing lists