Re: [RFC 0/2] new kfifo API

On Tuesday 04 August 2009, Stefani Seibold wrote:
Am Dienstag, den 04.08.2009, 15:45 +0200 schrieb Arnd Bergmann:
On Tuesday 04 August 2009, Stefani Seibold wrote:
Your second version is ok in this regard because it uses the original
size logic.

Does it mean you like it now ;-) ???? I think we are on a good way!

It looks much better now, but I still think you are doing too many
things at once, and I disagree about the locking changes.


I had a look at the user of the kfifo_put and kfifo_get, most did not
really depend on the spinlock, because there are single read/single
write users.

The question is not so much what the common use case is but what
the safe one is. Except for Andi's NMI driver, everyone can use
the locked version, even if many could use the unlocked version
if they want it to be faster.

You could of course change the existing users of the locked
interface to use the unlocked one, but I wouldn't bother.
Changing the behaviour of an existing interface without changing
the name is not so nice though.

If you read the original discussion about kfifo, a lot of
it focused around how locking should be done:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2004-09/4195.html

Of course I realize that the locking gets into the way if you
want to do copy_{to,from}_user, but the last person that had
this problem also worked around that by defining user access
only for the unlocked versions:
http://linux.derkeiler.com/Mailing-Lists/Kernel/2007-06/msg04959.html

No idea why this was never merged ;-)

[PATCH 2/x] kfifo: add DEFINE_KFIFO and friends
[PATCH 3/x] kfifo: add kfifo_{to,from}_user
[PATCH 4/x] kfifo: add kfifo_{get,put}_rec
[PATCH 5/x] kfifo: ...


Thats will work... but the first patch will break something. Why is
everybody depending on this stupid split patch dogma, beside it make
sense or not?

multiple reasons:

- it makes reviews easier. You want the patches to get merged, so it's
your job to make it easy to see how they help and don't cause bugs.

- the patches with the separate changelogs become part of the
bisectable git history. If something breaks, we can find the specific
change that caused it and see the problem, potentially reverting
the patch.

- the patch description becomes part of the documentation of the code.
if you don't understand why code is done in a specific way, you can
look at the patch that introduced it. Your 0/2 mail contents are very
detailed, but don't get anywhere, while a patch description lives
forever.

All of these are much more important for infrastructure changes than
for device drivers.

If i do this in that why, then the first patch will change 13 files, and
modify about 70 lines.

That doesn't sound too much.

About the locking stuff, I think it should best be left in place.
The __kfifo_{get,put} functions should probably be declared part
of the official interface and documented as such -- people are
using them anyways. It's generally a good idea to have the obvious
interface work in an entirely safe way (kfifo_get doing all the
locking it might need), with a __foo variant of the same function
for people that want the extra performance and know what they are
doing.


Believe me, nobody need this locking stuff. The common use case is one
writer/one reader, so it is complete useless. Have a look on the current
source file wich use it.

ok. Mostly true, but looking at it conservatively:

sonypi.c: multiple readers: sonypi_misc_read can be called concurrently,
which is probably a driver bug

meyeioc.c: ioctl can race with irq

sony-laptop.c: sony_pic_irq can race with sony_nc_notify, sonypi_misc_read
can race with do_sony_laptop_release_key

cxio_resource.c: I have no idea what that code does, do you?

I would also leave out the recsize argument, using an 'unsigned short'
for the record length unconditionally won't waste any real space but
simplifies both the implementation and the interface.


You are wrong. It is designed to be look free until one reader and one
writer is in use. If i split the record operation i must introduce a
lock, because between two kfifo_put a kfifo_get operation can happen (or
visa verse).

I don't understand what you mean by that or what it has to do with
the recsize argument.

Finally, I don't see a reason for the optional KFIFO_F_NOTRIM argument.
If you have fixed records, I would guess that you always need it
anyway, so you could just make it the default and remove the function
argument.

The design is to have variable length records, so it it not always true
that the destination of an operation have enough space. Cutting of the
record is not always what then is desired. It also does not cost any
code or performance since it is handle by __build_constant_p and will be
complete optimized away.

I did not object to the implementation complexity but everybody having
to specify an extra argument which is likely to be the same in all cases,
just like you argue that we don't need both locked and unlocked versions
when everyone could live with the unlocked API.

I would get a little bit confused ... Why did you not say "please add
only the kfifo_from_user() and kfifo_to_user() stuff and throw the rest
away"?. I am thankful for your review and your objections, but if my
idea is melt down in that way i would prefer not do the job.

Why do you have so much fear to change an interfaces which is currently
used by very few sources? Most of them are not critical.

especially because it is so rarely used, any regressions would likely
not get found early, so it would be better to only touch the code
when the change is "obviously correct" and a significant improvement.

Adding new interfaces obviously doesn't cause regressions, but counts
as untested (or bitrotten) code until there are actual users in the
tree, which you don't have.

So my offer is to split it into 5 patches against the current 2.6.31
tree:

[PATCH 1/x] kfifo code cleanup and preparation (includes for broken sources)
[PATCH 2/x] kfifo: remove spinlock (includes fix for broken sources)
[PATCH 3/x] kfifo: add DEFINE_KFIFO and friends
[PATCH 4/x] kfifo: add kfifo_{to,from}_user
[PATCH 5/x] kfifo: add kfifo_{get,put}_rec

Is this acceptable? It would be nice to have your support.

Yes, that looks good.

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

Relevant Pages

  • Re: [PATCH 1/6] Unified AVR32/AT91 MCI Driver
    ... To use this new driver on a at91 the platform driver for your board ... See the following patch for an example of how to do ... tristate "AT91 SD/MMC Card Interface support" ...
    (Linux-Kernel)
  • Re: [PATCH] input: driver for USB VoIP phones with CM109 chipset #2
    ... Could you test this new additional patch? ... It seems that the ctl URB is shared between buzzer and keypress ... To compile this driver as a module, choose M here: ... * CM109 usb communication interface ...
    (Linux-Kernel)
  • Re: [PATCH] lirc for 2.5/2.6 kernels - 20030802
    ... >> uses wrong interface, it is unsuitable for the kernel. ... Patch is here. ... Also driver still needs some cleanups...]. ... +static int debug = 0; ...
    (Linux-Kernel)
  • [PATCH] IPMI: driver model and sysfs support
    ... Here is a patch against 2.6.13 I've been working on (and mentioned to ... It is loosely inspired by the USB driver model ... A device struct is created for each system interface in the ... each system interface and the bmc it interfaces to. ...
    (Linux-Kernel)
  • Re: Not support for CT4730 chip?
    ... Have you by any chance tried my es137x patch? ... Adds locking to the driver to make it MPSAFE. ... I've been using the S/PDIF output since February and the locking since ...
    (freebsd-current)