Re: [Xen-devel] [PATCH] Improve performance of IOCTL_PRIVCMD_MMAPBATCH_V2

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 16/11/12 14:45, Mats Petersson wrote:

Add "xen/privcmd:" prefix to subject.

> This patch makes the IOCTL_PRIVCMD_MMAPBATCH_V2 (and older V1 version)
> map multiple (typically 1024) pages at a time rather than one page at a
> time, despite the pages being non-consecutive MFNs. The main change is
> to pass a pointer to an array of mfns, rather than one mfn. To support
> error reporting, we also pass an err_ptr. If err_ptr is NULL, it indicates
> we want the contiguous pages behaviour, so the mfn value is incremented
> rather than the pointer itself. Performance of mapping guest memory into
> Dom0 is improved by a factor of around 6 with this change.

Can you include details on the test and the raw figures as well?

> Signed-off-by: Mats Petersson <mats.petersson@xxxxxxxxxx>
> ---
>  arch/x86/xen/mmu.c    |   47 ++++++++++++++++++++++++++-------
>  drivers/xen/privcmd.c |   70 
> ++++++++++++++++++++++++++++++++++++++++++-------
>  include/xen/xen-ops.h |    5 ++--
>  3 files changed, 100 insertions(+), 22 deletions(-)
> 
> diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c
> index dcf5f2d..c5e23ba 100644
> --- a/arch/x86/xen/mmu.c
> +++ b/arch/x86/xen/mmu.c
> @@ -2477,7 +2477,8 @@ void __init xen_hvm_init_mmu_ops(void)
>  #define REMAP_BATCH_SIZE 16
>  
>  struct remap_data {
> -     unsigned long mfn;
> +     unsigned long *mfn;
> +     int contiguous;

bool.

>       pgprot_t prot;
>       struct mmu_update *mmu_update;
>  };
> @@ -2486,7 +2487,13 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, 
> pgtable_t token,
>                                unsigned long addr, void *data)
>  {
>       struct remap_data *rmd = data;
> -     pte_t pte = pte_mkspecial(pfn_pte(rmd->mfn++, rmd->prot));
> +     pte_t pte = pte_mkspecial(pfn_pte(*rmd->mfn, rmd->prot));
> +     /* If we have a contigious range, just update the mfn itself,
> +        else update pointer to be "next mfn". */
> +     if (rmd->contiguous)
> +             (*rmd->mfn)++;
> +     else
> +             rmd->mfn++;
>  
>       rmd->mmu_update->ptr = virt_to_machine(ptep).maddr;
>       rmd->mmu_update->val = pte_val_ma(pte);
> @@ -2495,16 +2502,17 @@ static int remap_area_mfn_pte_fn(pte_t *ptep, 
> pgtable_t token,
>       return 0;
>  }
>  
> +
>  int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
>                              unsigned long addr,
> -                            unsigned long mfn, int nr,
> -                            pgprot_t prot, unsigned domid)
> +                            unsigned long *mfn, int nr,
> +                            int *err_ptr, pgprot_t prot,
> +                            unsigned domid)
>  {
> +     int err;
>       struct remap_data rmd;
>       struct mmu_update mmu_update[REMAP_BATCH_SIZE];
> -     int batch;
>       unsigned long range;
> -     int err = 0;
>  
>       if (xen_feature(XENFEAT_auto_translated_physmap))
>               return -EINVAL;
> @@ -2515,9 +2523,15 @@ int xen_remap_domain_mfn_range(struct vm_area_struct 
> *vma,
>  
>       rmd.mfn = mfn;
>       rmd.prot = prot;
> +     /* We use the err_ptr to indicate if there we are doing a contigious
> +      * mapping or a discontigious mapping. */
> +     rmd.contiguous = !err_ptr;

This is non-obvious for an API call.  Suggest having two wrapper
functions for the two different use cases that share a common internal
implementation.

e.g.,

int xen_remap_domain_mfn_array(struct vm_area_struct *vma,
                       unsigned long addr,
                       unsigned long *mfns, int nr,
                       int *err_ptr,
                       pgprot_t prot, unsigned domid)

int xen_remap_domain_mfn_range(struct vm_area_struct *vma,
                       unsigned long addr,
                       unsigned long start_mfn, unsigned long end_mfn,
                       pgprot_t prot, unsigned domid)

It would be nice if the API calls had some docs as well (e.g., in
kernel-doc style).

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel