Re: [PATCH 2/7] kfifo: move out spinlock

[Stefani Seibold <stefani@seibold.net>]

Am Montag, den 17.08.2009, 07:48 +0100 schrieb Alan Cox:
> > kfifo has no business assuming that the caller wants to use
> > spin_lock() locking.
> > 
> > If we want to add wrapper helpers around kfifo to reduce code
> > duplication in callers, and if one of those wrapper helpers provides
> > spinlock-based locking then fine.
> 
> Those wrappers happen to be called kfifo_get and kfifo_put
> 
> > But the happens-to-use-spin_lock functions shouldn't be called
> > kfifo_get(), because that steals namespace from the unlocked functions,
> > and makes the naming for the happens-to-use-mutex_lock functions look
> > weird.
> 
> All over the kernel unlocked function versions have a leading _ name.
> It's the kernel convention.

Thats is not true in every case. Have a look at list.h - That was the
pattern i have implemented the new kfifo API.

> 
> The other thing I must say I dislike about these patches is the
> gratuitious 'let's rename all the functions' approach it takes. The kfifo
> API is documented, used and random API of the year type changes mess
> stuff up and cause unneeded churn.

First: Non of my eight linux kernel developer books does mention this
API. The only place is the in-kernel documentation and the man pages of
the "kernel hackers manual".

The main reason to do this was to design a cleaner interface. Because
there are very few users of this API, i thought it is a good time and
chance to do this.

My first draft version does also not renamed this functions, but there
was some concerns about the new functionality without modification the
function names.

Also the remove of the spinlock made is necessary to rename the
functions for preventing miss-use by out-of-kernel-tree drivers.

I think the break is not so hard if you believe. All you have to do is
to replace or kfifo_get() into kfifo_out_locked() and kfifo_put() into
kfifo_in_locked() if you really need the old behavior.

The thing is that we get no progress without changes.

> 
> The implementation itself is a really really nice idea.
> 
> 

Thanks for the flowers ;-)