Re: [Xen-devel] [PATCH 4/6] xen-gntdev: Use find_vma rather than iterating our vma list manually

[Jeremy Fitzhardinge <jeremy@xxxxxxxx>]

On 12/14/2010 06:55 AM, Daniel De Graaf wrote:
> This should be faster if many mappings exist,
Yes, seems reasonable.

>  and also removes
> the only user of vma that is not related to PTE modification.

What do you mean? Oh, you mean map->vma?



> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  drivers/xen/gntdev.c |   34 ++++++++++------------------------
>  1 files changed, 10 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 773b76c..a73f07c 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -74,6 +74,8 @@ struct grant_map {
>       struct granted_page pages[0];
>  };
>  
> +static struct vm_operations_struct gntdev_vmops;

Is it OK for this to be all NULL?

> +
>  /* ------------------------------------------------------------------ */
>  
>  static void gntdev_print_maps(struct gntdev_priv *priv,
> @@ -142,23 +144,6 @@ static struct grant_map *gntdev_find_map_index(struct 
> gntdev_priv *priv, int ind
>       return NULL;
>  }
>  
> -static struct grant_map *gntdev_find_map_vaddr(struct gntdev_priv *priv,
> -                                            unsigned long vaddr)
> -{
> -     struct grant_map *map;
> -
> -     list_for_each_entry(map, &priv->maps, next) {
> -             if (!map->vma)
> -                     continue;
> -             if (vaddr < map->vma->vm_start)
> -                     continue;
> -             if (vaddr >= map->vma->vm_end)
> -                     continue;
> -             return map;
> -     }
> -     return NULL;
> -}
> -
>  static int gntdev_del_map(struct grant_map *map)
>  {
>       int i;
> @@ -510,6 +495,7 @@ static long gntdev_ioctl_get_offset_for_vaddr(struct 
> gntdev_priv *priv,
>                                             struct 
> ioctl_gntdev_get_offset_for_vaddr __user *u)
>  {
>       struct ioctl_gntdev_get_offset_for_vaddr op;
> +     struct vm_area_struct *vma;
>       struct grant_map *map;
>  
>       if (copy_from_user(&op, u, sizeof(op)) != 0)
> @@ -518,16 +504,16 @@ static long gntdev_ioctl_get_offset_for_vaddr(struct 
> gntdev_priv *priv,
>               printk("%s: priv %p, offset for vaddr %lx\n", __FUNCTION__, 
> priv,
>                      (unsigned long)op.vaddr);
>  
> -     spin_lock(&priv->lock);
> -     map = gntdev_find_map_vaddr(priv, op.vaddr);
> -     if (map == NULL ||
> -         map->vma->vm_start != op.vaddr) {
> -             spin_unlock(&priv->lock);
> +     vma = find_vma(current->mm, op.vaddr);
> +     if (!vma)
>               return -EINVAL;
> -     }
> +
> +     map = vma->vm_private_data;
> +     if (vma->vm_ops != &gntdev_vmops || !map)
> +             return -EINVAL;

I know this is perfectly OK, but I'd be happier if you don't refer to
vm_private_data as "map" until the vma has been confirmed to be a gntdev
one.

> +
>       op.offset = map->index << PAGE_SHIFT;
>       op.count = map->count;
> -     spin_unlock(&priv->lock);
>  
>       if (copy_to_user(u, &op, sizeof(op)) != 0)
>               return -EFAULT;

    J

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel