Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   "Wang, Wei W" <wei.w.wang@intel.com>
To:     David Matlack <dmatlack@google.com>
CC:     "Christopherson,, Sean" <seanjc@google.com>,
        "pbonzini@redhat.com" <pbonzini@redhat.com>,
        "vipinsh@google.com" <vipinsh@google.com>,
        "ajones@ventanamicro.com" <ajones@ventanamicro.com>,
        "eric.auger@redhat.com" <eric.auger@redhat.com>,
        "kvm@vger.kernel.org" <kvm@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v1 00/18] KVM selftests code consolidation and cleanup
Thread-Topic: [PATCH v1 00/18] KVM selftests code consolidation and cleanup
Thread-Index: AQHY55yiwQJVciN2Gk26RCSAzzsjaa4hMvUAgACUSLA=
Date:   Thu, 27 Oct 2022 12:18:08 +0000
Message-ID: <DS0PR11MB63731F2B467D4084F5C8D9B5DC339@DS0PR11MB6373.namprd11.prod.outlook.com>
References: <20221024113445.1022147-1-wei.w.wang@intel.com>
 <Y1mlJqKdFtlgG3jR@google.com>
In-Reply-To: <Y1mlJqKdFtlgG3jR@google.com>
Accept-Language: en-US
Content-Language: en-US
dlp-product: dlpe-windows
dlp-reaction: no-action
dlp-version: 11.6.500.17
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?L/uk+gD0k5D1jr4CWKGpmM3LWRrkUjlzvCP0YwSGDqI3y8gb9kGj1GNdzTLa?=
 =?us-ascii?Q?jhvnB2fDO9fxqB/sUkTgbEvjYDnK/BZU/j9aSum92Qkm5MWAU2z68z4oak5i?=
 =?us-ascii?Q?ToHqA60LZd4VdAyw6+zNQRvQ5YUGGWgFZLynF3uDdHDN9U48Jy1n0ZiT+ZQk?=
 =?us-ascii?Q?CeWyvgGUA+WLUbYwRftz+y6Ac2l7jsBweT/gK65CSdLbqMI5aUubolR8k3Xt?=
 =?us-ascii?Q?cSTCnXDAJUCyttGw6miU9AlY56HkoR0WaFrEpCPEJFZY5ejEJMEiZl3nU21s?=
 =?us-ascii?Q?a9iwa67UmkwDcRH8oJCGldV6IHt3TlWntQQKXXqqN8mpXPs3X0GhmUmmNz+Z?=
 =?us-ascii?Q?01vKTMlF4YBWtG9FMCWiS6UeU3sPq4giG1fl8XS+1tWoPI5/+Dyrpa2RPdy5?=
 =?us-ascii?Q?SlW/P/JqG/X0D7wpTyuS8QJWs/aZ9yF5lWZKFmIwxEAr6Lz5dhXXZwD5Pp03?=
 =?us-ascii?Q?vEcbWnIY9LidF7T9b6jy5BnmwJlICoSmJA2AxYo5Tk3uaqgU9Y5vi1jMjI5l?=
 =?us-ascii?Q?vOh07C5vMpa8ugl8osydoxbP0C+ht1nNT8zOhPsQCyPzg+C8cxYY3Bka/z1O?=
 =?us-ascii?Q?74Ki914H/lnV7uaFQ63HKeDrQXQxNzh2CFk5WAJ1tl9rcJNbY/JqJKDAw49m?=
 =?us-ascii?Q?NE+3WvBenUnukxrBzSHDUmkr4uIoidENwJlPDnStWxuZbB34UEDU6pkRH9EL?=
 =?us-ascii?Q?mEhoECS7t6Ve8U0+faaOoI5GERpX7JB+ET0RTed4PmqM+DhsHxiXNxS/zHTM?=
 =?us-ascii?Q?qlwPmlfrwW9FG81fIxQ39Uq5lr58w+f/Xlrjnnmtmu68JZfXGl+8/FJLZRPZ?=
 =?us-ascii?Q?BpRY6vyJILngiOXWDiUAkIOCukl3cIcYJuJWYVKeWr4BOUJFwdUYkuwdyYAW?=
 =?us-ascii?Q?WOulibibuZ61psTTNMw7/ANvgzRxzEOOnIACwYMRVHwdiAr2B5v8emAs5n/W?=
 =?us-ascii?Q?BoNgIr3/I9yK/CaZ1fOydigMMWmpyMBQmVK1ry1Je60hzJdh447bCahVXuNf?=
 =?us-ascii?Q?vup99Rhi3iG/pGbj+KgePL1z/pgQ5YhxcZlUhaE8llwB9Vpyfl16UBQns7LB?=
 =?us-ascii?Q?9JmWbyBmxBbKy9wpKBRkZPy672IOgZpCFXIqwvwjFV98WJQgGT870n+6elm6?=
 =?us-ascii?Q?wsk+W6seROd52pCXLFe1KDZo7qeh6EgkkIBvbMPkpcG0TvrmU7Im6rpGvAEV?=
 =?us-ascii?Q?vjeuZrNiBHMlOA3M+U/tsALizRJpSJNklrEEGWggZlbzNVOb3y7h761HrqIv?=
 =?us-ascii?Q?QLdWMSJZIiuDqnWCB2Wv7IIm1rlnEgvKK7uh41UcL24IZeeM4NQ1ZywROSj4?=
 =?us-ascii?Q?QYvqUpNtkSLm3rFlqDwnLDWdEldDsS8G7nR1IFWHxNkoe+nB7jpuFmL7J/MW?=
 =?us-ascii?Q?41vt1gOn55f9b2Hg+uKno/BRRrHhq98aq+ak+WsZ5xFw4GseSmIHpee0zgXN?=
 =?us-ascii?Q?a5rExSIoG6HfiuNGm2LxHPBgiQjLsBmCqotYF3uWJYUSreX73lkxVg3d1+9x?=
 =?us-ascii?Q?r/3VYUW4f2u89ruK3ysXFrgmdaH2khuwLmzIQPcCN1J99aKNGkBBNZV80tns?=
 =?us-ascii?Q?49KzlaOIIjWGhm2vYs66ukfX81Ific6VP8EEg0IB?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DS0PR11MB6373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 7cdd2982-5f3e-4d20-0e39-08dab8154ef3
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Oct 2022 12:18:08.4253
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 5CYLt9bnPOxOksQqWQ82cFiBJHxKonF4BmapSt3usT/NjR4ss61sZrQOXSAaMbsJ2L5z8Eikrn5DJSxrUn9G5g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB6444
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.6 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Thursday, October 27, 2022 5:23 AM, David Matlack:
> On Mon, Oct 24, 2022 at 07:34:27PM +0800, Wei Wang wrote:
> > This patch series intends to improve kvm selftests with better code
> > consolidation using the helper functions to perform vcpu and thread
> > related operations.
> >
> > In general, several aspects are improved:
> > 1) The current users allocate an array of vcpu pointers to the vcpus th=
at
> >    are added to a vm, and an array of vcpu threads. This isn't necessar=
y
> >    as kvm_vm already maintains a list of added vcpus. This series chang=
es
> >    the list of vcpus in the kvm_vm struct to a vcpu array for users to
> >    work with and removes each user's own allocation of such vcpu arrays=
.
> >    Aslo add the vcpu thread to the kvm_vcpu struct, so that users don't
> >    need to explicitly allocate a thread array to manage vcpu threads on
> >    their own.
> > 2) Change the users to use the helper functions provided by this series
> >    with the following enhancements:
> >    - Many users working with pthread_create/join forgot to check if
> >      error on returning. The helper functions have handled thoses insid=
e,
> >      so users don't need to handle them by themselves;
> >    - The vcpu threads created via the helper functions are named in
> >      "vcpu-##id" format. Naming the threads facilitates debugging,
> >      performance tuning, runtime pining etc;
> >    - helper functions named with "vm_vcpu_threads_" iterates over all t=
he
> >      vcpus that have been added to the vm. Users don't need a explicit
> >      loop to go through the added cpus by themselves.
> > 3) kvm_vcpu is used as the interface parameter to the vcpu thread's
> >    start routine, and the user specific data is made to be the private
> >    data in kvm_vcpu. This can simplify the user specific data structure=
s,
> >    as kvm_vcpu has already included the required info for the thread, f=
or
> >    example, in patch 13, the cpu_idx field from "struct vcpu_thread"
> >    is a duplicate of vcpu->id.
>=20
> I haven't dug too much into the actual code yet, but I have some high lev=
el
> feedback based on a quick look through the series:
>=20
>  - Use the format "KVM: selftests: <Decsription>" for the shortlog.

