linux-kernel - Re: [PATCH v1 03/18] KVM: selftests/kvm_util: helper functions for vcpus and threads

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: <Y1nMQp11RKTDX7HX@google.com>
Date:   Thu, 27 Oct 2022 00:09:38 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Wei Wang <wei.w.wang@...el.com>
Cc:     pbonzini@...hat.com, dmatlack@...gle.com, vipinsh@...gle.com,
        ajones@...tanamicro.com, eric.auger@...hat.com,
        kvm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 03/18] KVM: selftests/kvm_util: helper functions for
 vcpus and threads

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
>