Re: [RFC, 2.6] a simple FIFO implementation

[Stelian Pop <stelian@popies.net>]

On Wed, Sep 15, 2004 at 03:30:13PM -0700, Andrew Morton wrote:

> > +struct kfifo {
> > +	unsigned int head;
> > +	unsigned int tail;
> > +	unsigned int size;
> > +	unsigned int len;
> > +	spinlock_t lock;
> > +	unsigned char *buffer;
> > +};
> 
> A circular buffer implementation needs only head and tail indices.  `size'
> above appears to be redundant.
> 
> Implementation-wise, the head and tail indices should *not* be constrained
> to be less than the size of the buffer.  They should be allowed to wrap all
> the way back to zero.  This allows you to distinguish between the
> completely-empty and completely-full states while using 100% of the storage.

Do you mean 'size' (the size of alloc'ed buffer) is redundant or 'len' 
(the amount of data in the FIFO) is redundant ? I see how 'len' could
be removed (and didn't do it in the first place because I choosed
code simplification over a 4 bytes gain in storage), but I hardly
see how 'size' could be removed...

> > +	unsigned long flags;
> > +
> > +	spin_lock_irqsave(&fifo->lock, flags);
> > +	
> > +	fifo->head = fifo->tail = 0;
> > +	fifo->len = 0;
> > +
> > +	spin_unlock_irqrestore(&fifo->lock, flags);
> > +}
> 
> The caller should provide the locking.  The spinlock should be removed.
> 
> Or maybe provide a separate higher-level API which does the locking for
> you.

Like in __kfifo_get which doesn't lock and kfifo_get which does ?

I don't want to remove it completly since the whole point of this 
spinlock is to protect the fifo contents...

> This is too big to inline.
[...]
> whitespace damage
[...]

Oops. I will post an updated patch later today.

Thanks for the feedback.

Stelian.
-- 
Stelian Pop <stelian@popies.net>