On Oct 1, 2014, at 4:35 PM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote: > 2014-10-01 10:40+0300, Nadav Amit: >> On Oct 1, 2014, at 3:09 AM, Radim Krčmář <rkrcmar@xxxxxxxxxx> wrote: >>> 2014-09-29 21:15+0300, Nadav Amit: >>>> @@ -657,9 +663,13 @@ bool kvm_irq_delivery_to_apic_fast(struct kvm *kvm, struct kvm_lapic *src, >>>> if (!map) >>>> goto out; >>>> >>>> + /* broadcast on physical or multiple clusters is done slowly */ >>>> + if (irq->dest_id == map->broadcast && >>> >>> (If we dropped the second && condition, broadcast check could be moved >>> before we rcu_dereference the map. Using kvm_apic_broadcast instead.) >>> >>>> + (irq->dest_mode == 0 || map->cid_mask != 0)) >>> >>> It's a clever and pretty hacky use of cid_mask[1] ... I think it would >>> be better to have a broadcast fast-path for all modes, so we would do >>> something like this >>> >>> if (irq->dest == kvm_apic_broadcast(apic)) >>> return HANDLE_BROADCAST(…); >>> >>> in the end and having 'return 0' fallback for now is closer to our goal. >> I agree that broadcast may be done more efficiently, but broadcast using shorthand is also done slowly today. > > True, we would probably completely rewrite both variants. > >> So I think it should go into different patch. > > (The rework, definitely, but there isn't much difference between 'goto > out' and 'return 0' after our broadcast check.) > >>> >>>> + goto out; I tried to implement your suggestion, but I don’t manage to make a nice and valid fix. The problem is that to determine the broadcast mode in the way you suggested, you need to have the lapic struct of one of the vCPUs with enabled apic. In kvm_irq_delivery_to_apic_fast, src may be NULL (if not issued by another CPU), or of a vCPU with disabled APIC. In such case I don’t know how to determine what is the broadcast message without looking at apic_map. That is the reason I used the apic_map before. Since I used the it, I do not use return but “goto out”, so rcu_read_unlock would be called. So unless you can suggest a better way, I would leave this part as is, and determine broadcast according to the new map->broadcast, as I did before. Thanks, Nadav
Attachment:
signature.asc
Description: Message signed with OpenPGP using GPGMail
