--- drivers/ieee1394/ohci1394.c.orig 2004-12-24 16:35:25.000000000 -0500
+++ drivers/ieee1394/ohci1394.c 2005-01-30 15:46:34.000000000 -0500
@@ -99,6 +99,7 @@
#include <asm/uaccess.h>
#include <linux/delay.h>
#include <linux/spinlock.h>
+#include <linux/workqueue.h>
#include <asm/pgtable.h>
#include <asm/page.h>
@@ -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; i<d->num_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);
}
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
> [<c0104c2e>] dump_stack+0x1e/0x20
> [<c011f8a2>] __might_sleep+0xa2/0xc0
> [<c028c660>] dma_pool_destroy+0x20/0x140
> [<f0b7affe>] free_dma_rcv_ctx+0x8e/0x150 [ohci1394]
> [<f0b774a4>] ohci_devctl+0x214/0x9b0 [ohci1394]
> [<f0e7aa99>] handle_iso_listen+0x2d9/0x310 [raw1394]
> [<f0e7f22b>] state_connected+0x29b/0x2b0 [raw1394]
> [<f0e7f2de>] raw1394_write+0x9e/0xd0 [raw1394]
> [<c0183ef2>] vfs_write+0xc2/0x170
> [<c018406b>] sys_write+0x4b/0x80
> [<c0103cb1>] 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.
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
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.
--- drivers/ieee1394/ohci1394.c.orig 2004-12-24 16:35:25.000000000 -0500
+++ drivers/ieee1394/ohci1394.c 2005-01-30 20:00:42.000000000 -0500
@@ -99,6 +99,7 @@
#include <asm/uaccess.h>
#include <linux/delay.h>
#include <linux/spinlock.h>
+#include <linux/workqueue.h>
#include <asm/pgtable.h>
#include <asm/page.h>
@@ -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 @@
if (d->spb == NULL) {
PRINT(KERN_ERR, "Failed to allocate split buffer");
- free_dma_rcv_ctx(d);
+ free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
return -ENOMEM;
}
@@ -2979,7 +3001,7 @@
} else {
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;
}
@@ -2991,7 +3013,7 @@
} else {
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;
}
}
@@ -3005,7 +3027,7 @@
if (ohci1394_register_iso_tasklet(ohci,
&ohci->ir_legacy_tasklet) < 0) {
PRINT(KERN_ERR, "No IR DMA context available");
- free_dma_rcv_ctx(d);
+ free_dma_rcv_ctx(d, OHCI_FREE_DMA_CTX_DIRECT, NULL);
return -EBUSY;
}
@@ -3355,6 +3377,7 @@
/* the IR DMA context is allocated on-demand; mark it inactive */
ohci->ir_legacy_context.ohci = NULL;
+ ohci->ir_legacy_context.prg_pool = NULL;
/* same for the IT DMA context */
ohci->it_legacy_context.ohci = NULL;
@@ -3377,16 +3400,32 @@
if (hpsb_add_host(host))
FAIL(-ENOMEM, "Failed to register host with highlevel");
+ ohci->irlc_free_dma = kmalloc(sizeof(struct work_struct), GFP_KERNEL);
+ if(ohci->irlc_free_dma == NULL)
+ FAIL(-ENOMEM, "Failed to allocate memory for ohci->irlc_free_dma");
+
ohci->init_state = OHCI_INIT_DONE;
return 0;
#undef FAIL
}
+static void ohci_free_irlc_pci_pool(void *data)
+{
+ if(data == NULL)
+ return;
+ pci_pool_destroy(data);
+ OHCI_DMA_FREE("dma_rcv prg pool");
+ return;
+}
+
static void ohci1394_pci_remove(struct pci_dev *pdev)
{
struct ti_ohci *ohci;
struct device *dev;
+
+ /* Ensure all dma ctx are freed */
+ flush_scheduled_work();
ohci = pci_get_drvdata(pdev);
if (!ohci)
@@ -3432,18 +3471,21 @@
/* The ohci_soft_reset() stops all DMA contexts, so we
* dont need to do this. */
/* Free AR dma */
- free_dma_rcv_ctx(&ohci->ar_req_context);
- free_dma_rcv_ctx(&ohci->ar_resp_context);
+ free_dma_rcv_ctx(&ohci->ar_req_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
+ free_dma_rcv_ctx(&ohci->ar_resp_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
/* Free AT dma */
free_dma_trm_ctx(&ohci->at_req_context);
free_dma_trm_ctx(&ohci->at_resp_context);
/* Free IR dma */
- free_dma_rcv_ctx(&ohci->ir_legacy_context);
+ free_dma_rcv_ctx(&ohci->ir_legacy_context, OHCI_FREE_DMA_CTX_DIRECT, NULL);
/* Free IT dma */
free_dma_trm_ctx(&ohci->it_legacy_context);
+
+ /* Free work struct for IR Legacy DMA */
+ kfree(ohci->irlc_free_dma);
case OHCI_INIT_HAVE_SELFID_BUFFER:
pci_free_consistent(ohci->dev, OHCI1394_SI_DMA_BUF_SIZE,
--- drivers/ieee1394/ohci1394.h.orig 2004-12-24 16:34:00.000000000 -0500
+++ drivers/ieee1394/ohci1394.h 2005-01-31 18:16:31.000000000 -0500
@@ -197,6 +197,8 @@
it is safe to free the legacy API context */
struct dma_rcv_ctx ir_legacy_context;
+ struct work_struct* irlc_free_dma;
+
struct ohci1394_iso_tasklet ir_legacy_tasklet;
/* iso transmit */
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
[<c0119eb1>] __might_sleep+0xa1/0xc0
[<c0144914>] kmem_cache_alloc+0x64/0x80
[<c037f07b>] dma_pool_create+0x7b/0x190
[<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
[<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
[<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
[<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
[<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
[<c015c80c>] vfs_write+0x14c/0x160
[<c015c8f1>] sys_write+0x51/0x80
[<c0102a39>] sysenter_past_esp+0x52/0x75
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
> [<c0119eb1>] __might_sleep+0xa1/0xc0
> [<c0144914>] kmem_cache_alloc+0x64/0x80
> [<c037f07b>] dma_pool_create+0x7b/0x190
> [<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
> [<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
> [<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
> [<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
> [<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
> [<c015c80c>] vfs_write+0x14c/0x160
> [<c015c8f1>] sys_write+0x51/0x80
> [<c0102a39>] 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
--
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
> > [<c0119eb1>] __might_sleep+0xa1/0xc0
> > [<c0144914>] kmem_cache_alloc+0x64/0x80
> > [<c037f07b>] dma_pool_create+0x7b/0x190
> > [<e09ede32>] alloc_dma_rcv_ctx+0x1a2/0x400 [ohci1394]
> > [<e09eb239>] ohci_devctl+0x3d9/0x640 [ohci1394]
> > [<e0bc5d4e>] handle_iso_listen+0xee/0x160 [raw1394]
> > [<e0bc878e>] state_connected+0x2de/0x2f0 [raw1394]
> > [<e0bc884e>] raw1394_write+0xae/0xe0 [raw1394]
> > [<c015c80c>] vfs_write+0x14c/0x160
> > [<c015c8f1>] sys_write+0x51/0x80
> > [<c0102a39>] 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
>
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.. :)
>
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
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
--
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
> > 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
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
[root@localhost pci0000:00]# cat ./0000:00:0a.0/0000: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 prg 4 256 16 1
ohci1394 rcv prg 4 256 16 1
After Closing Kino
[root@localhost pci0000:00]# cat ./0000:00:0a.0/0000:02:00.0/pools
poolinfo - 0.1
ohci1394 trm prg 32 64 64 1
ohci1394 trm prg 32 64 64 1
ohci1394 rcv prg 4 256 16 1
ohci1394 rcv prg 4 256 16 1
Parag
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
> [root@localhost pci0000:00]# cat ./0000:00:0a.0/0000: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 prg 4 256 16 1
> ohci1394 rcv prg 4 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
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/
--