Re: [RFC, 2.6] a simple FIFO implementation

[Stelian Pop <stelian@popies.net>]

On Wed, Sep 15, 2004 at 08:27:54AM -0500, Dmitry Torokhov wrote:

> On Monday 13 September 2004 08:52 am, Stelian Pop wrote:
> > +static inline unsigned int kfifo_len(struct kfifo *fifo) {
> > +       unsigned long flags;
> > +       unsigned int result;
> > +       
> > +       spin_lock_irqsave(&fifo->lock, flags);
> > +       
> > +       result = fifo->len;
> > +
> > +       spin_unlock_irqrestore(&fifo->lock, flags);
> > +
> > +       return result;
> > +}
> 
> Hi,
> 
> I do not think that taking/releasing spinlock here serves any purpose as
> len can get canged right after releasing the lock and therefore no longer
> valid... 

Indeed, removed.

> And relying on result of kfifo_len to decide if the FIF can be
> drained is not reliable so probably the inteface is better off without this
> function.

Still, one should have a way to get at least a hint on whether the
FIFO has data or not (think poll() implementation).

What about adding a note in the header saying the function can be racy
and does not guarantee a subsequent kfifo_get will succeed ?

> Also I think that most users would put only sertain structures (homogenous?)
> in their FIFOs so maybe it should be:
> 
> struct kfifo *kfifo_alloc(unsigned int el_size, unsigned int len)
> unsigned int kfifo_put(struct kfifo *fifo, void *buffer)
> unsigned int kfifo_get(struct kfifo *fifo, void *buffer)

It's easy to adapt the generic interface to what you want, but difficult
to do the contrary. Let's be the most generic here (who knows, maybe 
somebody wants to add several structures at once...).

> Also, don't we have a rule that for functions the opening curly brace should
> be on a new line?

Sure, corrected.

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