| Message ID | 20110528205701.GA1788@doriath.ww600.siemens.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 31 May 2011, Russell King - ARM Linux wrote: > Enabling CONFIG_ZONE_DMA is not a fix, it's making the problem vanish off > the radar. It will mean that the drivers using GFP_DMA will never get > fixed up. > Disabling CONFIG_ZONE_DMA is an optimization, do you agree? I asked on Sunday what the downsides of enabling the option on arm are, and you didn't mention any. So what's the problem with my patch to enable it for this driver since it is using GFP_DMA? You claim that it'll never get removed again to return to the _optimized_ case, yet my patch is guaranteed to work for that driver in all configurations at the moment. I don't think we should be fighting for the optimized case where the alternative has no downside. [ Patching the page allocator to also emit a line to the dmesg to direct users directly to enabling CONFIG_ZONE_DMA wouldn't be a problem, either. ] > Change it to be a soft WARN_ON for one release. Anything else will just > result in the problem being IGNORED. > The problem certainly isn't being ignored in this thread, or in the patch that I sent to fix Dmitry's issue by default, so that doesn't seem to be the case. What would be ignored, though, is if it just emitted a WARN_ON() without failing the allocation so everything works perfectly.
On Tue, May 31, 2011 at 01:03:03PM -0700, David Rientjes wrote: > The problem certainly isn't being ignored in this thread, or in the patch > that I sent to fix Dmitry's issue by default, so that doesn't seem to be > the case. What would be ignored, though, is if it just emitted a > WARN_ON() without failing the allocation so everything works perfectly. Sorry, you did not send a patch to fix it. You sent a *bodge* to enable the DMA zone. As long as you insist that's a valid fix, you're going to carry zero credibility with me. The fact is that this driver should not be using GFP_DMA to allocate things which aren't even DMA buffers, and its use of GFP_DMA should be removed. But rather than look at that and work it out, and then produce a patch to sort that out, the only thing you can do is come up with bodge to enable the DMA zone, and continue to insist that's the right solution. I repeat, enabling the DMA zone for this driver is a pure and utter bodge, and the change to make these allocations fail _will_ and _has_ caused regressions. Your whinge that we should re-enable the DMA zone which has been disabled for quite a long time now to work around this new restriction is extremely idiotic. Do the right thing. Make allocations to GFP_DMA zones _warn_ first for a cycle so that affected drivers can be fixed. Then for the next cycle, make it a hard failure.
On Tue, 31 May 2011, Russell King - ARM Linux wrote: > > The problem certainly isn't being ignored in this thread, or in the patch > > that I sent to fix Dmitry's issue by default, so that doesn't seem to be > > the case. What would be ignored, though, is if it just emitted a > > WARN_ON() without failing the allocation so everything works perfectly. > > Sorry, you did not send a patch to fix it. You sent a *bodge* to enable > the DMA zone. As long as you insist that's a valid fix, you're going > to carry zero credibility with me. > > The fact is that this driver should not be using GFP_DMA to allocate > things which aren't even DMA buffers, and its use of GFP_DMA should be > removed. But rather than look at that and work it out, and then produce > a patch to sort that out, the only thing you can do is come up with > bodge to enable the DMA zone, and continue to insist that's the right > solution. > I'm not going to hack on an arm driver when I have no idea what its DMA requirements are and have no ability to test the change. I'll rely on the authors or maintainers of that driver to figure it out. In the meantime, I suggested enabling CONFIG_ZONE_DMA for that driver because, in its current state, it is using GFP_DMA and you haven't provided a single reason why enabling CONFIG_ZONE_DMA is going to cause an issue. In other words, I cannot do your work for you: if GFP_DMA can be removed from that driver, great, in the meantime it should enable CONFIG_ZONE_DMA to fix the invalid configurations that are currently allowed. It's simple: if you're going to pass GFP_DMA to the page allocator, you need to require CONFIG_ZONE_DMA for it to be useful. Anything else is an invalid configuration and is error prone. > Your whinge that we should re-enable the DMA zone which has been > disabled for quite a long time now to work around this new restriction > is extremely idiotic. > The restriction isn't new: GFP_DMA only makes sense with CONFIG_ZONE_DMA. The fact that the page allocator completely ignored GFP_DMA in the past for CONFIG_ZONE_DMA=n doesn't change that. That's obviously error prone since it will return memory from anywhere simply because of the fact that it is an invalid configuration.
On Tue, May 31, 2011 at 03:11:40PM -0700, David Rientjes wrote: > The restriction isn't new: GFP_DMA only makes sense with CONFIG_ZONE_DMA. > The fact that the page allocator completely ignored GFP_DMA in the past > for CONFIG_ZONE_DMA=n doesn't change that. That's obviously error prone > since it will return memory from anywhere simply because of the fact that > it is an invalid configuration. Your approach to this is wrong. Make it warn for one release. Give people a chance to fix things before they become a regression. Then make it a hard failure.
On Wed, Jun 01, 2011 at 08:54:20AM +0100, Russell King - ARM Linux wrote: > On Tue, May 31, 2011 at 03:11:40PM -0700, David Rientjes wrote: > > The restriction isn't new: GFP_DMA only makes sense with CONFIG_ZONE_DMA. > > The fact that the page allocator completely ignored GFP_DMA in the past > > for CONFIG_ZONE_DMA=n doesn't change that. That's obviously error prone > > since it will return memory from anywhere simply because of the fact that > > it is an invalid configuration. > > Your approach to this is wrong. Make it warn for one release. Give > people a chance to fix things before they become a regression. Then > make it a hard failure. And to prove that its not just this driver, I've now received a report that it fails with the CF PATA driver on Zaurus. Should we make the PATA subsystem select CONFIG_ZONE_DMA too, or should we give people some grace period to fix the drivers as _everyone_ except you is suggesting. Please do the sensible thing. Make it warn for a release like everyone is telling you to.
From 18f033175030a1d42cf58b4930682a35de3e7412 Mon Sep 17 00:00:00 2001 From: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> Date: Sun, 29 May 2011 00:53:29 +0400 Subject: [PATCH] pxa: don't ask for a buffer from DMA zone PXA don't have special DMA zone. And since 197b59ae6e8bee56fcef37ea2482dc08414e2ac (mm: fail GFP_DMA allocations when ZONE_DMA is not configured) allocation with GFP_DMA set will fail with a trace like this: ------------[ cut here ]------------ WARNING: at mm/page_alloc.c:2251 __alloc_pages_nodemask+0xa0/0x5ac() Modules linked in: [<c00385b0>] (unwind_backtrace+0x0/0xf0) from [<c0050b1c>] (warn_slowpath_common+0x4c/0x64) [<c0050b1c>] (warn_slowpath_common+0x4c/0x64) from [<c0050b4c>] (warn_slowpath_null+0x18/0x1c) [<c0050b4c>] (warn_slowpath_null+0x18/0x1c) from [<c00908ec>] (__alloc_pages_nodemask+0xa0/0x5ac) [<c00908ec>] (__alloc_pages_nodemask+0xa0/0x5ac) from [<c0090e74>] (__get_free_pages+0x10/0x3c) [<c0090e74>] (__get_free_pages+0x10/0x3c) from [<c01d608c>] (pxa_irda_init_iobuf+0x18/0x48) [<c01d608c>] (pxa_irda_init_iobuf+0x18/0x48) from [<c01d61d8>] (pxa_irda_probe+0x11c/0x32c) [<c01d61d8>] (pxa_irda_probe+0x11c/0x32c) from [<c019474c>] (platform_drv_probe+0x14/0x18) [<c019474c>] (platform_drv_probe+0x14/0x18) from [<c0193508>] (really_probe+0xa0/0x158) [<c0193508>] (really_probe+0xa0/0x158) from [<c019360c>] (driver_probe_device+0x4c/0x64) [<c019360c>] (driver_probe_device+0x4c/0x64) from [<c0193684>] (__driver_attach+0x60/0x84) [<c0193684>] (__driver_attach+0x60/0x84) from [<c0192d78>] (bus_for_each_dev+0x48/0x84) [<c0192d78>] (bus_for_each_dev+0x48/0x84) from [<c01926b8>] (bus_add_driver+0xa8/0x220) [<c01926b8>] (bus_add_driver+0xa8/0x220) from [<c0193c7c>] (driver_register+0xac/0x13c) [<c0193c7c>] (driver_register+0xac/0x13c) from [<c0033440>] (do_one_initcall+0x94/0x16c) [<c0033440>] (do_one_initcall+0x94/0x16c) from [<c00083f4>] (kernel_init+0x94/0x140) [<c00083f4>] (kernel_init+0x94/0x140) from [<c00348d0>] (kernel_thread_exit+0x0/0x8) ---[ end trace 0b8bf08f70147098 ]--- Signed-off-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com> --- drivers/net/irda/pxaficp_ir.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/net/irda/pxaficp_ir.c b/drivers/net/irda/pxaficp_ir.c index 001ed0a..a5ebaef 100644 --- a/drivers/net/irda/pxaficp_ir.c +++ b/drivers/net/irda/pxaficp_ir.c @@ -805,7 +805,7 @@ static int pxa_irda_resume(struct platform_device *_dev) static int pxa_irda_init_iobuf(iobuff_t *io, int size) { - io->head = kmalloc(size, GFP_KERNEL | GFP_DMA); + io->head = kmalloc(size, GFP_KERNEL); if (io->head != NULL) { io->truesize = size; io->in_frame = FALSE; -- 1.7.4.4