lkml.org 
[lkml]   [2022]   [Oct]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRE: [PATCH v1 00/18] KVM selftests code consolidation and cleanup
Date
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.

\
 
 \ /
  Last update: 2022-10-27 14:18    [W:0.258 / U:1.720 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site