Date:	Tue, 24 Aug 2010 11:04:23 +0200
From:	Stefani Seibold <stefani@...bold.net>
To:	Huang Ying <ying.huang@...el.com>
Cc:	Andrew Morton <akpm@...ux-foundation.org>,
	Andi Kleen <andi@...stfloor.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [RFC -v2] kfifo writer side lock-less support

Am Dienstag, den 24.08.2010, 16:43 +0800 schrieb Huang Ying:
> On Tue, 2010-08-24 at 15:55 +0800, Stefani Seibold wrote:
> > Hi Huang,
> > 
> > Am Dienstag, den 24.08.2010, 09:42 +0800 schrieb Huang Ying:
> > > Hi, Stefani,
> > > 
> > > The sample code is attached with the mail too. If it is necessary, I
> > > can put the sample code into samples/kfifo directory if the basic
> > > idea of the patch is accepted.
> > > 
> > 
> > The samples use a own implementation of a record handling. Please use
> > the one provided by the kfifo API.
> >

Again: Fix this.

> The patch adds only 1 field (unsigned int) to struct __kfifo. I think
> that should be acceptable. Because sizeof(struct __kfifo) should be much
> smaller that __kfifo->mask + 1 in most cases.

I don't know what you mean with "because sizeof(struct __kfifo) should
be much smaller that __kfifo->mask + 1 in most cases". I am convinced
that you did not really understand the kfifo code. sizeof(struct
__kfifo) is constant and __kfifo->mask + 1 is the fifo size in elements,
which is not constant. Before you answering study the code first!

And is not acceptable to bload the struct __kfifo, because it will never
need by the most users.

> 
> For macros, only INIT_KFIFO, kfifo_reset and kfifo_put is enlarged a
> little (one instruction?).
> 

No, you add also code to kfifo_avail, so you enlarge two of the most
used macros. And again:

Rule number one - do not increase the code size if the functionality is
not needed and used!

> And yes, I added some new functions and macros. But if I implement
> another ring buffer instead of making kfifo lock-less (for multiple
> writers), I need to implement more functions and macros, the code size
> increment will be larger, isn't it?
> 
> > And there a lot of bugs on a first small review.
> > 
> > Your kfifo_skip_len does not handle record fifos. User of kfifo_put,
> > kfifo_avail will get an increased code size. Your kfifo_is_full isn't
> > longer working correctly. You did not fixed kfifo_in(), so this function
> > will not work. And so on....
> 
> After we reach the consensus on the general idea, we can look at these
> issues one by one.
> 

No, first fix your bugs...

> > > Some idea of the lock-less algorithm in the patch comes from Steven
> > > Rostedt's trace ring buffer implementation.
> > > 
> > > The new xxx_ptr interface and kfifo_iter makes it possible to
> > > write/read content of kfifo in-place in addition to copying out/in.
> > > 
> > 
> > The regular use case of a fifo is that there is one write and one
> > reader. In this case, the current implementation of the kfifo structure
> > is lock less.
> > 
> > So i you need this, than it would be better to use the ring buffer.
> 

But you waste a clean interface designed together with the community.

> I really need multiple-writers and one reader in APEI GHES support, and
> I need lock-less in writer side (because the buffer need to be written
> in NMI handler). So I can not use the original kfifo implementation.
> 

I believe you that you need it, but the question is: Is there more users
who need it. And i am sure, there are no more users or very very few.

So for the protocol a big NAK!

- 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