linux-kernel - RE: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <DS0PR11MB63731F2B467D4084F5C8D9B5DC339@DS0PR11MB6373.namprd11.prod.outlook.com>
Date:   Thu, 27 Oct 2022 12:18:08 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     David Matlack <dmatlack@...gle.com>
CC:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "vipinsh@...gle.com" <vipinsh@...gle.com>,
        "ajones@...tanamicro.com" <ajones@...tanamicro.com>,
        "eric.auger@...hat.com" <eric.auger@...hat.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

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 that
> >    are added to a vm, and an array of vcpu threads. This isn't necessary
> >    as kvm_vm already maintains a list of added vcpus. This series changes
> >    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 inside,
> >      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 the
> >      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 structures,
> >    as kvm_vcpu has already included the required info for the thread, for
> >    example, in patch 13, the cpu_idx field from "struct vcpu_thread"
> >    is a duplicate of vcpu->id.
> 
> I haven't dug too much into the actual code yet, but I have some high level
> feedback based on a quick look through the series:
> 
>  - 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 format?
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 many
usages independent of each other).

> 
>  - Make the shortlog more specific. "vcpu related code consolidation" is
>    vague.
> 
>  - 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".
> 
>  - 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.