Date:   Thu, 2 Feb 2023 18:51:47 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Vipin Sharma <vipinsh@google.com>
Cc:     pbonzini@redhat.com, vkuznets@redhat.com, dmatlack@google.com,
        kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
        Marc Zyngier <maz@kernel.org>,
        James Morse <james.morse@arm.com>,
        m Suzuki K Poulose <suzuki.poulose@arm.com>,
        Oliver Upton <oliver.upton@linux.dev>,
        Zenghui Yu <yuzenghui@huawei.com>,
        Anup Patel <anup@brainfault.org>,
        Atish Patra <atishp@atishpatra.org>,
        Christian Borntraeger <borntraeger@linux.ibm.com>,
        Janosch Frank <frankja@linux.ibm.com>,
        Claudio Imbrenda <imbrenda@linux.ibm.com>,
        David Hildenbrand <david@redhat.com>
Subject: Re: [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test
 assertion common.
Message-ID: <Y9wGQx89zI3TMU1Y@google.com>
References: <20221212183720.4062037-1-vipinsh@google.com>
 <20221212183720.4062037-13-vipinsh@google.com>
 <Y9r0q9cuK/ifu+OW@google.com>
 <CAHVum0fEmEAQSxozb1BTTy-d3UGrsvhjt8V5FXQPrX5wOYqpPQ@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHVum0fEmEAQSxozb1BTTy-d3UGrsvhjt8V5FXQPrX5wOYqpPQ@mail.gmail.com>
Precedence: bulk

On Thu, Feb 02, 2023, Vipin Sharma wrote:
> On Wed, Feb 1, 2023 at 3:24 PM Sean Christopherson <seanjc@google.com> wrote:
> > I love the cleanup, but in the future, please don't squeeze KVM-wide changes in
> > the middle of an otherwise arch-specific series unless it's absolutely necessary.
> > I get why you added the macro before copy-pasting more code into a new test, but
> > the unfortunate side effect is that complicates grabbing the entire series.
> >
> 
> Make sense. So what is preferable:
> 1. Make the big cleanup identified during a series as the last patches
> in that series?
> 2. Have two series and big cleanups rebased on top of the initial series?
> 
> Or, both 1 & 2 are acceptable depending on the cleanup?

  3. Post the cleanup independently, but make a note so that maintainers know
     that there may be conflicts and/or missed cleanup opportunities.

#1 is rarely going to be the best option.  The big cleanup is going to necessitate
Cc'ing a lot of people that don't care about the base arch-specific changes, so
unless the base changes are one or two trivial patches, a lot of people end up
having to wade through a lot of noise.  And aside from annoying people, that also
makes it more likely that someone will overlook the cleanup.

As for #2 vs. #3, #3 is probably a better option in most cases.  For broad cleanups,
odds are very good that there will be other conflicts beyond just the changes _you_
have in-flight.  E.g. in this case, any new tests and/or asserts that are in-flight,
sitting in other trees, etc., will suffer the same fate.  I.e. whoever applies the
cleanup is going to need to resolve conflicts and/or look for other cleanup
opportunities anyways.  For a scenario like this, a way to make life easy for the
maintainer applying the cleanup would be to provide a script, e.g. single grep
command, to look for potential cleanup spots.  That communicates to the maintainer
that there may be silent "conflicts" and makes it easier for them to resolve such
conflicts.

Posting the cleanup separately means the two series/patches can proceed
independently, e.g. respinning one doesn't screw up the other, maintainers can
take the patches in whatever order they prefer, etc.

There are undoubtedly exceptions, e.g. if the resulting conflicts are really nasty,
but those should be few and far between.