LinuxLists.cc - [PATCH v1 00/18] KVM selftests code consolidation and cleanup

[permalink] [raw]

Subject: [PATCH v1 09/18] KVM: selftests/steal_time: vcpu related code consolidation and cleanup

Remove the unnecessary definition of array of the vcpu pointers and
re-use the one from the kvm_vm struct (i.e. vm->vcpus[]). Use the helper
function to create the time stealing thread with name.

Also add a check of the pthread_join return value.

Signed-off-by: Wei Wang <[email protected]>
---
tools/testing/selftests/kvm/steal_time.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
index db8967f1a17b..857ed2c073fc 100644
--- a/tools/testing/selftests/kvm/steal_time.c
+++ b/tools/testing/selftests/kvm/steal_time.c
@@ -8,7 +8,6 @@
#include <stdio.h>
#include <time.h>
#include <sched.h>
-#include <pthread.h>
#include <linux/kernel.h>
#include <asm/kvm.h>
#include <asm/kvm_para.h>
@@ -241,7 +240,7 @@ static void run_vcpu(struct kvm_vcpu *vcpu)

int main(int ac, char **av)
{
- struct kvm_vcpu *vcpus[NR_VCPUS];
+ struct kvm_vcpu **vcpus;
struct kvm_vm *vm;
pthread_attr_t attr;
pthread_t thread;
@@ -250,7 +249,7 @@ int main(int ac, char **av)
long stolen_time;
long run_delay;
bool verbose;
- int i;
+ int i, r;

verbose = ac > 1 && (!strncmp(av[1], "-v", 3) || !strncmp(av[1], "--verbose", 10));

@@ -262,7 +261,8 @@ int main(int ac, char **av)
pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);

/* Create a VM and an identity mapped memslot for the steal time structure */
- vm = vm_create_with_vcpus(NR_VCPUS, guest_code, vcpus);
+ vm = vm_create_with_vcpus(NR_VCPUS, guest_code, NULL);
+ vcpus = vm->vcpus;
gpages = vm_calc_num_guest_pages(VM_MODE_DEFAULT, STEAL_TIME_SIZE * NR_VCPUS);
vm_userspace_mem_region_add(vm, VM_MEM_SRC_ANONYMOUS, ST_GPA_BASE, 1, gpages, 0);
virt_map(vm, ST_GPA_BASE, ST_GPA_BASE, gpages);
@@ -290,11 +290,14 @@ int main(int ac, char **av)

/* Steal time from the VCPU. The steal time thread has the same CPU affinity as the VCPUs. */
run_delay = get_run_delay();
- pthread_create(&thread, &attr, do_steal_time, NULL);
+ __pthread_create_with_name(&thread, &attr,
+ do_steal_time, NULL, "steal-time");
do
sched_yield();
while (get_run_delay() - run_delay < MIN_RUN_DELAY_NS);
- pthread_join(thread, NULL);
+ r = pthread_join(thread, NULL);
+ TEST_ASSERT(!r, "failed to join the time stealing thread");
+
run_delay = get_run_delay() - run_delay;
TEST_ASSERT(run_delay >= MIN_RUN_DELAY_NS,
"Expected run_delay >= %ld, got %ld",
--
2.27.0

2022-10-24 12:38:14

[permalink] [raw]

Subject: [PATCH v1 10/18] KVM: selftests/tsc_scaling_sync: vcpu related code consolidation

Remove the unnecessary definition of the vcpu_threads[] array, as
it has beend included in the kvm_vcpu struct. Use the helper functions
to create and join the vcpu threads.

Signed-off-by: Wei Wang <[email protected]>
---
.../selftests/kvm/x86_64/tsc_scaling_sync.c | 25 ++++---------------
1 file changed, 5 insertions(+), 20 deletions(-)

diff --git a/tools/testing/selftests/kvm/x86_64/tsc_scaling_sync.c b/tools/testing/selftests/kvm/x86_64/tsc_scaling_sync.c
index 47139aab7408..34a8beef42b6 100644
--- a/tools/testing/selftests/kvm/x86_64/tsc_scaling_sync.c
+++ b/tools/testing/selftests/kvm/x86_64/tsc_scaling_sync.c
@@ -15,7 +15,6 @@
#include <time.h>
#include <sched.h>
#include <signal.h>
-#include <pthread.h>

#define NR_TEST_VCPUS 20

@@ -44,18 +43,15 @@ static void guest_code(void)
}

