RE: [PATCH v1] KVM: selftests: name the threads

[Wang, Wei W]

On Thursday, October 20, 2022 1:29 AM, David Matlack wrote:
> > +/*
> > + * Create a named thread
> > + *
> > + * Input Args:
> 
> thread is missing here

It's an output, I think.

> 
> > + *   attr - the attributes for the new thread
> > + *   start_routine - the routine to run in the thread context
> > + *   arg - the argument passed to start_routine
> > + *   vcpu_id - the id of the vcpu
> > + *
> > + * Output Args:
> > + *   thread - the thread to be created
> > + *
> > + * Create a vcpu thread with the name in "vcpu##id" format.
> > + */
> > +void kvm_create_vcpu_thread(pthread_t *thread, const pthread_attr_t
> > +*attr,
> 
> If I'm reading the patch correctly, attr is always NULL for vCPU threads, so just
> drop it until we need it?

Yeah, sounds good.

> 
> > +			void *(*start_routine)(void *), void *arg, int vcpu_id)
> 
> I think it would be helpful to tie the vcpu_id to something real, rather than
> leaving it up to the caller. How about passing in the struct kvm_vcpu here and
> using vcpu->id in the thread name?

Yes, with the cleanup you mentioned below, it would be better.

> 
> Another cleanup we could do on top of this series would be to stash the vCPU
> pthread_t into struct kvm_vcpu, 

Yes, I also thought about this when I made this patch. I have other optimizations along
with this change, and this will have a lot more changes, but we will get much better code
consolidation. Let me post them together in v2 to discuss.

> which would eliminate another parameter here
> and make this API super clean:
> 
> void vcpu_create_thread(struct kvm_vcpu *vcpu, void *(*fn)(void *), void *arg) {
> 	...
> }
> > +{
> > +	char vcpu_name[6];
> > +
> > +	sprintf(vcpu_name, "%s%d", "vcpu", vcpu_id);
> 
> There's no need to dynamically insert "vcpu". Also, could we include a dash to
> make the name slightly easier to parse? Putting it together...

Right ;)
Thanks!