Date:	Thu, 04 Feb 2010 17:52:40 +0100
From:	Stefani Seibold <stefani@...bold.net>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	linux-kernel <linux-kernel@...r.kernel.org>,
	Andi Kleen <andi@...stfloor.org>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	Alan Cox <alan@...rguk.ukuu.org.uk>,
	Theodore Tso <tytso@....edu>,
	"Ira W. Snyder" <iws@...o.caltech.edu>
Subject: Re: [PATCH] enhanced reimplemention of the kfifo API

Am Mittwoch, den 03.02.2010, 15:05 -0800 schrieb Andrew Morton:
> On Wed, 03 Feb 2010 23:09:51 +0100
> Stefani Seibold <stefani@...bold.net> wrote:
> 
> > Am Mittwoch, den 03.02.2010, 12:05 -0800 schrieb Andrew Morton:
> > > On Wed, 27 Jan 2010 14:00:43 +0100
> > > Stefani Seibold <stefani@...bold.net> wrote:
> > > 
> > > > This is a complete reimplementation of the new kfifo API, which is now 
> > > > really generic, type save and type definable. 
> > > > 
> > > > 
> > > > ...
> > > >
> > > > diff -u -N -r -p linux-2.6.33-rc5.dummy/include/linux/kfifo.h linux-2.6.33-rc5.new/include/linux/kfifo.h
> > > > --- linux-2.6.33-rc5.dummy/include/linux/kfifo.h	1970-01-01 01:00:00.000000000 +0100
> > > > +++ linux-2.6.33-rc5.new/include/linux/kfifo.h	2010-01-27 13:34:06.324991898 +0100
> > > >
> > > > ...
> > > >
> > > > +/*
> > > > + * DECLARE_KFIFO_PTR - macro to declare a fifo pointer object
> > > > + * @fifo: name of the declared fifo
> > > > + * @type: type of the fifo elements
> > > > + * @size: the number of elements in the fifo, this must be a power of 2.
> > > > + */
> > > 
> > > Throughout this patch the comments are in kerneldoc form, but they
> > > don't use the /** opening token, so the kerneldoc tools will ignore them.
> > > 
> > > Please fix that up and test it.
> > > 
> > > > +#define DECLARE_KFIFO_PTR(fifo, type)	STRUCT_KFIFO_PTR(type) fifo
> > > > +
> > > > +/* helper macro */
> > > > +#define	__is_kfifo_ptr(fifo)	(sizeof(*fifo) == sizeof(struct __kfifo))
> > > 
> > > This is weird.  It's a compile-time constant.  What's it here for?
> > 
> > I need it to distinguish between real in place fifo, where the fifo
> > array is a part of the structure, and the fifo type where the array is
> > outside of the fifo structure.
> 
> OK, please add a comment?

I will do this.

> 
> > > > +/*
> > > > + * INIT_KFIFO - Initialize a fifo declared by DECLARE_KFIFO
> > > > + * @fifo: name of the declared fifo datatype
> > > > + */
> > > > +#define INIT_KFIFO(fifo)	fifo = __kfifo_initializer(fifo)
> > > 
> > > Some versions of gcc generate quite poor code for
> > > 
> > > 	struct foo f = { ... };
> > > 
> > > It would generate an instance of the struct into the stack and then
> > > would memcpy it over, iirc.  Please take a look, see if that's
> > > happening with this construct (might need to use an old gcc version)
> > > and if so, see if there's a fix?
> > > 
> > 
> > The current version of the kfifo implementation did it in the same way.
> > Why is it now wrong?
> 
> It was wrong before, too.
> 
> > Which gcc version do you mean?
> 
> I don't recall.  The oldest gcc the kernel permits is 3.2, so checking
> any 3.x should be OK.
> 

I will try to build a gcc 3 and check it.

