Re: [RFC PATCH 1/8] vb2: push the mmap semaphore down to __buf_prepare()

[Hans Verkuil <hverkuil@xs4all.nl>]

On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Thursday 21 November 2013 16:21:59 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> Rather than taking the mmap semaphore at a relatively high-level function,
>> push it down to the place where it is really needed.
>>
>> It was placed in vb2_queue_or_prepare_buf() to prevent racing with other
>> vb2 calls, however, I see no way that any race can happen.
> 
> What about the following scenario ? Both QBUF calls are performed on the same 
> buffer.
> 
> 	CPU 0							CPU 1
> 	-------------------------------------------------------------------------
> 	QBUF								QBUF
> 		locks the queue mutex				waits for the queue mutex
> 	vb2_qbuf
> 	vb2_queue_or_prepare_buf
> 	__vb2_qbuf
> 		checks vb->state, calls
> 	__buf_prepare
> 	call_qop(q, wait_prepare, q);
> 		unlocks the queue mutex
> 										locks the queue mutex
> 									vb2_qbuf
> 									vb2_queue_or_prepare_buf
> 									__vb2_qbuf
> 										checks vb->state, calls
> 									__buf_prepare
> 									call_qop(q, wait_prepare, q);
> 										unlocks the queue mutex
> 
> 									queue the buffer, set buffer
> 									 state to queue
> 
> 	queue the buffer, set buffer
> 	 state to queue
> 
> 
> We would thus end up queueing the buffer twice. The vb->state check needs to 
> be performed after the brief release of the queue mutex.

Good point, I hadn't thought about that scenario. However, using mmap_sem to
introduce a large critical section just to protect against state changes is IMHO
not the right approach. Why not introduce a VB2_BUF_STATE_PREPARING state?

That's set at the start of __buf_prepare while the queue mutex is still held, and
which prevents other threads of queuing the same buffer again. If the prepare fails,
then the state is reverted back to DEQUEUED.

__fill_v4l2_buffer() will handle the PREPARING state as if it was the DEQUEUED state.

What do you think?

Regards,

	Hans