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

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



On 12/14/2010 04:15 PM, Jeremy Fitzhardinge wrote:
> 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

This will also allow easier testing of what pages need to be unmapped
(more obvious in the HVM version). I also find it less confusing to
populate the hypercall arguments immediately before the hypercall, but
that's likely a matter of opinion. It only saves 46 bytes per page, so
if it seems more complex it could be dropped.

>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  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);
>>  
> 


-- 
Daniel De Graaf
National Security Agency

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