On Wed, 5 Jan 2011, Konrad Rzeszutek Wilk wrote:
> > +MODULE_LICENSE("GPL");
> > +MODULE_AUTHOR("Derek G. Murray <Derek.Murray@xxxxxxxxxxxx>, "
> > + "Gerd Hoffmann <kraxel@xxxxxxxxxx>");
> > +MODULE_DESCRIPTION("User-space granted page access driver");
> > +
> > +static int debug = 0;
> > +module_param(debug, int, 0644);
>
> You need to add:
> MODULE_PARM_DESC
>
I removed the debug option altogether and substituted all the if (debug)
printk with pr_debug.
> > +static int limit = 1024;
> > +module_param(limit, int, 0644);
>
> ditto
I added MODULE_PARM_DESC in this case.
> > +
> > +struct gntdev_priv {
> > + struct list_head maps;
> > + uint32_t used;
> > + uint32_t limit;
> > + spinlock_t lock;
>
> and some description about what the lock is protecting.
done
> > + struct mm_struct *mm;
> > + struct mmu_notifier mn;
> > +};
> > +
> > +struct grant_map {
> > + struct list_head next;
> > + struct gntdev_priv *priv;
> > + struct vm_area_struct *vma;
> > + int index;
> > + int count;
> > + int flags;
> > + int is_mapped;
> > + struct ioctl_gntdev_grant_ref *grants;
> > + struct gnttab_map_grant_ref *map_ops;
> > + struct gnttab_unmap_grant_ref *unmap_ops;
> > +};
> > +
> > +/* ------------------------------------------------------------------ */
> > +
> > +static void gntdev_print_maps(struct gntdev_priv *priv,
> > + char *text, int text_index)
> > +{
> > + struct grant_map *map;
> > +
> > + printk("%s: maps list (priv %p, usage %d/%d)\n",
> > + __FUNCTION__, priv, priv->used, priv->limit);
> > + list_for_each_entry(map, &priv->maps, next)
> > + printk(" index %2d, count %2d %s\n",
>
> Use pr_debug.
>
I changed all the printk into pr_debug apart from the few that actually
print error or warning messages.
> > + err = unmap_grant_pages(map,
> > + (mstart - map->vma->vm_start) >>
> > PAGE_SHIFT,
> > + (mend - mstart) >> PAGE_SHIFT);
> > + WARN_ON(err);
>
> Ah you WARN here.. so two WARN_ON in case the hypervisor call fails.
> Maybe you just remove the WARN_ON in the unmap_grant_pages and let this
> WARN_ON do the job?
Right. I have done the same in map_grant_pages for consistency.
> > + err = unmap_grant_pages(map, 0, map->count);
>
> Ok, so offset set to zero (might want a put /* offset */ comment in there.
>
done
> > +static int gntdev_open(struct inode *inode, struct file *flip)
> > +{
> > + struct gntdev_priv *priv;
> > +
> > + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + INIT_LIST_HEAD(&priv->maps);
> > + spin_lock_init(&priv->lock);
> > + priv->limit = limit;
> > +
> > + priv->mm = get_task_mm(current);
> > + if (!priv->mm) {
> > + kfree(priv);
> > + return -ENOMEM;
> > + }
> > + priv->mn.ops = &gntdev_mmu_ops;
> > + mmu_notifier_register(&priv->mn, priv->mm);
>
> You might want to check that this does not fail.
>
done
> > + mmput(priv->mm);
> > +
> > + flip->private_data = priv;
> > + if (debug)
> > + printk("%s: priv %p\n", __FUNCTION__, priv);
> > +
> > + return 0;
> > +}
> > +
> > +static int gntdev_release(struct inode *inode, struct file *flip)
> > +{
> > + struct gntdev_priv *priv = flip->private_data;
> > + struct grant_map *map;
> > + int err;
> > +
> > + if (debug)
> > + printk("%s: priv %p\n", __FUNCTION__, priv);
> > +
> > + spin_lock(&priv->lock);
> > + while (!list_empty(&priv->maps)) {
> > + map = list_entry(priv->maps.next, struct grant_map, next);
> > + err = gntdev_del_map(map);
> > + if (WARN_ON(err))
>
> Hmm, so if we fail (b/c we get -EBUSY) we free it? Would it be possible
> to race with whoever is responsible for releasing this VMA (mn_release)
> clearning this map while the holder of this map is trying to dereference
> the map->unmap_ops ?
>
> Oh wait, you and mn_release are both using a spinlock.. so, under what
> conditions would this actually happend?
It shouldn't be possible because at the time gntdev_release is called
all the vma should have been unmmapped by mn_release already.
However I would like to keep the WARN_ON anyway because it might turn
out to be useful in the future.
>
> > + gntdev_free_map(map);
> > +
> > + }
> > + spin_unlock(&priv->lock);
> > +
> > + mmu_notifier_unregister(&priv->mn, priv->mm);
> > + kfree(priv);
> > + return 0;
> > +}
> > +
> > +static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
>
> The decleration is for 'long' but you return int. Looking at the
> other ioctl (kvm ones) they all seem to return 'int' so this
> decleration looks wrong.
>
Unlocked_ioctl in file_operations returns long, hence gntdev_ioctl has
to return long. At this point it makes sense to return long from all the
ioctl related functions in gntdev.
> > + struct ioctl_gntdev_unmap_grant_ref
> > __user *u)
> > +{
> > + struct ioctl_gntdev_unmap_grant_ref op;
> > + struct grant_map *map;
> > + int err = -EINVAL;
>
> Not -ENOENT? If you can't find the map .. well, the arguments are valid.
> It is just that the map is not found. Or are the tools hard-coded to look
> for -EINVAL?
>
No, I don't think so. ENOENT is more appropriate.
> > +static long gntdev_ioctl_set_max_grants(struct gntdev_priv *priv,
> > + struct ioctl_gntdev_set_max_grants
> > __user *u)
> > +{
> > + struct ioctl_gntdev_set_max_grants op;
> > +
> > + if (copy_from_user(&op, u, sizeof(op)) != 0)
> > + return -EFAULT;
> > + if (debug)
> > + printk("%s: priv %p, limit %d\n", __FUNCTION__, priv,
> > op.count);
> > + if (op.count > limit)
> > + return -EINVAL;
>
> Not -E2BIG?
>
OK
> > + vma->vm_ops = &gntdev_vmops;
> > +
> > + vma->vm_flags |= VM_RESERVED;
> > + vma->vm_flags |= VM_DONTCOPY;
> > + vma->vm_flags |= VM_DONTEXPAND;
>
> You can squish those.
done
> > + if (err) {
> > + goto unlock_out;
> > + if (debug)
> > + printk("%s: find_grant_ptes() failure.\n",
> > __FUNCTION__);
>
> Heh.. . You do a 'goto' and then 'if debug..' Swap them around.
>
done
> > + }
> > +
> > + err = map_grant_pages(map);
> > + if (err) {
> > + goto unlock_out;
> > + if (debug)
> > + printk("%s: map_grant_pages() failure.\n",
> > __FUNCTION__);
>
> Ditto.
done
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel
|
Home