[PATCH v2] drm: do not sleep on vblank while holding a mutex

[PATCH v2] drm: do not sleep on vblank while holding a mutex

[Ilija Hadzic]

drm_wait_vblank must be DRM_UNLOCKED because otherwise it
will grab the drm_global_mutex and then go to sleep until the vblank
event it is waiting for. That can wreck havoc in the windowing system
because if one process issues this ioctl, it will block all other
processes for the duration of all vblanks between the current and the
one it is waiting for. In some cases it can block the entire windowing
system. So, make this ioctl DRM_UNLOCKED, wrap the remaining
(very short) critical section with dev->vbl_lock spinlock, and
add a comment to the code explaining what we are protecting with
the lock.

v2: incorporate comments received from Daniel Vetter and
    Michel Daenzer.

Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
---
 drivers/gpu/drm/drm_drv.c |    2 +-
 drivers/gpu/drm/drm_irq.c |   14 +++++++++++++-
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index e159f17..c990dab 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -124,7 +124,7 @@ static struct drm_ioctl_desc drm_ioctls[] = {
 	DRM_IOCTL_DEF(DRM_IOCTL_SG_ALLOC, drm_sg_alloc_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 	DRM_IOCTL_DEF(DRM_IOCTL_SG_FREE, drm_sg_free, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
 
-	DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, 0),
+	DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, DRM_UNLOCKED),
 
 	DRM_IOCTL_DEF(DRM_IOCTL_MODESET_CTL, drm_modeset_ctl, 0),
 
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 3830e9e..c8b4da8 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1160,6 +1160,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	union drm_wait_vblank *vblwait = data;
 	int ret = 0;
 	unsigned int flags, seq, crtc, high_crtc;
+	unsigned long irqflags;
 
 	if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled))
 		return -EINVAL;
@@ -1193,6 +1194,13 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	}
 	seq = drm_vblank_count(dev, crtc);
 
+	/* the lock protects this section from itself executed in */
+	/* the context of another PID, ensuring that the process that */
+	/* wrote dev->last_vblank_wait is really the last process */
+	/* that was here; processes waiting on different CRTCs */
+	/* do not need to be interlocked, but rather than introducing */
+	/* a new per-crtc lock, we reuse vbl_lock for simplicity */
+	spin_lock_irqsave(&dev->vbl_lock, irqflags);
 	switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
 	case _DRM_VBLANK_RELATIVE:
 		vblwait->request.sequence += seq;
@@ -1200,12 +1208,15 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	case _DRM_VBLANK_ABSOLUTE:
 		break;
 	default:
+		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 		ret = -EINVAL;
 		goto done;
 	}
 
