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

[David Vrabel <david.vrabel@citrix.com>]

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