Message-Id: <200908121843.21123.arnd@arndb.de>
Date:	Wed, 12 Aug 2009 18:43:21 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Stefani Seibold <stefani@...bold.net>
Cc:	"linux-kernel" <linux-kernel@...r.kernel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>
Subject: Re: [PATCH 0/7] new kfifo API

On Wednesday 12 August 2009, Stefani Seibold wrote:
> Am Mittwoch, den 12.08.2009, 16:58 +0200 schrieb Arnd Bergmann:
> > On Wednesday 12 August 2009, Stefani Seibold wrote:
> > > This is a proposal of a new generic kernel FIFO implementation.

> > Making the fifo itself a member of the data structure is
> > a very good idea, which should be what the changeset
> > comment describes (why that is done, not how of course).
> > 
> > The other change in here is that you add a 'spinlock_t *'
> > argument to kfifo_alloc and kfifo_init. AFAICT this is
> > unrelated, and it gets reverted by the next patch, so it
> > would be more straightforward to leave that change out.
> > 
> 
> That is the problem: If i do to much in one patch set it is bad... but
> if i try to make smaller patch which are viable, than i need some
> intermediate steps. So what is the right way???

In general, smaller patches are better. As I said above, your
patch 1 actually does two things when it should just do one.

Since the patch immediately following it undoes one of the two
changes, the right option is to not do that part at all.
   
> > >  Part 2: Move out spinlock
> > 
> > I don't see this one as worthwhile, but I don't mind
> > either. The contents look correct. 
> 
> Andrew like the idea too, because it gibe the user the freedom of choice
> which locking he needs, in most case no locking is required.

Well, the original code already gives you the option by calling
__kfifo_get instead of __kfifo_put, as a number of drivers do.
Your patch does not change that, it only means that you save a few
bytes in the kfifo struct if you don't use a lock.

This was one of the options suggested for the initial submission
of the kfifo API back in 2004, and it was not chosen for some reason.
As I said, I don't mind much either way, but ideally you would try
to find out what the reason for doing it the other way initially
was, and describing in your patch what has changed now.

I suppose something along the lines of "we used to think that most
users would need the lock, it turned out that most actually don't,
so let's safe a tiny bit of space and time for the common case" would
do.

> > That one is new, right?
> > 
> > It's probably better to have this, just to make sure everyone
> > understands that the API is different now and no (out of tree)
> > drivers or patches accidentally break.
> 
> Yes, it new. It was your suggestion to make the new API miss use save. I
> like the idea, but i am not very happy with the names. The old names was
> much more logical.

Well, it's probably bad to move to a less logical naming, so if you
can't come up with something that is as good as the old one but still
different, you might want to leave this change out after all.

> > >  Part 7: add record handling functions (does some reorg)
> > 
> > This one adds a lot of extra complexity in unused code.
> > I'll add my Acked-by to the first six patches if you do the
> > minor modifications I mention above, but for the last one,
> > I suggest you either add an actual user of that code, or find
> > someone else to advocate its inclusion.
> 
> Thats the reason why i made a separate patch for this. But i fixed a lot
> of your objections since last time. Please have a look on it. It solves
> much of the use case needed by the current users.
> 
> a.) It doesn't copy anything if the FIFO supply not enough data
> b.) It doesn't copy anything if the target buffer has not enough space
> c.) It returns the number of bytes which could not copied, or 0 if okay.
> This makes error handling much easier.
> d.) It gives device driver developer more flexibility to store variable
> length data into the FIFO.
> e.) It also gives a better support for fixed record size entries.

ok, good. I'll have another look the next time you submit this.

> I still have an actual user for this... a lot of my device drivers
> depend on it and some of them are also interesting for the community,
> like my USB NRP-Sensor (Rohde&Schwarz) driver. 

Ok, fair enough, that's all I was asking for. So I guess you should just
leave out this patch from the current series and submit it together with
one of your drivers using the interface. That will make it much clearer
how the code is used.

	Arnd <><
--
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