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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

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.

David

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