From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel De Graaf Subject: Re: [PATCH] xen/gntdev, gntalloc: Remove unneeded VM flags Date: Mon, 07 Mar 2011 15:01:46 -0500 Message-ID: <4D7539AA.1000101@tycho.nsa.gov> References: <1299521976-8952-1-git-send-email-dgdegra@tycho.nsa.gov> <1299527629.3731.15.camel@localhost.localdomain> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1299527629.3731.15.camel@localhost.localdomain> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xensource.com Errors-To: xen-devel-bounces@lists.xensource.com To: Ian Campbell Cc: "xen-devel@lists.xensource.com" , "konrad.wilk@oracle.com" List-Id: xen-devel@lists.xenproject.org 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 >> --- >> 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; >> > >