WARNING - OLD ARCHIVES

This is an archived copy of the Xen.org mailing list, which we have preserved to ensure that existing links to archives are not broken. The live archive, which contains the latest emails, can be found at http://lists.xen.org/
   
 
 
Xen 
 
Home Products Support Community News
 
   
 

xen-devel

[Xen-devel] Re: [PATCH 5/7] xen-gntdev: Add reference counting to maps

To: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
Subject: [Xen-devel] Re: [PATCH 5/7] xen-gntdev: Add reference counting to maps
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Thu, 16 Dec 2010 16:49:22 -0800
Cc: xen-devel@xxxxxxxxxxxxxxxxxxx, Ian.Campbell@xxxxxxxxxx
Delivery-date: Thu, 16 Dec 2010 16:50:07 -0800
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <1292545063-32107-6-git-send-email-dgdegra@xxxxxxxxxxxxx>
List-help: <mailto:xen-devel-request@lists.xensource.com?subject=help>
List-id: Xen developer discussion <xen-devel.lists.xensource.com>
List-post: <mailto:xen-devel@lists.xensource.com>
List-subscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=subscribe>
List-unsubscribe: <http://lists.xensource.com/mailman/listinfo/xen-devel>, <mailto:xen-devel-request@lists.xensource.com?subject=unsubscribe>
References: <1292545063-32107-1-git-send-email-dgdegra@xxxxxxxxxxxxx> <1292545063-32107-6-git-send-email-dgdegra@xxxxxxxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.13) Gecko/20101209 Fedora/3.1.7-0.35.b3pre.fc14 Lightning/1.0b3pre Thunderbird/3.1.7
On 12/16/2010 04:17 PM, Daniel De Graaf wrote:
> This allows userspace to perform mmap() on the gntdev device and then
> immediately close the filehandle or remove the mapping using the
> remove ioctl, with the mapped area remaining valid until unmapped.
> This also fixes an infinite loop when a gntdev device is closed
> without first unmapping all areas.
>
> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> ---
>  drivers/xen/gntdev.c |   67 +++++++++++++++++++++++--------------------------
>  1 files changed, 31 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 6a3c9e4..f1fc8fa 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -66,16 +66,18 @@ struct granted_page {
>  
>  struct grant_map {
>       struct list_head next;
> -     struct gntdev_priv *priv;
>       struct vm_area_struct *vma;
>       int index;
>       int count;
> +     atomic_t users;

Does this need to be atomic?  Won't it be happening under spinlock anyway?

>       int is_mapped:1;
>       int is_ro:1;
>       struct page **pages;
>       struct granted_page pginfo[0];
>  };
>  
> +static void unmap_grant_pages(struct grant_map *map, int offset, int pages);
> +
>  /* ------------------------------------------------------------------ */
>  
>  static void gntdev_print_maps(struct gntdev_priv *priv,
> @@ -112,6 +114,7 @@ static struct grant_map *gntdev_alloc_map(int count,
>       }
>  
>       add->index = 0;
> +     atomic_set(&add->users, 1);
>       add->count = count;
>       for(i = 0; i < count; i++)
>               add->pginfo[i].target = grants[i];
> @@ -144,7 +147,6 @@ static void gntdev_add_map(struct gntdev_priv *priv, 
> struct grant_map *add)
>       list_add_tail(&add->next, &priv->maps);
>  
>  done:
> -     add->priv = priv;
>       if (debug)
>               gntdev_print_maps(priv, "[new]", add->index);
>       spin_unlock(&priv->lock);
> @@ -165,33 +167,26 @@ static struct grant_map *gntdev_find_map_index(struct 
> gntdev_priv *priv, int ind
>       return NULL;
>  }
>  
> -static int gntdev_del_map(struct grant_map *map)
> +static void gntdev_put_map(struct grant_map *map)
>  {
>       int i;
>  
> -     if (map->vma)
> -             return -EBUSY;
> -     if (map->is_mapped)
> -             for (i = 0; i < map->count; i++)
> -                     if (map->pginfo[i].handle)
> -                             return -EBUSY;
> +     if (!map)
> +             return;
>  
> -     atomic_sub(map->count, &pages_mapped);
> -     list_del(&map->next);
> -     return 0;
> -}
> +     if (!atomic_dec_and_test(&map->users))
> +             return;
>  
> -static void gntdev_free_map(struct grant_map *map)
> -{
> -     unsigned i;
> +     atomic_sub(map->count, &pages_mapped);
>  
> -     if (!map)
> -             return;
> +     if (!use_ptemod)
> +             unmap_grant_pages(map, 0, map->count);
>  
>       for (i = 0; i < map->count; i++) {
>               if (map->pages[i])
>                       __free_page(map->pages[i]);
>       }
> +     kfree(map->pages);
>       kfree(map);
>  }
>  
> @@ -310,6 +305,7 @@ static void gntdev_vma_close(struct vm_area_struct *vma)
>       map->is_mapped = 0;
>       map->vma = NULL;
>       vma->vm_private_data = NULL;
> +     gntdev_put_map(map);
>  }
>  
>  static int gntdev_vma_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> @@ -425,7 +421,6 @@ static int gntdev_release(struct inode *inode, struct 
> file *flip)
>  {
>       struct gntdev_priv *priv = flip->private_data;
>       struct grant_map *map;
> -     int err;
>  
>       if (debug)
>               printk("%s: priv %p\n", __FUNCTION__, priv);
> @@ -433,10 +428,8 @@ static int gntdev_release(struct inode *inode, struct 
> file *flip)
>       spin_lock(&priv->lock);
>       while (!list_empty(&priv->maps)) {
>               map = list_entry(priv->maps.next, struct grant_map, next);
> -             err = gntdev_del_map(map);
> -             if (WARN_ON(err))
> -                     gntdev_free_map(map);
> -
> +             list_del(&map->next);
> +             gntdev_put_map(map);
>       }
>       spin_unlock(&priv->lock);
>  
> @@ -487,18 +480,14 @@ static long gntdev_ioctl_map_grant_ref(struct 
> gntdev_priv *priv,
>  
>       if (copy_to_user(u, &op, sizeof(op))) {
>               err = -EFAULT;
> -             goto out_remove;
> +             goto out_free;
>       }
>       err = 0;
>  out_free:
>       kfree(grants);
>       return err;
> -out_remove:
> -     spin_lock(&priv->lock);
> -     gntdev_del_map(map);
> -     spin_unlock(&priv->lock);
>  out_free_map:
> -     gntdev_free_map(map);
> +     gntdev_put_map(map);
>       goto out_free;
>  }
>  
> @@ -507,7 +496,7 @@ static long gntdev_ioctl_unmap_grant_ref(struct 
> gntdev_priv *priv,
>  {
>       struct ioctl_gntdev_unmap_grant_ref op;
>       struct grant_map *map;
> -     int err = -EINVAL;
> +     int err;
>  
>       if (copy_from_user(&op, u, sizeof(op)) != 0)
>               return -EFAULT;
> @@ -517,11 +506,13 @@ static long gntdev_ioctl_unmap_grant_ref(struct 
> gntdev_priv *priv,
>  
>       spin_lock(&priv->lock);
>       map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
> -     if (map)
> -             err = gntdev_del_map(map);
> +     if (map) {
> +             list_del(&map->next);
> +             gntdev_put_map(map);
> +             err = 0;
> +     } else
> +             err = -EINVAL;

What prevents unmap_grant_ref being called multiple times?

>       spin_unlock(&priv->lock);
> -     if (!err)
> -             gntdev_free_map(map);
>       return err;
>  }
>  
> @@ -599,13 +590,15 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>       map = gntdev_find_map_index(priv, index, count);
>       if (!map)
>               goto unlock_out;
> -     if (map->vma)
> +     if (use_ptemod && map->vma)
>               goto unlock_out;
>       if (priv->mm != vma->vm_mm) {
>               printk("%s: Huh? Other mm?\n", __FUNCTION__);
>               goto unlock_out;
>       }
>  
> +     atomic_inc(&map->users);
> +
>       vma->vm_ops = &gntdev_vmops;
>  
>       vma->vm_flags |= VM_RESERVED;
> @@ -614,7 +607,9 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>       vma->vm_flags |= VM_PFNMAP;
>  
>       vma->vm_private_data = map;
> -     map->vma = vma;
> +
> +     if (use_ptemod)
> +             map->vma = vma;
>  
>       map->is_ro = !(vma->vm_flags & VM_WRITE);
>  

    J

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