Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Sat, Feb 19, 2005 at 11:36:05AM -0800, David Brownell wrote: > > Those allocations could be from _using_ a dma pool ... but they're > not from just creating one! > > The cost of creating the dma_pool is the cost of one small kmalloc() > plus (the expensive part) the /sys/devices/.../pools sysfs attribute > is created along with the first pool. (Use that instead of slabinfo > for those pool allocations.) That's why the normal spot to create and > destroy dma pools is in driver probe() and remove() methods. OK, then I misread drivers/base/pool.c and my objection to the patch is unfounded. > If you want to allocate gobs of other stuff at the same time, that's > your choice ... but it'd be a separate issue. Cost to create a > dma_pool is significantly less than PAGE_SIZE, and there's no good > reason to allocate or destroy those pools anywhere near an IRQ context. I agree. raw1394 does far too much with irqs disabled, and moving this stuff to probe() will fix part of that problem. Jody > > - Dave > - > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to [EMAIL PROTECTED] > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Saturday 19 February 2005 12:50 pm, Parag Warudkar wrote: > On Saturday 19 February 2005 02:36 pm, David Brownell wrote: > > The cost of creating the dma_pool is the cost of one small kmalloc() > > plus (the expensive part) the /sys/devices/.../pools sysfs attribute > > is created along with the first pool. (Use that instead of slabinfo > > for those pool allocations.) That's why the normal spot to create and > > destroy dma pools is in driver probe() and remove() methods. > > What's the format of /sys/devices/.../pools (Name of pool, ? ? ? ?) ? Can > the > memory consumption be derived from it? See what drivers/base/dmapool.c tells you; yes that consumption can be derived from the pool statistics. > Here is what the ohci pools look during data read (Kino->Capture) and after > closing Kino - > > During Kino Capture > [EMAIL PROTECTED] pci:00]# cat ./:00:0a.0/:02:00.0/pools > poolinfo - 0.1 > ohci1394 rcv prg 16 256 16 1 --> This one is in question > ohci1394 trm prg 32 64 64 1 > ohci1394 trm prg 32 64 64 1 > ohci1394 rcv prg4 256 16 1 > ohci1394 rcv prg4 256 16 1 I suggest you get rid of the spaces in those names, to make it easier to use things like "awk" to massage those numbers. The "ohci1394" is implied by the fact that no other driver could be bound to that device, for example. In this case, other than the fact that you've created multiple pools with the same names (!), that line translates to 16 blocks in use out of 256 available, 16 bytes per block, just one page. I suspect that on some systems it should be bumping up the minimum block size to match the CPU cacheline size. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Saturday 19 February 2005 02:36 pm, David Brownell wrote: > The cost of creating the dma_pool is the cost of one small kmalloc() > plus (the expensive part) the /sys/devices/.../pools sysfs attribute > is created along with the first pool. (Use that instead of slabinfo > for those pool allocations.) That's why the normal spot to create and > destroy dma pools is in driver probe() and remove() methods. What's the format of /sys/devices/.../pools (Name of pool, ? ? ? ?) ? Can the memory consumption be derived from it? Here is what the ohci pools look during data read (Kino->Capture) and after closing Kino - During Kino Capture [EMAIL PROTECTED] pci:00]# cat ./:00:0a.0/:02:00.0/pools poolinfo - 0.1 ohci1394 rcv prg 16 256 16 1 --> This one is in question ohci1394 trm prg 32 64 64 1 ohci1394 trm prg 32 64 64 1 ohci1394 rcv prg4 256 16 1 ohci1394 rcv prg4 256 16 1 After Closing Kino [EMAIL PROTECTED] pci:00]# cat ./:00:0a.0/:02:00.0/pools poolinfo - 0.1 ohci1394 trm prg 32 64 64 1 ohci1394 trm prg 32 64 64 1 ohci1394 rcv prg4 256 16 1 ohci1394 rcv prg4 256 16 1 Parag - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
> > Jody - Is the 200K waste for sure or do you want me to verify it by some > > means? ( Reason I am asking is firstly, Dave Brownell was quite sure it > > wasn't that costly and secondly, I am hoping it isn't.. ;) > > I'm not sure, but I looked through the code and it seems to allocate: > - 16 buffers of 2x PAGE_SIZE (= 131072 on i386) > - 16 buffers of PAGE_SIZE (= 65536 on i386) > - various other smaller structures. > > I'm not sure how to actually _measure_ how much memory this is using. > slabinfo isn't useful, at least on my system, because the 1394 > allocations get lost in the noise of other activity. Those allocations could be from _using_ a dma pool ... but they're not from just creating one! The cost of creating the dma_pool is the cost of one small kmalloc() plus (the expensive part) the /sys/devices/.../pools sysfs attribute is created along with the first pool. (Use that instead of slabinfo for those pool allocations.) That's why the normal spot to create and destroy dma pools is in driver probe() and remove() methods. If you want to allocate gobs of other stuff at the same time, that's your choice ... but it'd be a separate issue. Cost to create a dma_pool is significantly less than PAGE_SIZE, and there's no good reason to allocate or destroy those pools anywhere near an IRQ context. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Saturday 19 February 2005 01:36 am, Jody McIntyre wrote: > I disagree because the impact of this bug is small. How often do you start > an ISO receive? If you think it needs to be fixed urgently, please > explain why - maybe I'm just missing somethnig. > I have to agree that the impact is small even for the people using ISO recv - I happen to use it quite frequently and it hasn't locked up on me yet. So I certainly don't need it fixed atm. It's just the "dmesg annoyance" if you will, to deal with :) ! > I'm not sure, but I looked through the code and it seems to allocate: > - 16 buffers of 2x PAGE_SIZE (= 131072 on i386) > - 16 buffers of PAGE_SIZE (= 65536 on i386) > - various other smaller structures. OTOH, if it allocates so much of memory while irqs disabled and holding locks, isn't there a good chance for the allocator to sleep and things to go wrong? Parag - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Fri, Feb 18, 2005 at 10:42:46AM -0500, Parag Warudkar wrote: > On Friday 18 February 2005 10:32 am, Dan Dennedy wrote: > > I have tested the patches (including for allocation), and it is working > > great, but should I only commit for now the deallocation patch? Hmm.. > > which is worse the debug or the 200K waste? > Thanks for following it up. > > IMHO, we should commit both patches for now since we don't have an > alternative > solution yet. I disagree because the impact of this bug is small. How often do you start an ISO receive? If you think it needs to be fixed urgently, please explain why - maybe I'm just missing somethnig. > Jody - Is the 200K waste for sure or do you want me to verify it by some > means? ( Reason I am asking is firstly, Dave Brownell was quite sure it > wasn't that costly and secondly, I am hoping it isn't.. ;) I'm not sure, but I looked through the code and it seems to allocate: - 16 buffers of 2x PAGE_SIZE (= 131072 on i386) - 16 buffers of PAGE_SIZE (= 65536 on i386) - various other smaller structures. I'm not sure how to actually _measure_ how much memory this is using. slabinfo isn't useful, at least on my system, because the 1394 allocations get lost in the noise of other activity. If you really need this fixed quickly, I'll find some time this weekend to examine the locks. In particular, I'm not sure what host_info_lock protects or why it needs to be held in so many places with irqs disabled. Jody > > Parag -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Friday 18 February 2005 10:32 am, Dan Dennedy wrote: > I have tested the patches (including for allocation), and it is working > great, but should I only commit for now the deallocation patch? Hmm.. > which is worse the debug or the 200K waste? Thanks for following it up. IMHO, we should commit both patches for now since we don't have an alternative solution yet. Jody - Is the 200K waste for sure or do you want me to verify it by some means? ( Reason I am asking is firstly, Dave Brownell was quite sure it wasn't that costly and secondly, I am hoping it isn't.. ;) Parag - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
I have tested the patches (including for allocation), and it is working great, but should I only commit for now the deallocation patch? Hmm.. which is worse the debug or the 200K waste? On Fri, 2005-02-11 at 22:54 -0500, Parag Warudkar wrote: > Jody, > This happens every time you connect a device which ends up doing > ISO_LISTEN_CHANNEL. We fixed the device disconnect case in -mm recently. > > I had sent you and Andrew an alternative patch which fixes this > dma_pool_create case as well as the dma_pool_destroy case, albeit with a > disadvantage - The patch does pre-allocation of the IR Legacy DMA in > _pci_probe and deallocates it in _pci_remove. However I am not truly > happy with it since it possibly wastes 200K of memory for people who > don't have devices which need it. > > As I said earlier, I think the way to fix this is via schedule_work > similar to the disconnect case, but it involves good amount of code > change. I am working on it - any better ideas most welcome. > > Dan - Can you try the attached patch - on top current -mm1? (It's pretty > no brainer that it will fix both cases but two testing heads are better > than one.. :) > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Jody, This happens every time you connect a device which ends up doing ISO_LISTEN_CHANNEL. We fixed the device disconnect case in -mm recently. I had sent you and Andrew an alternative patch which fixes this dma_pool_create case as well as the dma_pool_destroy case, albeit with a disadvantage - The patch does pre-allocation of the IR Legacy DMA in _pci_probe and deallocates it in _pci_remove. However I am not truly happy with it since it possibly wastes 200K of memory for people who don't have devices which need it. As I said earlier, I think the way to fix this is via schedule_work similar to the disconnect case, but it involves good amount of code change. I am working on it - any better ideas most welcome. Dan - Can you try the attached patch - on top current -mm1? (It's pretty no brainer that it will fix both cases but two testing heads are better than one.. :) Thanks Parag On Fri, 2005-02-11 at 13:43 -0500, Jody McIntyre wrote: > On Fri, Feb 11, 2005 at 10:35:33AM -0500, Dan Dennedy wrote: > > > I am testing this patch in the same manner as you: exiting Kino capture. > > I am getting a similar error in a different location. Can you look into > > it, please? > > > > Debug: sleeping function called from invalid context at mm/slab.c:2082 > > in_atomic():1, irqs_disabled():1 > > [] __might_sleep+0xa1/0xc0 > > [] kmem_cache_alloc+0x64/0x80 > > [] dma_pool_create+0x7b/0x190 > > [] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394] > > [] ohci_devctl+0x3d9/0x640 [ohci1394] > > [] handle_iso_listen+0xee/0x160 [raw1394] > > [] state_connected+0x2de/0x2f0 [raw1394] > > [] raw1394_write+0xae/0xe0 [raw1394] > > [] vfs_write+0x14c/0x160 > > [] sys_write+0x51/0x80 > > [] sysenter_past_esp+0x52/0x75 > > Does this happen on exit or on startup? Looks like allocation problems, > which will be harder to fix, since you can't return to userland until > the allocation is complete. AFAICT the correct fix is to use > finer-grained locks, hold them for less time, and not use _irq or > _irqsave unless necessary. host_info_lock, in particular, is held for > far too long. > > Jody > > > > > > > > > > > --- > > SF email is sponsored by - The IT Product Guide > > Read honest & candid reviews on hundreds of IT Products from real users. > > Discover which products truly live up to the hype. Start reading now. > > http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click > > ___ > > mailing list [EMAIL PROTECTED] > > https://lists.sourceforge.net/lists/listinfo/linux1394-devel > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Fri, Feb 11, 2005 at 10:35:33AM -0500, Dan Dennedy wrote: > I am testing this patch in the same manner as you: exiting Kino capture. > I am getting a similar error in a different location. Can you look into > it, please? > > Debug: sleeping function called from invalid context at mm/slab.c:2082 > in_atomic():1, irqs_disabled():1 > [] __might_sleep+0xa1/0xc0 > [] kmem_cache_alloc+0x64/0x80 > [] dma_pool_create+0x7b/0x190 > [] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394] > [] ohci_devctl+0x3d9/0x640 [ohci1394] > [] handle_iso_listen+0xee/0x160 [raw1394] > [] state_connected+0x2de/0x2f0 [raw1394] > [] raw1394_write+0xae/0xe0 [raw1394] > [] vfs_write+0x14c/0x160 > [] sys_write+0x51/0x80 > [] sysenter_past_esp+0x52/0x75 Does this happen on exit or on startup? Looks like allocation problems, which will be harder to fix, since you can't return to userland until the allocation is complete. AFAICT the correct fix is to use finer-grained locks, hold them for less time, and not use _irq or _irqsave unless necessary. host_info_lock, in particular, is held for far too long. Jody > > > > > --- > SF email is sponsored by - The IT Product Guide > Read honest & candid reviews on hundreds of IT Products from real users. > Discover which products truly live up to the hype. Start reading now. > http://ads.osdn.com/?ad_id=6595&alloc_id=14396&op=click > ___ > mailing list [EMAIL PROTECTED] > https://lists.sourceforge.net/lists/listinfo/linux1394-devel -- - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
On Sun, 2005-01-30 at 20:19 -0500, Parag Warudkar wrote: > Attached is the reworked patch to take care of Andrew's suggestions - > > 1) Allocate the work struct dynamically in struct ti_ohci during device > probe, free it during device remove > 2) In ohci1394_pci_remove, ensure queued work, if any, is flushed before > the device is removed (As I understand, this should work for both device > removal cases as well as module removal - correct?) > 3) Rework the free_dma_rcv_ctx - It is called in multiple contexts by > different callers - teach it to free the dma according to caller's wish > - either direct free where caller determines the context is safe to > sleep OR delayed via schedule_work when caller wants to call it from a > context where it cannot sleep. So it now takes 2 extra arguments - > method to free - direct or delayed and if it is to be freed delayed, an > appropriate work_struct. > > Andrew - flush_scheduled_work does not touch work which isn't shceduled > - so I don't think INIT_WORK in setup is necessary. Let me know if I am > missing something here. (I tried INIT_WORK in ochi1394_pci_probe and > putting flush_scheduled_work in ohci1394_pci_remove - It did not result > in calling the work function after I did rmmod, since it wasn't scheduled.) > I am testing this patch in the same manner as you: exiting Kino capture. I am getting a similar error in a different location. Can you look into it, please? Debug: sleeping function called from invalid context at mm/slab.c:2082 in_atomic():1, irqs_disabled():1 [] __might_sleep+0xa1/0xc0 [] kmem_cache_alloc+0x64/0x80 [] dma_pool_create+0x7b/0x190 [] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394] [] ohci_devctl+0x3d9/0x640 [ohci1394] [] handle_iso_listen+0xee/0x160 [raw1394] [] state_connected+0x2de/0x2f0 [raw1394] [] raw1394_write+0xae/0xe0 [raw1394] [] vfs_write+0x14c/0x160 [] sys_write+0x51/0x80 [] sysenter_past_esp+0x52/0x75 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Forgot to attach ohci.h diff which is affected by this change as well.
Meanwhile, David Brownell suggested it would be much simpler to move the
dma allocations to _probe and deallocations to _remove.
I am working on it and so far haven't got it to work.
Andrew - What do you think? If you too feel allocating pci_pool for the
legacy contexts (only disadvantage I see is that the pool will be
allocated even if it is not used, that is if ISO_LISTEN_CHANNEL isn't
called any time) in _probe an removing it in _remove is a better thing
to do, I will concentrate on getting that to work and we can forget
about this patch. I feel there might be some reason why the current code
treats the legacy IR and IT DMA ctxs differently (on-demand
allocation).. Ben?
Parag
Parag Warudkar wrote:
Attached is the reworked patch to take care of Andrew's suggestions -
1) Allocate the work struct dynamically in struct ti_ohci during
device probe, free it during device remove
2) In ohci1394_pci_remove, ensure queued work, if any, is flushed
before the device is removed (As I understand, this should work for
both device removal cases as well as module removal - correct?)
3) Rework the free_dma_rcv_ctx - It is called in multiple contexts by
different callers - teach it to free the dma according to caller's
wish - either direct free where caller determines the context is safe
to sleep OR delayed via schedule_work when caller wants to call it
from a context where it cannot sleep. So it now takes 2 extra
arguments - method to free - direct or delayed and if it is to be
freed delayed, an appropriate work_struct.
Andrew - flush_scheduled_work does not touch work which isn't
shceduled - so I don't think INIT_WORK in setup is necessary. Let me
know if I am missing something here. (I tried INIT_WORK in
ochi1394_pci_probe and putting flush_scheduled_work in
ohci1394_pci_remove - It did not result in calling the work function
after I did rmmod, since it wasn't scheduled.)
Parag
--- drivers/ieee1394/ohci1394.c.orig2004-12-24 16:35:25.0 -0500
+++ drivers/ieee1394/ohci1394.c 2005-01-30 20:00:42.0 -0500
@@ -99,6 +99,7 @@
#include
#include
#include
+#include
#include
#include
@@ -161,6 +162,10 @@
#define PRINT(level, fmt, args...) \
printk(level "%s: fw-host%d: " fmt "\n" , OHCI1394_DRIVER_NAME, ohci->host->id
, ## args)
+/* Instruct free_dma_rcv_ctx how to free dma pools */
+#define OHCI_FREE_DMA_CTX_DIRECT 0
+#define OHCI_FREE_DMA_CTX_DELAYED 1
+
static char version[] __devinitdata =
"$Rev: 1223 $ Ben Collins <[EMAIL PROTECTED]>";
@@ -176,7 +181,8 @@
enum context_type type, int ctx, int num_desc,
int buf_size, int split_buf_size, int
context_base);
static void stop_dma_rcv_ctx(struct dma_rcv_ctx *d);
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d);
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d , int method,
+struct work_struct* work);
static int alloc_dma_trm_ctx(struct ti_ohci *ohci, struct dma_trm_ctx *d,
enum context_type type, int ctx, int num_desc,
@@ -184,6 +190,8 @@
static void ohci1394_pci_remove(struct pci_dev *pdev);
+static void ohci_free_irlc_pci_pool(void* data);
+
#ifndef __LITTLE_ENDIAN
static unsigned hdr_sizes[] =
{
@@ -1117,7 +1125,8 @@
if (ohci->ir_legacy_channels == 0) {
stop_dma_rcv_ctx(&ohci->ir_legacy_context);
- free_dma_rcv_ctx(&ohci->ir_legacy_context);
+ INIT_WORK(ohci->irlc_free_dma, ohci_free_irlc_pci_pool,
ohci->ir_legacy_context.prg_pool);
+ free_dma_rcv_ctx(&ohci->ir_legacy_context,
OHCI_FREE_DMA_CTX_DELAYED, ohci->irlc_free_dma);
DBGMSG("ISO receive legacy context deactivated");
}
break;
@@ -2876,7 +2885,8 @@
}
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d)
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d, int method,
+ struct work_struct* work)
{
int i;
struct ti_ohci *ohci = d->ohci;
@@ -2903,8 +2913,20 @@
pci_pool_free(d->prg_pool, d->prg_cpu[i],
d->prg_bus[i]);
OHCI_DMA_FREE("consistent dma_rcv prg[%d]", i);
}
- pci_pool_destroy(d->prg_pool);
- OHCI_DMA_FREE("dma_rcv prg pool");
+ if(method == OHCI_FREE_DMA_CTX_DIRECT)
+ {
+ pci_pool_destroy(d->prg_pool);
+ OHCI_DMA_FREE("dma_rcv prg pool");
+ }
+
+ else if(method == OHCI_FREE_DMA_CTX_DELAYED)
+ {
+ schedule_work(work);
+ }
+
+ else
+ PRINT(KERN_ERR, "Invalid method passed - %d", method);
+
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Attached is the reworked patch to take care of Andrew's suggestions -
1) Allocate the work struct dynamically in struct ti_ohci during device
probe, free it during device remove
2) In ohci1394_pci_remove, ensure queued work, if any, is flushed before
the device is removed (As I understand, this should work for both device
removal cases as well as module removal - correct?)
3) Rework the free_dma_rcv_ctx - It is called in multiple contexts by
different callers - teach it to free the dma according to caller's wish
- either direct free where caller determines the context is safe to
sleep OR delayed via schedule_work when caller wants to call it from a
context where it cannot sleep. So it now takes 2 extra arguments -
method to free - direct or delayed and if it is to be freed delayed, an
appropriate work_struct.
Andrew - flush_scheduled_work does not touch work which isn't shceduled
- so I don't think INIT_WORK in setup is necessary. Let me know if I am
missing something here. (I tried INIT_WORK in ochi1394_pci_probe and
putting flush_scheduled_work in ohci1394_pci_remove - It did not result
in calling the work function after I did rmmod, since it wasn't scheduled.)
Parag
--- drivers/ieee1394/ohci1394.c.orig2004-12-24 16:35:25.0 -0500
+++ drivers/ieee1394/ohci1394.c 2005-01-30 20:00:42.0 -0500
@@ -99,6 +99,7 @@
#include
#include
#include
+#include
#include
#include
@@ -161,6 +162,10 @@
#define PRINT(level, fmt, args...) \
printk(level "%s: fw-host%d: " fmt "\n" , OHCI1394_DRIVER_NAME, ohci->host->id
, ## args)
+/* Instruct free_dma_rcv_ctx how to free dma pools */
+#define OHCI_FREE_DMA_CTX_DIRECT 0
+#define OHCI_FREE_DMA_CTX_DELAYED 1
+
static char version[] __devinitdata =
"$Rev: 1223 $ Ben Collins <[EMAIL PROTECTED]>";
@@ -176,7 +181,8 @@
enum context_type type, int ctx, int num_desc,
int buf_size, int split_buf_size, int
context_base);
static void stop_dma_rcv_ctx(struct dma_rcv_ctx *d);
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d);
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d , int method,
+struct work_struct* work);
static int alloc_dma_trm_ctx(struct ti_ohci *ohci, struct dma_trm_ctx *d,
enum context_type type, int ctx, int num_desc,
@@ -184,6 +190,8 @@
static void ohci1394_pci_remove(struct pci_dev *pdev);
+static void ohci_free_irlc_pci_pool(void* data);
+
#ifndef __LITTLE_ENDIAN
static unsigned hdr_sizes[] =
{
@@ -1117,7 +1125,8 @@
if (ohci->ir_legacy_channels == 0) {
stop_dma_rcv_ctx(&ohci->ir_legacy_context);
- free_dma_rcv_ctx(&ohci->ir_legacy_context);
+ INIT_WORK(ohci->irlc_free_dma, ohci_free_irlc_pci_pool,
ohci->ir_legacy_context.prg_pool);
+ free_dma_rcv_ctx(&ohci->ir_legacy_context,
OHCI_FREE_DMA_CTX_DELAYED, ohci->irlc_free_dma);
DBGMSG("ISO receive legacy context deactivated");
}
break;
@@ -2876,7 +2885,8 @@
}
-static void free_dma_rcv_ctx(struct dma_rcv_ctx *d)
+static void free_dma_rcv_ctx(struct dma_rcv_ctx *d, int method,
+ struct work_struct* work)
{
int i;
struct ti_ohci *ohci = d->ohci;
@@ -2903,8 +2913,20 @@
pci_pool_free(d->prg_pool, d->prg_cpu[i],
d->prg_bus[i]);
OHCI_DMA_FREE("consistent dma_rcv prg[%d]", i);
}
- pci_pool_destroy(d->prg_pool);
- OHCI_DMA_FREE("dma_rcv prg pool");
+ if(method == OHCI_FREE_DMA_CTX_DIRECT)
+ {
+ pci_pool_destroy(d->prg_pool);
+ OHCI_DMA_FREE("dma_rcv prg pool");
+ }
+
+ else if(method == OHCI_FREE_DMA_CTX_DELAYED)
+ {
+ schedule_work(work);
+ }
+
+ else
+ PRINT(KERN_ERR, "Invalid method passed - %d", method);
+
kfree(d->prg_cpu);
kfree(d->prg_bus);
}
@@ -2938,7 +2960,7 @@
if (d->buf_cpu == NULL || d->buf_bus == NULL) {
PRINT(KERN_ERR, "Failed to allocate dma buffer");
- free_dma_rcv_ctx(d);
+ free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
return -ENOMEM;
}
memset(d->buf_cpu, 0, d->num_desc * sizeof(quadlet_t*));
@@ -2950,7 +2972,7 @@
if (d->prg_cpu == NULL || d->prg_bus == NULL) {
PRINT(KERN_ERR, "Failed to allocate dma prg");
- free_dma_rcv_ctx(d);
+ free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
return -ENOMEM;
}
memset(d->prg_cpu, 0, d->num_desc * sizeof(struct dma_cmd*));
@@ -2960,7 +2982,7 @@
i
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Parag Warudkar <[EMAIL PROTECTED]> wrote: > > Andrew Morton wrote: > > ... > >- We'll need a flush_workqueue() in the teardown function for that data > > structure to ensure that any pending callbacks have completed before we > > free the storage. > > > > > By saying flush_workqueue did you intend to suggest using separate work > queue for ohci1394? No. Using keventd and flush_scheduled_work() should be OK in this application. rmmod isn't very common. > > Care needs to be taken to ensure that the work_struct is suitably > > initialised so that the flush_workqueue() will work OK even if the > > callback has never been scheduled. > > > > > Didn't understand this one - Is this about properly NULL'ing elements > in work queue so flush_workqueue doesn't touch them? Can you elaborate > please? Well, it's just that the shutdown code needs to run flush_scheduled_work() or flush_workqueue() on a work_struct which may or may not have been used. So we need to ensure that it has sane contents. Just run INIT_WORK() on it in the setup code. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Andrew Morton wrote: yup. But what happens if someone removes the module while ohci_free_dma_work_fn() is still pending? Suggestions: - The work_struct cannot be on the stack. The code as you have it will read gunk from the stack when the delayed work executes. The work_struct needs to be placed into some ohci data structure which has the appropriate lifetime. That might be struct ti_ohci. Or not. Ok, got it. - We'll need a flush_workqueue() in the teardown function for that data structure to ensure that any pending callbacks have completed before we free the storage. By saying flush_workqueue did you intend to suggest using separate work queue for ohci1394? I think that might be the way to go since otherwise ohci1394 would have to wait on all other irrelevant work elements in the global queue to be finished. This will help in solving the other problem of dma_pool_create as well - we could then schedule_work for all create_pools and just do a flush_workqueue before starting to actually use the device. (Might help in reducing those sound skips when I attach my camcorder :) Error handling has to change a good amount (right now the init function returns ENOMEM if dma_pool_create fails and all is done. With this approach of asynch alloc , init function will not know if the allocation failed / succeeded. All other functions (like say resulting from sys_read) will have to explicitly check and return error to user if the pool create had failed.) Care needs to be taken to ensure that the work_struct is suitably initialised so that the flush_workqueue() will work OK even if the callback has never been scheduled. Didn't understand this one - Is this about properly NULL'ing elements in work queue so flush_workqueue doesn't touch them? Can you elaborate please? - You have several typecasts between struct pci_pool* and void*. These defeat typechecking. It's better to leave these casts out. Will leave them out. Thanks Parag - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Parag Warudkar <[EMAIL PROTECTED]> wrote: > > Problem - ohci1394.c:ohci_devctl ends up calling dma_pool_destroy from > invalid context. Below is the dmesg output when I exit Kino after video > capture - > > Debug: sleeping function called from invalid context at > include/asm/semaphore.h:107 > in_atomic():1, irqs_disabled():1 > [] dump_stack+0x1e/0x20 > [] __might_sleep+0xa2/0xc0 > [] dma_pool_destroy+0x20/0x140 > [] free_dma_rcv_ctx+0x8e/0x150 [ohci1394] > [] ohci_devctl+0x214/0x9b0 [ohci1394] > [] handle_iso_listen+0x2d9/0x310 [raw1394] > [] state_connected+0x29b/0x2b0 [raw1394] > [] raw1394_write+0x9e/0xd0 [raw1394] > [] vfs_write+0xc2/0x170 > [] sys_write+0x4b/0x80 > [] sysenter_past_esp+0x52/0x75 Yes, that's certainly wrong. > Attached patch against 2.6.11-rc2 (tested to work normally with a > camcorder device) enables ohci_devctl to defer the work of destroying > the dma pool by using a work queue, so the dma pool is destroyed in a > valid context and we no longer get an error in dmesg (duh :) yup. But what happens if someone removes the module while ohci_free_dma_work_fn() is still pending? Suggestions: - The work_struct cannot be on the stack. The code as you have it will read gunk from the stack when the delayed work executes. The work_struct needs to be placed into some ohci data structure which has the appropriate lifetime. That might be struct ti_ohci. Or not. - We'll need a flush_workqueue() in the teardown function for that data structure to ensure that any pending callbacks have completed before we free the storage. Care needs to be taken to ensure that the work_struct is suitably initialised so that the flush_workqueue() will work OK even if the callback has never been scheduled. - You have several typecasts between struct pci_pool* and void*. These defeat typechecking. It's better to leave these casts out. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] ohci1394: dma_pool_destroy while in_atomic() && irqs_disabled()
Problem - ohci1394.c:ohci_devctl ends up calling dma_pool_destroy from
invalid context. Below is the dmesg output when I exit Kino after video
capture -
Debug: sleeping function called from invalid context at
include/asm/semaphore.h:107
in_atomic():1, irqs_disabled():1
[] dump_stack+0x1e/0x20
[] __might_sleep+0xa2/0xc0
[] dma_pool_destroy+0x20/0x140
[] free_dma_rcv_ctx+0x8e/0x150 [ohci1394]
[] ohci_devctl+0x214/0x9b0 [ohci1394]
[] handle_iso_listen+0x2d9/0x310 [raw1394]
[] state_connected+0x29b/0x2b0 [raw1394]
[] raw1394_write+0x9e/0xd0 [raw1394]
[] vfs_write+0xc2/0x170
[] sys_write+0x4b/0x80
[] sysenter_past_esp+0x52/0x75
Attached patch against 2.6.11-rc2 (tested to work normally with a
camcorder device) enables ohci_devctl to defer the work of destroying
the dma pool by using a work queue, so the dma pool is destroyed in a
valid context and we no longer get an error in dmesg (duh :)
Note : There still exists one more similar problem with ohci1394 - that
is with dma_pool_create being called from invalid context - this happens
in response to ISO_LISTEN_CHANNEL. I get dmesg debug for this case which
is similar to the above. My analysis is that this one is non-trivial to
fix. More on this in a separate mail.
Patch is attached since T'Bird messes up with the inlined one. (Pls.
suggest a good email client for kernel patch stuff :)
Signed-off-by: Parag Warudkar ([EMAIL PROTECTED])
Parag
--- drivers/ieee1394/ohci1394.c.orig2004-12-24 16:35:25.0 -0500
+++ drivers/ieee1394/ohci1394.c 2005-01-30 15:46:34.0 -0500
@@ -99,6 +99,7 @@
#include
#include
#include
+#include
#include
#include
@@ -164,6 +165,7 @@
static char version[] __devinitdata =
"$Rev: 1223 $ Ben Collins <[EMAIL PROTECTED]>";
+
/* Module Parameters */
static int phys_dma = 1;
module_param(phys_dma, int, 0644);
@@ -184,6 +186,8 @@
static void ohci1394_pci_remove(struct pci_dev *pdev);
+static void ohci_free_dma_work_fn(void* data);
+
#ifndef __LITTLE_ENDIAN
static unsigned hdr_sizes[] =
{
@@ -1130,6 +1134,13 @@
return retval;
}
+static void ohci_free_dma_work_fn(void* data)
+{
+ struct pci_pool* prg_pool = (struct pci_pool*) data;
+ pci_pool_destroy(prg_pool);
+ OHCI_DMA_FREE("dma_rcv prg pool");
+}
+
/***
* rawiso ISO reception*
***/
@@ -2898,13 +2909,14 @@
kfree(d->buf_bus);
}
if (d->prg_cpu) {
+ struct work_struct ohci_free_dma_work;
for (i=0; inum_desc; i++)
if (d->prg_cpu[i] && d->prg_bus[i]) {
pci_pool_free(d->prg_pool, d->prg_cpu[i],
d->prg_bus[i]);
OHCI_DMA_FREE("consistent dma_rcv prg[%d]", i);
}
- pci_pool_destroy(d->prg_pool);
- OHCI_DMA_FREE("dma_rcv prg pool");
+ INIT_WORK(&ohci_free_dma_work, ohci_free_dma_work_fn, (void*)
d->prg_pool);
+ schedule_work(&ohci_free_dma_work);
kfree(d->prg_cpu);
kfree(d->prg_bus);
}
