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
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
