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
|
Home