| Message ID | 20221024113445.1022147-4-wei.w.wang@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM selftests code consolidation and cleanup | expand |
On Mon, Oct 24, 2022, Wei Wang wrote: > @@ -14,4 +15,23 @@ > for (i = 0, vcpu = vm->vcpus[0]; \ > vcpu && i < KVM_MAX_VCPUS; vcpu = vm->vcpus[++i]) > > +void __pthread_create_with_name(pthread_t *thread, const pthread_attr_t *attr, Can these return pthread_t instead of taking them as a param and have a "void" return? I'm pretty sure pthread_t is an integer type in most implementations, i.e. can be cheaply copied by value. > + void *(*start_routine)(void *), void *arg, char *name); Add a typedef for the payload, both to make it harder to screw up, and to make the code more readable. Does pthread really not provide one already? > +void pthread_create_with_name(pthread_t *thread, > + void *(*start_routine)(void *), void *arg, char *name); Align params, e.g. void pthread_create_with_name(pthread_t *thread, void *(*start_routine)(void *), void *arg, char *name); Poking out past the 80 char soft limit is much preferable to arbitrary indentation. Please fix this in all patches. > struct userspace_mem_regions { > diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c > index 1f69f5ca8356..ba3e774087fb 100644 > --- a/tools/testing/selftests/kvm/lib/kvm_util.c > +++ b/tools/testing/selftests/kvm/lib/kvm_util.c > @@ -2006,3 +2006,175 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, > break; > } > } > + > +/* > + * Create a named thread with user's attribute > + * > + * Input Args: > + * attr - the attribute of the thread to create > + * start_routine - the routine to run in the thread context > + * arg - the argument passed to start_routine > + * name - the name of the thread > + * > + * Output Args: > + * thread - the thread to be created > + * > + * Create a thread with a user specified name. > + */ Please don't add function comments, we're opportunistically removing the existing boilerplate ones as we go. Most of the comments, like this one, add very little value as it's pretty obvious what the function does and what the params are. > +void __pthread_create_with_name(pthread_t *thread, const pthread_attr_t *attr, > + void *(*start_routine)(void *), void *arg, char *name) > +{ > + int r; > + > + r = pthread_create(thread, NULL, start_routine, arg); > + TEST_ASSERT(!r, "thread(%s) creation failed, r = %d", name, r); Assuming 'r' is an errno, pretty print its name with strerror(). > + r = pthread_setname_np(*thread, name); > + TEST_ASSERT(!r, "thread(%s) setting name failed, r = %d", name, r); Same here. > +} ... > +void vm_vcpu_threads_create(struct kvm_vm *vm, > + void *(*start_routine)(void *), uint32_t private_data_size) I vote (very strongly) to not deal with allocating private data. The private data isn't strictly related to threads, and the vast majority of callers don't need private data, i.e. the param is dead weight in most cases. And unless I'm missing something, it's trivial to move to a separate helper, though honestly even that seems like overkill. Wait, looking further, it already is a separate helper... Forcing a bunch of callers to specify '0' just to eliminate one function call in a handful of cases is not a good tradeoff. > +void vm_vcpu_threads_private_data_alloc(struct kvm_vm *vm, uint32_t data_size) As above, this isn't strictly related to threads, e.g. vm_alloc_vcpu_private_data() > +{ > + struct kvm_vcpu *vcpu; > + int i; > + > + vm_iterate_over_vcpus(vm, vcpu, i) { > + vcpu->private_data = calloc(1, data_size); > + TEST_ASSERT(vcpu->private_data, "%s: failed", __func__); > + } > +} > -- > 2.27.0 >
On Thursday, October 27, 2022 8:10 AM, Sean Christopherson wrote: > On Mon, Oct 24, 2022, Wei Wang wrote: > > @@ -14,4 +15,23 @@ > > for (i = 0, vcpu = vm->vcpus[0]; \ > > vcpu && i < KVM_MAX_VCPUS; vcpu = vm->vcpus[++i]) > > > > +void __pthread_create_with_name(pthread_t *thread, const > > +pthread_attr_t *attr, > > Can these return pthread_t instead of taking them as a param and have a "void" > return? I'm pretty sure pthread_t is an integer type in most implementations, > i.e. can be cheaply copied by value. Yes, sounds good. > > > + void *(*start_routine)(void *), void *arg, char *name); > > Add a typedef for the payload, both to make it harder to screw up, and to make > the code more readable. Does pthread really not provide one already? You meant typedef for start_routine? I searched throughout pthread.h, and didn't find it. Maybe we could create one here. > > +void vm_vcpu_threads_create(struct kvm_vm *vm, > > + void *(*start_routine)(void *), uint32_t private_data_size) > > I vote (very strongly) to not deal with allocating private data. The private data > isn't strictly related to threads, and the vast majority of callers don't need private > data, i.e. the param is dead weight in most cases. > > And unless I'm missing something, it's trivial to move to a separate helper, > though honestly even that seems like overkill. > > Wait, looking further, it already is a separate helper... Forcing a bunch of > callers to specify '0' just to eliminate one function call in a handful of cases is not > a good tradeoff. The intention was to do the allocation within one vm_for_each_vcpu() iteration when possible. Just a micro-optimization, but no problem, we can keep them separate if that looks better (simpler).
On Thu, Oct 27, 2022, Wang, Wei W wrote: > On Thursday, October 27, 2022 8:10 AM, Sean Christopherson wrote: > > > +void vm_vcpu_threads_create(struct kvm_vm *vm, > > > + void *(*start_routine)(void *), uint32_t private_data_size) > > > > I vote (very strongly) to not deal with allocating private data. The private data > > isn't strictly related to threads, and the vast majority of callers don't need private > > data, i.e. the param is dead weight in most cases. > > > > And unless I'm missing something, it's trivial to move to a separate helper, > > though honestly even that seems like overkill. > > > > Wait, looking further, it already is a separate helper... Forcing a bunch of > > callers to specify '0' just to eliminate one function call in a handful of cases is not > > a good tradeoff. > > The intention was to do the allocation within one vm_for_each_vcpu() > iteration when possible. Just a micro-optimization, but no problem, we can keep > them separate if that looks better (simpler). Keep them separate, that level of optimization is not something that's ever going to be noticeable. I don't want to say that performance is an afterthought for KVM selftests, but in common code it's definitely way down the list of priorities because even the most naive implementation for things like configuring vCPUs is going to have a runtime measured in milliseconds.
diff --git a/tools/testing/selftests/kvm/include/kvm_util.h b/tools/testing/selftests/kvm/include/kvm_util.h index 5d5c8968fb06..036ed05e72e6 100644 --- a/tools/testing/selftests/kvm/include/kvm_util.h +++ b/tools/testing/selftests/kvm/include/kvm_util.h @@ -6,6 +6,7 @@ */ #ifndef SELFTEST_KVM_UTIL_H #define SELFTEST_KVM_UTIL_H +#include <pthread.h> #include "kvm_util_base.h" #include "ucall_common.h" @@ -14,4 +15,23 @@ for (i = 0, vcpu = vm->vcpus[0]; \ vcpu && i < KVM_MAX_VCPUS; vcpu = vm->vcpus[++i]) +void __pthread_create_with_name(pthread_t *thread, const pthread_attr_t *attr, + void *(*start_routine)(void *), void *arg, char *name); + +void pthread_create_with_name(pthread_t *thread, + void *(*start_routine)(void *), void *arg, char *name); + +void __vcpu_thread_create(struct kvm_vcpu *vcpu, const pthread_attr_t *attr, + void *(*start_routine)(void *), uint32_t private_data_size); + +void vcpu_thread_create(struct kvm_vcpu *vcpu, void *(*start_routine)(void *), + uint32_t private_data_size); + +void vm_vcpu_threads_create(struct kvm_vm *vm, + void *(*start_routine)(void *), uint32_t private_data_size); + +void vm_vcpu_threads_join(struct kvm_vm *vm); + +void vm_vcpu_threads_private_data_alloc(struct kvm_vm *vm, uint32_t data_size); + #endif /* SELFTEST_KVM_UTIL_H */ diff --git a/tools/testing/selftests/kvm/include/kvm_util_base.h b/tools/testing/selftests/kvm/include/kvm_util_base.h index c90a9609b853..d0d6aaec0098 100644 --- a/tools/testing/selftests/kvm/include/kvm_util_base.h +++ b/tools/testing/selftests/kvm/include/kvm_util_base.h @@ -55,6 +55,8 @@ struct kvm_vcpu { struct kvm_dirty_gfn *dirty_gfns; uint32_t fetch_index; uint32_t dirty_gfns_count; + pthread_t thread; + void *private_data; }; struct userspace_mem_regions { diff --git a/tools/testing/selftests/kvm/lib/kvm_util.c b/tools/testing/selftests/kvm/lib/kvm_util.c index 1f69f5ca8356..ba3e774087fb 100644 --- a/tools/testing/selftests/kvm/lib/kvm_util.c +++ b/tools/testing/selftests/kvm/lib/kvm_util.c @@ -2006,3 +2006,175 @@ void __vm_get_stat(struct kvm_vm *vm, const char *stat_name, uint64_t *data, break; } } + +/* + * Create a named thread with user's attribute + * + * Input Args: + * attr - the attribute of the thread to create + * start_routine - the routine to run in the thread context + * arg - the argument passed to start_routine + * name - the name of the thread + * + * Output Args: + * thread - the thread to be created + * + * Create a thread with a user specified name. + */ +void __pthread_create_with_name(pthread_t *thread, const pthread_attr_t *attr, + void *(*start_routine)(void *), void *arg, char *name) +{ + int r; + + r = pthread_create(thread, NULL, start_routine, arg); + TEST_ASSERT(!r, "thread(%s) creation failed, r = %d", name, r); + r = pthread_setname_np(*thread, name); + TEST_ASSERT(!r, "thread(%s) setting name failed, r = %d", name, r); +} + +/* + * Create a named thread with the default thread attribute + * + * Input Args: + * start_routine - the routine to run in the thread context + * arg - the argument passed to start_routine + * name - the name of the thread + * + * Output Args: + * thread - the thread to be created + * + * Create a thread with a user specified name and default thread attribute. + */ +void pthread_create_with_name(pthread_t *thread, + void *(*start_routine)(void *), void *arg, char *name) +{ + __pthread_create_with_name(thread, NULL, start_routine, arg, name); +} + +/* + * Create a vcpu thread with user's attribute + * + * Input Args: + * vcpu - the vcpu for which the thread is created + * attr - the attribute of the vcpu thread + * start_routine - the routine to run in the thread context + * private_data_size - the size of the user's per-vcpu private_data + * + * Output Args: + * None + * + * Create a vcpu thread with user provided attribute and the name in + * "vcpu-##id" format. + */ +void __vcpu_thread_create(struct kvm_vcpu *vcpu, const pthread_attr_t *attr, + void *(*start_routine)(void *), uint32_t private_data_size) +{ + char vcpu_name[16]; + + if (private_data_size) { + vcpu->private_data = calloc(1, private_data_size); + TEST_ASSERT(vcpu->private_data, "%s: failed", __func__); + } + + sprintf(vcpu_name, "vcpu-%d", vcpu->id); + __pthread_create_with_name(&vcpu->thread, attr, + start_routine, (void *)vcpu, vcpu_name); +} + +/* + * Create a vcpu thread with the default thread attribute + * + * Input Args: + * vcpu - the vcpu for which the thread is created + * start_routine - the routine to run in the thread context + * private_data_size - the size of the user's per-vcpu private_data + * + * Output Args: + * None + * + * Create a vcpu thread with the default thread attribute and the name in + * "vcpu-##id" format, and allocate memory to be used as the vcpu thread's + * private data if private_data_size isn't 0. + */ +void vcpu_thread_create(struct kvm_vcpu *vcpu, void *(*start_routine)(void *), + uint32_t private_data_size) +{ + __vcpu_thread_create(vcpu, NULL, start_routine, private_data_size); +} + +/* + * Create vcpu threads for all the vcpus that have been created for a VM + * + * Input Args: + * vm - the VM for which the vcpu threads are created + * start_routine - the routine to run in the thread context + * private_data_size - the size of the user's per-vcpu private_data + * + * Output Args: + * None + * + * Create vcpu threads for all the vcpus that have been created for the VM, + * and the thread name in "vcpu-##id" format. Allocate memory to each vcpu + * thread to be used for its private data if private_data_size isn't 0. + */ +void vm_vcpu_threads_create(struct kvm_vm *vm, + void *(*start_routine)(void *), uint32_t private_data_size) +{ + struct kvm_vcpu *vcpu; + uint32_t i; + + vm_iterate_over_vcpus(vm, vcpu, i) + vcpu_thread_create(vcpu, start_routine, private_data_size); + +} + +/* + * Join the VM's vcpu threads + * + * Input Args: + * vm - the VM for which its vcpu threads should join + * + * Output Args: + * None + * + * Iterate over all the vcpus and join the threads. + */ +void vm_vcpu_threads_join(struct kvm_vm *vm) +{ + struct kvm_vcpu *vcpu; + void *one_failure; + unsigned long failures = 0; + int r, i; + + vm_iterate_over_vcpus(vm, vcpu, i) { + r = pthread_join(vcpu->thread, &one_failure); + TEST_ASSERT(r == 0, "failed to join vcpu %d thread", i); + failures += (unsigned long)one_failure; + } + + TEST_ASSERT(!failures, "%s: failed", __func__); +} + +/* + * Allocate memory used for private data of the vm's vcpus + * + * Input Args: + * vm - the VM for which its vcpus will be assigned the allocated memory + * data_size - the size of the memory to allocate + * + * Output Args: + * None + * + * Allocate memory to be used for private data of each vcpu that has been + * created for vm. + */ +void vm_vcpu_threads_private_data_alloc(struct kvm_vm *vm, uint32_t data_size) +{ + struct kvm_vcpu *vcpu; + int i; + + vm_iterate_over_vcpus(vm, vcpu, i) { + vcpu->private_data = calloc(1, data_size); + TEST_ASSERT(vcpu->private_data, "%s: failed", __func__); + } +}
Add a vcpu thread field to the kvm_vcpu struct, so that each user doesn't need to define an array of such threads on their own. The private_data pointer is added and optionally used to hold user specific data, and type casting to the user's data type will be performed in the user vcpu thread's start_routine. A couple of the helper functions are added to support vcpu related operations: pthread_create_with_name is provided to create general threads with user specified name. vcpu_thread_create is provided to create a vcpu thread with name in "vcpu##id" format, vm_vcpu_threads_create is provided to create vcpu threads for the vcpus that have been created for a vm. The thread naming facilitates debugging, performance tuning, runtime pining etc. An example is shown below reported from "top". With naming the vcpu threads, the per-vcpu info becomes more noticeable: PID USER PR NI VIRT RES SHR S %CPU %MEM TIME+ COMMAND 4464 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.97 dirty_log_perf_ 4467 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu0 4469 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.93 vcpu2 4470 root 20 0 4248684 4.0g 1628 R 99.9 26.2 0:50.94 vcpu3 4468 root 20 0 4248684 4.0g 1628 R 99.7 26.2 0:50.93 vcpu1 vm_vcpu_threads_join is provided to join all the vcpu threads. vm_vcpu_threads_private_data_alloc is provided to allocate memory used for user specific private data to each vcpu that have been created to the vm. Signed-off-by: Wei Wang <wei.w.wang@intel.com> --- .../testing/selftests/kvm/include/kvm_util.h | 20 ++ .../selftests/kvm/include/kvm_util_base.h | 2 + tools/testing/selftests/kvm/lib/kvm_util.c | 172 ++++++++++++++++++ 3 files changed, 194 insertions(+)