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

[Jeremy Fitzhardinge <jeremy@goop.org>]

On 12/14/2010 06:55 AM, Daniel De Graaf wrote:
> The entire hypercall argument list isn't required; only selected
> fields from the hypercall need to be tracked between the ioctl, map,
> and unmap operations.

Is the rationale of this patch to save memory?  If so, how much does it
save.

(This patch seems sensible in principle, but it doesn't seem to save
much complexity.)

    J
> Signed-off-by: Daniel De Graaf <dgdegra@tycho.nsa.gov>
> ---
>  drivers/xen/gntdev.c |  195 +++++++++++++++++++++++++++++--------------------
>  1 files changed, 115 insertions(+), 80 deletions(-)
>
> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> index 62639af..773b76c 100644
> --- a/drivers/xen/gntdev.c
> +++ b/drivers/xen/gntdev.c
> @@ -55,17 +55,23 @@ struct gntdev_priv {
>  	struct mmu_notifier mn;
>  };
>  
> +struct granted_page {
> +	u64 pte_maddr;
> +	union {
> +		struct ioctl_gntdev_grant_ref target;
> +		grant_handle_t handle;
> +	};
> +};
> +
>  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;
> +	int is_mapped:1;
> +	int is_ro:1;
> +	struct granted_page pages[0];
>  };
>  
>  /* ------------------------------------------------------------------ */
> @@ -83,40 +89,28 @@ static void gntdev_print_maps(struct gntdev_priv *priv,
>  		       map->index == text_index && text ? text : "");
>  }
>  
> -static struct grant_map *gntdev_alloc_map(struct gntdev_priv *priv, int count)
> +static struct grant_map *gntdev_alloc_map(int count,
> +		struct ioctl_gntdev_grant_ref* grants)
>  {
>  	struct grant_map *add;
> +	int i;
>  
> -	add = kzalloc(sizeof(struct grant_map), GFP_KERNEL);
> -	if (NULL == add)
> +	add = kzalloc(sizeof(*add) + sizeof(add->pages[0])*count, GFP_KERNEL);
> +	if (!add)
>  		return NULL;
>  
> -	add->grants    = kzalloc(sizeof(add->grants[0])    * count, GFP_KERNEL);
> -	add->map_ops   = kzalloc(sizeof(add->map_ops[0])   * count, GFP_KERNEL);
> -	add->unmap_ops = kzalloc(sizeof(add->unmap_ops[0]) * count, GFP_KERNEL);
> -	if (NULL == add->grants  ||
> -	    NULL == add->map_ops ||
> -	    NULL == add->unmap_ops)
> -		goto err;
> -
> -	add->index = 0;
>  	add->count = count;
> -	add->priv  = priv;
> +	for(i = 0; i < count; i++)
> +		add->pages[i].target = grants[i];
>  
>  	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;
>  
> +	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);
> @@ -127,8 +121,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);
>  }
>  
>  static struct grant_map *gntdev_find_map_index(struct gntdev_priv *priv, int index,
> @@ -169,9 +165,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->pages[i].handle)
> +				return -EBUSY;
>  
>  	atomic_sub(map->count, &pages_mapped);
>  	list_del(&map->next);
> @@ -182,9 +179,6 @@ 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);
>  }
>  
> @@ -199,51 +193,91 @@ static int find_grant_ptes(pte_t *pte, pgtable_t token, unsigned long addr, void
>  	BUG_ON(pgnr >= map->count);
>  	pte_maddr  = (u64)pfn_to_mfn(page_to_pfn(token)) << PAGE_SHIFT;
>  	pte_maddr += (unsigned long)pte & ~PAGE_MASK;
> -	gnttab_set_map_op(&map->map_ops[pgnr], pte_maddr, map->flags,
> -			  map->grants[pgnr].ref,
> -			  map->grants[pgnr].domid);
> -	gnttab_set_unmap_op(&map->unmap_ops[pgnr], pte_maddr, map->flags,
> -			    0 /* handle */);
> +	map->pages[pgnr].pte_maddr = pte_maddr;
> +
>  	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;
>  
> +	flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
> +	if (map->is_ro)
> +		flags |= GNTMAP_readonly;
> +
> +	map_ops = kzalloc(sizeof(map_ops[0]) * map->count, GFP_TEMPORARY);
> +	if (!map_ops)
> +		return -ENOMEM;
> +	
> +	for(i=0; i < map->count; i++)
> +		gnttab_set_map_op(&map_ops[i], map->pages[i].pte_maddr, flags,
> +				  map->pages[i].target.ref,
> +				  map->pages[i].target.domid);
>  	if (debug)
>  		printk("%s: map %d+%d\n", __FUNCTION__, map->index, map->count);
>  	err = HYPERVISOR_grant_table_op(GNTTABOP_map_grant_ref,
> -					map->map_ops, map->count);
> +					map_ops, 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) {
> +			map->pages[i].pte_maddr = 0;
>  			err = -EINVAL;
> -		map->unmap_ops[i].handle = map->map_ops[i].handle;
> +		} else {
> +			map->pages[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->pages[offset+i].pte_maddr, flags,
> +		                    map->pages[offset+i].handle);
>  	if (debug)
>  		printk("%s: map %d+%d [%d+%d]\n", __FUNCTION__,
>  		       map->index, map->count, offset, pages);
>  	err = HYPERVISOR_grant_table_op(GNTTABOP_unmap_grant_ref,
> -					map->unmap_ops + offset, pages);
> +					unmap_ops, 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);
> +		map->pages[offset+i].pte_maddr = 0;
> +		map->pages[offset+i].handle = 0;
>  	}
> -	return err;
> +out:
> +	if (unmap_ops != &unmap_single)
> +		kfree(unmap_ops);
>  }
>  
>  /* ------------------------------------------------------------------ */
> @@ -282,7 +316,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) {
> @@ -301,10 +334,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);
>  	}
>  	spin_unlock(&priv->lock);
>  }
> @@ -321,7 +353,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) {
> @@ -331,8 +362,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);
>  }
> @@ -401,6 +431,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)
> @@ -411,38 +442,45 @@ static long gntdev_ioctl_map_grant_ref(struct gntdev_priv *priv,
>  	if (unlikely(op.count <= 0))
>  		return -EINVAL;
>  
> +	grants = kmalloc(sizeof(grants[0]) * op.count, GFP_TEMPORARY);
> +	if (!grants)
> +		return -ENOMEM;
> +	
> +	if (copy_from_user(grants, u->refs, sizeof(grants[0]) * op.count)) {
> +		err = -EFAULT;
> +		goto out_free;
> +	}
> +
>  	err = -ENOMEM;
> -	map = gntdev_alloc_map(priv, op.count);
> +	map = gntdev_alloc_map(op.count, grants);
>  	if (!map)
> -		return err;
> -
> -	if (copy_from_user(map->grants, &u->refs,
> -			   sizeof(map->grants[0]) * op.count) != 0) {
> -		gntdev_free_map(map);
> -		return err;
> -	}
> +		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;
>  	}
>  
> -	spin_lock(&priv->lock);
>  	gntdev_add_map(priv, map);
>  	op.index = map->index << PAGE_SHIFT;
> -	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;
> +	if (copy_to_user(u, &op, sizeof(op))) {
> +		err = -EFAULT;
> +		goto out_remove;
>  	}
> -	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;
>  }
>  
>  static long gntdev_ioctl_unmap_grant_ref(struct gntdev_priv *priv,
> @@ -556,10 +594,7 @@ static int gntdev_mmap(struct file *flip, struct vm_area_struct *vma)
>  
>  	vma->vm_private_data = map;
>  	map->vma = vma;
> -
> -	map->flags = GNTMAP_host_map | GNTMAP_application_map | GNTMAP_contains_pte;
> -	if (!(vma->vm_flags & VM_WRITE))
> -		map->flags |= GNTMAP_readonly;
> +	map->is_ro = !(vma->vm_flags & VM_WRITE);
>  
>  	spin_unlock(&priv->lock);
>