[RFC PATCHES] First version of the V4L2 core locking patches

[RFC PATCHES] First version of the V4L2 core locking patches

[Hans Verkuil]

Hi all,

I've made a first version of the core locking patches available here:

http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/test

I'm actually surprised how trivial the patches are. Which makes me wonder if
I am overlooking something, it feels too easy.

One thing I did not yet have time to analyze fully is if it is really OK to
unlock/relock the vdev_lock in videobuf_waiton. I hope it is, because without
this another thread will find it impossible to access the video node while it
is in waiton.

Currently I've only tested with vivi. I hope to be able to spend more time
this week for a more thorough analysis and converting a few more drivers to
this.

In the meantime, please feel free to shoot at this code!

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCHES] First version of the V4L2 core locking patches

[Mauro Carvalho Chehab]

Em 20-09-2010 18:37, Hans Verkuil escreveu:
> Hi all,
> 
> I've made a first version of the core locking patches available here:
> 
> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/test
> 
> I'm actually surprised how trivial the patches are. Which makes me wonder if
> I am overlooking something, it feels too easy.
> 
> One thing I did not yet have time to analyze fully is if it is really OK to
> unlock/relock the vdev_lock in videobuf_waiton. I hope it is, because without
> this another thread will find it impossible to access the video node while it
> is in waiton.
> 
> Currently I've only tested with vivi. I hope to be able to spend more time
> this week for a more thorough analysis and converting a few more drivers to
> this.
> 
> In the meantime, please feel free to shoot at this code!

Hi Hans,

This patch will likely break most drivers:
	http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=commitdiff;h=d1ca35f3e69d909a958eb1cf8c75dd1c0bb2a98c

In the case of events and videobuf_waiton, it doesn't seem to be safe to just
unlock when waiting for an event.

For example, in the case of videobuf_waiton, the code for it is:

#define WAITON_CONDITION (vb->state != VIDEOBUF_ACTIVE &&\
				vb->state != VIDEOBUF_QUEUED)
int videobuf_waiton(struct videobuf_buffer *vb, int non_blocking, int intr)
{
	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);

	if (non_blocking) {
		if (WAITON_CONDITION)
			return 0;
		else
			return -EAGAIN;
	}

	if (intr)
		return wait_event_interruptible(vb->done, WAITON_CONDITION);
	else
		wait_event(vb->done, WAITON_CONDITION);

	return 0;
}

When called internally, it have the vb mutex_locked, while, when called externally, it
doesn't.

By looking on other parts where vb->done is protected, like on videobuf_queue_cancel:

	spin_lock_irqsave(q->irqlock, flags);
	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
		if (NULL == q->bufs[i])
			continue;
		if (q->bufs[i]->state == VIDEOBUF_QUEUED) {
			list_del(&q->bufs[i]->queue);
			q->bufs[i]->state = VIDEOBUF_ERROR;
			wake_up_all(&q->bufs[i]->done);
		}
	}
	spin_unlock_irqrestore(q->irqlock, flags);

It is clear that vb state is protected by a spinlock, and not by a mutex. Using a mutex
there makes no sense at all. Instead of touching a mutex, callers of this function should
be reviewed to not call a mutex.

So, the better approach for videobuf_waiton would be to protect it with a
spinlock.

Also, your patches assume that no driver will touch at vdev lock before calling videobuf_waiton().
This seems to be a risky assumption. So, the better would be to define it as:

static int is_state_active_or_queued(struct videobuf_buffer *vb, struct videobuf_queue *q, )
{
	bool rc;

	spin_lock_irqsave(q->irqlock, flags);
	rc = (vb->state != VIDEOBUF_ACTIVE) && (vb->state != VIDEOBUF_QUEUED));
	spin_unlock_irqrestore(q->irqlock, flags);

	return rc;
};

int videobuf_waiton(struct videobuf_queue *q, struct videobuf_buffer *vb, int non_blocking, int intr)
{
	rc = 0;
 	bool is_vdev_locked;
	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);

	/*
	 * If there's nothing to wait, just return		
	 */
	if (is_state_active_or_queued(vb, q))
		return 0;

	if (non_blocking)
		return -EAGAIN;

	/*
	 * Need to sleep in order to wait for videobufs to complete.
	 * It is not a good idea to sleep while waiting for an event with the dev lock hold,
	 * as it will block any other access to the device. Just unlock it while waiting,
	 * locking it again at the end.
	 */

 	is_vdev_locked = (q->vdev_lock && mutex_is_locked(q->vdev_lock)) ? true : false;
	if (is_vdev_locked)
		mutex_unlock(q->vdev_lock);
	if (intr)
		return wait_event_interruptible(vb->done, is_state_active_or_queued(vb, q));
	else
		wait_event(vb->done, is_state_active_or_queued(vb, q));
	if (is_vdev_locked)
		mutex_lock(q->vdev_lock);

	return 0;
}

