Re: [Xen-devel] [PATCH 1/6] xen-gntdev: Fix circular locking dependency

[Ian Campbell <Ian.Campbell@xxxxxxxxxxxxx>]

On Tue, 2010-12-14 at 21:40 +0000, Daniel De Graaf wrote:
> On 12/14/2010 04:11 PM, Jeremy Fitzhardinge wrote:
> > On 12/14/2010 06:55 AM, Daniel De Graaf wrote:
> >> apply_to_page_range will acquire PTE lock while priv->lock is held, and
> >> mn_invl_range_start tries to acquire priv->lock with PTE already held.
> >> Fix by not holding priv->lock during the entire map operation.
> > 
> > Is priv->lock needed to protect the contents of map?
> > 
> >     J
> 
> No, since the map can only be mapped once (checked by map->vma assignment
> while the lock is held). The unmap ioctl will return -EBUSY until
> an munmap() is called on the area, so it also can't race, and the other
> users are only active once the mmap operation completes.

Sounds reasonable enough to me. There are a few unlocked accesses to
vma->map: 
        gntdev_del_map (when called from gntdev_ioctl_map_grant_ref)
        gntdev_vma_close
are these safe? If so then it would be worth a comment about why.

Anyway this patch appears to resolve the lockdep warning I was seeing
with 2.6.37 with qemu-xen backed block devices.

Ian.

> 
> > 
> >> Signed-off-by: Daniel De Graaf <dgdegra@xxxxxxxxxxxxx>
> >> ---
> >>  drivers/xen/gntdev.c |   19 +++++++++----------
> >>  1 files changed, 9 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/drivers/xen/gntdev.c b/drivers/xen/gntdev.c
> >> index a33e443..c582804 100644
> >> --- a/drivers/xen/gntdev.c
> >> +++ b/drivers/xen/gntdev.c
> >> @@ -581,23 +581,22 @@ static int gntdev_mmap(struct file *flip, struct 
> >> vm_area_struct *vma)
> >>    if (!(vma->vm_flags & VM_WRITE))
> >>            map->flags |= GNTMAP_readonly;
> >>  
> >> +  spin_unlock(&priv->lock);
> >> +
> >>    err = apply_to_page_range(vma->vm_mm, vma->vm_start,
> >>                              vma->vm_end - vma->vm_start,
> >>                              find_grant_ptes, map);
> >> -  if (err) {
> >> -          goto unlock_out;
> >> -          if (debug)
> >> -                  printk("%s: find_grant_ptes() failure.\n", 
> >> __FUNCTION__);
> >> -  }
> >> +  if (err)
> >> +          return err;
> >>  
> >>    err = map_grant_pages(map);
> >> -  if (err) {
> >> -          goto unlock_out;
> >> -          if (debug)
> >> -                  printk("%s: map_grant_pages() failure.\n", 
> >> __FUNCTION__);
> >> -  }
> >> +  if (err)
> >> +          return err;
> >> +
> >>    map->is_mapped = 1;
> >>  
> >> +  return 0;
> >> +
> >>  unlock_out:
> >>    spin_unlock(&priv->lock);
> >>    return err;
> >



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