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: > >> KVM does not deliver x2APIC broadcast messages with physical mode. Intel SDM > >> (10.12.9 ICR Operation in x2APIC Mode) states: "A destination ID value of > >> FFFF_FFFFH is used for broadcast of interrupts in both logical destination and > >> physical destination modes." > > > > Btw. have you hit it in the wild? (It was there so long that I suppose > > that all OSes use a shorthand …) > Not in real OSes, but in some tests I run. Great, thanks. (Would be a -stable material otherwise.) > >> The fix tries to combine broadcast in different modes. Broadcast can be done > >> in the fast-path if the delivery-mode is logical and there is only a single > >> cluster. Otherwise, do it slowly. > > > > (Flat logical xapic or current logical x2apic; it doesn't have to do > > much with clusters from Intel's vocabulary …) > I meant if the ICR destination mode is logical (not the delivery-mode) and there is no more than one cluster (if clusters exist in this mode). > [It also applies to logical cluster-mode.] Does this phrasing makes sense to you? 'irq->dest_mode == 0' means that it is logical destination, but 'map->cid_mask != 0' is a variable that doesn't have a well defined relationship to the number of clusters -- it just works for the two cases right now. (Flat logical xapic has 0 clusters :) The original wording is better (xapic logical cluster has cid_mask=0xf), and I was fine with it (just pointing out that it is confusing), which is why I put my comment into parentheses. It would be best to drop the condition and comment IMO. > > xapic broadcast is 0xff in flat and cluster mode, so it would actually > > be another bugfix -- we miss the latter one right now; not that it has > > any users. > I actually submitted a fix for cluster mode - http://www.spinics.net/lists/kvm/msg106351.html > I now see Paolo did not apply it or responded. I will combine the two patches to one patch-set for v2. Oh, I didn't see that, please do. > >> @@ -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; > > > > > > --- > > 1: When we fix logical x2apic, it will have cid_mask > 0 and hopefully > > no-one uses it now, broken and capped at one 16 CPU cluster. > > That would leave us with flat logical mode that supports up to 8 > > CPUs, which is a hard performance decision -- I think that most > > guests will use other modes and the universe will lose more cycles on > > checking for this than it would on broadcasting through slow path :) > > > Thanks for the feedback. > > I completely agree with your revisions, yet I think they are orthogonal and should go into different patches. > Since my current project focus on validation of hypervisors, I am not sure I will have the time to do it well soon. Sure, what you are doing seems much more important. > Regardless, I am not sure whether the performance impact of taking the slow-path is significant. I agree, so there is little reason to do the extra check for performance reasons. The check makes the code worse, which makes performance gain the only possible excuse for it. -- 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