Cheers,
Mauro

Re: [RFC PATCHES] First version of the V4L2 core locking patches

[Hans Verkuil]

Hi Mauro,

> Em 20-09-2010 18:37, Hans Verkuil escreveu:
>> Hi all,
>>
>> I've made a first version of the core locking patches available here:
>>
>> http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=shortlog;h=refs/heads/test
>>
>> I'm actually surprised how trivial the patches are. Which makes me
>> wonder if
>> I am overlooking something, it feels too easy.
>>
>> One thing I did not yet have time to analyze fully is if it is really OK
>> to
>> unlock/relock the vdev_lock in videobuf_waiton. I hope it is, because
>> without
>> this another thread will find it impossible to access the video node
>> while it
>> is in waiton.
>>
>> Currently I've only tested with vivi. I hope to be able to spend more
>> time
>> this week for a more thorough analysis and converting a few more drivers
>> to
>> this.
>>
>> In the meantime, please feel free to shoot at this code!
>
> Hi Hans,
>
> This patch will likely break most drivers:
> 	http://git.linuxtv.org/hverkuil/v4l-dvb.git?a=commitdiff;h=d1ca35f3e69d909a958eb1cf8c75dd1c0bb2a98c

This was indeed something I wanted to review more closely.

> In the case of events and videobuf_waiton, it doesn't seem to be safe to
> just
> unlock when waiting for an event.
>
> For example, in the case of videobuf_waiton, the code for it is:
>
> #define WAITON_CONDITION (vb->state != VIDEOBUF_ACTIVE &&\
> 				vb->state != VIDEOBUF_QUEUED)
> int videobuf_waiton(struct videobuf_buffer *vb, int non_blocking, int
> intr)
> {
> 	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
>
> 	if (non_blocking) {
> 		if (WAITON_CONDITION)
> 			return 0;
> 		else
> 			return -EAGAIN;
> 	}
>
> 	if (intr)
> 		return wait_event_interruptible(vb->done, WAITON_CONDITION);
> 	else
> 		wait_event(vb->done, WAITON_CONDITION);
>
> 	return 0;
> }
>
> When called internally, it have the vb mutex_locked, while, when called
> externally, it
> doesn't.
>
> By looking on other parts where vb->done is protected, like on
> videobuf_queue_cancel:
>
> 	spin_lock_irqsave(q->irqlock, flags);
> 	for (i = 0; i < VIDEO_MAX_FRAME; i++) {
> 		if (NULL == q->bufs[i])
> 			continue;
> 		if (q->bufs[i]->state == VIDEOBUF_QUEUED) {
> 			list_del(&q->bufs[i]->queue);
> 			q->bufs[i]->state = VIDEOBUF_ERROR;
> 			wake_up_all(&q->bufs[i]->done);
> 		}
> 	}
> 	spin_unlock_irqrestore(q->irqlock, flags);
>
> It is clear that vb state is protected by a spinlock, and not by a mutex.
> Using a mutex
> there makes no sense at all. Instead of touching a mutex, callers of this
> function should
> be reviewed to not call a mutex.
>
> So, the better approach for videobuf_waiton would be to protect it with a
> spinlock.

Sounds reasonable.

> Also, your patches assume that no driver will touch at vdev lock before
> calling videobuf_waiton().
> This seems to be a risky assumption. So, the better would be to define it
> as:
>
> static int is_state_active_or_queued(struct videobuf_buffer *vb, struct
> videobuf_queue *q, )
> {
> 	bool rc;
>
> 	spin_lock_irqsave(q->irqlock, flags);
> 	rc = (vb->state != VIDEOBUF_ACTIVE) && (vb->state != VIDEOBUF_QUEUED));
> 	spin_unlock_irqrestore(q->irqlock, flags);
>
> 	return rc;
> };
>
> int videobuf_waiton(struct videobuf_queue *q, struct videobuf_buffer *vb,
> int non_blocking, int intr)
> {
> 	rc = 0;
>  	bool is_vdev_locked;
> 	MAGIC_CHECK(vb->magic, MAGIC_BUFFER);
>
> 	/*
> 	 * If there's nothing to wait, just return
> 	 */
> 	if (is_state_active_or_queued(vb, q))
> 		return 0;
>
> 	if (non_blocking)
> 		return -EAGAIN;
>
> 	/*
> 	 * Need to sleep in order to wait for videobufs to complete.
> 	 * It is not a good idea to sleep while waiting for an event with the dev
> lock hold,
> 	 * as it will block any other access to the device. Just unlock it while
> waiting,
> 	 * locking it again at the end.
> 	 */
>
>  	is_vdev_locked = (q->vdev_lock && mutex_is_locked(q->vdev_lock)) ? true
> : false;
> 	if (is_vdev_locked)
> 		mutex_unlock(q->vdev_lock);
> 	if (intr)
> 		return wait_event_interruptible(vb->done, is_state_active_or_queued(vb,
> q));

This obviously needs to save the return value and continue to make sure
the lock is taken again.

> 	else
> 		wait_event(vb->done, is_state_active_or_queued(vb, q));
> 	if (is_vdev_locked)
> 		mutex_lock(q->vdev_lock);
>
> 	return 0;
> }

Agreed. Thanks for reviewing this, it was the one patch that I knew I had
to look into more closely. I'll incorporate your changes.

Regards,

         Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco

Re: [RFC PATCHES] First version of the V4L2 core locking patches

[Mauro Carvalho Chehab]

Em Tue, 21 Sep 2010 15:50:13 +0200
"Hans Verkuil" <hverkuil@xs4all.nl> escreveu:

> Hi Mauro,
> 
> > Em 20-09-2010 18:37, Hans Verkuil escreveu:
...
> > 	/*
> > 	 * Need to sleep in order to wait for videobufs to complete.
> > 	 * It is not a good idea to sleep while waiting for an event with the dev
> > lock hold,
> > 	 * as it will block any other access to the device. Just unlock it while
> > waiting,
> > 	 * locking it again at the end.
> > 	 */
> >
> >  	is_vdev_locked = (q->vdev_lock && mutex_is_locked(q->vdev_lock)) ? true
> > : false;
> > 	if (is_vdev_locked)
> > 		mutex_unlock(q->vdev_lock);
> > 	if (intr)
> > 		return wait_event_interruptible(vb->done, is_state_active_or_queued(vb,
> > q));
> 
> This obviously needs to save the return value and continue to make sure
> the lock is taken again.

Yeah, it should be:
 rc = wait_event_interruptible(vb->done, is_state_active_or_queued(vb, q));

and return rc at the end.
 
> > 	else
> > 		wait_event(vb->done, is_state_active_or_queued(vb, q));
> > 	if (is_vdev_locked)
> > 		mutex_lock(q->vdev_lock);
> >
> > 	return 0;
> > }
> 
> Agreed. Thanks for reviewing this, it was the one patch that I knew I had
> to look into more closely. I'll incorporate your changes.

Ok, thanks.


-- 

Cheers,
Mauro

Re: [RFC PATCHES] First version of the V4L2 core locking patches

[Hans Verkuil]

On Tuesday, September 21, 2010 19:14:07 Mauro Carvalho Chehab wrote:
> Em Tue, 21 Sep 2010 15:50:13 +0200
> "Hans Verkuil" <hverkuil@xs4all.nl> escreveu:
> 
> > Hi Mauro,
> > 
> > > Em 20-09-2010 18:37, Hans Verkuil escreveu:
> ...
> > > 	/*
> > > 	 * Need to sleep in order to wait for videobufs to complete.
> > > 	 * It is not a good idea to sleep while waiting for an event with the dev
> > > lock hold,
> > > 	 * as it will block any other access to the device. Just unlock it while
> > > waiting,
> > > 	 * locking it again at the end.
> > > 	 */
> > >
> > >  	is_vdev_locked = (q->vdev_lock && mutex_is_locked(q->vdev_lock)) ? true
> > > : false;
> > > 	if (is_vdev_locked)
> > > 		mutex_unlock(q->vdev_lock);
> > > 	if (intr)
> > > 		return wait_event_interruptible(vb->done, is_state_active_or_queued(vb,
> > > q));
> > 
> > This obviously needs to save the return value and continue to make sure
> > the lock is taken again.
> 
> Yeah, it should be:
>  rc = wait_event_interruptible(vb->done, is_state_active_or_queued(vb, q));
> 
> and return rc at the end.
>  
> > > 	else
> > > 		wait_event(vb->done, is_state_active_or_queued(vb, q));
> > > 	if (is_vdev_locked)
> > > 		mutex_lock(q->vdev_lock);
> > >
> > > 	return 0;
> > > }
> > 
> > Agreed. Thanks for reviewing this, it was the one patch that I knew I had
> > to look into more closely. I'll incorporate your changes.
> 
> Ok, thanks.

I added a patch with basically this code to my test tree. I will try to convert
and test a few more drivers, but that will probably be Friday. If the conversion
goes well then I plan to post a pull request by Friday or Saturday with somewhat
cleaned up patches.

I strongly suspect that for the 2-3 weeks after that I will not be able to
continue with this, so I hope others will take over from me.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco