Messages in this thread | | | Date | Thu, 16 Sep 2004 08:43:21 +0200 | From | Stelian Pop <> | Subject | Re: [RFC, 2.6] a simple FIFO implementation |
| |
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> - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |