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: Jeremy Fitzhardinge <jeremy@xxxxxxxx>
Subject: Re: [Xen-devel] [PATCH] [PVOPS] fix gntdev on PAE
From: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Date: Thu, 3 Jun 2010 10:32:47 +0100
Cc: "xen-devel@xxxxxxxxxxxxxxxxxxx" <xen-devel@xxxxxxxxxxxxxxxxxxx>, Hoffmann <kraxel@xxxxxxxxxx>, Gerd, Stefano Stabellini <Stefano.Stabellini@xxxxxxxxxxxxx>
Delivery-date: Thu, 03 Jun 2010 02:33:46 -0700
Envelope-to: www-data@xxxxxxxxxxxxxxxxxxx
In-reply-to: <4C0690A5.7070000@xxxxxxxx>
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> <4C0690A5.7070000@xxxxxxxx>
Sender: xen-devel-bounces@xxxxxxxxxxxxxxxxxxx
User-agent: Alpine 2.00 (DEB 1167 2008-08-23)
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.

---


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