Re: [RFC 0/2] new kfifo API
On Monday 03 August 2009, Stefani Seibold wrote: > Am Montag, den 03.08.2009, 16:42 +0200 schrieb Arnd Bergmann: > > My guess is that more importantly > > > > - few people so far needed the functionality. > > This is not true, that is only your view. Don't speak for other people. > A lot of device driver developer has its own implement of a fifo. I have > written a lot of device drivers for embedded system and i always missed > a clean designed and implemented fifo API subsystem. As I said, I was only guessing from the only evidence we both had, which is the current use in the kernel. Your extrapolation was that it did not get used much because of the quality of the existing API, my extrapolation was that there is no need for it (at least I was never looking for it in any of my drivers). > > This sounds all nice, and your code looks clean and usable, but really, > > what's your point? > > > > If you have a new driver that will use all the new features, please > > just tell us, that would make your point much clearer. Also, if > > you can quantify an advantage ( bytes code size reduce, yy% > > performance improvement on Y benchmark) that would be really > > helpful. > > > > Yes, i have some drivers where i use a former version of the kfifo API. > But the real advantage is not benchmarking. > > First if we have a useable kfifo API i think other developer will use > it. And this will save memory. Being able to simplify code is obviously good, but the normal approach of doing it is to make it obvious in what ways. Exchanging a whole API at once makes it unobvious what changes you do for which purpose. If you split out every logical change into a separate patch, you can easily show how to subsequently make use of that. That also avoids the problem of adding functions that end up being unused. Submitting patches would also make it easier to review than a rewrite. > Third: The old API did not have a kfifo_to_user oder a kfifo_from_user > functionality, so everybody wo need to store userspace data must write > it own version of this. That sounds like a feature that can be easily separated from the incompatible API changes. > Fourth: We don't discus about a havy API change, it is more subtle. And > it doesn't blow up the kernel, okay, a little little bit. But i am sure > that this well be gained back, if/when developer use this new API. Can you separate the incompatible API changes from the compatible extensions? Arnd <>< --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [RFC 0/2] new kfifo API
Am Montag, den 03.08.2009, 20:23 +0200 schrieb Arnd Bergmann: > On Monday 03 August 2009, Stefani Seibold wrote: > > Am Montag, den 03.08.2009, 16:42 +0200 schrieb Arnd Bergmann: > > > My guess is that more importantly > > > > > > - few people so far needed the functionality. > > > > This is not true, that is only your view. Don't speak for other people. > > A lot of device driver developer has its own implement of a fifo. I have > > written a lot of device drivers for embedded system and i always missed > > a clean designed and implemented fifo API subsystem. > > As I said, I was only guessing from the only evidence we both had, which > is the current use in the kernel. Your extrapolation was that it did > not get used much because of the quality of the existing API, my > extrapolation was that there is no need for it (at least I was > never looking for it in any of my drivers). > Maybe. > > > This sounds all nice, and your code looks clean and usable, but really, > > > what's your point? > > > > > > If you have a new driver that will use all the new features, please > > > just tell us, that would make your point much clearer. Also, if > > > you can quantify an advantage ( bytes code size reduce, yy% > > > performance improvement on Y benchmark) that would be really > > > helpful. > > > > > > > Yes, i have some drivers where i use a former version of the kfifo API. > > But the real advantage is not benchmarking. > > > > First if we have a useable kfifo API i think other developer will use > > it. And this will save memory. > > Being able to simplify code is obviously good, but the normal > approach of doing it is to make it obvious in what ways. Exchanging > a whole API at once makes it unobvious what changes you do for which > purpose. > It is not a complete change of the API. There are two main things which are different: The kfifo_get and kfifo_put functions are extended by two new parameters: flag and total. Setting both to there will result in the old behavior. And i remove the spinlock which is logical not a part of the fifo and should be handled outside if necessary. > If you split out every logical change into a separate patch, > you can easily show how to subsequently make use of that. > That also avoids the problem of adding functions that end up > being unused. > > Submitting patches would also make it easier to review than > a rewrite. > Agree, but the code is very tiny and the functionality depends on the whole. Since many of the functions are inlines, which will not generate any code until there are used. So the kernel will not be bloated. > > Third: The old API did not have a kfifo_to_user oder a kfifo_from_user > > functionality, so everybody wo need to store userspace data must write > > it own version of this. > > That sounds like a feature that can be easily separated from the > incompatible API changes. > > > Fourth: We don't discus about a havy API change, it is more subtle. And > > it doesn't blow up the kernel, okay, a little little bit. But i am sure > > that this well be gained back, if/when developer use this new API. > > Can you separate the incompatible API changes from the compatible > extensions? > No, thats not makes not real sense. If i do this i have exactly the old API. Again, the API is not really incompatible, the change is from the view of the old API very tiny. > Arnd <>< Stefani --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [RFC 0/2] new kfifo API
On 08/03/2009 09:42 AM, Arnd Bergmann wrote: > On Monday 03 August 2009, Stefani Seibold wrote: >> This is a proposal of a new generic kernel FIFO implementation. >> >> The current kernel fifo API is not very widely used, because it has to many >> constrains. Only 13 files in the current 2.6.30 used it. FIFO's are >> like list are a very basic thing and a kfifo API which handles the most use >> case would save a lot of time and memory resources. >> >> I think there are the following reasons why kfifo is not in use. >> >> - There is a need of a spinlock despite you need it or not >> - A fifo can only allocated dynamically >> - There is no support for data records inside a fifo >> - The FIFO size can only a power of two For iscsi, the only thing we have not liked with the current code is having to have the fifo a power of 2. It has not been that big a deal though. --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [RFC 0/2] new kfifo API
On Monday 03 August 2009, Stefani Seibold wrote: > This is a proposal of a new generic kernel FIFO implementation. > > The current kernel fifo API is not very widely used, because it has to many > constrains. Only 13 files in the current 2.6.30 used it. FIFO's are > like list are a very basic thing and a kfifo API which handles the most use > case would save a lot of time and memory resources. > > I think there are the following reasons why kfifo is not in use. > > - There is a need of a spinlock despite you need it or not > - A fifo can only allocated dynamically > - There is no support for data records inside a fifo > - The FIFO size can only a power of two > - The API is to tall, some important functions are missing My guess is that more importantly - few people so far needed the functionality. All of the existing users are in relatively obscure device drivers, a total of 16 committers have touched code using it since 2.6.12 and almost all the changes were in iSCSI (cc:ing maintainer). > So i decided to extend the kfifo in a more generic way without blowing up > the API to much. This was my design goals: > > - Generic usage: For kernel internal use or device driver > - Provide an API for the most use case > - Preserve memory resource > - Linux style habit: DECLARE_KFIFO, DEFINE_KFIFO and INIT_KFIFO Macros > - Slim API: The whole API provides 13 functions. > - Ability to handle variable length records. Three type of records are > supported: > - Records between 0-255 bytes, with a record size field of 1 bytes > - Records between 0-65535 bytes, with a record size field of 2 bytes > - Byte stream, which no record size field > Inside a fifo this record types it is not a good idea to mix them together. > - Direct copy_to_user from the fifo and copy_from_user into the fifo. > - Single structure: The fifo structure contains the management variables and > the buffer. No extra indirection is needed to access the fifo buffer. > - Lockless access: if only one reader and one writer is active on the fifo, > which is the common use case, there is no additional locking necessary. > - Performance > - Easy to use This sounds all nice, and your code looks clean and usable, but really, what's your point? If you have a new driver that will use all the new features, please just tell us, that would make your point much clearer. Also, if you can quantify an advantage ( bytes code size reduce, yy% performance improvement on Y benchmark) that would be really helpful. > One thing is that the new API is not compatible with the old one. I had > a look at the current user of the old kfifo API and it is easy to adapt it to > the new API. These are the files which use currently the kfifo API: > > /usr/src/linux/./drivers/char/nozomi.c > /usr/src/linux/./drivers/char/sonypi.c > /usr/src/linux/./drivers/infiniband/hw/cxgb3/cxio_resource.c > /usr/src/linux/./drivers/media/video/meye.c > /usr/src/linux/./drivers/net/wireless/libertas/main.c > /usr/src/linux/./drivers/platform/x86/fujitsu-laptop.c > /usr/src/linux/./drivers/platform/x86/sony-laptop.c > /usr/src/linux/./drivers/scsi/libiscsi.c > /usr/src/linux/./drivers/scsi/libiscsi_tcp.c > /usr/src/linux/./drivers/scsi/libsrp.c > /usr/src/linux/./drivers/usb/host/fhci.h > /usr/src/linux/./net/dccp/probe.c > > I will do this job if there is a tendency for substitute the old API. So i ask > for comments I don't think we should risk introducing regressions if the only possible benefit is to make an interface easy to use that almost nobody uses. We have also recently gained the include/linux/ring_buffer.h API which appears to be a superset of the kfifo API. Arnd <>< --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "open-iscsi" group. To post to this group, send email to open-iscsi@googlegroups.com To unsubscribe from this group, send email to open-iscsi+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/open-iscsi -~--~~~~--~~--~--~---
Re: [RFC 0/2] new kfifo API
Am Montag, den 03.08.2009, 16:42 +0200 schrieb Arnd Bergmann: > On Monday 03 August 2009, Stefani Seibold wrote: > > This is a proposal of a new generic kernel FIFO implementation. > > > > The current kernel fifo API is not very widely used, because it has to many > > constrains. Only 13 files in the current 2.6.30 used it. FIFO's are > > like list are a very basic thing and a kfifo API which handles the most use > > case would save a lot of time and memory resources. > > > > I think there are the following reasons why kfifo is not in use. > > > > - There is a need of a spinlock despite you need it or not > > - A fifo can only allocated dynamically > > - There is no support for data records inside a fifo > > - The FIFO size can only a power of two > > - The API is to tall, some important functions are missing > > My guess is that more importantly > > - few people so far needed the functionality. > This is not true, that is only your view. Don't speak for other people. A lot of device driver developer has its own implement of a fifo. I have written a lot of device drivers for embedded system and i always missed a clean designed and implemented fifo API subsystem. > All of the existing users are in relatively obscure device drivers, > a total of 16 committers have touched code using it since 2.6.12 > and almost all the changes were in iSCSI (cc:ing maintainer). > > > So i decided to extend the kfifo in a more generic way without blowing up > > the API to much. This was my design goals: > > > > - Generic usage: For kernel internal use or device driver > > - Provide an API for the most use case > > - Preserve memory resource > > - Linux style habit: DECLARE_KFIFO, DEFINE_KFIFO and INIT_KFIFO Macros > > - Slim API: The whole API provides 13 functions. > > - Ability to handle variable length records. Three type of records are > > supported: > > - Records between 0-255 bytes, with a record size field of 1 bytes > > - Records between 0-65535 bytes, with a record size field of 2 bytes > > - Byte stream, which no record size field > > Inside a fifo this record types it is not a good idea to mix them > > together. > > - Direct copy_to_user from the fifo and copy_from_user into the fifo. > > - Single structure: The fifo structure contains the management variables and > > the buffer. No extra indirection is needed to access the fifo buffer. > > - Lockless access: if only one reader and one writer is active on the fifo, > > which is the common use case, there is no additional locking necessary. > > - Performance > > - Easy to use > > This sounds all nice, and your code looks clean and usable, but really, > what's your point? > > If you have a new driver that will use all the new features, please > just tell us, that would make your point much clearer. Also, if > you can quantify an advantage ( bytes code size reduce, yy% > performance improvement on Y benchmark) that would be really > helpful. > Yes, i have some drivers where i use a former version of the kfifo API. But the real advantage is not benchmarking. First if we have a useable kfifo API i think other developer will use it. And this will save memory. Second: The feature to store variable length records in the fifo is IMHO very nice. It save memory and give you a very flexible method to store datagrams. Especially many usb devices which works with variable data packages length. So the new API gives a very easy way to queue this kind of datagrams. Third: The old API did not have a kfifo_to_user oder a kfifo_from_user functionality, so everybody wo need to store userspace data must write it own version of this. Fourth: We don't discus about a havy API change, it is more subtle. And it doesn't blow up the kernel, okay, a little little bit. But i am sure that this well be gained back, if/when developer use this new API. > > One thing is that the new API is not compatible with the old one. I had > > a look at the current user of the old kfifo API and it is easy to adapt it > > to > > the new API. These are the files which use currently the kfifo API: > > > > /usr/src/linux/./drivers/char/nozomi.c > > /usr/src/linux/./drivers/char/sonypi.c > > /usr/src/linux/./drivers/infiniband/hw/cxgb3/cxio_resource.c > > /usr/src/linux/./drivers/media/video/meye.c > > /usr/src/linux/./drivers/net/wireless/libertas/main.c > > /usr/src/linux/./drivers/platform/x86/fujitsu-laptop.c > > /usr/src/linux/./drivers/platform/x86/sony-laptop.c > > /usr/src/linux/./drivers/scsi/libiscsi.c > > /usr/src/linux/./drivers/scsi/libiscsi_tcp.c > > /usr/src/linux/./drivers/scsi/libsrp.c > > /usr/src/linux/./drivers/usb/host/fhci.h > > /usr/src/linux/./net/dccp/probe.c > > > > I will do this job if there is a tendency for substitute the old API. So i > > ask > > for comments > > I don't think we should risk introducing regressions if the only possible > benefit is to make an interface easy to use that almost nobody uses. I think this is a typical ki
