[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [RFC/PATCH] Improve speed of mapping guest memory into Dom0
Mats, Your patch has been white-space damaged and will not apply. You should use git send-email which will do the right thing. I've also CC'd Konrad who is the maintainer for the Xen parts of the kernel. On 14/11/12 11:13, Mats Petersson wrote: > [a long, rambling commit message?] The text as-is isn't really suitable for a commit message (too much irrelevant stuff) and there is no suitable subject line. > I have also found that the munmap() call used to unmap the guest memory > from Dom0 is about 35% slower in 3.7 kernel than in the 2.6 kernel (3.8M > cycles vs 2.8M cycles). This performance reduction only occurs with 32-bit guests is the Xen then traps-and-emulates both halves of the PTE write. > I think this could be made quicker by using a > direct write of zero rather than the compare exchange operation that is > currently used [which traps into Xen, performs the compare & exchange] - This is something I noticed but never got around to producing a patch. How about this (uncomplied!) patch? -- a/mm/memory.c +++ b/mm/memory.c @@ -1146,8 +1146,16 @@ again: page->index > details->last_index)) continue; } - ptent = ptep_get_and_clear_full(mm, addr, pte, - tlb->fullmm); + /* + * No need for the expensive atomic get and + * clear for anonymous mappings as the dirty + * and young bits are not used. + */ + if (PageAnon(page)) + pte_clear(mm, addr, pte); + else + ptent = ptep_get_and_clear_full(mm, addr, pte, + tlb->fullmm); tlb_remove_tlb_entry(tlb, pte, addr); if (unlikely(!page)) continue; Now for the patch itself. On the whole, the approach looks good and the real-word performance improvements are nice. Specific comments inline. > --- a/arch/x86/xen/mmu.c > +++ b/arch/x86/xen/mmu.c > @@ -2542,3 +2561,77 @@ out: > return err; > } > EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_range); > + > +/* like xen_remap_domain_mfn_range, but does a list of mfn's, rather > + * than the, for xen, quite useless, consecutive pages. > + */ /* * Like xen_remap_domain_mfn_range(), but more efficiently handles MFNs * that are not contiguous (which is common for a domain's memory). */ > +int xen_remap_domain_mfn_list(struct vm_area_struct *vma, > + unsigned long addr, > + unsigned long *mfn, int nr, > + int *err_ptr, > + pgprot_t prot, unsigned domid) xen_remap_domain_mfn_array() ? It's not taking a list. > +{ > + struct remap_list_data rmd; > + struct mmu_update mmu_update[REMAP_BATCH_SIZE]; This is surprisingly large (256 bytes) but I note that the existing xen_remap_domain_mfn_range() does the same thing so I guess it's ok. > + int batch; > + int done; > + unsigned long range; > + int err = 0; > + > + if (xen_feature(XENFEAT_auto_translated_physmap)) > + return -EINVAL; > + > + prot = __pgprot(pgprot_val(prot) | _PAGE_IOMAP); > + > + BUG_ON(!((vma->vm_flags & (VM_PFNMAP | VM_IO)) == (VM_PFNMAP | > VM_IO))); > + > + rmd.mfn = mfn; > + rmd.prot = prot; > + > + while (nr) { > + batch = min(REMAP_BATCH_SIZE, nr); > + range = (unsigned long)batch << PAGE_SHIFT; > + > + rmd.mmu_update = mmu_update; > + err = apply_to_page_range(vma->vm_mm, addr, range, > + remap_area_mfn_list_pte_fn, &rmd); > + if (err) > + { > + printk("xen_remap_domain_mfn_list: apply_to_range: > err=%d\n", err); Stray printk? > + goto out; > + } > + > + err = HYPERVISOR_mmu_update(mmu_update, batch, &done, domid); > + if (err < 0) > + { > + int i; > + /* TODO: We should remove this printk later */ > + printk("xen_remap_domain_mfn_list: mmu_update: err=%d, > done=%d, batch=%d\n", err, done, batch); Yes, you should... > + err_ptr[done] = err; > + > + /* now do the remaining part of this batch */ > + for(i = done+1; i < batch; i++) > + { > + int tmp_err = HYPERVISOR_mmu_update(&mmu_update[i], 1, > NULL, domid); > + if (tmp_err < 0) > + { > + err_ptr[i] = tmp_err; > + } > + } There's no need to fall back to doing it page-by-page here. You can still batch the remainder. > + > + goto out; > + } > + > + nr -= batch; > + addr += range; > + err_ptr += batch; > + } > + > + err = 0; > +out: > + > + xen_flush_tlb_all(); Probably not that important anymore since we would now do far fewer TLB flushes, but this TLB flush is only needed by the PTEs being updated were already present -- if they're all clear then TLB flush can be omitted entirely. > + > + return err; > +} > +EXPORT_SYMBOL_GPL(xen_remap_domain_mfn_list); > diff --git a/drivers/xen/privcmd.c b/drivers/xen/privcmd.c > index 8adb9cc..b39a7b7 100644 > --- a/drivers/xen/privcmd.c > +++ b/drivers/xen/privcmd.c > @@ -151,6 +151,41 @@ static int traverse_pages(unsigned nelem, size_t size, > return ret; > } > > +/* > + * Similar to traverse_pages, but use each page as a "block" of > + * data to be processed as one unit. > + */ > +static int traverse_pages_block(unsigned nelem, size_t size, > + struct list_head *pos, > + int (*fn)(void *data, int nr, void *state), > + void *state) > +{ > + void *pagedata; > + unsigned pageidx; > + int ret = 0; > + > + BUG_ON(size > PAGE_SIZE); > + > + pageidx = PAGE_SIZE; > + pagedata = NULL; /* hush, gcc */ What was gcc upset about? I don't see anything it could get confused about. > @@ -260,17 +295,17 @@ struct mmap_batch_state { > xen_pfn_t __user *user_mfn; > }; > > -static int mmap_batch_fn(void *data, void *state) > +static int mmap_batch_fn(void *data, int nr, void *state) > { > xen_pfn_t *mfnp = data; > + > struct mmap_batch_state *st = state; > int ret; > > - ret = xen_remap_domain_mfn_range(st->vma, st->va & PAGE_MASK, > *mfnp, 1, > - st->vma->vm_page_prot, st->domain); > + BUG_ON(nr < 0); Is this BUG_ON() useful? David _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |