[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCHv4 03/14] xen/grant-table: pre-populate kernel unmap ops for xen_gnttab_unmap_refs()



On Mon, 26 Jan 2015, David Vrabel wrote:
> When unmapping grants, instead of converting the kernel map ops to
> unmap ops on the fly, pre-populate the set of unmap ops.
> 
> This allows the grant unmap for the kernel mappings to be trivially
> batched in the future.
> 
> Signed-off-by: David Vrabel <david.vrabel@xxxxxxxxxx>
> ---
>  arch/arm/include/asm/xen/page.h |    2 +-
>  arch/arm/xen/p2m.c              |    2 +-
>  arch/x86/include/asm/xen/page.h |    2 +-
>  arch/x86/xen/p2m.c              |   21 ++++++++++-----------
>  drivers/xen/gntdev.c            |   26 ++++++++++++++++++--------
>  drivers/xen/grant-table.c       |    4 ++--
>  include/xen/grant_table.h       |    2 +-
>  7 files changed, 34 insertions(+), 25 deletions(-)
> 
> diff --git a/arch/arm/include/asm/xen/page.h b/arch/arm/include/asm/xen/page.h
> index 68c739b..2f7e6ff 100644
> --- a/arch/arm/include/asm/xen/page.h
> +++ b/arch/arm/include/asm/xen/page.h
> @@ -92,7 +92,7 @@ extern int set_foreign_p2m_mapping(struct 
> gnttab_map_grant_ref *map_ops,
>                                  struct page **pages, unsigned int count);
>  
>  extern int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref 
> *unmap_ops,
> -                                  struct gnttab_map_grant_ref *kmap_ops,
> +                                  struct gnttab_unmap_grant_ref *kunmap_ops,
>                                    struct page **pages, unsigned int count);
>  
>  bool __set_phys_to_machine(unsigned long pfn, unsigned long mfn);
> diff --git a/arch/arm/xen/p2m.c b/arch/arm/xen/p2m.c
> index 0548577..cb7a14c 100644
> --- a/arch/arm/xen/p2m.c
> +++ b/arch/arm/xen/p2m.c
> @@ -102,7 +102,7 @@ int set_foreign_p2m_mapping(struct gnttab_map_grant_ref 
> *map_ops,
>  EXPORT_SYMBOL_GPL(set_foreign_p2m_mapping);
>  
>  int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref *unmap_ops,
> -                           struct gnttab_map_grant_ref *kmap_ops,
> +                           struct gnttab_unmap_grant_ref *kunmap_ops,
>                             struct page **pages, unsigned int count)
>  {
>       int i;
> diff --git a/arch/x86/include/asm/xen/page.h b/arch/x86/include/asm/xen/page.h
> index 5eea099..e9f52fe 100644
> --- a/arch/x86/include/asm/xen/page.h
> +++ b/arch/x86/include/asm/xen/page.h
> @@ -55,7 +55,7 @@ extern int set_foreign_p2m_mapping(struct 
> gnttab_map_grant_ref *map_ops,
>                                  struct gnttab_map_grant_ref *kmap_ops,
>                                  struct page **pages, unsigned int count);
>  extern int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref 
> *unmap_ops,
> -                                  struct gnttab_map_grant_ref *kmap_ops,
> +                                  struct gnttab_unmap_grant_ref *kunmap_ops,
>                                    struct page **pages, unsigned int count);
>  extern unsigned long m2p_find_override_pfn(unsigned long mfn, unsigned long 
> pfn);
>  
> diff --git a/arch/x86/xen/p2m.c b/arch/x86/xen/p2m.c
> index 70fb507..df40b28 100644
> --- a/arch/x86/xen/p2m.c
> +++ b/arch/x86/xen/p2m.c
> @@ -816,7 +816,7 @@ static struct page *m2p_find_override(unsigned long mfn)
>  }
>  
>  static int m2p_remove_override(struct page *page,
> -                            struct gnttab_map_grant_ref *kmap_op,
> +                            struct gnttab_unmap_grant_ref *kunmap_op,
>                              unsigned long mfn)
>  {
>       unsigned long flags;
> @@ -840,7 +840,7 @@ static int m2p_remove_override(struct page *page,
>       list_del(&page->lru);
>       spin_unlock_irqrestore(&m2p_override_lock, flags);
>  
> -     if (kmap_op != NULL) {
> +     if (kunmap_op != NULL) {
>               if (!PageHighMem(page)) {
>                       struct multicall_space mcs;
>                       struct gnttab_unmap_and_replace *unmap_op;
> @@ -855,13 +855,13 @@ static int m2p_remove_override(struct page *page,
>                        * issued. In this case handle is going to -1 because
>                        * it hasn't been modified yet.
>                        */
> -                     if (kmap_op->handle == -1)
> +                     if (kunmap_op->handle == -1)
>                               xen_mc_flush();
>                       /*
>                        * Now if kmap_op->handle is negative it means that the
>                        * hypercall actually returned an error.
>                        */
> -                     if (kmap_op->handle == GNTST_general_error) {
> +                     if (kunmap_op->handle == GNTST_general_error) {
>                               pr_warn("m2p_remove_override: pfn %lx mfn %lx, 
> failed to modify kernel mappings",
>                                       pfn, mfn);
>                               put_balloon_scratch_page();
> @@ -873,9 +873,9 @@ static int m2p_remove_override(struct page *page,
>                       mcs = __xen_mc_entry(
>                               sizeof(struct gnttab_unmap_and_replace));
>                       unmap_op = mcs.args;
> -                     unmap_op->host_addr = kmap_op->host_addr;
> +                     unmap_op->host_addr = kunmap_op->host_addr;
>                       unmap_op->new_addr = scratch_page_address;
> -                     unmap_op->handle = kmap_op->handle;
> +                     unmap_op->handle = kunmap_op->handle;
>  
>                       MULTI_grant_table_op(mcs.mc,
>                               GNTTABOP_unmap_and_replace, unmap_op, 1);
> @@ -887,7 +887,6 @@ static int m2p_remove_override(struct page *page,
>  
>                       xen_mc_issue(PARAVIRT_LAZY_MMU);
>  
> -                     kmap_op->host_addr = 0;
>                       put_balloon_scratch_page();
>               }
>       }
> @@ -912,7 +911,7 @@ static int m2p_remove_override(struct page *page,
>  }
>  
>  int clear_foreign_p2m_mapping(struct gnttab_unmap_grant_ref *unmap_ops,
> -                           struct gnttab_map_grant_ref *kmap_ops,
> +                           struct gnttab_unmap_grant_ref *kunmap_ops,
>                             struct page **pages, unsigned int count)
>  {
>       int i, ret = 0;
> @@ -921,7 +920,7 @@ int clear_foreign_p2m_mapping(struct 
> gnttab_unmap_grant_ref *unmap_ops,
>       if (xen_feature(XENFEAT_auto_translated_physmap))
>               return 0;
>  
> -     if (kmap_ops &&
> +     if (kunmap_ops &&
>           !in_interrupt() &&
>           paravirt_get_lazy_mode() == PARAVIRT_LAZY_NONE) {
>               arch_enter_lazy_mmu_mode();
> @@ -942,8 +941,8 @@ int clear_foreign_p2m_mapping(struct 
> gnttab_unmap_grant_ref *unmap_ops,
>               ClearPagePrivate(pages[i]);
>               set_phys_to_machine(pfn, pages[i]->index);
>  
> -             if (kmap_ops)
> -                     ret = m2p_remove_override(pages[i], &kmap_ops[i], mfn);
> +             if (kunmap_ops)
> +                     ret = m2p_remove_override(pages[i], &kunmap_ops[i], 
> mfn);
>               if (ret)
>                       goto out;
>       }
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 073b4a1..32f6bfe 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -91,6 +91,7 @@ struct grant_map {
>       struct gnttab_map_grant_ref   *map_ops;
>       struct gnttab_unmap_grant_ref *unmap_ops;
>       struct gnttab_map_grant_ref   *kmap_ops;
> +     struct gnttab_unmap_grant_ref *kunmap_ops;
>       struct page **pages;
>  };
>  
> @@ -124,6 +125,7 @@ static void gntdev_free_map(struct grant_map *map)
>       kfree(map->map_ops);
>       kfree(map->unmap_ops);
>       kfree(map->kmap_ops);
> +     kfree(map->kunmap_ops);
>       kfree(map);
>  }
>  
> @@ -140,11 +142,13 @@ static struct grant_map *gntdev_alloc_map(struct 
> gntdev_priv *priv, int count)
>       add->map_ops   = kcalloc(count, sizeof(add->map_ops[0]), GFP_KERNEL);
>       add->unmap_ops = kcalloc(count, sizeof(add->unmap_ops[0]), GFP_KERNEL);
>       add->kmap_ops  = kcalloc(count, sizeof(add->kmap_ops[0]), GFP_KERNEL);
> +     add->kunmap_ops = kcalloc(count, sizeof(add->kunmap_ops[0]), 
> GFP_KERNEL);
>       add->pages     = kcalloc(count, sizeof(add->pages[0]), GFP_KERNEL);
>       if (NULL == add->grants    ||
>           NULL == add->map_ops   ||
>           NULL == add->unmap_ops ||
>           NULL == add->kmap_ops  ||
> +         NULL == add->kunmap_ops ||
>           NULL == add->pages)
>               goto err;
>  
> @@ -155,6 +159,7 @@ static struct grant_map *gntdev_alloc_map(struct 
> gntdev_priv *priv, int count)
>               add->map_ops[i].handle = -1;
>               add->unmap_ops[i].handle = -1;
>               add->kmap_ops[i].handle = -1;
> +             add->kunmap_ops[i].handle = -1;
>       }
>  
>       add->index = 0;
> @@ -261,8 +266,6 @@ static int map_grant_pages(struct grant_map *map)
>                       gnttab_set_map_op(&map->map_ops[i], addr, map->flags,
>                               map->grants[i].ref,
>                               map->grants[i].domid);
> -                     gnttab_set_unmap_op(&map->unmap_ops[i], addr,
> -                             map->flags, -1 /* handle */);
>               }
>       } else {
>               /*
> @@ -290,13 +293,20 @@ static int map_grant_pages(struct grant_map *map)
>               return err;
>  
>       for (i = 0; i < map->count; i++) {
> -             if (map->map_ops[i].status)
> +             if (map->map_ops[i].status) {
>                       err = -EINVAL;
> -             else {
> -                     BUG_ON(map->map_ops[i].handle == -1);
> -                     map->unmap_ops[i].handle = map->map_ops[i].handle;
> -                     pr_debug("map handle=%d\n", map->map_ops[i].handle);
> +                     continue;
>               }
> +
> +             gnttab_set_unmap_op(&map->unmap_ops[i],
> +                                 map->map_ops[i].host_addr,
> +                                 map->flags,
> +                                 map->map_ops[i].handle);

If !use_ptemod (AKA XENFEAT_auto_translated_physmap), we end up calling
__pa(addr) twice, that is potentially incorrect.

You might be better off avoiding gnttab_set_unmap_op, and open-coding it.


> +             if (use_ptemod)
> +                     gnttab_set_unmap_op(&map->kunmap_ops[i],
> +                                         map->kmap_ops[i].host_addr,
> +                                         map->flags | GNTMAP_host_map,
> +                                         map->kmap_ops[i].handle);
>       }
>       return err;
>  }

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.