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

Laurent Pinchart
2013-11-26T15:42:30Z

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.

--
Regards,

Laurent Pinchart