Re: [RFC 0/2] new kfifo API

[Arnd Bergmann <arnd@arndb.de>]

Going through your list again:

On Monday 03 August 2009, Stefani Seibold wrote:
> - Generic usage: For kernel internal use or device driver

no change here, right?

> - Linux style habit: DECLARE_KFIFO, DEFINE_KFIFO and INIT_KFIFO Macros

DEFINE_KFIFO looks useful, but I probably wouldn't expose
the other macros, so you could define them as __KFIFO_* or
integrate them into a larger DEFINE_KFIFO.

> - Ability to handle variable length records. Three type of records are
>   supported:
>   - Records between 0-255 bytes, with a record size field of 1 bytes
>   - Records between 0-65535 bytes, with a record size field of 2 bytes
>   - Byte stream, which no record size field
>   Inside a fifo this record types it is not a good idea to mix them together.

Not sure if having both 1 and 2 byte record lengths really helps.
If you want to avoid mixing the two, maybe just leave the existing
API for byte streams in a compatible, and provide extra functions
for records with a definite length.

> - Direct copy_to_user from the fifo and copy_from_user into the fifo.

Sounds useful, as mentioned.

> - Single structure: The fifo structure contains the management variables and
>   the buffer. No extra indirection is needed to access the fifo buffer.

I see two problems here:

1. you can no longer use preallocated buffers, which limits the possible
users to those that are unrestricted to the type of allocation.
2. The size of the buffer is no longer power-of-two. In fact, it's guaranteed
to be non-power-of-two because kmalloc gives you a power-of-two allocation
but now you also put the struct kfifo in there.

Users that need a power-of-two buffer (the common case) now waste almost
50% of the space.

The requirement for power-of-two also meant a much faster __kfifo_off
function on certain embedded platforms that don't have an integer division
instruction in hardware.

> - Lockless access: if only one reader and one writer is active on the fifo,
>   which is the common use case, there is no additional locking necessary.

The existing API already provides this, you can simply call __kfifo_put()
and __kfifo_get() directly, rather than their wrappers, which are just
helpers to make the code more robust. Half the existing users do that
already.

	Arnd <><