> > > > +/*
> > > > + * kfifo_initialized - Check if kfifo is initialized.
> > > > + * @fifo: fifo to check
> > > > + * Return %true if FIFO is initialized, otherwise %false.
> > > > + * Assumes the fifo was 0 before.
> > > > + *
> > > > + * Note: for in place fifo's this macro returns alway true
> > > > + */
> > > > +#define kfifo_initialized(fifo) ((fifo)->kfifo.data != NULL)
> > > 
> > > kfifo_initialized() is suspicious.  Any code which doesn't know whether
> > > its kfifo was initialised is probably confused, lame and broken. 
> > > Fortunately kfifo_initialized() is not used anywhere.  Please prepare a
> > > separate patch to kill it sometime.
> > > 
> > 
> > This function was introduced as a patch by andi kleen and you have it
> > already applied.
> 
> Let's delete it?
> 

Can you clarify this with andi please?

> > > > +/*
> > > > + * kfifo_esize - returns the size of the element managed by the fifo
> > > > + * @fifo: the fifo to be used.
> > > > + */
> > > > +#define kfifo_esize(fifo)	((fifo)->kfifo.esize)
> > > 
> > > This doesn't seem to be used anywhere either.
> > > 
> > 
> > It is a part of the API. It returns the the size of the handled element,
> > If a driver developer need it, it is there.
> 
> Well, if there's no proven need for it now, it will remain dead code
> for an unknown period of time.
> 
> Also, there was no need to implement this as a macro (was there?).  If
> not, conversion to a more typesafe C function would be better.
> 

It is not possible to write this as a function, because the parameter
fifo is an arbitrary type. There is no fifo type, only a template and a
macro which generates a fifo which managed a given type, where the
generic "struct __kfifo" part is embedded.

> > > > +/*
> > > > + * kfifo_recsize - returns the size of the record length field
> > > > + * @fifo: the fifo to be used.
> > > > + */
> > > > +#define kfifo_recsize(fifo)	(sizeof(*(fifo)->rectype))
> > > 
> > > Neither is this.  What's going on?
> > > 
> > 
> > Dito... This returns the size of the record length field. 0 means this
> > is not a variable record fifo.
> 
> ditto.
> 
> > > > +/*
> > > > + * kfifo_out_locked - gets some data from the fifo using a spinlock for locking
> > > > + * @fifo: the fifo to be used.
> > > > + * @buf: pointer to the storage buffer
> > > > + * @n: max. number of elements to get
> > > > + * @lock: pointer to the spinlock to use for locking.
> > > > + *
> > > > + * This macro get the data from the fifo and return the numbers of elements
> > > > + * copied.
> > > > + */
> > > > +#define	kfifo_out_locked(fifo, buf, n, lock) \
> > > > +__kfifo_check( \
> > > > +({ \
> > > > +	unsigned long __flags; \
> > > > +	unsigned int __ret; \
> > > > +	spin_lock_irqsave(lock, __flags); \
> > > > +	__ret = kfifo_out(fifo, buf, n); \
> > > > +	spin_unlock_irqrestore(lock, __flags); \
> > > > +	__ret; \
> > > > +}) \
> > > > +)
> > > 
> > > This is poorly named.  Generally "foo_locked" is to be called when the
> > > caller has already taken the lock.  This identifier inverts that
> > > convention.
> > > 
> > 
> > This is the same name as the current kfifo API. Renaming it would break
> > a lot of drivers. But if there is no complain and you insist i will
> > rename it and fix the current users.
> 
> argh, we goofed.
> 
> yeah, it'd be nice to fix it sometime, please.  Not urgent.
> 
> A good way to fix it would be to add a new function with a new name
> then migrate all callers over to that name then when it's done, remove
> the old name.
> 

I will do this in the next release. Would be kfifo_out_spinlocked() and
kfifo_in_spinlocked() for the new names okay?

> > > > + * @fifo: the fifo to be used.
> > > > + * @to: where the data must be copied.
> > > > + * @len: the size of the destination buffer.
> > > > + * @copied: pointer to output variable to store the number of copied bytes.
> > > > + *
> > > > + * This macro copies at most @len bytes from the fifo into the
> > > > + * @to buffer and returns -EFAULT/0.
> > > > + *
> > > > + * Note that with only one concurrent reader and one concurrent
> > > > + * writer, you don't need extra locking to use these macro.
> > > > + */
> > > > +#define	kfifo_to_user(fifo, to, len, copied) \
> > > > +__kfifo_check( \
> > > > +({ \
> > > > +	typeof(fifo + 1) __tmp = (fifo); \
> > > > +	void __user *__to = (to); \
> > > > +	unsigned int __len = (len); \
> > > > +	unsigned int *__copied = (copied); \
> > > > +	const size_t __recsize = sizeof(*__tmp->rectype); \
> > > > +	struct __kfifo *__kfifo = &__tmp->kfifo; \
> > > > +	(__recsize) ? \
> > > > +	__kfifo_to_user_r(__kfifo, __to, __len, __copied, __recsize) : \
> > > > +	__kfifo_to_user(__kfifo, __to, __len, __copied); \
> > > > +}) \
> > > > +)
> > > 
> > > Do these have any callers?
> > > 
> > 
> > Currently not, but this is a part of the current kfifo implementation. A
> > lot of developer like this idea and it will be used in the near future.
> 
> OK.  Please describe the presently-unused interfaces in the changelog,
> with particular attention to the reason for including them at this
> time.
> 

OK

> > > > +static inline unsigned int kfifo_unused(struct __kfifo *fifo)
> > > > +{
> > > > +	return (fifo->mask + 1) - (fifo->in - fifo->out);
> > > > +}
> > > 
> > > hm, how does this differ from kfifo_avail()?
> > > 
> > 
> > This is similar to kfifo_avail, but due the nature of macro and type
> > conversation i have not longer the original type information available.
> > So i can't call the macros in kfifo.h from the functions defined in
> > kfifo.c.
> 
> OK, please add a comment there.  (As a general rule: if this reader
> needed more info then other readers will want more info as well).
> 
> > > > +static unsigned long kfifo_copy_from_user(struct __kfifo *fifo,
> > > > +	const void *src, unsigned int len, unsigned int off,
> > > > +	unsigned int *copied)
> > > > +{
> > > > +	unsigned int size = fifo->mask + 1;
> > > > +	unsigned int esize = fifo->esize;
> > > > +	unsigned int l;
> > > > +	unsigned long ret;
> > > > +
> > > > +	off &= fifo->mask;
> > > > +	if (esize != 1) {
> > > > +		off *= esize;
> > > > +		size *= esize;
> > > > +		len *= esize;
> > > > +	}
> > > > +	l = min(len, size - off);
> > > > +
> > > > +	ret = copy_from_user(fifo->data + off, src, l);
> > > > +	if (unlikely(ret))
> > > > +		ret = roundup_diff(ret + len - l, esize);
> > > > +	else {
> > > > +		ret = copy_from_user(fifo->data, src + l, len - l);
> > > > +		if (unlikely(ret))
> > > > +			ret = roundup_diff(ret, esize);
> > > > +	}
> > > > +	smp_wmb();
> > > > +	if (copied)
> > > 
> > > Can `copied' ever be NULL?  If not, remove this test.
> > 
> > It was only a feature. If you don't like i will remove it.
> 
> No strong opinion.  But if present callers don't pass NULL then that's
> a sign that future callers won't, too.  We may as well save the
> speed/space overhead until we actually need it.
> 
> 

This will be fixed in the next release.

One offer to solve the egg and chicken problem: Let us include the
functions kfifo_to_user(), kfifo_from_user(),  kfifo_esize(),
kfifo_recsize() and the dynamic record handling. If there will be no
users in at least 9 months we remove it from the API. We talk here about
400 bytes of code.

In the mean time me and other developer will have a change to modify the
existing driver to the new API and/or post drivers or core kernel code
which is using this functionality. 

Stefani


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists