[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v2 4/4] x86/vmx: Rewrite vmx_sync_pir_to_irr() to be more efficient
On 27.08.2024 15:57, Andrew Cooper wrote: > There are two issues. First, pi_test_and_clear_on() pulls the cache-line to > the CPU and dirties it even if there's nothing outstanding, but the final > for_each_set_bit() is O(256) when O(8) would do, Nit: That's bitmap_for_each() now, I think. And again ... > and would avoid multiple > atomic updates to the same IRR word. > > Rewrite it from scratch, explaining what's going on at each step. > > Bloat-o-meter reports 177 -> 145 (net -32), but the better aspect is the > removal calls to __find_{first,next}_bit() hidden behind for_each_set_bit(). ... here, and no underscore prefixes on the two find functions. > --- a/xen/arch/x86/hvm/vmx/vmx.c > +++ b/xen/arch/x86/hvm/vmx/vmx.c > @@ -2317,18 +2317,72 @@ static void cf_check vmx_deliver_posted_intr(struct > vcpu *v, u8 vector) > > static void cf_check vmx_sync_pir_to_irr(struct vcpu *v) > { > - struct vlapic *vlapic = vcpu_vlapic(v); > - unsigned int group, i; > - DECLARE_BITMAP(pending_intr, X86_NR_VECTORS); > + struct pi_desc *desc = &v->arch.hvm.vmx.pi_desc; > + union { > + uint64_t _64[X86_NR_VECTORS / (sizeof(uint64_t) * 8)]; Using unsigned long here would imo be better, as that's what matches struct pi_desc's DECLARE_BITMAP(). > + uint32_t _32[X86_NR_VECTORS / (sizeof(uint32_t) * 8)]; > + } vec; > + uint32_t *irr; > + bool on; > > - if ( !pi_test_and_clear_on(&v->arch.hvm.vmx.pi_desc) ) > + /* > + * The PIR is a contended cacheline which bounces between the CPU(s) and > + * IOMMU(s). An IOMMU updates the entire PIR atomically, but we can't > + * express the same on the CPU side, so care has to be taken. > + * > + * First, do a plain read of ON. If the PIR hasn't been modified, this > + * will keep the cacheline Shared and not pull it Excusive on the current > + * CPU. > + */ > + if ( !pi_test_on(desc) ) > return; > > - for ( group = 0; group < ARRAY_SIZE(pending_intr); group++ ) > - pending_intr[group] = pi_get_pir(&v->arch.hvm.vmx.pi_desc, group); > + /* > + * Second, if the plain read said that ON was set, we must clear it with > + * an atomic action. This will bring the cachline to Exclusive on the Nit (from my spell checker): cacheline. > + * current CPU. > + * > + * This should always succeed because noone else should be playing with > + * the PIR behind our back, but assert so just in case. > + */ > + on = pi_test_and_clear_on(desc); > + ASSERT(on); > + > + /* > + * The cacheline is now Exclusive on the current CPU, and because ON was "is" is pretty ambitious. We can only hope it (still) is. > + * set, some other entity (an IOMMU, or Xen on another CPU) has indicated > + * that at PIR needs re-scanning. Stray "at"? > + * > + * Note: Entities which can't update the entire cacheline atomically > + * (i.e. Xen on another CPU) are required to update PIR first, then > + * set ON. Therefore, there is a corner case where we may have > + * found and processed the PIR updates "last time around" and only > + * found ON this time around. This is fine; the logic still > + * operates correctly. > + * > + * Atomically read and clear the entire pending bitmap as fast as we, to Missing "can" before the comma? > + * reduce the window where another entity may steal the cacheline back > + * from us. This is a performance concern, not a correctness concern. > If > + * the another entity does steal the cacheline back, we'll just wait to "the other"? > + * get it back again. > + */ > + for ( unsigned int i = 0; i < ARRAY_SIZE(vec._64); ++i ) > + vec._64[i] = xchg(&desc->pir[i], 0); > + > + /* > + * Finally, merge the pending vectors into IRR. The IRR register is > + * scattered in memory, so we have to do this 32 bits at a time. > + */ > + irr = (uint32_t *)&vcpu_vlapic(v)->regs->data[APIC_IRR]; > + for ( unsigned int i = 0; i < ARRAY_SIZE(vec._32); ++i ) > + { > + if ( !vec._32[i] ) > + continue; > > - bitmap_for_each ( i, pending_intr, X86_NR_VECTORS ) > - vlapic_set_vector(i, &vlapic->regs->data[APIC_IRR]); > + asm ( "lock or %[val], %[irr]" > + : [irr] "+m" (irr[i * 0x10]) This wants to be irr * 4 only, to account for sizeof(*irr) == 4. Jan
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |