[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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |