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

Hans Verkuil
2013-11-27T07:12:45Z

On 11/26/2013 04:42 PM, Laurent Pinchart wrote:
> Hi Hans,
>
> On Friday 22 November 2013 10:02:49 Hans Verkuil wrote:
>> On 11/21/2013 08:04 PM, Laurent Pinchart wrote:
>>> 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?
>
> Note that we use the queue mutex to do so, not mmap_sem. The problem is that
> we can't release the queue mutex in the middle of a critical section without
> risking being preempted by another task. Introducing a new state might be
> possible if it effectively breaks the critical section in two independent
> parts.
>
>> 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?
>
> I'll have to review that in details given the potential complexity of locking
> issues :-) I'm not opposed to the idea, if it works I believe we should do it.
>

Do you want to think about this first, or shall I make a new patch that you can
then review?

Regards,

Hans