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

Re: [Xen-devel] [PATCH] [PVOPS] fix gntdev on PAE



On 06/03/2010 02:32 AM, Stefano Stabellini wrote:
> On Wed, 2 Jun 2010, Jeremy Fitzhardinge wrote:
>   
>> I hit this often, mostly when mucking around with xenstored (maybe when
>> it exits?).  Other people have reported it too.
>>
>> Does it really need to be a rwlock?
>>
>>     
> I see you really don't like rwlocks :)
>
> All right, this version uses plain spinlocks instead.
>   

Thanks, that looks good.  I tweaked it a little bit (kfree doesn't
block, so it can be done under spinlock which simplifies the release
loop), and I'm about to give it a test spin.

    J

> ---
>
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index ddc59cc..6a3e207 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -28,7 +28,7 @@
>  #include <linux/types.h>
>  #include <linux/uaccess.h>
>  #include <linux/sched.h>
> -#include <linux/rwsem.h>
> +#include <linux/spinlock.h>
>  
>  #include <xen/xen.h>
>  #include <xen/grant_table.h>
> @@ -51,7 +51,7 @@ struct gntdev_priv {
>       struct list_head maps;
>       uint32_t used;
>       uint32_t limit;
> -     struct rw_semaphore sem;
> +     spinlock_t lock;
>       struct mm_struct *mm;
>       struct mmu_notifier mn;
>  };
> @@ -84,9 +84,9 @@ static void gntdev_print_maps(struct gntdev_priv *priv,
>                      map->index == text_index && text ? text : "");
>  }
>  
> -static struct grant_map *gntdev_add_map(struct gntdev_priv *priv, int count)
> +static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int 
> count)
>  {
> -     struct grant_map *map, *add;
> +     struct grant_map *add;
>  
>       add = kzalloc(sizeof(struct grant_map), GFP_KERNEL);
>       if (NULL == add)
> @@ -104,8 +104,20 @@ static struct grant_map *gntdev_add_map(struct 
> gntdev_priv *priv, int count)
>       add->count = count;
>       add->priv  = priv;
>  
> -     if (add->count + priv->used > priv->limit)
> -             goto err;
> +     if (add->count + priv->used <= priv->limit)
> +             return add;
> +
> +err:
> +     kfree(add->grants);
> +     kfree(add->map_ops);
> +     kfree(add->unmap_ops);
> +     kfree(add);
> +     return NULL;
> +}
> +
> +static void gntdev_add_map(struct gntdev_priv *priv, struct grant_map *add)
> +{
> +     struct grant_map *map;
>  
>       list_for_each_entry(map, &priv->maps, next) {
>               if (add->index + add->count < map->index) {
> @@ -120,14 +132,6 @@ done:
>       priv->used += add->count;
>       if (debug)
>               gntdev_print_maps(priv, "[new]", add->index);
> -     return add;
> -
> -err:
> -     kfree(add->grants);
> -     kfree(add->map_ops);
> -     kfree(add->unmap_ops);
> -     kfree(add);
> -     return NULL;
>  }
>  
>  static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv, int 
> index,
> @@ -174,11 +178,17 @@ static int gntdev_del_map(struct grant_map *map)
>  
>       map->priv->used -= map->count;
>       list_del(&map->next);
> +     return 0;
> +}
> +
> +static void gntdev_free_map(struct grant_map *map)
> +{
> +     if (!map)
> +             return;
>       kfree(map->grants);
>       kfree(map->map_ops);
>       kfree(map->unmap_ops);
>       kfree(map);
> -     return 0;
>  }
>  
>  /* ------------------------------------------------------------------ */
> @@ -277,7 +287,7 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>       unsigned long mstart, mend;
>       int err;
>  
> -     down_read(&priv->sem);
> +     spin_lock(&priv->lock);
>       list_for_each_entry(map, &priv->maps, next) {
>               if (!map->vma)
>                       continue;
> @@ -299,7 +309,7 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>                                       (mend - mstart) >> PAGE_SHIFT);
>               WARN_ON(err);
>       }
> -     up_read(&priv->sem);
> +     spin_unlock(&priv->lock);
>  }
>  
>  static void mn_invl_page(struct mmu_notifier *mn,
> @@ -316,7 +326,7 @@ static void mn_release(struct mmu_notifier *mn,
>       struct grant_map *map;
>       int err;
>  
> -     down_read(&priv->sem);
> +     spin_lock(&priv->lock);
>       list_for_each_entry(map, &priv->maps, next) {
>               if (!map->vma)
>                       continue;
> @@ -327,7 +337,7 @@ static void mn_release(struct mmu_notifier *mn,
>               err = unmap_grant_pages(map, 0, map->count);
>               WARN_ON(err);
>       }
> -     up_read(&priv->sem);
> +     spin_unlock(&priv->lock);
>  }
>  
>  struct mmu_notifier_ops gntdev_mmu_ops = {
> @@ -347,7 +357,7 @@ static int gntdev_open(struct inode *inode, struct file 
> *flip)
>               return -ENOMEM;
>  
>       INIT_LIST_HEAD(&priv->maps);
> -     init_rwsem(&priv->sem);
> +     spin_lock_init(&priv->lock);
>       priv->limit = limit;
>  
>       priv->mm = get_task_mm(current);
> @@ -375,13 +385,21 @@ static int gntdev_release(struct inode *inode, struct 
> file *flip)
>       if (debug)
>               printk("%s: priv %p\n", __FUNCTION__, priv);
>  
> -     down_write(&priv->sem);
> -     while (!list_empty(&priv->maps)) {
> -             map = list_entry(priv->maps.next, struct grant_map, next);
> -             err = gntdev_del_map(map);
> -             WARN_ON(err);
> +     while (1) {
> +             spin_lock(&priv->lock);
> +             if (!list_empty(&priv->maps)) {
> +                     map = list_entry(priv->maps.next, struct grant_map, 
> next);
> +                     err = gntdev_del_map(map);
> +                     spin_unlock(&priv->lock);
> +                     if (err)
> +                             WARN_ON(err);
> +                     else
> +                             gntdev_free_map(map);
> +             } else {
> +                     spin_unlock(&priv->lock);
> +                     break;
> +             }
>       }
> -     up_write(&priv->sem);
>       mmu_notifier_unregister(&priv->mn, priv->mm);
>       kfree(priv);
>       return 0;
> @@ -404,27 +422,29 @@ static long gntdev_ioctl_map_grant_ref(struct 
> gntdev_priv *priv,
>       if (unlikely(op.count > priv->limit))
>               return -EINVAL;
>  
> -     down_write(&priv->sem);
>       err = -ENOMEM;
> -     map = gntdev_add_map(priv, op.count);
> +     map = gntdev_alloc_map(priv, op.count);
>       if (!map)
> -             goto err_unlock;
> -
> -     err = -ENOMEM;
> +             return err;
>       if (copy_from_user(map->grants, &u->refs,
> -                        sizeof(map->grants[0]) * op.count) != 0)
> -             goto err_free;
> +                        sizeof(map->grants[0]) * op.count) != 0) {
> +             gntdev_free_map(map);
> +             return err;
> +     }
> +
> +     spin_lock(&priv->lock);
> +     gntdev_add_map(priv, map);
> +
>       op.index = map->index << PAGE_SHIFT;
> -     if (copy_to_user(u, &op, sizeof(op)) != 0)
> -             goto err_free;
> -     up_write(&priv->sem);
> +     spin_unlock(&priv->lock);
> +     if (copy_to_user(u, &op, sizeof(op)) != 0) {
> +             spin_lock(&priv->lock);
> +             gntdev_del_map(map);
> +             spin_unlock(&priv->lock);
> +             gntdev_free_map(map);
> +             return err;
> +     }
>       return 0;
> -
> -err_free:
> -     gntdev_del_map(map);
> -err_unlock:
> -     up_write(&priv->sem);
> -     return err;
>  }
>  
>  static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv,
> @@ -440,11 +460,13 @@ static long gntdev_ioctl_unmap_grant_ref(struct 
> gntdev_priv *priv,
>               printk("%s: priv %p, del %d+%d\n", __FUNCTION__, priv,
>                      (int)op.index, (int)op.count);
>  
> -     down_write(&priv->sem);
> +     spin_lock(&priv->lock);
>       map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
>       if (map)
>               err = gntdev_del_map(map);
> -     up_write(&priv->sem);
> +     spin_unlock(&priv->lock);
> +     if (!err)
> +             gntdev_free_map(map);
>       return err;
>  }
>  
> @@ -460,16 +482,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);
>  
> -     down_read(&priv->sem);
> +     spin_lock(&priv->lock);
>       map = gntdev_find_map_vaddr(priv, op.vaddr);
>       if (map == NULL ||
>           map->vma->vm_start != op.vaddr) {
> -             up_read(&priv->sem);
> +             spin_unlock(&priv->lock);
>               return -EINVAL;
>       }
>       op.offset = map->index << PAGE_SHIFT;
>       op.count = map->count;
> -     up_read(&priv->sem);
> +     spin_unlock(&priv->lock);
>  
>       if (copy_to_user(u, &op, sizeof(op)) != 0)
>               return -EFAULT;
> @@ -488,9 +510,9 @@ static long gntdev_ioctl_set_max_grants(struct 
> gntdev_priv *priv,
>       if (op.count > limit)
>               return -EINVAL;
>  
> -     down_write(&priv->sem);
> +     spin_lock(&priv->lock);
>       priv->limit = op.count;
> -     up_write(&priv->sem);
> +     spin_unlock(&priv->lock);
>       return 0;
>  }
>  
> @@ -538,7 +560,7 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>               printk("%s: map %d+%d at %lx (pgoff %lx)\n", __FUNCTION__,
>                      index, count, vma->vm_start, vma->vm_pgoff);
>  
> -     down_read(&priv->sem);
> +     spin_lock(&priv->lock);
>       map = gntdev_find_map_index(priv, index, count);
>       if (!map)
>               goto unlock_out;
> @@ -580,7 +602,7 @@ static int gntdev_mmap(struct file *flip, struct 
> vm_area_struct *vma)
>       map->is_mapped = 1;
>  
>  unlock_out:
> -     up_read(&priv->sem);
> +     spin_unlock(&priv->lock);
>       return err;
>  }
>  
>
>   


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


 


Rackspace

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