Re: [PATCHv2] KVM: optimize apic interrupt delivery

[Avi Kivity <avi@redhat.com>]

On 09/11/2012 05:46 PM, Gleb Natapov wrote:
> On Tue, Sep 11, 2012 at 04:26:17PM +0300, Avi Kivity wrote:
>> On 09/11/2012 04:02 PM, Gleb Natapov wrote:
>> > Most interrupt are delivered to only one vcpu. Use pre-build tables to
>> > find interrupt destination instead of looping through all vcpus. In case
>> > of logical mode loop only through vcpus in a logical cluster irq is sent
>> > to.
>> > 
>> >    * fix rcu issues pointed to by MST. All but one. Still use
>> >      call_rcu(). Do not think this is serious issue. If it is should be
>> >      solved by RCU subsystem.
>> 
>> Agree.
>> 
>> Patch looks good but some minor comments follow.
>> 
>> >  struct kvm_arch {
>> >  	unsigned int n_used_mmu_pages;
>> >  	unsigned int n_requested_mmu_pages;
>> > @@ -528,6 +536,8 @@ struct kvm_arch {
>> >  	struct kvm_ioapic *vioapic;
>> >  	struct kvm_pit *vpit;
>> >  	int vapics_in_nmi_mode;
>> > +	struct kvm_apic_map *apic_map;
>> > +	struct mutex apic_map_lock;
>> 
>> Reversing the order will make it clearer what the lock protects.
>> 
> Hmm, OK. I thought names make it clear.

They do, but it is conventional and good practice to put the lock in front.

> 
>> >  
>> > +static void kvm_apic_get_logical_id(u32 ldr, bool flat, u8 ldr_bits,
>> > +		u16 *cid, u16 *lid)
>> > +{
>> > +	if (ldr_bits == 32) {
>> > +		*cid = ldr >> 16;
>> > +		*lid = ldr & 0xffff;
>> > +	} else {
>> > +		ldr = GET_APIC_LOGICAL_ID(ldr);
>> > +
>> > +		if (flat) {
>> > +			*cid = 0;
>> > +			*lid = ldr;
>> > +		} else {
>> > +			*cid = ldr >> 4;
>> > +			*lid = ldr & 0xf;
>> > +		}
>> > +	}
>> > +}
>> 
>> You could precaclulate lid_shift/lid_mask/cid_shift/cid_mask and have
>> just one version here.  In fact you could drop the function.
>> 
> You mean precalculate them in recalculate_apic_map() and store in kvm_apic_map?

Yes.

> 
>> > +
>> > +		new->phys_map[kvm_apic_id(apic)] = apic;
>> > +		kvm_apic_get_logical_id(kvm_apic_get_reg(apic, APIC_LDR),
>> > +				new->flat, new->ldr_bits, &cid, &lid);
>> > +
>> > +		if (lid)
>> > +			new->logical_map[cid][ffs(lid) - 1] = apic;
>> > +	}
>> > +out:
>> > +	old = kvm->arch.apic_map;
>> 
>> rcu_dereference(), just for kicks.
>> 
> MST says rcu_dereference_protected() but honestly I look at it and
> rcu_dereference_check(...., 1) and condition they check are so obviously
> correct in the code that using them is just a clutter. 

They say to the reader, or to a static checker "this case was considered
and that is the conclusion we arrived at".  Using the variable directly
says nothing.

> In more complex
> cases, when dereference happens far away from locking it have its point.
> If you insist on it here should we add it too irq routing code too?

Thanks.

>> >  /*
>> > @@ -6319,6 +6320,7 @@ void kvm_arch_destroy_vm(struct kvm *kvm)
>> >  		put_page(kvm->arch.apic_access_page);
>> >  	if (kvm->arch.ept_identity_pagetable)
>> >  		put_page(kvm->arch.ept_identity_pagetable);
>> > +	kfree(kvm->arch.apic_map);
>> 
>> rcu_dereference(), even though it cannot be needed here, to shut down
>> static code checkers.
>> 
> How to run those code checkers? Do they complain about irq routing code?
> Just curious.

No idea.  Julia Lawall sends patches from time to time with this kind of
things, so people do run them.


-- 
error compiling committee.c: too many arguments to function