I know it's not common to see so far, but curious is this the required form=
at?
I didn't find where it's documented. If it's indeed a requirement, probably=
 we
also need to enhance checkpatch.pl to detect this.

If it's not required, I think it is more obvious to have /sub_field in the =
title,
e.g. selftests/hardware_disable_test, to outline which specific part of
selftests the patch is changing. (the selftests are growing larger with man=
y
usages independent of each other).

>=20
>  - Make the shortlog more specific. "vcpu related code consolidation" is
>    vague.
>=20
>  - Do not introduce bugs and then fix them in subsequent commits.  This
>    breaks bisection. For example, kvm_page_table_test is broken at "KVM:
>    selftests/kvm_util: use vm->vcpus[] when create vm with vcpus" and
>    then fixed by "KVM: selftests/kvm_page_table_test: vcpu related code
>    consolidation".
>=20
>  - Try to limit each patch to one logical change. This is somewhat more
>    art than science, but the basic idea is to avoid changing too much at
>    once so that the code is easier to review and bisect. For example,
>    "KVM: selftests/perf_test_util: vcpu related code consolidation" has
>    a list of 6 different changes being made in the commit description.
>    This is a sure sign this commit should be broken up. The same applies
>    to many of the other patches. This will also make it easier to come
>    up with more specific shortlogs.

OK, will re-organize the patches.