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

[Jeremy Fitzhardinge <jeremy@goop.org>]

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@tycho.nsa.gov>
> ---
>  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