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

[Daniel De Graaf <dgdegra@tycho.nsa.gov>]

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@tycho.nsa.gov>
>> ---
>>  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;
>>  
> 
>