Re: [Xen-devel] [PATCH] xen/gntdev, gntalloc: Remove unneeded VM flags

[Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>]

On 03/07/2011 02:53 PM, Ian Campbell wrote:
> On Mon, 2011-03-07 at 18:19 +0000, Daniel De Graaf wrote:
>> The only time when granted pages need to be treated specially is when
>> using Xen's PTE modification for grant mappings owned by another domain.
>> Otherwise, the area does not require VM_DONTCOPY and VM_PFNMAP, since it
>> can be accessed just like any other page of RAM.
> 
> This needs clarifying that it only applies to gntalloc and/or HVM
> guests, I don't think it applies to gntdev in PV guests, right?
> 
> The actual patch seems to make some reference counting changes which
> don't seem to be covered by the above description, and they don't seem
> to be balanced (I see increments but no decrements).
> 
> Ian.

PV guests use PTE modification in gntdev, so this does not make any changes
to them. I can clarify that in the commit message if it is helpful.

The reference counting changes are balanced by the already-existing close
functions. The open function was not needed before now because it is only
used when the VM area is being copied on fork, which was not possible. I
can add a note of this if it would clarify the commit.

>>
>> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
>> ---
>>  drivers/xen/gntalloc.c |   14 ++++++++++++--
>>  drivers/xen/gntdev.c   |   16 ++++++++++++++--
>>  2 files changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/xen/gntalloc.c b/drivers/xen/gntalloc.c
>> index a7ffdfe..f6832f4 100644
>> --- a/drivers/xen/gntalloc.c
>> +++ b/drivers/xen/gntalloc.c
>> @@ -427,6 +427,17 @@ static long gntalloc_ioctl(struct file *filp, unsigned 
>> int cmd,
>>      return 0;
>>  }
>>  
>> +static void gntalloc_vma_open(struct vm_area_struct *vma)
>> +{
>> +    struct gntalloc_gref *gref = vma->vm_private_data;
>> +    if (!gref)
>> +            return;
>> +
>> +    spin_lock(&gref_lock);
>> +    gref->users++;
>> +    spin_unlock(&gref_lock);
>> +}
>> +
>>  static void gntalloc_vma_close(struct vm_area_struct *vma)
>>  {
>>      struct gntalloc_gref *gref = vma->vm_private_data;
>> @@ -441,6 +452,7 @@ static void gntalloc_vma_close(struct vm_area_struct 
>> *vma)
>>  }
>>  
>>  static struct vm_operations_struct gntalloc_vmops = {
>> +    .open = gntalloc_vma_open,
>>      .close = gntalloc_vma_close,
>>  };
>>  
>> @@ -471,8 +483,6 @@ static int gntalloc_mmap(struct file *filp, struct 
>> vm_area_struct *vma)
>>      vma->vm_private_data = gref;
>>  
>>      vma->vm_flags |= VM_RESERVED;
>> -    vma->vm_flags |= VM_DONTCOPY;
>> -    vma->vm_flags |= VM_PFNMAP | VM_PFN_AT_MMAP;
>>  
>>      vma->vm_ops = &gntalloc_vmops;
>>  
>> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
>> index 2faf797..69c787a 100644
>> --- a/drivers/xen/gntdev.c
>> +++ b/drivers/xen/gntdev.c
>> @@ -334,17 +334,26 @@ static int unmap_grant_pages(struct grant_map *map, 
>> int offset, int pages)
>>  
>>  /* ------------------------------------------------------------------ */
>>  
>> +static void gntdev_vma_open(struct vm_area_struct *vma)
>> +{
>> +    struct grant_map *map = vma->vm_private_data;
>> +
>> +    pr_debug("gntdev_vma_open %p\n", vma);
>> +    atomic_inc(&map->users);
>> +}
>> +
>>  static void gntdev_vma_close(struct vm_area_struct *vma)
>>  {
>>      struct grant_map *map = vma->vm_private_data;
>>  
>> -    pr_debug("close %p\n", vma);
>> +    pr_debug("gntdev_vma_close %p\n", vma);
>>      map->vma = NULL;
>>      vma->vm_private_data = NULL;
>>      gntdev_put_map(map);
>>  }
>>  
>>  static struct vm_operations_struct gntdev_vmops = {
>> +    .open = gntdev_vma_open,
>>      .close = gntdev_vma_close,
>>  };
>>  
>> @@ -656,7 +665,10 @@ static int gntdev_mmap(struct file *flip, struct 
>> vm_area_struct *vma)
>>  
>>      vma->vm_ops = &gntdev_vmops;
>>  
>> -    vma->vm_flags |= VM_RESERVED|VM_DONTCOPY|VM_DONTEXPAND|VM_PFNMAP;
>> +    vma->vm_flags |= VM_RESERVED|VM_DONTEXPAND;
>> +
>> +    if (use_ptemod)
>> +            vma->vm_flags |= VM_DONTCOPY|VM_PFNMAP;
>>  
>>      vma->vm_private_data = map;
>>  
> 
>

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