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.
