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!
