linux-kernel - RE: [PATCH v1] KVM: selftests: name the 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: <DS0PR11MB6373943463AEA7C3617CAE30DC2A9@DS0PR11MB6373.namprd11.prod.outlook.com>
Date:   Thu, 20 Oct 2022 11:37:19 +0000
From:   "Wang, Wei W" <wei.w.wang@...el.com>
To:     David Matlack <dmatlack@...gle.com>
CC:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "vipinsh@...gle.com" <vipinsh@...gle.com>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v1] KVM: selftests: name the threads

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!