[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCH 3/7] xen-gntdev: Remove unneeded structures from grant_map tracking data



> @@ -134,6 +133,7 @@ static void gntdev_add_map(struct gntdev_priv *priv, 
> struct grant_map *add)
>  {
>       struct grant_map *map;
>  
> +     spin_lock(&priv->lock);
>       list_for_each_entry(map, &priv->maps, next) {
>               if (add->index + add->count < map->index) {
>                       list_add_tail(&add->next, &map->next);
> @@ -144,8 +144,10 @@ static void gntdev_add_map(struct gntdev_priv *priv, 
> struct grant_map *add)
>       list_add_tail(&add->next, &priv->maps);
>  
>  done:
> +     add->priv = priv;
>       if (debug)
>               gntdev_print_maps(priv, "[new]", add->index);
> +     spin_unlock(&priv->lock);

That looks like you are also fixing a bug?
>  }
>  
>  static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv, int 
> index,
> @@ -186,9 +188,10 @@ static int gntdev_del_map(struct grant_map *map)
>  
>       if (map->vma)
>               return -EBUSY;
> -     for (i = 0; i < map->count; i++)
> -             if (map->unmap_ops[i].handle)
> -                     return -EBUSY;
> +     if (map->is_mapped)
> +             for (i = 0; i < map->count; i++)
> +                     if (map->pginfo[i].handle)
> +                             return -EBUSY;
>  
>       atomic_sub(map->count, &pages_mapped);
>       list_del(&map->next);
> @@ -202,15 +205,10 @@ static void gntdev_free_map(struct grant_map *map)
>       if (!map)
>               return;
>  
> -     if (map->pages)
> -             for (i = 0; i < map->count; i++) {
> -                     if (map->pages[i])
> -                             __free_page(map->pages[i]);
> -             }
> -     kfree(map->pages);
> -     kfree(map->grants);
> -     kfree(map->map_ops);
> -     kfree(map->unmap_ops);
> +     for (i = 0; i < map->count; i++) {
> +             if (map->pages[i])
> +                     __free_page(map->pages[i]);
> +     }
>       kfree(map);
>  }
>  
> @@ -223,53 +221,99 @@ static int find_grant_ptes(pte_t *pte, pgtable_t token, 
> unsigned long addr, void
>       u64 pte_maddr;
>  
>       BUG_ON(pgnr >= map->count);
> +
>       pte_maddr = arbitrary_virt_to_machine(pte).maddr;
> +     map->pginfo[pgnr].pte_maddr = pte_maddr;
>  
> -     gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr,
> -                       GNTMAP_contains_pte | map->flags,
> -                       map->grants[pgnr].ref,
> -                       map->grants[pgnr].domid);
> -     gnttab_set_unmap_op(&map->unmap_ops[pgnr], pte_maddr,
> -                         GNTMAP_contains_pte | map->flags,
> -                         0 /* handle */);
>       return 0;
>  }
>  
>  static int map_grant_pages(struct grant_map *map)
>  {
> -     int i, err = 0;
> +     int i, flags, err = 0;
> +     struct gnttab_map_grant_ref* map_ops = NULL;
>  
> +     flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;

I am not sure if the GNTMAP_contains_pte is correct here. Stefano mentioned
that is used to determine how many arguments to put in the hypercall. Looking
at the previous usage - it was only done on the unmap_op, while you enforce
it on map_op too?

> +     if (map->is_ro)
> +             flags |= GNTMAP_readonly;
> +
> +     err = -ENOMEM;
> +     map_ops = kzalloc(sizeof(map_ops[0]) * map->count, GFP_TEMPORARY);
> +     if (!map_ops)
> +             goto out;
> +
> +     for(i=0; i < map->count; i++) {
> +             gnttab_set_map_op(&map_ops[i], map->pginfo[i].pte_maddr, flags,
> +                               map->pginfo[i].target.ref,
> +                               map->pginfo[i].target.domid);
> +     }
>       if (debug)
>               printk("%s: map %d+%d\n", __FUNCTION__, map->index, map->count);
> -     err = gnttab_map_refs(map->map_ops, map->pages, map->count);
> +
> +     err = gnttab_map_refs(map_ops, map->pages, map->count);
> +
>       if (WARN_ON(err))
> -             return err;
> +             goto out;
>  
>       for (i = 0; i < map->count; i++) {
> -             if (map->map_ops[i].status)
> +             if (map_ops[i].status) {
> +                     __free_page(map->pages[i]);
> +                     map->pages[i] = NULL;
>                       err = -EINVAL;
> -             map->unmap_ops[i].handle = map->map_ops[i].handle;
> +             } else {
> +                     map->pginfo[i].handle = map_ops[i].handle;
> +             }
>       }
> +
> +out:
> +     kfree(map_ops);
>       return err;
>  }
>  
> -static int unmap_grant_pages(struct grant_map *map, int offset, int pages)
> +static void unmap_grant_pages(struct grant_map *map, int offset, int pages)
>  {
> -     int i, err = 0;
> +     int i, flags, err = 0;
> +     struct gnttab_unmap_grant_ref *unmap_ops;
> +     struct gnttab_unmap_grant_ref unmap_single;
> +
> +     if (pages > 1) {
> +             unmap_ops = kzalloc(sizeof(unmap_ops[0]) * pages,
> +                                 GFP_TEMPORARY);
> +             if (unlikely(!unmap_ops)) {
> +                     for(i=0; i < pages; i++)
> +                             unmap_grant_pages(map, offset + i, 1);
> +                     return;
> +             }
> +     } else {
> +             unmap_ops = &unmap_single;
> +     }
> +
> +     flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
> +     if (map->is_ro)
> +             flags |= GNTMAP_readonly;
>  
> +     for(i=0; i < pages; i++)
> +             gnttab_set_unmap_op(&unmap_ops[i],
> +                                 map->pginfo[offset+i].pte_maddr, flags,
> +                                 map->pginfo[offset+i].handle);
>       if (debug)
>               printk("%s: map %d+%d [%d+%d]\n", __FUNCTION__,
>                      map->index, map->count, offset, pages);
> -     err = gnttab_unmap_refs(map->unmap_ops + offset, map->pages, pages);
> +
> +     err = gnttab_unmap_refs(unmap_ops, map->pages + offset, pages);
> +
>       if (WARN_ON(err))
> -             return err;
> +             goto out;
>  
>       for (i = 0; i < pages; i++) {
> -             if (map->unmap_ops[offset+i].status)
> -                     err = -EINVAL;
> -             map->unmap_ops[offset+i].handle = 0;
> +             WARN_ON(unmap_ops[i].status);

Why change it from err to WARN_ON? I think the caller of this function
checks for this too so you would end up with two WARN_ON?

Also, you don't add the offset value to i here..


> +             __free_page(map->pages[offset+i]);
> +             map->pages[offset+i] = NULL;
> +             map->pginfo[offset+i].handle = 0;
>       }
> -     return err;
> +out:
> +     if (unmap_ops != &unmap_single)
> +             kfree(unmap_ops);
>  }
>  
>  /* ------------------------------------------------------------------ */
> @@ -308,7 +352,6 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>       struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
>       struct grant_map *map;
>       unsigned long mstart, mend;
> -     int err;
>  
>       spin_lock(&priv->lock);
>       list_for_each_entry(map, &priv->maps, next) {
> @@ -327,10 +370,9 @@ static void mn_invl_range_start(struct mmu_notifier *mn,
>                              __FUNCTION__, map->index, map->count,
>                              map->vma->vm_start, map->vma->vm_end,
>                              start, end, mstart, mend);
> -             err = unmap_grant_pages(map,
> -                                     (mstart - map->vma->vm_start) >> 
> PAGE_SHIFT,
> -                                     (mend - mstart) >> PAGE_SHIFT);
> -             WARN_ON(err);
> +             unmap_grant_pages(map,
> +                               (mstart - map->vma->vm_start) >> PAGE_SHIFT,
> +                               (mend - mstart) >> PAGE_SHIFT);

Ah, so you rememoved the WARN_ON here. What is the reason for doing so?

>       }
>       spin_unlock(&priv->lock);
>  }
> @@ -347,7 +389,6 @@ static void mn_release(struct mmu_notifier *mn,
>  {
>       struct gntdev_priv *priv = container_of(mn, struct gntdev_priv, mn);
>       struct grant_map *map;
> -     int err;
>  
>       spin_lock(&priv->lock);
>       list_for_each_entry(map, &priv->maps, next) {
> @@ -357,8 +398,7 @@ static void mn_release(struct mmu_notifier *mn,
>                       printk("%s: map %d+%d (%lx %lx)\n",
>                              __FUNCTION__, map->index, map->count,
>                              map->vma->vm_start, map->vma->vm_end);
> -             err = unmap_grant_pages(map, 0, map->count);
> -             WARN_ON(err);
> +             unmap_grant_pages(map, 0, map->count);
>       }
>       spin_unlock(&priv->lock);
>  }
> @@ -427,6 +467,7 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv 
> *priv,
>  {
>       struct ioctl_gntdev_map_grant_ref op;
>       struct grant_map *map;
> +     struct ioctl_gntdev_grant_ref* grants;
>       int err;
>  
>       if (copy_from_user(&op, u, sizeof(op)) != 0)
> @@ -437,38 +478,45 @@ static long gntdev_ioctl_map_grant_ref(struct 
> gntdev_priv *priv,
>       if (unlikely(op.count <= 0))
>               return -EINVAL;
>  
> -     err = -ENOMEM;
> -     map = gntdev_alloc_map(priv, op.count);
> -     if (!map)
> -             return err;
> +     grants = kmalloc(sizeof(grants[0]) * op.count, GFP_TEMPORARY);
> +     if (!grants)
> +             return -ENOMEM;
>  
> -     if (copy_from_user(map->grants, &u->refs,
> -                        sizeof(map->grants[0]) * op.count) != 0) {
> -             gntdev_free_map(map);
> -             return err;
> +     if (copy_from_user(grants, u->refs, sizeof(grants[0]) * op.count)) {
> +             err = -EFAULT;
> +             goto out_free;
>       }
>  
> +     err = -ENOMEM;
> +     map = gntdev_alloc_map(op.count, grants);
> +     if (!map)
> +             goto out_free;
> +
>       if (unlikely(atomic_add_return(op.count, &pages_mapped) > limit))
>       {
>               if (debug)
>                       printk("%s: can't map: over limit\n", __FUNCTION__);
> -             gntdev_free_map(map);
> -             return err;
> +             goto out_free_map;

I just noticed it now, but shouldn't we also free grants? That looks like
it needs a seperate bug patch thought.
>       }
>  
> -     spin_lock(&priv->lock);
>       gntdev_add_map(priv, map);
>       op.index = map->index << PAGE_SHIFT;
> -     spin_unlock(&priv->lock);

Ah, so you moved the spinlock down. I presume it is OK to have op.index be
unprotected.
>  
> -     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;
> +     if (copy_to_user(u, &op, sizeof(op))) {
> +             err = -EFAULT;
> +             goto out_remove;

Hmm, should we free the grants on this exit path?

>       }
> -     return 0;
> +     err = 0;
> +out_free:
> +     kfree(grants);
> +     return err;
> +out_remove:
> +     spin_lock(&priv->lock);
> +     gntdev_del_map(map);
> +     spin_unlock(&priv->lock);
> +out_free_map:
> +     gntdev_free_map(map);
> +     goto out_free;
>  }

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.