-	if (flags & _DRM_VBLANK_EVENT)
+	if (flags & _DRM_VBLANK_EVENT) {
+		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
+	}
 
 	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
 	    (seq - vblwait->request.sequence) <= (1<<23)) {
@@ -1215,6 +1226,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
 	DRM_DEBUG("waiting on vblank count %d, crtc %d\n",
 		  vblwait->request.sequence, crtc);
 	dev->last_vblank_wait[crtc] = vblwait->request.sequence;
+	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
 	DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
 		    (((drm_vblank_count(dev, crtc) -
 		       vblwait->request.sequence) <= (1 << 23)) ||
-- 
1.7.7

Re: [PATCH v2] drm: do not sleep on vblank while holding a mutex

[Daniel Vetter]

On Fri, Oct 28, 2011 at 05:42:23PM -0400, Ilija Hadzic wrote:
> drm_wait_vblank must be DRM_UNLOCKED because otherwise it
> will grab the drm_global_mutex and then go to sleep until the vblank
> event it is waiting for. That can wreck havoc in the windowing system
> because if one process issues this ioctl, it will block all other
> processes for the duration of all vblanks between the current and the
> one it is waiting for. In some cases it can block the entire windowing
> system. So, make this ioctl DRM_UNLOCKED, wrap the remaining
> (very short) critical section with dev->vbl_lock spinlock, and
> add a comment to the code explaining what we are protecting with
> the lock.
> 
> v2: incorporate comments received from Daniel Vetter and
>     Michel Daenzer.
> 
> Signed-off-by: Ilija Hadzic <ihadzic@research.bell-labs.com>
> ---
>  drivers/gpu/drm/drm_drv.c |    2 +-
>  drivers/gpu/drm/drm_irq.c |   14 +++++++++++++-
>  2 files changed, 14 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> index e159f17..c990dab 100644
> --- a/drivers/gpu/drm/drm_drv.c
> +++ b/drivers/gpu/drm/drm_drv.c
> @@ -124,7 +124,7 @@ static struct drm_ioctl_desc drm_ioctls[] = {
>  	DRM_IOCTL_DEF(DRM_IOCTL_SG_ALLOC, drm_sg_alloc_ioctl, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  	DRM_IOCTL_DEF(DRM_IOCTL_SG_FREE, drm_sg_free, DRM_AUTH|DRM_MASTER|DRM_ROOT_ONLY),
>  
> -	DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, 0),
> +	DRM_IOCTL_DEF(DRM_IOCTL_WAIT_VBLANK, drm_wait_vblank, DRM_UNLOCKED),
>  
>  	DRM_IOCTL_DEF(DRM_IOCTL_MODESET_CTL, drm_modeset_ctl, 0),
>  
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 3830e9e..c8b4da8 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1160,6 +1160,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	union drm_wait_vblank *vblwait = data;
>  	int ret = 0;
>  	unsigned int flags, seq, crtc, high_crtc;
> +	unsigned long irqflags;
>  
>  	if ((!drm_dev_to_irq(dev)) || (!dev->irq_enabled))
>  		return -EINVAL;
> @@ -1193,6 +1194,13 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	}
>  	seq = drm_vblank_count(dev, crtc);
>  
> +	/* the lock protects this section from itself executed in */
> +	/* the context of another PID, ensuring that the process that */
> +	/* wrote dev->last_vblank_wait is really the last process */
> +	/* that was here; processes waiting on different CRTCs */
> +	/* do not need to be interlocked, but rather than introducing */
> +	/* a new per-crtc lock, we reuse vbl_lock for simplicity */

Multiline comments are done with one pair of /* */, see CodingStyle

> +	spin_lock_irqsave(&dev->vbl_lock, irqflags);
>  	switch (vblwait->request.type & _DRM_VBLANK_TYPES_MASK) {
>  	case _DRM_VBLANK_RELATIVE:
>  		vblwait->request.sequence += seq;
> @@ -1200,12 +1208,15 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	case _DRM_VBLANK_ABSOLUTE:
>  		break;
>  	default:
> +		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  		ret = -EINVAL;
>  		goto done;
>  	}
>  
> -	if (flags & _DRM_VBLANK_EVENT)
> +	if (flags & _DRM_VBLANK_EVENT) {
> +		spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
>  		return drm_queue_vblank_event(dev, crtc, vblwait, file_priv);
> +	}
>  
>  	if ((flags & _DRM_VBLANK_NEXTONMISS) &&
>  	    (seq - vblwait->request.sequence) <= (1<<23)) {
> @@ -1215,6 +1226,7 @@ int drm_wait_vblank(struct drm_device *dev, void *data,
>  	DRM_DEBUG("waiting on vblank count %d, crtc %d\n",
>  		  vblwait->request.sequence, crtc);
>  	dev->last_vblank_wait[crtc] = vblwait->request.sequence;
> +	spin_unlock_irqrestore(&dev->vbl_lock, irqflags);

Imo the only thing we need to protect here is dev->last_vblank_wait
against concurrent access from drm_vblank_info in drm_info.c

But the locking there is horribly broken: It grabs dev->struct_mutex, but
that protects exactly nothing, afaics:
- dev->vblank_refcount is an atomic
- vblank_count is protected by a the handrolled seqlock in
  drm_vblank_count
- dev->last_vblank_wait was protected by the global lock, but is now
  protected by the dev->vbl_spinlock on the write side
- dev->vblank_inmodeset is protected by the global lock for ums setups
  (locked ioctl) and by dev->mode_config.mutex for kms

And I just don't see anything else you might protect here - vblwait is
just the ioctl data copied in by the drm core ioctl helpers.

So I don't quite see what you protect here with that spinlock. Imo
- either make dev->last_vblank_wait into an atomic_t
- or just don't care about it, locking of vblank_info is already horribly
  broken.
In both cases you don't need the dev->vbl_lock spinlock.

I personally vote for no additional locking at all here and just drop the
global lock. Or maybe I'm blind and we need this. In that case, please
clue me up.

>  	DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
>  		    (((drm_vblank_count(dev, crtc) -
>  		       vblwait->request.sequence) <= (1 << 23)) ||

One line below there's the curios case of us rechecking dev->irq_enabled.
Without any locking in place. But afaics this is only changed by the
driver at setup and teardown when we are guaranteed (hopefully) that
there's no open fd and hence no way to call an ioctl. So checking once at
the beginning should be good enough. Whatever.
-Daniel

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] drm: do not sleep on vblank while holding a mutex

[Ilija Hadzic]

On Sat, 29 Oct 2011, Daniel Vetter wrote:
>> +	/* the lock protects this section from itself executed in */
>> +	/* the context of another PID, ensuring that the process that */
>> +	/* wrote dev->last_vblank_wait is really the last process */
>> +	/* that was here; processes waiting on different CRTCs */
>> +	/* do not need to be interlocked, but rather than introducing */
>> +	/* a new per-crtc lock, we reuse vbl_lock for simplicity */
>
> Multiline comments are done with one pair of /* */, see CodingStyle
>


Will fix the multiline comments (though scripts/checkpatch.pl didn't 
complain so I figured it was OK)


>
> I personally vote for no additional locking at all here and just drop the
> global lock. Or maybe I'm blind and we need this. In that case, please
> clue me up.
>


What I am doing with the lock, is makeing sure that the last vblank wait 
reported is really last. You said in your previous comment (which I agree 
with) that the value of having last_vblank_wait in debugfs is in case of 
hangs. In that case you want to see who really was the last one to issue 
the vblank.

Now suppose that the drm_wait_vblank is enteret in the context of two 
different PIDs and suppose that there are no locks. Let's say that the 
first process wants to wait on vblank N and the second wants to wait on 
vblank N+1. First process can reach the point just before it wants to 
write to last_vblank_wait and lose the processor there (so it has N in 
vblank.request (on its stack) calculated, but it has not written it into 
last_vblank_wait yet (because it lost the CPU right there). Then the 
second process gets the CPU, and executes and let's say that it goes all 
the way through so it writes N+1 into last_vblank_wait and then schedules 
away (it called DRM_WAIT_ON). Now the first process gets the CPU where it 
left off and overwrites N+1 in last_vblank_wait with N, which is now 
stale.

I explained in the comment (and in my note this morning), that the only 
race condition is against itself in the context of different processes.
The above is the scenario.

Race against drm_vblank_info exists in theory, but in practice doesn't 
matter because the reader of debugfs (or proc file system) runs 
asynchronously (and typically at slower pace) from processes issuing 
vblank waits (if it's a human looking at the filesystem then definitely 
much slower pace). Since processes issuing vblanks are overwriting the 
same last_vblank_value, drm_vblank_info is already doing a form of 
downsampling and debugfs is losing information, anyway, so who cares about 
the lock. In case of a hang, the value of last_vblank_wait doesn't change,
so again the lock in drm_vblank_info doesn't change anything. So adding a 
lock there (which would have to be vbl_lock to make it usable) is only a 
matter of theoretical correctness, but no practical value. So I decided 
not to touch drm_vblank_info.

drm_wait_vblank, however, needs a protection from itself as I explained 
above.


>>  	DRM_WAIT_ON(ret, dev->vbl_queue[crtc], 3 * DRM_HZ,
>>  		    (((drm_vblank_count(dev, crtc) -
>>  		       vblwait->request.sequence) <= (1 << 23)) ||
>
> One line below there's the curios case of us rechecking dev->irq_enabled.
> Without any locking in place. But afaics this is only changed by the
> driver at setup and teardown when we are guaranteed (hopefully) that
> there's no open fd and hence no way to call an ioctl. So checking once at
> the beginning should be good enough. Whatever.

It's probably redundant statement because it won't be reached if 
irq_enabled is false and there is nothing to change it to true at 
runtime, but that's a topic for a different patch.

-- Ilija

Re: [PATCH v2] drm: do not sleep on vblank while holding a mutex

[Daniel Vetter]

On Fri, Oct 28, 2011 at 09:44:54PM -0500, Ilija Hadzic wrote:
> Now suppose that the drm_wait_vblank is enteret in the context of
> two different PIDs and suppose that there are no locks. Let's say
> that the first process wants to wait on vblank N and the second
> wants to wait on vblank N+1. First process can reach the point just
> before it wants to write to last_vblank_wait and lose the processor
> there (so it has N in vblank.request (on its stack) calculated, but
> it has not written it into last_vblank_wait yet (because it lost the
> CPU right there). Then the second process gets the CPU, and executes
> and let's say that it goes all the way through so it writes N+1 into
> last_vblank_wait and then schedules away (it called DRM_WAIT_ON).
> Now the first process gets the CPU where it left off and overwrites
> N+1 in last_vblank_wait with N, which is now stale.

Ok, and here's why your locking (or any locking that drops the lock before
calling schedule) won't work: Dropping the lock allows more than one
process waiting for a vblank on the same crtc (the hole point of this
exerices). So let's say N processes are waiting for vblank on crtc A, but
we only have one storage place for last_vblank_wait. So by necessity, it's
gonna be stale for N-1 processes. And because the scheduler is free to
take away the cpu between the unlock and the schedule, you can freely pick
for which process it's gonna be the right one, there's zero ordering
guarantees possible.

By throwing in a modeset call that disables vblanks it's pretty easy to
come up with a process that waits for vblank x+1 (when x is the last
vblank before the vblank counter got stopped before the modeset) and
last_vblank_wait == x.

Which is why I think adding decent tracepoints would be much better, but
that's a different patch.

[snip]

Agreed on not locking the debug file and postponing other cleanups, that's
all material for different patches.

Yours, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH v2] drm: do not sleep on vblank while holding a mutex

[Ilija Hadzic]

On Sat, 29 Oct 2011, Daniel Vetter wrote:

>
> Ok, and here's why your locking (or any locking that drops the lock before
> calling schedule) won't work: [SNIP]
>

You just came full circle. Recall that in my v1 patch I went all the way 
to enqueuing the process in the wait queue before dropping the lock. That 
would have guaranteed that if there is a hangup, what last_vblank_wait 
says is the last is really the last. If there is no hang, then it doesn't 
matter because last_vlank_wait is constantly overwritten (and is indeed 
stale for N-1 processes). However, that was disliked in the review and I 
didn't want to argue.

So in the interest of making progress, it looks that you would be happy if 
this patch just dropped DRM_UNLOCKED and declared that we don't care about 
(potentially theoretical) critical section related to last_vblank_wait.

If that's the case, I'll cut a relatvely trivial patch that drops 
DRM_UNLOCKED from this system call to solve the problem that I pointed 
earlier in this thread and leave all the rest of the locking discussion 
for other patches.

Would that be fine by you ?

thanks,

Ilija

Re: [PATCH v2] drm: do not sleep on vblank while holding a mutex

[Daniel Vetter]

On Sat, Oct 29, 2011 at 06:59:42AM -0500, Ilija Hadzic wrote:
> On Sat, 29 Oct 2011, Daniel Vetter wrote:
> >Ok, and here's why your locking (or any locking that drops the lock before
> >calling schedule) won't work: [SNIP]
> 
> You just came full circle. Recall that in my v1 patch I went all the
> way to enqueuing the process in the wait queue before dropping the
> lock. That would have guaranteed that if there is a hangup, what
> last_vblank_wait says is the last is really the last. If there is no
> hang, then it doesn't matter because last_vlank_wait is constantly
> overwritten (and is indeed stale for N-1 processes). However, that
> was disliked in the review and I didn't want to argue.

Not quite full circle. Holding the lock until you're on the waitqueue
doesn't really change anything - we wake up everybody on every vblank
anyway. The thing I was nitpicking over was that vblank_last_wait isn't
really protected on the read side, but as you've pointed out, that's not a
big problem. Essentially the only thing about vblank_last_wait that we can
guarantee is that somewhen somebody tried to wait on this vblank. Nothing
else is possible (and the reason why I think we should ditch this all and
replace it we some decent tracing - material for a differen patch though).

> So in the interest of making progress, it looks that you would be
> happy if this patch just dropped DRM_UNLOCKED and declared that we
> don't care about (potentially theoretical) critical section related
> to last_vblank_wait.
> 
> If that's the case, I'll cut a relatvely trivial patch that drops
> DRM_UNLOCKED from this system call to solve the problem that I
> pointed earlier in this thread and leave all the rest of the locking
> discussion for other patches.
> 
> Would that be fine by you ?

Yeah.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48