Re: [PATCH 1/2] kfifo: round up the fifo size power of 2

[Yuanhan Liu]

On Tue, Oct 30, 2012 at 11:52:10PM -0700, Andrew Morton wrote:
> On Wed, 31 Oct 2012 07:30:33 +0100 Stefani Seibold <stefani@xxxxxxxxxxx> wrote:
> 
> > > Yes, and I guess the same to give them a 64-element one.
> > > 
> > > > 
> > > > If there's absolutely no prospect that the kfifo code will ever support
> > > > 100-byte fifos then I guess we should rework the API so that the caller
> > > > has to pass in log2 of the size, not the size itself.  That way there
> > > > will be no surprises and no mistakes.
> > > > 
> > > > That being said, the power-of-2 limitation isn't at all intrinsic to a
> > > > fifo, so we shouldn't do this.  Ideally, we'd change the kfifo
> > > > implementation so it does what the caller asked it to do!
> > > 
> > > I'm fine with removing the power-of-2 limitation. Stefani, what's your
> > > comment on that?
> > > 
> > 
> > You can't remove the power-of-2-limitation, since this would result in a
> > performance decrease (bit wise and vs. modulo operation).
> 
> Probably an insignificant change in performance.
> 
> It could be made much smaller by just never doing the modulus operation
> - instead do
> 
> 	if (++index == max)
> 		index = 0;
> 
> this does introduce one problem: it's no longer possible to distinguish
> the "full" and "empty" states by comparing the head and tail indices. 
> But that is soluble.

Hi Andrew,

Yes, it is soluble. How about the following solution?

Add 2 more fields(in_off and out_off) in __kfifo structure, so that in
and out will keep increasing each time, while in_off and out_off will be
wrapped to head if goes to the end of fifo buffer.

So, we can use in and out for counting unused space, and distinguish the
"full" and "empty" state, and also, of course no need for locking.

Stefani, sorry for quite late reply. I checked all the code used kfifo_alloc
and kfifo_init. Firstly, there are a lot of users ;-)

And secondly, I did find some examples used kfifo as it supports
none-power-of-2 kfifo. Say, the one at drivers/hid/hid-logitech-dj.c:
       if (kfifo_alloc(&djrcv_dev->notif_fifo,
                       DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report),
                       GFP_KERNEL)) {

which means it wants to allocate a kfifo buffer which can store
DJ_MAX_NUMBER_NOTIFICATIONS(8 here) dj_report(each 15 bytes) at once.

And DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report) = 8 * 15.
Then current code would allocate a size of rounddown_power_of_2(120) =
64 bytes, which can hold 4 dj_report only once, which is a half of expected.

There are few more examples like this.

And, kfifo_init used a pre-allocated buffer, it would be a little strange
to ask user to pre-allocate a power of 2 size aligned buffer.

So, I guess it's would be good to support none-power-of-2 kfifo?

I know you care the performance a lot. Well, as Andrew said, it may
introduce a little insignificant drop(no modulus, few more add/dec).
Thus, do you have some benchmarks for that? I can have a test to check
if it is a insignificant change on performance or not :)

Thanks!

	--yliu

> 
> > Andrew is right, this is an API miss design.  So it would be good to
> > rework the kfifo_init () and kfifo_alloc() to pass in log2 of the size,
> > not the size itself.
> 
> The power-of-2 thing is just a restriction in the current
> implementation - it's not a good idea to cement that into the
> interface.  Of course, it could later be uncemented if the
> implementation's restriction was later relaxed.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/