Re: [Xen-devel] [PATCHv4 10/14] xen/gntdev: convert priv->lock to a mutex

[Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>]

On Tue, 27 Jan 2015, David Vrabel wrote:
> On 26/01/15 21:07, Stefano Stabellini wrote:
> > On Mon, 26 Jan 2015, David Vrabel wrote:
> >> On 26/01/15 18:57, Stefano Stabellini wrote:
> >>>
> >>>> @@ -443,14 +443,14 @@ 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;
> >>>>  
> >>>> -        spin_lock(&priv->lock);
> >>>> +        mutex_lock(&priv->lock);
> >>>>          list_for_each_entry(map, &priv->maps, next) {
> >>>>                  unmap_if_in_range(map, start, end);
> >>>>          }
> >>>>          list_for_each_entry(map, &priv->freeable_maps, next) {
> >>>>                  unmap_if_in_range(map, start, end);
> >>>>          }
> >>>> -        spin_unlock(&priv->lock);
> >>>> +        mutex_unlock(&priv->lock);
> >>>>  }
> >>>
> >>> I don't think that mmu_notifier callbacks are allowed to sleep:
> >>>
> >>> https://lkml.org/lkml/2010/1/25/187
> >>
> >> I don't think that limitation exists any more.  SRCU is used and you can
> >> sleep between tlb_gather_mmu()/tlb_finish_mmu().
> >>
> >> Perhaps you could point to something that isn't 5 years old?
> > 
> > Point taken.
> > 
> > However the problem is that I couldn't find anything that points in the
> > other direction either. If you look at include/linux/mmu_notifier.h, it
> > doesn't state that the callbacks can sleep, except for:
> > 
> >  * The invalidate_range() function is called under the ptl
> >  * spin-lock and not allowed to sleep.
> > 
> > Therefore maybe we can assume that the others are allowed to sleep,
> > because there are no comments about it?
> 
> 1. DEBUG_ATOMIC_SLEEP didn't trigger.
> 2. The documentation doesn't exclude sleeping (unlike for other ops).
> 3. Looking at the code I see nothing that would prevent sleeping and
> plenty of changes to actually allow this.
> 4. Other drivers (e.g., the i915 driver) sleep.
 
OK, I am convinced. Sorry for being stubborn.
You can add:

Reviewed-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel