MIME-Version: 1.0
References: <20221212183720.4062037-1-vipinsh@google.com> <20221212183720.4062037-13-vipinsh@google.com>
 <Y9r0q9cuK/ifu+OW@google.com> <CAHVum0fEmEAQSxozb1BTTy-d3UGrsvhjt8V5FXQPrX5wOYqpPQ@mail.gmail.com>
 <Y9wGQx89zI3TMU1Y@google.com>
In-Reply-To: <Y9wGQx89zI3TMU1Y@google.com>
From:   Vipin Sharma <vipinsh@google.com>
Date:   Thu, 2 Feb 2023 10:59:23 -0800
Message-ID: <CAHVum0dFG6gFTQ=JzMkX5Yw-BO7jtUEQyVww6TpN9wk_hQMpqw@mail.gmail.com>
Subject: Re: [Patch v4 12/13] KVM: selftests: Make vCPU exit reason test
 assertion common.
To:     Sean Christopherson <seanjc@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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 2, 2023 at 10:51 AM Sean Christopherson <seanjc@google.com> wrote:
>
> 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.

This is a good idea, to provide a grep or at least provide hints on
how one has found places to edit. I will keep this in mind. Thanks

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