[linux-dvb] [PATCH] make dvb_ringbuffer compatible to dmxdev_buffer

[Oliver Endriss]

Ralph Metzler wrote:
> Andreas Oberritter writes:
>  > On Tue, 2006-03-14 at 01:03 +0100, Oliver Endriss wrote:
>  > > Andreas Oberritter wrote:
>  > > > From: Andreas Oberritter <obi at linuxtv.org>
>  > > > 
>  > > > Added variable 'error' to struct dvb_ringbuffer, which is set to zero on
>  > > > init() and flush(). Also reset read an write pointers to zero on flush()
>  > > > to get less fragmented data.
>  > > > 
>  > > > Signed-off-by: Andreas Oberritter <obi at linuxtv.org>
>  > > > ---
>  > > > 
>  > > > A patch to make dmxdev use dvb_ringbuffer will follow.
>  > > > 
>  > > > diff -r 427667c87c7b linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c
>  > > > --- a/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Sun Mar 12 00:03:47 2006 -0300
>  > > > +++ b/linux/drivers/media/dvb/dvb-core/dvb_ringbuffer.c	Mon Mar 13 16:02:46 2006 +0100
>  > > > ...
>  > > > @@ -86,7 +87,8 @@ ssize_t dvb_ringbuffer_avail(struct dvb_
>  > > >  
>  > > >  void dvb_ringbuffer_flush(struct dvb_ringbuffer *rbuf)
>  > > >  {
>  > > > -	rbuf->pread = rbuf->pwrite;
>  > > > +	rbuf->pread = rbuf->pwrite = 0;
>  > >                                    +++
>  > > 
>  > > Attention, this will convert dvb_ringbuffer_flush() into a writer!
>  > > 
>  > > from dvb_ringbuffer.h:
>  > > | ** (2) If there is exactly one reader and one writer, there is no need
>  > > | **     to lock read or write operations.
>  > > | **     Two or more readers must be locked against each other.
>  > > | **     Flushing the buffer counts as a read operation.
>  > >          +++++++++++++++++++++++++++++++++++++++++++++++
>  > > | **     Two or more writers must be locked against each other.
>  > > 
>  > > With this patch flushing the ring buffer is a read _and_ a write
>  > > operation. It might break existing code. Are you aware of that?
>  > 
>  > Oliver, can you please take a look at the existing code? It's a
>  > performance gain if it doesn't break.
> 
> It will definitely break the code. Just look at the writing calls.
> If pwrite changes to 0 right in the middle of one, you will have problems.
> For this case (pwrite=0) you should not get segmentation faults 
> but pwrite can end up in a wrong position (!=0) and there will be
> some bogus data at the beginning of the buffer.
>  
>  > Generally I'd expect a flush to return a buffer into its initial state.
> 
> Then you will have to add mutexes and/or locks, at least in the
> write and flush calls. This will of course complicate the code and 
> also harm performance. But maybe in your case less than the
> fragmentation you were talking about? 

Ack. It will break the av7110 driver. I did not check other drivers.

We would have to add spin_lock/spin_unlock calls around each
ringbuffer_write which would reduce write performance.

How could setting the buffer ptr to 0 increase performance?
Usually it will save a single wrap of the buffer.

Anyway, if you really need this, you should add a new function which
implements this behaviour (dvb_ringbuffer_reset?).
Whenever it is used write locking will be required.

CU
Oliver

-- 
--------------------------------------------------------
VDR Remote Plugin available at
http://www.escape-edv.de/endriss/vdr/
--------------------------------------------------------