[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2] x86/domain_page: implement pure per-vCPU mapping infrastructure
On Fri, 2020-02-21 at 11:50 +0000, Wei Liu wrote: > On Thu, Feb 06, 2020 at 06:58:23PM +0000, Hongyan Xia wrote: > > ... > > > > Benchmarks > > (baseline uses direct map instead of the mapcache in > > map_domain_page, > > old is the existing mapcache and new is after this patch): > > > > Geekbench on a 32-vCPU guest, > > > > performance drop old new > > single core 0.04% 0.18% > > Do you know why the new mapcache has more overhead than the old one > in > the single core case? To be honest I think this is within noise territory. The benchmarks were run 5 times, I can average over even more runs to confirm. > [...] > > > void unmap_domain_page(const void *ptr) > > { > > - unsigned int idx; > > + unsigned int idx, glb_idx; > > struct vcpu *v; > > - struct mapcache_domain *dcache; > > - unsigned long va = (unsigned long)ptr, mfn, flags; > > - struct vcpu_maphash_entry *hashent; > > + struct mapcache_vcpu *vcache; > > + unsigned long va = (unsigned long)ptr, mfn, hashmfn, flags; > > > > if ( va >= DIRECTMAP_VIRT_START ) > > return; > > @@ -189,114 +144,78 @@ void unmap_domain_page(const void *ptr) > > v = mapcache_current_vcpu(); > > ASSERT(v && is_pv_vcpu(v)); > > > > - dcache = &v->domain->arch.pv.mapcache; > > - ASSERT(dcache->inuse); > > + vcache = v->arch.pv.mapcache; > > + ASSERT(vcache); > > > > - idx = PFN_DOWN(va - MAPCACHE_VIRT_START); > > - mfn = l1e_get_pfn(MAPCACHE_L1ENT(idx)); > > - hashent = &v->arch.pv.mapcache.hash[MAPHASH_HASHFN(mfn)]; > > Please also assert the va in question is within the range allocated > to > this vcpu. Sure. > > + glb_idx = PFN_DOWN(va - MAPCACHE_VIRT_START); > > + idx = glb_idx - v->vcpu_id * MAPCACHE_VCPU_ENTRIES; > > > > local_irq_save(flags); > > > > - if ( hashent->idx == idx ) > > - { > > - ASSERT(hashent->mfn == mfn); > > - ASSERT(hashent->refcnt); > > - hashent->refcnt--; > > - } > > - else if ( !hashent->refcnt ) > > - { > > - if ( hashent->idx != MAPHASHENT_NOTINUSE ) > > - { > > - /* /First/, zap the PTE. */ > > - ASSERT(l1e_get_pfn(MAPCACHE_L1ENT(hashent->idx)) == > > - hashent->mfn); > > - l1e_write(&MAPCACHE_L1ENT(hashent->idx), l1e_empty()); > > - /* /Second/, mark as garbage. */ > > - set_bit(hashent->idx, dcache->garbage); > > - } > > - > > - /* Add newly-freed mapping to the maphash. */ > > - hashent->mfn = mfn; > > - hashent->idx = idx; > > - } > > - else > > - { > > - /* /First/, zap the PTE. */ > > - l1e_write(&MAPCACHE_L1ENT(idx), l1e_empty()); > > - /* /Second/, mark as garbage. */ > > - set_bit(idx, dcache->garbage); > > - } > > + mfn = l1e_get_pfn(MAPCACHE_L1ENT(glb_idx)); > > + hashmfn = vcache->hash_mfn[MAPHASH_HASHFN(mfn)]; > > + > > + vcache->refcnt[idx]--; > > + /* If refcnt drops to 0, we only clear inuse when it's not in > > the maphash. */ > > It would be clearer to "when it has been evicted from maphash" to > match > the terminology in the map routine. Will reword in next revision. > > + if ( hashmfn != mfn && !vcache->refcnt[idx] ) > > + __clear_bit(idx, vcache->inuse); > > Also, please flush the linear address here and the other __clear_bit > location. I flush when a new entry is taking a slot. Yeah, it's probably better to flush earlier whenever a slot is no longer in use. > > > > local_irq_restore(flags); > > } > > > > -int mapcache_domain_init(struct domain *d) > > +int mapcache_vcpu_init(struct vcpu *v) > > { > > + struct domain *d = v->domain; > > struct mapcache_domain *dcache = &d->arch.pv.mapcache; > > - unsigned int bitmap_pages; > > - > > - ASSERT(is_pv_domain(d)); > > + unsigned long i; > > Since you're changing this anyway -- we normally use unsigned int as > index type. > > > + unsigned int ents = d->max_vcpus * MAPCACHE_VCPU_ENTRIES; > > > > #ifdef NDEBUG > > if ( !mem_hotplug && max_page <= > > PFN_DOWN(__pa(HYPERVISOR_VIRT_END - 1)) ) > > return 0; > > #endif > > > > - BUILD_BUG_ON(MAPCACHE_VIRT_END + PAGE_SIZE * (3 + > > - 2 * PFN_UP(BITS_TO_LONGS(MAPCACHE_ENTRIES) * > > sizeof(long))) > > > - MAPCACHE_VIRT_START + (PERDOMAIN_SLOT_MBYTES << > > 20)); > > - bitmap_pages = PFN_UP(BITS_TO_LONGS(MAPCACHE_ENTRIES) * > > sizeof(long)); > > - dcache->inuse = (void *)MAPCACHE_VIRT_END + PAGE_SIZE; > > - dcache->garbage = dcache->inuse + > > - (bitmap_pages + 1) * PAGE_SIZE / > > sizeof(long); > > - > > - spin_lock_init(&dcache->lock); > > - > > - return create_perdomain_mapping(d, (unsigned long)dcache- > > >inuse, > > - 2 * bitmap_pages + 1, > > - NIL(l1_pgentry_t *), NULL); > > -} > > - > > -int mapcache_vcpu_init(struct vcpu *v) > > -{ > > - struct domain *d = v->domain; > > - struct mapcache_domain *dcache = &d->arch.pv.mapcache; > > - unsigned long i; > > - unsigned int ents = d->max_vcpus * MAPCACHE_VCPU_ENTRIES; > > - unsigned int nr = PFN_UP(BITS_TO_LONGS(ents) * sizeof(long)); > > - > > - if ( !is_pv_vcpu(v) || !dcache->inuse ) > > + if ( is_idle_vcpu(v) || is_hvm_vcpu(v) ) > > return 0; > > > > + BUILD_BUG_ON(MAPCACHE_VIRT_END > ARG_XLAT_VIRT_START); > > + > > if ( ents > dcache->entries ) > > { > > + int rc; > > + > > + ASSERT(ents * PAGE_SIZE <= (PERDOMAIN_SLOT_MBYTES << 20)); > > + > > /* Populate page tables. */ > > - int rc = create_perdomain_mapping(d, MAPCACHE_VIRT_START, > > ents, > > + rc = create_perdomain_mapping(d, MAPCACHE_VIRT_START, > > ents, > > NIL(l1_pgentry_t *), > > NULL); > > > > - /* Populate bit maps. */ > > - if ( !rc ) > > - rc = create_perdomain_mapping(d, (unsigned > > long)dcache->inuse, > > - nr, NULL, NIL(struct > > page_info *)); > > - if ( !rc ) > > - rc = create_perdomain_mapping(d, (unsigned > > long)dcache->garbage, > > - nr, NULL, NIL(struct > > page_info *)); > > - > > if ( rc ) > > return rc; > > > > dcache->entries = ents; > > Given that: > > 1. mapcache_domain is now a structure with only one member. > 2. ents is a constant throughout domain's lifecycle. > > You can replace mapcache_domain with a boolean -- > mapcache_mapping_populated (?) in arch.pv. > > If I'm not mistaken, the size of the mapping is derived from the vcpu > being initialised, so a further improvement is to lift the mapping > creation out of mapcache_vcpu_init. But you can just XEN_DOMCTL_max_vcpus on a running domain to increase its max_vcpus count, so that ents is not constant? > > } > > > > - /* Mark all maphash entries as not in use. */ > > BUILD_BUG_ON(MAPHASHENT_NOTINUSE < MAPCACHE_ENTRIES); > > + /* MAPHASH_ENTRIES has to be power-of-two to make hashing > > work. */ > > + BUILD_BUG_ON(MAPHASH_ENTRIES & (MAPHASH_ENTRIES - 1)); > > + /* > > + * Since entries in the maphash also occupy inuse slots, we > > have to make > > + * sure MAPCACHE_VCPU_ENTRIES is large enough to accommodate > > both the > > + * maphash and a nested page table walk. > > + */ > > + BUILD_BUG_ON(MAPCACHE_VCPU_ENTRIES - MAPHASH_ENTRIES < > > + CONFIG_PAGING_LEVELS * CONFIG_PAGING_LEVELS); > > + > > + v->arch.pv.mapcache = xzalloc(struct mapcache_vcpu); > > + if ( !v->arch.pv.mapcache ) > > + return -ENOMEM; > > + > > + /* Mark all maphash entries as not in use. */ > > for ( i = 0; i < MAPHASH_ENTRIES; i++ ) > > { > > - struct vcpu_maphash_entry *hashent = &v- > > >arch.pv.mapcache.hash[i]; > > - > > - hashent->mfn = ~0UL; /* never valid to map */ > > - hashent->idx = MAPHASHENT_NOTINUSE; > > + v->arch.pv.mapcache->hash_mfn[i] = ~0UL; > > mfn_x(INVALID_MFN) here. Or you can change the type of the array to > mfn_t. mfn_t is a better idea. Thanks. Hongyan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxxxxxxxxx https://lists.xenproject.org/mailman/listinfo/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |