Re: [PATCH 5/5] kfifo: log based kfifo API
On Tue, Jan 08, 2013 at 10:16:46AM -0800, Dmitry Torokhov wrote: > Hi Yuanhan, > > On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote: > > The current kfifo API take the kfifo size as input, while it rounds > > _down_ the size to power of 2 at __kfifo_alloc. This may introduce > > potential issue. > > > > Take the code at drivers/hid/hid-logitech-dj.c as example: > > > > if (kfifo_alloc(&djrcv_dev->notif_fifo, > >DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct > > dj_report), > >GFP_KERNEL)) { > > > > Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report) > > is 15. > > > > Which means it wants to allocate a kfifo buffer which can store 8 > > dj_report entries at once. The expected kfifo buffer size would be > > 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the > > size to rounddown_power_of_2(120) = 64, and then allocate a buf > > with 64 bytes, which I don't think this is the original author want. > > > > With the new log API, we can do like following: > > > > int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS * > > sizeof(struct dj_report)); > > > > if (kfifo_alloc(&djrcv_dev->notif_fifo, kfifo_size_order, GFP_KERNEL)) { > > > > This make sure we will allocate enough kfifo buffer for holding > > DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries. > > Why don't you simply change __kfifo_alloc to round the allocation up > instead of down? Hi Dmitry, Yes, it would be neat and that was my first reaction as well. I then sent out a patch, but it was NACKed by Stefani(the original kfifo author). Here is the link: https://lkml.org/lkml/2012/10/26/144 Then Stefani proposed to change the API to take log of size as input to root fix this kind of issues. And here it is. Thanks. --yliu -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] kfifo: log based kfifo API
Dmitry Torokhov wrote: >Hi Yuanhan, > >On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote: >> The current kfifo API take the kfifo size as input, while it rounds >> _down_ the size to power of 2 at __kfifo_alloc. This may introduce >> potential issue. >> >> Take the code at drivers/hid/hid-logitech-dj.c as example: >> >> if (kfifo_alloc(&djrcv_dev->notif_fifo, >>DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct >dj_report), >>GFP_KERNEL)) { >> >> Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct >dj_report) >> is 15. >> >> Which means it wants to allocate a kfifo buffer which can store 8 >> dj_report entries at once. The expected kfifo buffer size would be >> 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the >> size to rounddown_power_of_2(120) = 64, and then allocate a buf >> with 64 bytes, which I don't think this is the original author want. >> >> With the new log API, we can do like following: >> >> int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS * >> sizeof(struct dj_report)); >> >> if (kfifo_alloc(&djrcv_dev->notif_fifo, kfifo_size_order, >GFP_KERNEL)) { >> >> This make sure we will allocate enough kfifo buffer for holding >> DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries. > >Why don't you simply change __kfifo_alloc to round the allocation up >instead of down? > >Thanks. > >-- >Dmitry >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" >in >the body of a message to majord...@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html Hi Dmitry, I agree. I don't see the benefit in pushing up the change to a kfifo internal decision/problem to many different places in the kernel. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] kfifo: log based kfifo API
Hi Yuanhan, On Tue, Jan 08, 2013 at 10:57:53PM +0800, Yuanhan Liu wrote: > The current kfifo API take the kfifo size as input, while it rounds > _down_ the size to power of 2 at __kfifo_alloc. This may introduce > potential issue. > > Take the code at drivers/hid/hid-logitech-dj.c as example: > > if (kfifo_alloc(&djrcv_dev->notif_fifo, >DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report), >GFP_KERNEL)) { > > Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report) > is 15. > > Which means it wants to allocate a kfifo buffer which can store 8 > dj_report entries at once. The expected kfifo buffer size would be > 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the > size to rounddown_power_of_2(120) = 64, and then allocate a buf > with 64 bytes, which I don't think this is the original author want. > > With the new log API, we can do like following: > > int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS * > sizeof(struct dj_report)); > > if (kfifo_alloc(&djrcv_dev->notif_fifo, kfifo_size_order, GFP_KERNEL)) { > > This make sure we will allocate enough kfifo buffer for holding > DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries. Why don't you simply change __kfifo_alloc to round the allocation up instead of down? Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] kfifo: log based kfifo API
The current kfifo API take the kfifo size as input, while it rounds _down_ the size to power of 2 at __kfifo_alloc. This may introduce potential issue. Take the code at drivers/hid/hid-logitech-dj.c as example: if (kfifo_alloc(&djrcv_dev->notif_fifo, DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report), GFP_KERNEL)) { Where, DJ_MAX_NUMBER_NOTIFICATIONS is 8, and sizeo of(struct dj_report) is 15. Which means it wants to allocate a kfifo buffer which can store 8 dj_report entries at once. The expected kfifo buffer size would be 8 * 15 = 120 then. While, in the end, __kfifo_alloc will turn the size to rounddown_power_of_2(120) = 64, and then allocate a buf with 64 bytes, which I don't think this is the original author want. With the new log API, we can do like following: int kfifo_size_order = order_base_2(DJ_MAX_NUMBER_NOTIFICATIONS * sizeof(struct dj_report)); if (kfifo_alloc(&djrcv_dev->notif_fifo, kfifo_size_order, GFP_KERNEL)) { This make sure we will allocate enough kfifo buffer for holding DJ_MAX_NUMBER_NOTIFICATIONS dj_report entries. Cc: Stefani Seibold Cc: Andrew Morton Cc: linux-omap@vger.kernel.org Cc: linuxppc-...@lists.ozlabs.org Cc: platform-driver-...@vger.kernel.org Cc: linux-in...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-r...@vger.kernel.org Cc: linux-me...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux-...@lists.infradead.org Cc: libertas-...@lists.infradead.org Cc: linux-wirel...@vger.kernel.org Cc: net...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: open-is...@googlegroups.com Cc: linux-s...@vger.kernel.org Cc: de...@driverdev.osuosl.org Cc: linux-ser...@vger.kernel.org Cc: linux-...@vger.kernel.org Cc: linux...@kvack.org Cc: d...@vger.kernel.org Cc: linux-s...@vger.kernel.org Signed-off-by: Yuanhan Liu --- arch/arm/plat-omap/Kconfig |2 +- arch/arm/plat-omap/mailbox.c|6 +++- arch/powerpc/sysdev/fsl_rmu.c |2 +- drivers/char/sonypi.c |9 --- drivers/hid/hid-logitech-dj.c |7 +++-- drivers/iio/industrialio-event.c|2 +- drivers/iio/kfifo_buf.c |3 +- drivers/infiniband/hw/cxgb3/cxio_resource.c |8 -- drivers/media/i2c/cx25840/cx25840-ir.c |9 +-- drivers/media/pci/cx23885/cx23888-ir.c |9 +-- drivers/media/pci/meye/meye.c |6 +--- drivers/media/pci/meye/meye.h |2 + drivers/media/rc/ir-raw.c |7 +++-- drivers/memstick/host/r592.h|2 +- drivers/mmc/card/sdio_uart.c|4 ++- drivers/mtd/sm_ftl.c|5 +++- drivers/net/wireless/libertas/main.c|4 ++- drivers/net/wireless/rt2x00/rt2x00dev.c |5 +-- drivers/pci/pcie/aer/aerdrv_core.c |3 +- drivers/platform/x86/fujitsu-laptop.c |5 ++- drivers/platform/x86/sony-laptop.c |6 ++-- drivers/rapidio/devices/tsi721.c|5 ++- drivers/scsi/libiscsi_tcp.c |6 +++- drivers/staging/omapdrm/omap_plane.c|5 +++- drivers/tty/n_gsm.c |4 ++- drivers/tty/nozomi.c|5 +-- drivers/tty/serial/ifx6x60.c|2 +- drivers/tty/serial/ifx6x60.h|3 +- drivers/tty/serial/kgdb_nmi.c |7 +++-- drivers/usb/host/fhci.h |4 ++- drivers/usb/serial/cypress_m8.c |4 +- drivers/usb/serial/io_ti.c |4 +- drivers/usb/serial/ti_usb_3410_5052.c |7 +++-- drivers/usb/serial/usb-serial.c |2 +- include/linux/kfifo.h | 31 +-- include/linux/rio.h |1 + include/media/lirc_dev.h|4 ++- kernel/kfifo.c |9 +-- mm/memory-failure.c |3 +- net/dccp/probe.c|6 +++- net/sctp/probe.c|6 +++- samples/kfifo/bytestream-example.c |8 +++--- samples/kfifo/dma-example.c |5 ++- samples/kfifo/inttype-example.c |7 +++-- samples/kfifo/record-example.c |6 ++-- 45 files changed, 142 insertions(+), 108 deletions(-) diff --git a/arch/arm/plat-omap/Kconfig b/arch/arm/plat-omap/Kconfig index 665870d..7eda02c 100644 --- a/arch/arm/plat-omap/Kconfig +++ b/arch/arm/plat-omap/Kconfig @@ -124,7 +124,7 @@ config OMAP_MBOX_FWK DSP, IVA1.0 and IVA2 in OMAP1/2/3. config OMAP_MBOX_KFIFO_SIZE - int "Mailbox kfifo default buffer size (bytes)" + int "Mailbox kfifo default buffer size (bytes, should be power of 2. If not, will roundup to power of 2"