-static void *run_vcpu(void *_cpu_nr)
+static void *run_vcpu(void *data)
{
- unsigned long vcpu_id = (unsigned long)_cpu_nr;
+ struct kvm_vcpu *vcpu = (struct kvm_vcpu *)data;
unsigned long failures = 0;
static bool first_cpu_done;
- struct kvm_vcpu *vcpu;

/* The kernel is fine, but vm_vcpu_add() needs locking */
pthread_spin_lock(&create_lock);

- vcpu = vm_vcpu_add(vm, vcpu_id, guest_code);
-
if (!first_cpu_done) {
first_cpu_done = true;
vcpu_set_msr(vcpu, MSR_IA32_TSC, TEST_TSC_OFFSET);
@@ -95,23 +91,12 @@ int main(int argc, char *argv[])
{
TEST_REQUIRE(kvm_has_cap(KVM_CAP_VM_TSC_CONTROL));

- vm = vm_create(NR_TEST_VCPUS);
+ vm = vm_create_with_vcpus(NR_TEST_VCPUS, guest_code, NULL);
vm_ioctl(vm, KVM_SET_TSC_KHZ, (void *) TEST_TSC_KHZ);

pthread_spin_init(&create_lock, PTHREAD_PROCESS_PRIVATE);
- pthread_t cpu_threads[NR_TEST_VCPUS];
- unsigned long cpu;
- for (cpu = 0; cpu < NR_TEST_VCPUS; cpu++)
- pthread_create(&cpu_threads[cpu], NULL, run_vcpu, (void *)cpu);
-
- unsigned long failures = 0;
- for (cpu = 0; cpu < NR_TEST_VCPUS; cpu++) {
- void *this_cpu_failures;
- pthread_join(cpu_threads[cpu], &this_cpu_failures);
- failures += (unsigned long)this_cpu_failures;
- }
-
- TEST_ASSERT(!failures, "TSC sync failed");
+ vm_vcpu_threads_create(vm, run_vcpu, 0);
+ vm_vcpu_threads_join(vm);
pthread_spin_destroy(&create_lock);
kvm_vm_free(vm);
return 0;
--
2.27.0

2022-10-24 12:38:25

[permalink] [raw]

Subject: [PATCH v1 16/18] KVM: selftest/arch_timer: vcpu related code consolidation

Remove the globally defined vcpu and pthread arrays, and reuse the one
from kvm_vm and kvm_vcpu. Also use the helper functions to create vcpu
threads with name.

Signed-off-by: Wei Wang <[email protected]>
---
.../selftests/kvm/aarch64/arch_timer.c | 42 +++++++------------
1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/tools/testing/selftests/kvm/aarch64/arch_timer.c b/tools/testing/selftests/kvm/aarch64/arch_timer.c
index 574eb73f0e90..7c1057e8fca7 100644
--- a/tools/testing/selftests/kvm/aarch64/arch_timer.c
+++ b/tools/testing/selftests/kvm/aarch64/arch_timer.c
@@ -23,7 +23,6 @@
#define _GNU_SOURCE

#include <stdlib.h>
-#include <pthread.h>
#include <linux/kvm.h>
#include <linux/sizes.h>
#include <linux/bitmap.h>
@@ -76,8 +75,6 @@ struct test_vcpu_shared_data {
uint64_t xcnt;
};

-static struct kvm_vcpu *vcpus[KVM_MAX_VCPUS];
-static pthread_t pt_vcpu_run[KVM_MAX_VCPUS];
static struct test_vcpu_shared_data vcpu_shared_data[KVM_MAX_VCPUS];

static int vtimer_irq, ptimer_irq;
@@ -212,7 +209,8 @@ static void guest_code(void)

static void *test_vcpu_run(void *arg)
{
- unsigned int vcpu_idx = (unsigned long)arg;
+ struct kvm_vcpu *vcpu = (struct kvm_vcpu *)arg;
+ unsigned int vcpu_idx = vcpu->id;
struct ucall uc;
struct kvm_vcpu *vcpu = vcpus[vcpu_idx];
struct kvm_vm *vm = vcpu->vm;
@@ -263,18 +261,19 @@ static uint32_t test_get_pcpu(void)
return pcpu;
}

-static int test_migrate_vcpu(unsigned int vcpu_idx)
+static int test_migrate_vcpu(struct kvm_vcpu *vcpu)
{
int ret;
cpu_set_t cpuset;
uint32_t new_pcpu = test_get_pcpu();
+ uint32_t vcpu_idx = vcpu->id;

CPU_ZERO(&cpuset);
CPU_SET(new_pcpu, &cpuset);

pr_debug("Migrating vCPU: %u to pCPU: %u\n", vcpu_idx, new_pcpu);

- ret = pthread_setaffinity_np(pt_vcpu_run[vcpu_idx],
+ ret = pthread_setaffinity_np(vcpu->thread,
sizeof(cpuset), &cpuset);

/* Allow the error where the vCPU thread is already finished */
@@ -287,6 +286,7 @@ static int test_migrate_vcpu(unsigned int vcpu_idx)

static void *test_vcpu_migration(void *arg)
{
+ struct kvm_vm *vm = (struct kvm_vm *)arg;
unsigned int i, n_done;
bool vcpu_done;

@@ -303,7 +303,7 @@ static void *test_vcpu_migration(void *arg)
continue;
}

- test_migrate_vcpu(i);
+ test_migrate_vcpu(vm->vcpus[i]);
}
} while (test_args.nr_vcpus != n_done);

@@ -314,31 +314,21 @@ static void test_run(struct kvm_vm *vm)
{
pthread_t pt_vcpu_migration;
unsigned int i;
- int ret;

pthread_mutex_init(&vcpu_done_map_lock, NULL);
vcpu_done_map = bitmap_zalloc(test_args.nr_vcpus);
TEST_ASSERT(vcpu_done_map, "Failed to allocate vcpu done bitmap\n");

- for (i = 0; i < (unsigned long)test_args.nr_vcpus; i++) {
- ret = pthread_create(&pt_vcpu_run[i], NULL, test_vcpu_run,
- (void *)(unsigned long)i);
- TEST_ASSERT(!ret, "Failed to create vCPU-%d pthread\n", i);
- }
+ vm_vcpu_threads_create(vm, test_vcpu_run, 0);

/* Spawn a thread to control the vCPU migrations */
if (test_args.migration_freq_ms) {
srand(time(NULL));
-
- ret = pthread_create(&pt_vcpu_migration, NULL,
- test_vcpu_migration, NULL);
- TEST_ASSERT(!ret, "Failed to create the migration pthread\n");
+ pthread_create_with_name(&pt_vcpu_migration,
+ test_vcpu_migration, vm, "control-thread");
}

-
- for (i = 0; i < test_args.nr_vcpus; i++)
- pthread_join(pt_vcpu_run[i], NULL);
-
+ vm_vcpu_threads_join(vm);
if (test_args.migration_freq_ms)
pthread_join(pt_vcpu_migration, NULL);

@@ -364,16 +354,16 @@ static int gic_fd;
static struct kvm_vm *test_vm_create(void)
{
struct kvm_vm *vm;
- unsigned int i;
- int nr_vcpus = test_args.nr_vcpus;
+ struct kvm_vcpu *vcpu;
+ int i, nr_vcpus = test_args.nr_vcpus;

- vm = vm_create_with_vcpus(nr_vcpus, guest_code, vcpus);
+ vm = vm_create_with_vcpus(nr_vcpus, guest_code, NULL);

vm_init_descriptor_tables(vm);
vm_install_exception_handler(vm, VECTOR_IRQ_CURRENT, guest_irq_handler);

- for (i = 0; i < nr_vcpus; i++)
- vcpu_init_descriptor_tables(vcpus[i]);
+ vm_iterate_over_cpus(vm, vcpu, i)
+ vcpu_init_descriptor_tables(vcpu);

ucall_init(vm, NULL);
test_init_timer_irq(vm);
--
2.27.0

2022-10-26 22:02:42

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

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.

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

>
> The changes have been tested on an SPR server. Patch 15 and 16 haven't
> been tested due to the lack of an ARM environment currently.
>
> Wei Wang (18):
> KVM: selftests/kvm_util: use array of pointers to maintain vcpus in
> kvm_vm
> KVM: selftests/kvm_util: use vm->vcpus[] when create vm with vcpus
> KVM: selftests/kvm_util: helper functions for vcpus and threads
> KVM: selftests/kvm_page_table_test: vcpu related code consolidation
> KVM: selftests/hardware_disable_test: code consolidation and cleanup
> KVM: selftests/dirty_log_test: vcpu related code consolidation
> KVM: selftests/max_guest_memory_test: vcpu related code consolidation
> KVM: selftests/set_memory_region_test: vcpu related code consolidation
> KVM: selftests/steal_time: vcpu related code consolidation and cleanup
> KVM: selftests/tsc_scaling_sync: vcpu related code consolidation
> KVM: selftest/xapic_ipi_test: vcpu related code consolidation
> KVM: selftests/rseq_test: name the migration thread and some cleanup
> KVM: selftests/perf_test_util: vcpu related code consolidation
> KVM: selftest/memslot_perf_test: vcpu related code consolidation
> KVM: selftests/vgic_init: vcpu related code consolidation
> KVM: selftest/arch_timer: vcpu related code consolidation
> KVM: selftests: remove the *vcpu[] input from __vm_create_with_vcpus
> KVM: selftests/kvm_create_max_vcpus: check KVM_MAX_VCPUS
>
> .../selftests/kvm/aarch64/arch_timer.c | 42 ++--
> .../testing/selftests/kvm/aarch64/vgic_init.c | 35 ++-
> .../selftests/kvm/access_tracking_perf_test.c | 18 +-
> .../selftests/kvm/demand_paging_test.c | 9 +-
> .../selftests/kvm/dirty_log_perf_test.c | 11 +-
> tools/testing/selftests/kvm/dirty_log_test.c | 16 +-
> .../selftests/kvm/hardware_disable_test.c | 56 ++---
> .../testing/selftests/kvm/include/kvm_util.h | 24 ++
> .../selftests/kvm/include/kvm_util_base.h | 12 +-
> .../selftests/kvm/include/perf_test_util.h | 9 +-
> .../selftests/kvm/kvm_create_max_vcpus.c | 7 +
> .../selftests/kvm/kvm_page_table_test.c | 16 +-
> tools/testing/selftests/kvm/lib/kvm_util.c | 217 +++++++++++++++---
> .../selftests/kvm/lib/perf_test_util.c | 68 ++----
> .../selftests/kvm/lib/x86_64/perf_test_util.c | 11 +-
> tools/testing/selftests/kvm/lib/x86_64/vmx.c | 2 +-
> .../selftests/kvm/max_guest_memory_test.c | 53 ++---
> .../kvm/memslot_modification_stress_test.c | 9 +-
> .../testing/selftests/kvm/memslot_perf_test.c | 137 +++++------
> tools/testing/selftests/kvm/rseq_test.c | 10 +-
> .../selftests/kvm/set_memory_region_test.c | 16 +-
> tools/testing/selftests/kvm/steal_time.c | 15 +-
> .../selftests/kvm/x86_64/tsc_scaling_sync.c | 25 +-
> .../selftests/kvm/x86_64/xapic_ipi_test.c | 54 ++---
> 24 files changed, 476 insertions(+), 396 deletions(-)
>
> --
> 2.27.0
>

2022-10-27 01:09:39

[permalink] [raw]

Subject: Re: [PATCH v1 09/18] KVM: selftests/steal_time: vcpu related code consolidation and cleanup

On Mon, Oct 24, 2022, Wei Wang wrote:
> Remove the unnecessary definition of array of the vcpu pointers and
> re-use the one from the kvm_vm struct (i.e. vm->vcpus[]). Use the helper
> function to create the time stealing thread with name.

One thing per patch.

> Also add a check of the pthread_join return value.
>
> Signed-off-by: Wei Wang <[email protected]>
> ---
> tools/testing/selftests/kvm/steal_time.c | 15 +++++++++------
> 1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/steal_time.c b/tools/testing/selftests/kvm/steal_time.c
> index db8967f1a17b..857ed2c073fc 100644
> --- a/tools/testing/selftests/kvm/steal_time.c
> +++ b/tools/testing/selftests/kvm/steal_time.c
> @@ -8,7 +8,6 @@
> #include <stdio.h>
> #include <time.h>
> #include <sched.h>
> -#include <pthread.h>
> #include <linux/kernel.h>
> #include <asm/kvm.h>
> #include <asm/kvm_para.h>
> @@ -241,7 +240,7 @@ static void run_vcpu(struct kvm_vcpu *vcpu)
>
> int main(int ac, char **av)
> {
> - struct kvm_vcpu *vcpus[NR_VCPUS];
> + struct kvm_vcpu **vcpus;
> struct kvm_vm *vm;
> pthread_attr_t attr;
> pthread_t thread;
> @@ -250,7 +249,7 @@ int main(int ac, char **av)
> long stolen_time;
> long run_delay;
> bool verbose;
> - int i;
> + int i, r;
>
> verbose = ac > 1 && (!strncmp(av[1], "-v", 3) || !strncmp(av[1], "--verbose", 10));
>
> @@ -262,7 +261,8 @@ int main(int ac, char **av)
> pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);
>
> /* Create a VM and an identity mapped memslot for the steal time structure */
> - vm = vm_create_with_vcpus(NR_VCPUS, guest_code, vcpus);
> + vm = vm_create_with_vcpus(NR_VCPUS, guest_code, NULL);
> + vcpus = vm->vcpus;

Just use vm->vcpus directly and drop the local variable, it's not that much more
churn and this looks quite odd.

2022-10-27 13:01:04

[permalink] [raw]

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.

2022-10-27 16:01:30

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Thu, Oct 27, 2022, Wang, Wei W wrote:
> On Thursday, October 27, 2022 5:23 AM, David Matlack:
> > 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?

It's definitely the preferred format.

> I didn't find where it's documented.

Heh, for all shortlog scopes, the "documentation" is `git log --pretty=oneline` :-)

> If it's indeed a requirement, probably we also need to enhance checkpatch.pl
> to detect this.

I like the idea in theory, but that'd be a daunting task to set up, and quite the
maintenance nightmare. There are probably thousands of file => scope mappings
throughout the kernel, with any number of exceptions and arbitrary rules.

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

I agree that "KVM: selftests:" is a rather large umbrella, but it's not obvious
that "KVM: selfetest/<test>" is unequivocally better, e.g. if someone is making a
change to x86_64/vmx_exception_with_invalid_guest_state.c, the scope will be

KVM: selftests/x86_64/vmx_exception_with_invalid_guest_state:

or

KVM: selftests/vmx_exception_with_invalid_guest_state:

which doesn't leave a whole lot of room for an actual shortlog.

When reviewing selftests patches, I do find myself pausing sometimes to see exactly
what file/test is being modified, but in almost all cases a quick glance at the
diffstat provides the answer. And on the flip side, having all selftests patches
exactly match "KVM: selftests:" makes it super easy to identify selftest changes,
i.e. it's mostly a wash overall in terms of efficiency (for me at least).

2022-10-27 17:24:01

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <[email protected]> wrote:
>
> On Thu, Oct 27, 2022, Wang, Wei W wrote:
> > On Thursday, October 27, 2022 5:23 AM, David Matlack:
> > > 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?
>
> It's definitely the preferred format.
>
> > I didn't find where it's documented.
>
> Heh, for all shortlog scopes, the "documentation" is `git log --pretty=oneline` :-)
>
> > If it's indeed a requirement, probably we also need to enhance checkpatch.pl
> > to detect this.
>
> I like the idea in theory, but that'd be a daunting task to set up, and quite the
> maintenance nightmare. There are probably thousands of file => scope mappings
> throughout the kernel, with any number of exceptions and arbitrary rules.

I was thinking about proposing this in checkpatch.pl, or in some
KVM-specific check script. It seems like the following rule: If a
commit only modifies files in tools/testing/selftests/kvm/*, then
requires the shortlog match the regex "KVM: selftests: .*". That would
handle the vast majority of cases without affecting other subsystems.

Sean are you more concerned that if we start validating shortlogs in
checkpatch.pl then eventually it will get too out of hand? (i.e. not
so concerned with this specific case, but the general problem?)

Either way, we should at least document the preferred KVM shortlog
styles in Documentation/virtual/kvm/.

2022-10-27 18:51:42

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Thu, Oct 27, 2022, David Matlack wrote:
> On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <[email protected]> wrote:
> > I like the idea in theory, but that'd be a daunting task to set up, and quite the
> > maintenance nightmare. There are probably thousands of file => scope mappings
> > throughout the kernel, with any number of exceptions and arbitrary rules.
>
> I was thinking about proposing this in checkpatch.pl, or in some
> KVM-specific check script. It seems like the following rule: If a
> commit only modifies files in tools/testing/selftests/kvm/*, then
> requires the shortlog match the regex "KVM: selftests: .*". That would
> handle the vast majority of cases without affecting other subsystems.
>
> Sean are you more concerned that if we start validating shortlogs in
> checkpatch.pl then eventually it will get too out of hand? (i.e. not
> so concerned with this specific case, but the general problem?)

Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't
going to fly. The checkpatch maintainers most definitely don't want to take on
the burden of maintaining subsystem rules. Letting one subsystem add custom rules
effectively opens the flood gates to all subsystems adding custom rules. And from
a KVM perspective, I don't want to have to get an Acked-by from a checkpatch
maintiainer just to tweak a KVM rule.

The only somewhat feasible approach I can think of would be to provide a generic
"language" for shortlog scope rules, and have checkpatch look for a well-known
file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even
that is a non-trivial problem to solve, as it means coming up with a "language"
that has a reasonable chance of working for many subsystems without generating too
many false positives.

It's definitely doable, and likely not actually a maintenance nightmare (I wrote
that thinking of modifying a common rules file). But it's still fairly daunting
as getting buy-in on something that affects the kernel at large tends to be easier
said then done. Then again, I'm probably being pessimistic due to my sub-par
regex+scripting skills :-)

2022-10-28 13:30:57

by Andrew Jones

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote:
> On Thu, Oct 27, 2022, David Matlack wrote:
> > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <[email protected]> wrote:
> > > I like the idea in theory, but that'd be a daunting task to set up, and quite the
> > > maintenance nightmare. There are probably thousands of file => scope mappings
> > > throughout the kernel, with any number of exceptions and arbitrary rules.
> >
> > I was thinking about proposing this in checkpatch.pl, or in some
> > KVM-specific check script. It seems like the following rule: If a
> > commit only modifies files in tools/testing/selftests/kvm/*, then
> > requires the shortlog match the regex "KVM: selftests: .*". That would
> > handle the vast majority of cases without affecting other subsystems.
> >
> > Sean are you more concerned that if we start validating shortlogs in
> > checkpatch.pl then eventually it will get too out of hand? (i.e. not
> > so concerned with this specific case, but the general problem?)
>
> Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't
> going to fly. The checkpatch maintainers most definitely don't want to take on
> the burden of maintaining subsystem rules. Letting one subsystem add custom rules
> effectively opens the flood gates to all subsystems adding custom rules. And from
> a KVM perspective, I don't want to have to get an Acked-by from a checkpatch
> maintiainer just to tweak a KVM rule.
>
> The only somewhat feasible approach I can think of would be to provide a generic
> "language" for shortlog scope rules, and have checkpatch look for a well-known
> file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even
> that is a non-trivial problem to solve, as it means coming up with a "language"
> that has a reasonable chance of working for many subsystems without generating too
> many false positives.
>
> It's definitely doable, and likely not actually a maintenance nightmare (I wrote
> that thinking of modifying a common rules file). But it's still fairly daunting
> as getting buy-in on something that affects the kernel at large tends to be easier
> said then done. Then again, I'm probably being pessimistic due to my sub-par
> regex+scripting skills :-)

How about adding support for checkpatch extension plugins? If we could add
a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify
checkpatch to run .checkpatch scripts in the patched files' directories
(and recursively in the parent directories) when found, then we'd get
custom checkpatch behaviors. The scripts wouldn't even have to be written
in Perl (but I say that a bit sadly, because I like Perl).

Thanks,
drew

2022-10-28 16:43:02

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Fri, Oct 28, 2022, Andrew Jones wrote:
> On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote:
> > On Thu, Oct 27, 2022, David Matlack wrote:
> > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <[email protected]> wrote:
> > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the
> > > > maintenance nightmare. There are probably thousands of file => scope mappings
> > > > throughout the kernel, with any number of exceptions and arbitrary rules.
> > >
> > > I was thinking about proposing this in checkpatch.pl, or in some
> > > KVM-specific check script. It seems like the following rule: If a
> > > commit only modifies files in tools/testing/selftests/kvm/*, then
> > > requires the shortlog match the regex "KVM: selftests: .*". That would
> > > handle the vast majority of cases without affecting other subsystems.
> > >
> > > Sean are you more concerned that if we start validating shortlogs in
> > > checkpatch.pl then eventually it will get too out of hand? (i.e. not
> > > so concerned with this specific case, but the general problem?)
> >
> > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't
> > going to fly. The checkpatch maintainers most definitely don't want to take on
> > the burden of maintaining subsystem rules. Letting one subsystem add custom rules
> > effectively opens the flood gates to all subsystems adding custom rules. And from
> > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch
> > maintiainer just to tweak a KVM rule.
> >
> > The only somewhat feasible approach I can think of would be to provide a generic
> > "language" for shortlog scope rules, and have checkpatch look for a well-known
> > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even
> > that is a non-trivial problem to solve, as it means coming up with a "language"
> > that has a reasonable chance of working for many subsystems without generating too
> > many false positives.
> >
> > It's definitely doable, and likely not actually a maintenance nightmare (I wrote
> > that thinking of modifying a common rules file). But it's still fairly daunting
> > as getting buy-in on something that affects the kernel at large tends to be easier
> > said then done. Then again, I'm probably being pessimistic due to my sub-par
> > regex+scripting skills :-)
>
> How about adding support for checkpatch extension plugins? If we could add
> a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify
> checkpatch to run .checkpatch scripts in the patched files' directories
> (and recursively in the parent directories) when found, then we'd get
> custom checkpatch behaviors. The scripts wouldn't even have to be written
> in Perl (but I say that a bit sadly, because I like Perl).

That will work for simple cases, but patches that touch files in multiple directories
will be messy. E.g. a patch that touches virt/kvm/ and arch/x86/kvm/ will have
two separate custom rules enforcing two different scopes.

Recursively executing plugins will also be problematic, e.g. except for KVM, arch/x86/
is maintained by the tip-tree folks, and the tip-tree is quite opinionated on all
sorts of things, whereas KVM tends to be a bit more relaxed.

Enforcing scope through plugins would also lead to some amount of duplicate code
throught subsystems.

Anyways, if someone wants to pursue this, these ideas and the "requirement" should
be run by the checkpatch maintainers. They have far more experience and authority
in this area, and I suspect we aren't the first people to want checkpatch to get
involved in enforcing shortlog scope.

2022-11-07 18:56:55

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <[email protected]> wrote:
>
> On Fri, Oct 28, 2022, Andrew Jones wrote:
> > On Thu, Oct 27, 2022 at 06:27:39PM +0000, Sean Christopherson wrote:
> > > On Thu, Oct 27, 2022, David Matlack wrote:
> > > > On Thu, Oct 27, 2022 at 8:44 AM Sean Christopherson <[email protected]> wrote:
> > > > > I like the idea in theory, but that'd be a daunting task to set up, and quite the
> > > > > maintenance nightmare. There are probably thousands of file => scope mappings
> > > > > throughout the kernel, with any number of exceptions and arbitrary rules.
> > > >
> > > > I was thinking about proposing this in checkpatch.pl, or in some
> > > > KVM-specific check script. It seems like the following rule: If a
> > > > commit only modifies files in tools/testing/selftests/kvm/*, then
> > > > requires the shortlog match the regex "KVM: selftests: .*". That would
> > > > handle the vast majority of cases without affecting other subsystems.
> > > >
> > > > Sean are you more concerned that if we start validating shortlogs in
> > > > checkpatch.pl then eventually it will get too out of hand? (i.e. not
> > > > so concerned with this specific case, but the general problem?)
> > >
> > > Ya, the general problem. Hardcoding anything KVM specific in checkpatch.pl isn't
> > > going to fly. The checkpatch maintainers most definitely don't want to take on
> > > the burden of maintaining subsystem rules. Letting one subsystem add custom rules
> > > effectively opens the flood gates to all subsystems adding custom rules. And from
> > > a KVM perspective, I don't want to have to get an Acked-by from a checkpatch
> > > maintiainer just to tweak a KVM rule.
> > >
> > > The only somewhat feasible approach I can think of would be to provide a generic
> > > "language" for shortlog scope rules, and have checkpatch look for a well-known
> > > file in relevant directories, e.g. add arch/x86/kvm/SCOPES or whatever. But even
> > > that is a non-trivial problem to solve, as it means coming up with a "language"
> > > that has a reasonable chance of working for many subsystems without generating too
> > > many false positives.
> > >
> > > It's definitely doable, and likely not actually a maintenance nightmare (I wrote
> > > that thinking of modifying a common rules file). But it's still fairly daunting
> > > as getting buy-in on something that affects the kernel at large tends to be easier
> > > said then done. Then again, I'm probably being pessimistic due to my sub-par
> > > regex+scripting skills :-)
> >
> > How about adding support for checkpatch extension plugins? If we could add
> > a plugin script, e.g. tools/testing/selftests/kvm/.checkpatch, and modify
> > checkpatch to run .checkpatch scripts in the patched files' directories
> > (and recursively in the parent directories) when found, then we'd get
> > custom checkpatch behaviors. The scripts wouldn't even have to be written
> > in Perl (but I say that a bit sadly, because I like Perl).
>
> That will work for simple cases, but patches that touch files in multiple directories
> will be messy. E.g. a patch that touches virt/kvm/ and arch/x86/kvm/ will have
> two separate custom rules enforcing two different scopes.
>
> Recursively executing plugins will also be problematic, e.g. except for KVM, arch/x86/
> is maintained by the tip-tree folks, and the tip-tree is quite opinionated on all
> sorts of things, whereas KVM tends to be a bit more relaxed.
>
> Enforcing scope through plugins would also lead to some amount of duplicate code
> throught subsystems.
>
> Anyways, if someone wants to pursue this, these ideas and the "requirement" should
> be run by the checkpatch maintainers. They have far more experience and authority
> in this area, and I suspect we aren't the first people to want checkpatch to get
> involved in enforcing shortlog scope.

Documenting would at least be an improvement over what we have today
since it would eliminate the need to re-explain the preferred rules
every time. We can just point to the documentation when reviewing
patches.

`git log --pretty=oneline` is not a great way to document shortlog
scopes because it does not explain the rules (e.g. when to use "KVM:
x86: " vs "KVM: x86/mmu: "), does not explain why things the way they
are, and is inconsistent since we don't always catch every patch that
goes by with a non-preferred shortlog scope.

2022-11-07 19:00:48

[permalink] [raw]

Subject: Re: [PATCH v1 00/18] KVM selftests code consolidation and cleanup

On Mon, Nov 07, 2022, David Matlack wrote:
> On Fri, Oct 28, 2022 at 8:49 AM Sean Christopherson <[email protected]> wrote:
> > Anyways, if someone wants to pursue this, these ideas and the "requirement" should
> > be run by the checkpatch maintainers. They have far more experience and authority
> > in this area, and I suspect we aren't the first people to want checkpatch to get
> > involved in enforcing shortlog scope.
>
> Documenting would at least be an improvement over what we have today
> since it would eliminate the need to re-explain the preferred rules
> every time. We can just point to the documentation when reviewing
> patches.

Agreed. And there are many other things I want to formalize for KVM x86, e.g.
testing expectations, health requirements for the various branches, what each
branch is used for etc...

If you want to send a patch for the shortlogs thing, maybe create

Documentation/process/maintainer-kvm-x86.rst

and link it into Documentation/process/maintainer-handbooks.rst?

> `git log --pretty=oneline` is not a great way to document shortlog
> scopes because it does not explain the rules (e.g. when to use "KVM:
> x86: " vs "KVM: x86/mmu: "), does not explain why things the way they
> are, and is inconsistent since we don't always catch every patch that
> goes by with a non-preferred shortlog scope.

2022-11-09 19:24:26