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