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

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

To: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] [PVOPS] fix gntdev on PAE
From: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Date: Wed, 02 Jun 2010 10:11:01 -0700
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Gerd Hoffmann <kraxel@xxxxxxxxxx>
Delivery-date: Wed, 02 Jun 2010 10:11:56 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <alpine.DEB.2.00.1006021505020.3401@kaball-desktop>
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: <alpine.DEB.2.00.1002011504070.24837@kaball-desktop> <alpine.DEB.2.00.1002011544380.24837@kaball-desktop> <4B71DA53.1080404@xxxxxxxx> <alpine.DEB.2.00.1002101219120.12653@kaball-desktop> <4BFFFD7C.5080708@xxxxxxxx> <alpine.DEB.2.00.1006011036520.3028@kaball-desktop> <4C0535E8.3030201@xxxxxxxx> <alpine.DEB.2.00.1006011733490.3401@kaball-desktop> <4C053975.9080906@xxxxxxxx> <alpine.DEB.2.00.1006021505020.3401@kaball-desktop>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1.9) Gecko/20100430 Fedora/3.0.4-2.fc12 Lightning/1.0b2pre Thunderbird/3.0.4
On 06/02/2010 07:11 AM, Stefano Stabellini wrote:
> On Tue, 1 Jun 2010, Jeremy Fitzhardinge wrote:
>   
>> On 06/01/2010 09:36 AM, Stefano Stabellini wrote:
>>     
>>>>> Performances shouldn't matter in this case.
>>>>> Something like this:
>>>>>   
>>>>>       
>>>>>           
>>>> The problem is that the rcu lock disables preemption, so anything inside
>>>> it must be non-scheduling.  So it would need to be a spinlock type
>>>> thing, I think.
>>>>     
>>>>         
>>> right, in fact rwlock is a rw spinlock if I am not mistaken
>>>   
>>>       
>> Ah, yes.  The problem with just making it a spinlock is that other parts
>> of the code do copy_from_user while holding it, so they would need to be
>> restructured to avoid that.
>>
>>     
> Yes, you are right. I fixed the patch to do that, however I couldn't
> actually reproduce the bug you are seeing so I couldn't test the error
> path at all...
>   

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?

    J

> ---
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index ddc59cc..0e571bd 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_types.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;
> +     rwlock_t rwlock;
>       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);
> +     read_lock(&priv->rwlock);
>       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);
> +     read_unlock(&priv->rwlock);
>  }
>  
>  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);
> +     read_lock(&priv->rwlock);
>       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);
> +     read_unlock(&priv->rwlock);
>  }
>  
>  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);
> +     priv->rwlock = RW_LOCK_UNLOCKED;
>       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) {
> +             write_lock(&priv->rwlock);
> +             if (!list_empty(&priv->maps)) {
> +                     map = list_entry(priv->maps.next, struct grant_map, 
> next);
> +                     err = gntdev_del_map(map);
> +                     write_unlock(&priv->rwlock);
> +                     if (err)
> +                             WARN_ON(err);
> +                     else
> +                             gntdev_free_map(map);
> +             } else {
> +                     write_unlock(&priv->rwlock);
> +                     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;
> +     }
> +
> +     write_lock(&priv->rwlock);
> +     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);
> +     write_unlock(&priv->rwlock);
> +     if (copy_to_user(u, &op, sizeof(op)) != 0) {
> +             write_lock(&priv->rwlock);
> +             gntdev_del_map(map);
> +             write_unlock(&priv->rwlock);
> +             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);
> +     write_lock(&priv->rwlock);
>       map = gntdev_find_map_index(priv, op.index >> PAGE_SHIFT, op.count);
>       if (map)
>               err = gntdev_del_map(map);
> -     up_write(&priv->sem);
> +     write_unlock(&priv->rwlock);
> +     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);
> +     read_lock(&priv->rwlock);
>       map = gntdev_find_map_vaddr(priv, op.vaddr);
>       if (map == NULL ||
>           map->vma->vm_start != op.vaddr) {
> -             up_read(&priv->sem);
> +             read_unlock(&priv->rwlock);
>               return -EINVAL;
>       }
>       op.offset = map->index << PAGE_SHIFT;
>       op.count = map->count;
> -     up_read(&priv->sem);
> +     read_unlock(&priv->rwlock);
>  
>       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);
> +     write_lock(&priv->rwlock);
>       priv->limit = op.count;
> -     up_write(&priv->sem);
> +     write_unlock(&priv->rwlock);
>       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);
> +     read_lock(&priv->rwlock);
>       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);
> +     read_unlock(&priv->rwlock);
>       return err;
>  }
>  
>
>   


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