Please be more polite to other people. After a197b59ae6 all allocations
with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
one warning during bootup is emited, no matter how many things fail).
This is a very crude change on behaviour. To be more civil, instead of
failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
memory on non-ZONE_DMA node.
This change should be reverted after one or two major releases, but
we should be more accurate rather than hoping for the best.
Signed-off-by: Dmitry Eremin-Solenikov <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Russell King - ARM Linux <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: KOSAKI Motohiro <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Andrew Morton <[email protected]>
---
mm/page_alloc.c | 5 +++--
1 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a4e1db3..e22dd4e 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2248,8 +2248,9 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
if (should_fail_alloc_page(gfp_mask, order))
return NULL;
#ifndef CONFIG_ZONE_DMA
- if (WARN_ON_ONCE(gfp_mask & __GFP_DMA))
- return NULL;
+ /* Change this back to hard failure after 3.0 or 3.1. For now give
+ * drivers people a chance to fix their drivers w/o causing breakage. */
+ WARN_ON(gfp_mask & __GFP_DMA);
#endif
/*
--
1.7.4.4
2011/6/1 Dmitry Eremin-Solenikov <[email protected]>:
> Please be more polite to other people. After a197b59ae6 all allocations
> with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> one warning during bootup is emited, no matter how many things fail).
> This is a very crude change on behaviour. To be more civil, instead of
> failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> memory on non-ZONE_DMA node.
>
> This change should be reverted after one or two major releases, but
> we should be more accurate rather than hoping for the best.
>
> Signed-off-by: Dmitry Eremin-Solenikov <[email protected]>
> Cc: David Rientjes <[email protected]>
> Cc: Russell King - ARM Linux <[email protected]>
> Cc: Mel Gorman <[email protected]>
> Cc: KOSAKI Motohiro <[email protected]>
> Cc: KAMEZAWA Hiroyuki <[email protected]>
> Cc: Rik van Riel <[email protected]>
> Cc: Andrew Morton <[email protected]>
Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
DMA_ZONE at all.
and a197b59ae6 only care x86 embedded case. If we accept your patch, I
can imagine
other people will claim warn foold is a bug. ;)
However, I think, you should explain which platform and drivers hit
this breakage.
Otherwise developers can't learn which platform should care.
Thanks.
On 6/1/11, KOSAKI Motohiro <[email protected]> wrote:
> 2011/6/1 Dmitry Eremin-Solenikov <[email protected]>:
>> Please be more polite to other people. After a197b59ae6 all allocations
>> with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
>> one warning during bootup is emited, no matter how many things fail).
>> This is a very crude change on behaviour. To be more civil, instead of
>> failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
>> memory on non-ZONE_DMA node.
>>
>> This change should be reverted after one or two major releases, but
>> we should be more accurate rather than hoping for the best.
>>
>> Signed-off-by: Dmitry Eremin-Solenikov <[email protected]>
>> Cc: David Rientjes <[email protected]>
>> Cc: Russell King - ARM Linux <[email protected]>
>> Cc: Mel Gorman <[email protected]>
>> Cc: KOSAKI Motohiro <[email protected]>
>> Cc: KAMEZAWA Hiroyuki <[email protected]>
>> Cc: Rik van Riel <[email protected]>
>> Cc: Andrew Morton <[email protected]>
>
> Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
> DMA_ZONE at all.
> and a197b59ae6 only care x86 embedded case. If we accept your patch, I
> can imagine
> other people will claim warn foold is a bug. ;)
I think that argument from a197b59ae6 is correct. Allocating with GFP_DMA
should fail if there is no ZONE_DMA. On the other hand linux/gfp.h clearly
specifies: "...Ignored on some platforms, used as appropriate on others".
So it's up to mm gurus to decide which way is correct. I'd be happy as long
as we don't have such nasty change of behaviour.
> However, I think, you should explain which platform and drivers hit
> this breakage.
> Otherwise developers can't learn which platform should care.
I've hit this with IrDA driver on PXA. Also I've seen the report regarding
other ARM platform (ep-something). Thus I've included Russell in the cc.
--
With best wishes
Dmitry
On Wed, 1 Jun 2011, Dmitry Eremin-Solenikov wrote:
> I've hit this with IrDA driver on PXA. Also I've seen the report regarding
> other ARM platform (ep-something). Thus I've included Russell in the cc.
>
So you want to continue to allow the page allocator to return pages from
anywhere, even when GFP_DMA is specified, just as though it was lowmem?
Why don't you actually address the problem with the driver you're
complaining about with the patch below, which I already posted to you a
few days ago?
If this arm driver is going to be using GFP_DMA unconditionally, it better
require CONFIG_ZONE_DMA for it to actually be meaningful until such time
as it can be removed if it's truly not needed or generalized to only
specific pieces of hardware.
---
drivers/net/irda/Kconfig | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
--- a/drivers/net/irda/Kconfig
+++ b/drivers/net/irda/Kconfig
@@ -374,6 +374,7 @@ config VIA_FIR
config PXA_FICP
tristate "Intel PXA2xx Internal FICP"
depends on ARCH_PXA && IRDA
+ select ZONE_DMA
help
Say Y or M here if you want to build support for the PXA2xx
built-in IRDA interface which can support both SIR and FIR.
On Wed, Jun 01, 2011 at 10:23:15AM -0700, David Rientjes wrote:
> On Wed, 1 Jun 2011, Dmitry Eremin-Solenikov wrote:
>
> > I've hit this with IrDA driver on PXA. Also I've seen the report regarding
> > other ARM platform (ep-something). Thus I've included Russell in the cc.
> >
>
> So you want to continue to allow the page allocator to return pages from
> anywhere, even when GFP_DMA is specified, just as though it was lowmem?
No. What *everyone* is asking for is to allow the situation which has
persisted thus far to continue for ONE MORE RELEASE but with a WARNING
so that these problems can be found without causing REGRESSIONS.
That is NOT an unreasonable request, but it seems that its far too much
to ask of you.
On 6/1/11, David Rientjes <[email protected]> wrote:
> On Wed, 1 Jun 2011, Dmitry Eremin-Solenikov wrote:
>
>> I've hit this with IrDA driver on PXA. Also I've seen the report regarding
>> other ARM platform (ep-something). Thus I've included Russell in the cc.
>>
>
> So you want to continue to allow the page allocator to return pages from
> anywhere, even when GFP_DMA is specified, just as though it was lowmem?
Yes and no. I'm asking for the grace period for the drivers authors to be able
to fix their code. After a grace period of one or two majors this permission
should be removed and your original patch should be effective.
> Why don't you actually address the problem with the driver you're
> complaining about with the patch below, which I already posted to you a
> few days ago?
>
> If this arm driver is going to be using GFP_DMA unconditionally, it better
> require CONFIG_ZONE_DMA for it to actually be meaningful until such time
> as it can be removed if it's truly not needed or generalized to only
> specific pieces of hardware.
No. This only workarounds the bug. And also a possible hundred of other bugs
in the PXA/etc. ARM drivers. Instead I'm asking for the way to
visualize all such
bugs.
Do you want to also add such workarounds to some PATA CF driver used on PXA?
To _any_ of the drivers allocating the GFP_DMA memory? Then CONFIG_ZONE_DMA
would serve no purpose. We can as well to drop that symbol. Believe me.
> ---
> drivers/net/irda/Kconfig | 1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/net/irda/Kconfig b/drivers/net/irda/Kconfig
> --- a/drivers/net/irda/Kconfig
> +++ b/drivers/net/irda/Kconfig
> @@ -374,6 +374,7 @@ config VIA_FIR
> config PXA_FICP
> tristate "Intel PXA2xx Internal FICP"
> depends on ARCH_PXA && IRDA
> + select ZONE_DMA
> help
> Say Y or M here if you want to build support for the PXA2xx
> built-in IRDA interface which can support both SIR and FIR.
>
--
With best wishes
Dmitry
On Wed, 1 Jun 2011, Dmitry Eremin-Solenikov wrote:
> > So you want to continue to allow the page allocator to return pages from
> > anywhere, even when GFP_DMA is specified, just as though it was lowmem?
>
> Yes and no. I'm asking for the grace period for the drivers authors to be able
> to fix their code. After a grace period of one or two majors this permission
> should be removed and your original patch should be effective.
>
You don't need to wait for the code to be fixed, you just need to enable
CONFIG_ZONE_DMA. This is a configuration issue. If that GFP_DMA can be
removed later, that's great, but right now it requires it. Why can't you
enable the config option?
On Wed, 1 Jun 2011, Russell King - ARM Linux wrote:
> On Wed, Jun 01, 2011 at 10:23:15AM -0700, David Rientjes wrote:
> > On Wed, 1 Jun 2011, Dmitry Eremin-Solenikov wrote:
> >
> > > I've hit this with IrDA driver on PXA. Also I've seen the report regarding
> > > other ARM platform (ep-something). Thus I've included Russell in the cc.
> > >
> >
> > So you want to continue to allow the page allocator to return pages from
> > anywhere, even when GFP_DMA is specified, just as though it was lowmem?
>
> No. What *everyone* is asking for is to allow the situation which has
> persisted thus far to continue for ONE MORE RELEASE but with a WARNING
> so that these problems can be found without causing REGRESSIONS.
>
> That is NOT an unreasonable request, but it seems that its far too much
> to ask of you.
Full ack.
David,
stop that nonsense already. You changed the behaviour and broke stuff
which was working fine before for whatever reason. That behaviour was
in the kernel for ages and we tolerated the abuse.
So making it a warning for this release and then break stuff which has
not been fixed is a sensible request and the only sensible approach.
If you think that you need to force that behaviour change now, then
you better go and audit _ALL_ GFP_DMA users yourself for correctness
and fix them case by case either by replacing the GFP_DMA flag or by
selecting ZONE_DMA with a proper changelog for every instance.
It's not up to your total ignorance of reality to break stuff at will
and then paper over the problems you caused by selecting ZONE_DMA
which will keep the abusers around forever.
Thanks,
tglx
On Wed, 1 Jun 2011, Thomas Gleixner wrote:
> > That is NOT an unreasonable request, but it seems that its far too much
> > to ask of you.
>
> Full ack.
>
> David,
>
> stop that nonsense already. You changed the behaviour and broke stuff
> which was working fine before for whatever reason. That behaviour was
> in the kernel for ages and we tolerated the abuse.
>
Did I nack this patch and not realize it?
Does my patch fix the warning for pxaficp_ir that would still be emitted
with this patch? If the driver uses GFP_DMA and nobody from the arm side
is prepared to remove it yet, then I'd suggest merging my patch until that
can be determined. Otherwise, you have no guarantees about where the
memory is actually coming from.
On Wed, 1 Jun 2011, David Rientjes wrote:
> On Wed, 1 Jun 2011, Thomas Gleixner wrote:
>
> > > That is NOT an unreasonable request, but it seems that its far too much
> > > to ask of you.
> >
> > Full ack.
> >
> > David,
> >
> > stop that nonsense already. You changed the behaviour and broke stuff
> > which was working fine before for whatever reason. That behaviour was
> > in the kernel for ages and we tolerated the abuse.
> >
>
> Did I nack this patch and not realize it?
No, you did not realize anything.
> Does my patch fix the warning for pxaficp_ir that would still be emitted
> with this patch? If the driver uses GFP_DMA and nobody from the arm side
Your patch does not fix anything. It papers over the problem and
that's the f@&^%%@^#ing wrong approach.
And just to be clear. You CANNOT fix a warning. You can fix the code
which causes the warning, but that's not what your patch is
doing. Your patch HIDES the problem.
> is prepared to remove it yet, then I'd suggest merging my patch until that
> can be determined. Otherwise, you have no guarantees about where the
> memory is actually coming from.
Did you actually try to understand what I wrote?
You decided that it's a BUG just because it should not be allowed. So
you changed the behaviour, which was perfectly fine before.
Now you try to paper over the problem by selecting ZONE_DMA and refuse
to give a grace period of _ONE_ kernel release.
IOW, you are preventing that the abusers of GFP_DMA are fixed
properly.
I can see that you neither have the bandwidth nor the knowledge to
analyse each user of GFP_DMA. And that should tell you something.
If you cannot fix it yourself, then f*(&!@$#ng not break it.
Thanks,
tglx
On Wed 2011-06-01 21:38:59, KOSAKI Motohiro wrote:
> 2011/6/1 Dmitry Eremin-Solenikov <[email protected]>:
> > Please be more polite to other people. After a197b59ae6 all allocations
> > with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> > one warning during bootup is emited, no matter how many things fail).
> > This is a very crude change on behaviour. To be more civil, instead of
> > failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> > memory on non-ZONE_DMA node.
> >
> > This change should be reverted after one or two major releases, but
> > we should be more accurate rather than hoping for the best.
>
> Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
> DMA_ZONE at all.
> and a197b59ae6 only care x86 embedded case. If we accept your patch, I
> can imagine
> other people will claim warn foold is a bug. ;)
I believe we should revert. It broke zaurus boot for metan...
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
(2011/06/02 4:46), Thomas Gleixner wrote:
> On Wed, 1 Jun 2011, David Rientjes wrote:
>> On Wed, 1 Jun 2011, Thomas Gleixner wrote:
>>
>>>> That is NOT an unreasonable request, but it seems that its far too much
>>>> to ask of you.
>>>
>>> Full ack.
>>>
>>> David,
>>>
>>> stop that nonsense already. You changed the behaviour and broke stuff
>>> which was working fine before for whatever reason. That behaviour was
>>> in the kernel for ages and we tolerated the abuse.
>>>
>>
>> Did I nack this patch and not realize it?
>
> No, you did not realize anything.
>
>> Does my patch fix the warning for pxaficp_ir that would still be emitted
>> with this patch? If the driver uses GFP_DMA and nobody from the arm side
>
> Your patch does not fix anything. It papers over the problem and
> that's the f@&^%%@^#ing wrong approach.
>
> And just to be clear. You CANNOT fix a warning. You can fix the code
> which causes the warning, but that's not what your patch is
> doing. Your patch HIDES the problem.
>
>> is prepared to remove it yet, then I'd suggest merging my patch until that
>> can be determined. Otherwise, you have no guarantees about where the
>> memory is actually coming from.
>
> Did you actually try to understand what I wrote?
>
> You decided that it's a BUG just because it should not be allowed. So
> you changed the behaviour, which was perfectly fine before.
>
> Now you try to paper over the problem by selecting ZONE_DMA and refuse
> to give a grace period of _ONE_ kernel release.
>
> IOW, you are preventing that the abusers of GFP_DMA are fixed
> properly.
>
> I can see that you neither have the bandwidth nor the knowledge to
> analyse each user of GFP_DMA. And that should tell you something.
>
> If you cannot fix it yourself, then f*(&!@$#ng not break it.
Then, the revert patch is here.
>From beaf8c4457fffeb3a4bfb3a5109fbcddcfc686cb Mon Sep 17 00:00:00 2001
From: KOSAKI Motohiro <[email protected]>
Date: Fri, 10 Jun 2011 15:27:06 +0900
Subject: [PATCH] Revert "mm: fail GFP_DMA allocations when ZONE_DMA is not configured"
Dmitry Eremin-Solenikov reported IrDA driver on PXA started to fail
after commit a197b59ae (mm: fail GFP_DMA allocations when ZONE_DMA
is not configured). Pavel Mechek mentioned it also broke zaurus.
Even though the concept of commit a197b59ae is correct. It's no worth
to break ARM. Thus this should be reverted until finished to audit
all GFP_DMA users.
This reverts commit a197b59ae6e8bee56fcef37ea2482dc08414e2ac.
Reported-by: Dmitry Eremin-Solenikov <[email protected]>
Reported-by: Pavel Machek <[email protected]>
Signed-off-by: KOSAKI Motohiro <[email protected]>
Cc: David Rientjes <[email protected]>
Cc: Russell King - ARM Linux <[email protected]>
Cc: Mel Gorman <[email protected]>
Cc: KAMEZAWA Hiroyuki <[email protected]>
Cc: Rik van Riel <[email protected]>
Cc: Andrew Morton <[email protected]>
---
mm/page_alloc.c | 4 ----
1 files changed, 0 insertions(+), 4 deletions(-)
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index a4e1db3..4e8985a 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -2247,10 +2247,6 @@ __alloc_pages_nodemask(gfp_t gfp_mask, unsigned int order,
if (should_fail_alloc_page(gfp_mask, order))
return NULL;
-#ifndef CONFIG_ZONE_DMA
- if (WARN_ON_ONCE(gfp_mask & __GFP_DMA))
- return NULL;
-#endif
/*
* Check the zones suitable for the gfp_mask contain at least one
--
1.7.3.1
On Fri, 10 Jun 2011 16:38:06 +0900 KOSAKI Motohiro <[email protected]> wrote:
> Subject: [PATCH] Revert "mm: fail GFP_DMA allocations when ZONE_DMA is not configured"
Confused. We reverted this over a week ago.
(2011/06/10 16:43), Andrew Morton wrote:
> On Fri, 10 Jun 2011 16:38:06 +0900 KOSAKI Motohiro <[email protected]> wrote:
>
>> Subject: [PATCH] Revert "mm: fail GFP_DMA allocations when ZONE_DMA is not configured"
>
> Confused. We reverted this over a week ago.
Oh, I'm sorry. I missed it. Please forget my stupid mail.
On 6/10/11, Andrew Morton <[email protected]> wrote:
> On Fri, 10 Jun 2011 16:38:06 +0900 KOSAKI Motohiro
> <[email protected]> wrote:
>
>> Subject: [PATCH] Revert "mm: fail GFP_DMA allocations when ZONE_DMA is not
>> configured"
>
> Confused. We reverted this over a week ago.
Should one submit a patch adding a warning to GFP_DMA allocations
w/o ZONE_DMA, or the idea of the original patch is wrong?
--
With best wishes
Dmitry
On Fri, Jun 10, 2011 at 12:11:42PM +0400, Dmitry Eremin-Solenikov wrote:
> On 6/10/11, Andrew Morton <[email protected]> wrote:
> > On Fri, 10 Jun 2011 16:38:06 +0900 KOSAKI Motohiro
> > <[email protected]> wrote:
> >
> >> Subject: [PATCH] Revert "mm: fail GFP_DMA allocations when ZONE_DMA is not
> >> configured"
> >
> > Confused. We reverted this over a week ago.
>
> Should one submit a patch adding a warning to GFP_DMA allocations
> w/o ZONE_DMA, or the idea of the original patch is wrong?
Linus was far from impressed by the original commit, saying:
| Using GFP_DMA is reasonable in a driver - on platforms where that
| matters, it should allocate from the DMA zone, on platforms where it
| doesn't matter it should be a no-op.
So no, not even a warning.
What is a useful exercise though is to remove GFP_DMA from those
allocations which should never have had GFP_DMA added - such as those
used for data structures which have nothing to do with DMA at all.
Also dma_alloc_coherent() should not be given GFP_DMA in any case.
On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> > Should one submit a patch adding a warning to GFP_DMA allocations
> > w/o ZONE_DMA, or the idea of the original patch is wrong?
>
> Linus was far from impressed by the original commit, saying:
> | Using GFP_DMA is reasonable in a driver - on platforms where that
> | matters, it should allocate from the DMA zone, on platforms where it
> | doesn't matter it should be a no-op.
>
> So no, not even a warning.
>
Any words of wisdom for users with CONFIG_ZONE_DMA=n that actually use
drivers where they need GFP_DMA? The page allocator should just silently
return memory from anywhere?
On Fri, Jun 10, 2011 at 11:54:16AM -0700, David Rientjes wrote:
> On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
>
> > > Should one submit a patch adding a warning to GFP_DMA allocations
> > > w/o ZONE_DMA, or the idea of the original patch is wrong?
> >
> > Linus was far from impressed by the original commit, saying:
> > | Using GFP_DMA is reasonable in a driver - on platforms where that
> > | matters, it should allocate from the DMA zone, on platforms where it
> > | doesn't matter it should be a no-op.
> >
> > So no, not even a warning.
> >
>
> Any words of wisdom for users with CONFIG_ZONE_DMA=n that actually use
> drivers where they need GFP_DMA? The page allocator should just silently
> return memory from anywhere?
See Linus' reply. I quote again "on platforms where it doesn't matter it
should be a no-op". If _you_ have a problem with that _you_ need to
discuss it with _Linus_, not me. I'm not going to be a middle-man sitting
between two people with different opinions.
On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> > > > Should one submit a patch adding a warning to GFP_DMA allocations
> > > > w/o ZONE_DMA, or the idea of the original patch is wrong?
> > >
> > > Linus was far from impressed by the original commit, saying:
> > > | Using GFP_DMA is reasonable in a driver - on platforms where that
> > > | matters, it should allocate from the DMA zone, on platforms where it
> > > | doesn't matter it should be a no-op.
> > >
> > > So no, not even a warning.
> > >
> >
> > Any words of wisdom for users with CONFIG_ZONE_DMA=n that actually use
> > drivers where they need GFP_DMA? The page allocator should just silently
> > return memory from anywhere?
>
> See Linus' reply. I quote again "on platforms where it doesn't matter it
> should be a no-op". If _you_ have a problem with that _you_ need to
> discuss it with _Linus_, not me. I'm not going to be a middle-man sitting
> between two people with different opinions.
>
We're talking about two different things. Linus is saying that if GFP_DMA
should be a no-op if the hardware doesn't require DMA memory because the
kernel was correctly compiled without CONFIG_ZONE_DMA. I'm asking about a
kernel that was incorrectly compiled without CONFIG_ZONE_DMA and now we're
returning memory from anywhere even though we actually require GFP_DMA.
If you don't want to form an opinion of your own, then I have no problem
cc'ing Linus on it. I don't think he'd object to a
#ifndef CONFIG_ZONE_DMA
WARN_ON_ONCE(1, "%s (%d): allocating DMA memory without DMA support -- "
"enable CONFIG_ZONE_DMA if needed.\n",
current->comm, current->pid);
#endif
On Fri, Jun 10, 2011 at 03:01:15PM -0700, David Rientjes wrote:
> On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> > See Linus' reply. I quote again "on platforms where it doesn't matter it
> > should be a no-op". If _you_ have a problem with that _you_ need to
> > discuss it with _Linus_, not me. I'm not going to be a middle-man sitting
> > between two people with different opinions.
>
> We're talking about two different things. Linus is saying that if GFP_DMA
> should be a no-op if the hardware doesn't require DMA memory because the
> kernel was correctly compiled without CONFIG_ZONE_DMA. I'm asking about a
> kernel that was incorrectly compiled without CONFIG_ZONE_DMA and now we're
> returning memory from anywhere even though we actually require GFP_DMA.
How do you distinguish between the two states? Answer: you can't.
> If you don't want to form an opinion of your own, then I have no problem
> cc'ing Linus on it.
I think I've made my position in this fairly clear with our previous
discussions on this. The fact of the matter is that there are some
drivers which use GFP_DMA because they need that on _some_ platforms
but not all.
Not all platforms are _broken_ with respect to DMA, and those which
aren't broken don't provide a DMA zone.
Therefore, it is _perfectly_ _legal_ to honor a GFP_DMA allocation on
a kernel without CONFIG_ZONE_DMA set.
That position appears to be reflected by Linus' response, so
> I don't think he'd object to a
>
> #ifndef CONFIG_ZONE_DMA
> WARN_ON_ONCE(1, "%s (%d): allocating DMA memory without DMA support -- "
> "enable CONFIG_ZONE_DMA if needed.\n",
> current->comm, current->pid);
> #endif
I think he will definitely object to this on the grounds that its
adding useless noise for conditions which are _perfectly_ _valid_.
On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> > We're talking about two different things. Linus is saying that if GFP_DMA
> > should be a no-op if the hardware doesn't require DMA memory because the
> > kernel was correctly compiled without CONFIG_ZONE_DMA. I'm asking about a
> > kernel that was incorrectly compiled without CONFIG_ZONE_DMA and now we're
> > returning memory from anywhere even though we actually require GFP_DMA.
>
> How do you distinguish between the two states? Answer: you can't.
>
By my warning which says "enable CONFIG_ZONE_DMA _if_ needed." The
alternative is to silently return memory from anywhere, which is what the
page allocator does now, which doesn't seem very user friendly when the
device randomly works depending on the chance it was actually allocated
from the DMA mask. If it actually wants DMA and the kernel is compiled
incorrectly, then I think a single line in the kernel log would be nice to
point them in the right direction. Users who disable the option usually
know what they're doing (it's only allowed for CONFIG_EXPERT on x86, for
example), so I don't think they'll mind the notification and choose to
ignore it.
On Fri, Jun 10, 2011 at 03:16:00PM -0700, David Rientjes wrote:
> On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
>
> > > We're talking about two different things. Linus is saying that if GFP_DMA
> > > should be a no-op if the hardware doesn't require DMA memory because the
> > > kernel was correctly compiled without CONFIG_ZONE_DMA. I'm asking about a
> > > kernel that was incorrectly compiled without CONFIG_ZONE_DMA and now we're
> > > returning memory from anywhere even though we actually require GFP_DMA.
> >
> > How do you distinguish between the two states? Answer: you can't.
> >
>
> By my warning which says "enable CONFIG_ZONE_DMA _if_ needed." The
> alternative is to silently return memory from anywhere, which is what the
> page allocator does now, which doesn't seem very user friendly when the
> device randomly works depending on the chance it was actually allocated
> from the DMA mask. If it actually wants DMA and the kernel is compiled
> incorrectly, then I think a single line in the kernel log would be nice to
> point them in the right direction. Users who disable the option usually
> know what they're doing (it's only allowed for CONFIG_EXPERT on x86, for
> example), so I don't think they'll mind the notification and choose to
> ignore it.
So those platforms which don't have a DMA zone, don't have any problems
with DMA, yet want to use the very same driver which does have a problem
on ISA hardware have to also put up with a useless notification that
their kernel might be broken?
Are you offering to participate on other architectures mailing lists to
answer all the resulting queries?
On Wed, Jun 1, 2011 at 3:04 AM, Dmitry Eremin-Solenikov
<[email protected]> wrote:
> Please be more polite to other people. After a197b59ae6 all allocations
> with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> one warning during bootup is emited, no matter how many things fail).
> This is a very crude change on behaviour. To be more civil, instead of
> failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> memory on non-ZONE_DMA node.
The whole thing already got reverted in commit 1fa7b6a29c613.
Linus
On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> So those platforms which don't have a DMA zone, don't have any problems
> with DMA, yet want to use the very same driver which does have a problem
> on ISA hardware have to also put up with a useless notification that
> their kernel might be broken?
>
> Are you offering to participate on other architectures mailing lists to
> answer all the resulting queries?
>
It all depends on the wording of the "warning", it should make it clear
that this is not always an error condition and only affects certain types
of hardware which the user may or may not have. If you have any
suggestions on how to alter "task (pid): attempted to allocate DMA memory
without DMA support -- enable CONFIG_ZONE_DMA if needed" to make that more
clear, be my guest. The alternative is that the ISA hardware cannot
handle the memory returned fails unexpectedly and randomly without any
clear indication of what the issue is. I think what would be worse is
time lost for someone to realize CONFIG_ZONE_DMA isn't enabled, and that
could be significant (and probably generate many bug reports on its own)
since it isn't immediately obvious without doing some debugging.
On Fri, Jun 10, 2011 at 03:30:35PM -0700, David Rientjes wrote:
> On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
> > So those platforms which don't have a DMA zone, don't have any problems
> > with DMA, yet want to use the very same driver which does have a problem
> > on ISA hardware have to also put up with a useless notification that
> > their kernel might be broken?
> >
> > Are you offering to participate on other architectures mailing lists to
> > answer all the resulting queries?
>
> It all depends on the wording of the "warning", it should make it clear
> that this is not always an error condition and only affects certain types
> of hardware which the user may or may not have.
I think people will still be worried when they get a warning. And there
are lots of platforms that don't need ZONE_DMA just because devices can
access the full RAM. As Russell said, same drivers may be used on
platforms that can actually do DMA only to certain areas of memory and
require ZONE_DMA (there are several examples on ARM).
If you want, you can add something like CONFIG_ARCH_HAS_ZONE_DMA across
all the platforms that support ZONE_DMA and only get the warning if
ZONE_DMA is available but not enabled.
--
Catalin
On 06/11/2011 03:45 AM, Catalin Marinas wrote:
> On Fri, Jun 10, 2011 at 03:30:35PM -0700, David Rientjes wrote:
>> On Fri, 10 Jun 2011, Russell King - ARM Linux wrote:
>>> So those platforms which don't have a DMA zone, don't have any problems
>>> with DMA, yet want to use the very same driver which does have a problem
>>> on ISA hardware have to also put up with a useless notification that
>>> their kernel might be broken?
>>>
>>> Are you offering to participate on other architectures mailing lists to
>>> answer all the resulting queries?
>>
>> It all depends on the wording of the "warning", it should make it clear
>> that this is not always an error condition and only affects certain types
>> of hardware which the user may or may not have.
>
> I think people will still be worried when they get a warning. And there
> are lots of platforms that don't need ZONE_DMA just because devices can
> access the full RAM. As Russell said, same drivers may be used on
> platforms that can actually do DMA only to certain areas of memory and
> require ZONE_DMA (there are several examples on ARM).
>
> If you want, you can add something like CONFIG_ARCH_HAS_ZONE_DMA across
> all the platforms that support ZONE_DMA and only get the warning if
> ZONE_DMA is available but not enabled.
It sounds to me like these drivers using GFP_DMA should really be fixed
to use the proper DMA API. That's what it's there for. The problem is
that GFP_DMA doesn't really mean anything generically other than "memory
suitable for DMA according to some criteria". On x86 it means low 16MB,
on some other platforms it presumably means other things, on others it
means nothing at all. It's quite likely that a driver that requests
GFP_DMA isn't likely to get exactly what it wants on all platforms (for
example on x86 the allocation will be constrained to the low 16MB which
is unnecessarily restrictive for most devices).
Hi Pavel,
On Thursday, June 02, 2011, Pavel Machek wrote:
> On Wed 2011-06-01 21:38:59, KOSAKI Motohiro wrote:
> > 2011/6/1 Dmitry Eremin-Solenikov <[email protected]>:
> > > Please be more polite to other people. After a197b59ae6 all allocations
> > > with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> > > one warning during bootup is emited, no matter how many things fail).
> > > This is a very crude change on behaviour. To be more civil, instead of
> > > failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> > > memory on non-ZONE_DMA node.
> > >
> > > This change should be reverted after one or two major releases, but
> > > we should be more accurate rather than hoping for the best.
> >
> > Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
> > DMA_ZONE at all.
> > and a197b59ae6 only care x86 embedded case. If we accept your patch, I
> > can imagine
> > other people will claim warn foold is a bug. ;)
>
> I believe we should revert. It broke zaurus boot for metan...
Is it still a problem, or has it been fixed in the meantime?
Rafael
Hi!
> > > > Please be more polite to other people. After a197b59ae6 all allocations
> > > > with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> > > > one warning during bootup is emited, no matter how many things fail).
> > > > This is a very crude change on behaviour. To be more civil, instead of
> > > > failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> > > > memory on non-ZONE_DMA node.
> > > >
> > > > This change should be reverted after one or two major releases, but
> > > > we should be more accurate rather than hoping for the best.
> > >
> > > Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
> > > DMA_ZONE at all.
> > > and a197b59ae6 only care x86 embedded case. If we accept your patch, I
> > > can imagine
> > > other people will claim warn foold is a bug. ;)
> >
> > I believe we should revert. It broke zaurus boot for metan...
>
> Is it still a problem, or has it been fixed in the meantime?
>
This was revered in 3.0-rc2.
--
metan
On Sunday, June 12, 2011, Cyril Hrubis wrote:
> Hi!
> > > > > Please be more polite to other people. After a197b59ae6 all allocations
> > > > > with GFP_DMA set on nodes without ZONE_DMA fail nearly silently (only
> > > > > one warning during bootup is emited, no matter how many things fail).
> > > > > This is a very crude change on behaviour. To be more civil, instead of
> > > > > failing emit noisy warnings each time smbd. tries to allocate a GFP_DMA
> > > > > memory on non-ZONE_DMA node.
> > > > >
> > > > > This change should be reverted after one or two major releases, but
> > > > > we should be more accurate rather than hoping for the best.
> > > >
> > > > Instaed of, shouldn't we revert a197b59ae6? Some arch don't have
> > > > DMA_ZONE at all.
> > > > and a197b59ae6 only care x86 embedded case. If we accept your patch, I
> > > > can imagine
> > > > other people will claim warn foold is a bug. ;)
> > >
> > > I believe we should revert. It broke zaurus boot for metan...
> >
> > Is it still a problem, or has it been fixed in the meantime?
> >
>
> This was revered in 3.0-rc2.
Cool, thanks!
On Sat, Jun 11, 2011 at 11:18:46AM -0600, Robert Hancock wrote:
> It sounds to me like these drivers using GFP_DMA should really be fixed
> to use the proper DMA API. That's what it's there for. The problem is
> that GFP_DMA doesn't really mean anything generically other than "memory
> suitable for DMA according to some criteria".
It would be really nice to have an allocation interface which takes a
DMA mask and returns memory which satisfies that DMA mask, but that's
a pipedream I've had for the last 12 years or so.
GFP_DMA is not about the DMA API - the DMA API does _not_ deal with
memory allocation for streaming transfers at all. It only deals with
coherent DMA memory allocation which is something completely different.
So it really isn't about "these drivers should be fixed to use the
proper DMA API" because there isn't one.
If the concensus is to remove GFP_DMA, there would need to be some ground
work done first:
(a) audit whether GFP_DMA is really needed (iow, is the allocated
structure actually DMA'd from/to, or did the driver writer just slap
GFP_DMA on it without thinking?)
(b) audit whether we have a DMA mask available at the GFP_DMA allocator
call site
That should reveal whether it is even possible to move to a different
solution.
Depending on (b), provide a new allocation API which takes the DMA mask
and internally calls the standard allocator with GFP_DMA if the DMA mask
is anything less than "all memory".