diff mbox

[v2] mm: dmapool: use provided gfp flags for all dma_alloc_coherent() calls

Message ID 1353421905-3112-1-git-send-email-m.szyprowski@samsung.com
State Accepted
Commit 387870f2d6d679746020fa8e25ef786ff338dc98
Headers show

Commit Message

Marek Szyprowski Nov. 20, 2012, 2:31 p.m. UTC
dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
regardless the flags provided by the caller. This causes excessive
pruning of emergency memory pools without any good reason. Additionaly,
on ARM architecture any driver which is using dmapools will sooner or
later  trigger the following error: 
"ERROR: 256 KiB atomic DMA coherent pool is too small!
Please increase it with coherent_pool= kernel parameter!".
Increasing the coherent pool size usually doesn't help much and only
delays such error, because all GFP_ATOMIC DMA allocations are always
served from the special, very limited memory pool.

This patch changes the dmapool code to correctly use gfp flags provided
by the dmapool caller.

Reported-by: Soeren Moch <smoch@web.de>
Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
Tested-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Soeren Moch <smoch@web.de>
---

changelog
v2:
 - removed all waitq related stuff
 - extended commit message

 mm/dmapool.c |   31 +++++++------------------------
 1 file changed, 7 insertions(+), 24 deletions(-)

Comments

Andrew Morton Nov. 20, 2012, 7:33 p.m. UTC | #1
On Tue, 20 Nov 2012 15:31:45 +0100
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> regardless the flags provided by the caller. This causes excessive
> pruning of emergency memory pools without any good reason. Additionaly,
> on ARM architecture any driver which is using dmapools will sooner or
> later  trigger the following error: 
> "ERROR: 256 KiB atomic DMA coherent pool is too small!
> Please increase it with coherent_pool= kernel parameter!".
> Increasing the coherent pool size usually doesn't help much and only
> delays such error, because all GFP_ATOMIC DMA allocations are always
> served from the special, very limited memory pool.
> 

Is this problem serious enough to justify merging the patch into 3.7? 
And into -stable kernels?
Jason Cooper Nov. 20, 2012, 8:27 p.m. UTC | #2
On Tue, Nov 20, 2012 at 11:33:25AM -0800, Andrew Morton wrote:
> On Tue, 20 Nov 2012 15:31:45 +0100
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> 
> > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > regardless the flags provided by the caller. This causes excessive
> > pruning of emergency memory pools without any good reason. Additionaly,
> > on ARM architecture any driver which is using dmapools will sooner or
> > later  trigger the following error: 
> > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > Please increase it with coherent_pool= kernel parameter!".
> > Increasing the coherent pool size usually doesn't help much and only
> > delays such error, because all GFP_ATOMIC DMA allocations are always
> > served from the special, very limited memory pool.
> > 
> 
> Is this problem serious enough to justify merging the patch into 3.7? 
> And into -stable kernels?

kirkwood and orion5x currently have the following code in their early
init:

/*
 * Some Kirkwood devices allocate their coherent buffers from atomic
 * context. Increase size of atomic coherent pool to make sure such the
 * allocations won't fail.
 */
init_dma_coherent_pool_size(SZ_1M);

We have a pending patch to do the same for mvebu (new armv7 Marvell
SoCs).  There is at least one reported real world case where even the
above isn't sufficient [1].

thx,

Jason.

[1] http://www.spinics.net/lists/arm-kernel/msg205495.html
Marek Szyprowski Nov. 21, 2012, 8:08 a.m. UTC | #3
Hello,

On 11/20/2012 8:33 PM, Andrew Morton wrote:
> On Tue, 20 Nov 2012 15:31:45 +0100
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > regardless the flags provided by the caller. This causes excessive
> > pruning of emergency memory pools without any good reason. Additionaly,
> > on ARM architecture any driver which is using dmapools will sooner or
> > later  trigger the following error:
> > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > Please increase it with coherent_pool= kernel parameter!".
> > Increasing the coherent pool size usually doesn't help much and only
> > delays such error, because all GFP_ATOMIC DMA allocations are always
> > served from the special, very limited memory pool.
> >
>
> Is this problem serious enough to justify merging the patch into 3.7?
> And into -stable kernels?

I wonder if it is a good idea to merge such change at the end of current
-rc period. It changes the behavior of dma pool allocations and I bet there
might be some drivers which don't care much about passed gfp flags, as for
ages it simply worked for them, even if the allocations were done from
atomic context. What do You think? Technically it is also not a pure bugfix,
so imho it shouldn't be considered for -stable.

On the other hand at least for ARM users of sata_mv driver (which is just
an innocent client of dma pool, correctly passing GFP_KERNEL flag) it would
solve the issues related to shortage of atomic pool for dma allocations what
might justify pushing it to 3.7.

Best regards
Andrew Morton Nov. 21, 2012, 8:36 a.m. UTC | #4
On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Hello,
> 
> On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > On Tue, 20 Nov 2012 15:31:45 +0100
> > Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > > regardless the flags provided by the caller. This causes excessive
> > > pruning of emergency memory pools without any good reason. Additionaly,
> > > on ARM architecture any driver which is using dmapools will sooner or
> > > later  trigger the following error:
> > > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > > Please increase it with coherent_pool= kernel parameter!".
> > > Increasing the coherent pool size usually doesn't help much and only
> > > delays such error, because all GFP_ATOMIC DMA allocations are always
> > > served from the special, very limited memory pool.
> > >
> >
> > Is this problem serious enough to justify merging the patch into 3.7?
> > And into -stable kernels?
> 
> I wonder if it is a good idea to merge such change at the end of current
> -rc period.

I'm not sure what you mean by this.

But what we do sometimes if we think a patch needs a bit more
real-world testing before backporting is to merge it into -rc1 in the
normal merge window, and tag it for -stable backporting.  That way it
gets a few weeks(?) testing in mainline before getting backported.
Marek Szyprowski Nov. 21, 2012, 9:20 a.m. UTC | #5
Hello,

On 11/21/2012 9:36 AM, Andrew Morton wrote:
> On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> > Hello,
> >
> > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > >
> > > > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > > > regardless the flags provided by the caller. This causes excessive
> > > > pruning of emergency memory pools without any good reason. Additionaly,
> > > > on ARM architecture any driver which is using dmapools will sooner or
> > > > later  trigger the following error:
> > > > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > > > Please increase it with coherent_pool= kernel parameter!".
> > > > Increasing the coherent pool size usually doesn't help much and only
> > > > delays such error, because all GFP_ATOMIC DMA allocations are always
> > > > served from the special, very limited memory pool.
> > > >
> > >
> > > Is this problem serious enough to justify merging the patch into 3.7?
> > > And into -stable kernels?
> >
> > I wonder if it is a good idea to merge such change at the end of current
> > -rc period.
>
> I'm not sure what you mean by this.
>
> But what we do sometimes if we think a patch needs a bit more
> real-world testing before backporting is to merge it into -rc1 in the
> normal merge window, and tag it for -stable backporting.  That way it
> gets a few weeks(?) testing in mainline before getting backported.

I just wondered that if it gets merged to v3.7-rc7 there won't be much time
for real-world testing before final v3.7 release. This patch is in
linux-next for over a week and I'm not aware of any issues, but -rc releases
gets much more attention and testing than linux-next tree.

If You think it's fine to put such change to v3.7-rc7 I will send a pull
request and tag it for stable asap.

Best regards
Andrew Morton Nov. 21, 2012, 7:17 p.m. UTC | #6
On Wed, 21 Nov 2012 10:20:07 +0100
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> Hello,
> 
> On 11/21/2012 9:36 AM, Andrew Morton wrote:
> > On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > > Hello,
> > >
> > > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > > Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > >
> > > > > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > > > > regardless the flags provided by the caller. This causes excessive
> > > > > pruning of emergency memory pools without any good reason. Additionaly,
> > > > > on ARM architecture any driver which is using dmapools will sooner or
> > > > > later  trigger the following error:
> > > > > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > > > > Please increase it with coherent_pool= kernel parameter!".
> > > > > Increasing the coherent pool size usually doesn't help much and only
> > > > > delays such error, because all GFP_ATOMIC DMA allocations are always
> > > > > served from the special, very limited memory pool.
> > > > >
> > > >
> > > > Is this problem serious enough to justify merging the patch into 3.7?
> > > > And into -stable kernels?
> > >
> > > I wonder if it is a good idea to merge such change at the end of current
> > > -rc period.
> >
> > I'm not sure what you mean by this.
> >
> > But what we do sometimes if we think a patch needs a bit more
> > real-world testing before backporting is to merge it into -rc1 in the
> > normal merge window, and tag it for -stable backporting.  That way it
> > gets a few weeks(?) testing in mainline before getting backported.
> 
> I just wondered that if it gets merged to v3.7-rc7 there won't be much time
> for real-world testing before final v3.7 release. This patch is in
> linux-next for over a week and I'm not aware of any issues, but -rc releases
> gets much more attention and testing than linux-next tree.
> 
> If You think it's fine to put such change to v3.7-rc7 I will send a pull
> request and tag it for stable asap.
> 

What I'm suggesting is that it be merged for 3.8-rc1 with a -stable
tag, then it will be backported into 3.7.x later on.
Marek Szyprowski Nov. 22, 2012, 12:55 p.m. UTC | #7
On 11/21/2012 8:17 PM, Andrew Morton wrote:
> On Wed, 21 Nov 2012 10:20:07 +0100
> Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > On 11/21/2012 9:36 AM, Andrew Morton wrote:
> > > On Wed, 21 Nov 2012 09:08:52 +0100 Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > > On 11/20/2012 8:33 PM, Andrew Morton wrote:
> > > > > On Tue, 20 Nov 2012 15:31:45 +0100
> > > > > Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > > > >
> > > > > > dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> > > > > > regardless the flags provided by the caller. This causes excessive
> > > > > > pruning of emergency memory pools without any good reason. Additionaly,
> > > > > > on ARM architecture any driver which is using dmapools will sooner or
> > > > > > later  trigger the following error:
> > > > > > "ERROR: 256 KiB atomic DMA coherent pool is too small!
> > > > > > Please increase it with coherent_pool= kernel parameter!".
> > > > > > Increasing the coherent pool size usually doesn't help much and only
> > > > > > delays such error, because all GFP_ATOMIC DMA allocations are always
> > > > > > served from the special, very limited memory pool.
> > > > > >
> > > > >
> > > > > Is this problem serious enough to justify merging the patch into 3.7?
> > > > > And into -stable kernels?
> > > >
> > > > I wonder if it is a good idea to merge such change at the end of current
> > > > -rc period.
> > >
> > > I'm not sure what you mean by this.
> > >
> > > But what we do sometimes if we think a patch needs a bit more
> > > real-world testing before backporting is to merge it into -rc1 in the
> > > normal merge window, and tag it for -stable backporting.  That way it
> > > gets a few weeks(?) testing in mainline before getting backported.
> >
> > I just wondered that if it gets merged to v3.7-rc7 there won't be much time
> > for real-world testing before final v3.7 release. This patch is in
> > linux-next for over a week and I'm not aware of any issues, but -rc releases
> > gets much more attention and testing than linux-next tree.
> >
> > If You think it's fine to put such change to v3.7-rc7 I will send a pull
> > request and tag it for stable asap.
>
> What I'm suggesting is that it be merged for 3.8-rc1 with a -stable
> tag, then it will be backported into 3.7.x later on.

OK, I will push it to v3.8-rc1 then and tag for stable.

Best regards
Soeren Moch Jan. 14, 2013, 11:56 a.m. UTC | #8
On 20.11.2012 15:31, Marek Szyprowski wrote:
> dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> regardless the flags provided by the caller. This causes excessive
> pruning of emergency memory pools without any good reason. Additionaly,
> on ARM architecture any driver which is using dmapools will sooner or
> later  trigger the following error:
> "ERROR: 256 KiB atomic DMA coherent pool is too small!
> Please increase it with coherent_pool= kernel parameter!".
> Increasing the coherent pool size usually doesn't help much and only
> delays such error, because all GFP_ATOMIC DMA allocations are always
> served from the special, very limited memory pool.
>
> This patch changes the dmapool code to correctly use gfp flags provided
> by the dmapool caller.
>
> Reported-by: Soeren Moch <smoch@web.de>
> Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Tested-by: Andrew Lunn <andrew@lunn.ch>
> Tested-by: Soeren Moch <smoch@web.de>

Now I tested linux-3.7.1 (this patch is included there) on my Marvell
Kirkwood system. I still see

   ERROR: 1024 KiB atomic DMA coherent pool is too small!
   Please increase it with coherent_pool= kernel parameter!

after several hours of runtime under heavy load with SATA and
DVB-Sticks (em28xx / drxk and dib0700).

As already reported earlier this patch improved the behavior compared to 
linux-3.6.x and 3.7.0 (error after several ten minutes runtime), but
I still see a regression compared to linux-3.5.x. With this kernel the
same system with same workload runs flawlessly.

Regards,
Soeren
Jason Cooper Jan. 15, 2013, 4:56 p.m. UTC | #9
Greg,

I've added you to the this thread hoping for a little insight into USB
drivers and their use of coherent and GFP_ATOMIC.  Am I barking up the
wrong tree by looking a the drivers?

On Mon, Jan 14, 2013 at 12:56:57PM +0100, Soeren Moch wrote:
> On 20.11.2012 15:31, Marek Szyprowski wrote:
> >dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
> >regardless the flags provided by the caller. This causes excessive
> >pruning of emergency memory pools without any good reason. Additionaly,
> >on ARM architecture any driver which is using dmapools will sooner or
> >later  trigger the following error:
> >"ERROR: 256 KiB atomic DMA coherent pool is too small!
> >Please increase it with coherent_pool= kernel parameter!".
> >Increasing the coherent pool size usually doesn't help much and only
> >delays such error, because all GFP_ATOMIC DMA allocations are always
> >served from the special, very limited memory pool.
> >
> >This patch changes the dmapool code to correctly use gfp flags provided
> >by the dmapool caller.
> >
> >Reported-by: Soeren Moch <smoch@web.de>
> >Reported-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> >Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >Tested-by: Andrew Lunn <andrew@lunn.ch>
> >Tested-by: Soeren Moch <smoch@web.de>
> 
> Now I tested linux-3.7.1 (this patch is included there) on my Marvell
> Kirkwood system. I still see
> 
>   ERROR: 1024 KiB atomic DMA coherent pool is too small!
>   Please increase it with coherent_pool= kernel parameter!
> 
> after several hours of runtime under heavy load with SATA and
> DVB-Sticks (em28xx / drxk and dib0700).

Could you try running the system w/o the em28xx stick and see how it
goes with v3.7.1?

thx,

Jason.

> As already reported earlier this patch improved the behavior
> compared to linux-3.6.x and 3.7.0 (error after several ten minutes
> runtime), but
> I still see a regression compared to linux-3.5.x. With this kernel the
> same system with same workload runs flawlessly.
> 
> Regards,
> Soeren
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Greg KH Jan. 15, 2013, 5:50 p.m. UTC | #10
On Tue, Jan 15, 2013 at 11:56:42AM -0500, Jason Cooper wrote:
> Greg,
> 
> I've added you to the this thread hoping for a little insight into USB
> drivers and their use of coherent and GFP_ATOMIC.  Am I barking up the
> wrong tree by looking a the drivers?

I don't understand, which drivers are you referring to?  USB host
controller drivers, or the "normal" drivers?  Most USB drivers use
GFP_ATOMIC if they are creating memory during their URB callback path,
as that is interrupt context.  But it shouldn't be all that bad, and the
USB core hasn't changed in a while, so something else must be causing
this.

greg k-h
Sebastian Hesselbarth Jan. 15, 2013, 8:05 p.m. UTC | #11
On 01/15/2013 05:56 PM, Jason Cooper wrote:
> Greg,
>
> I've added you to the this thread hoping for a little insight into USB
> drivers and their use of coherent and GFP_ATOMIC.  Am I barking up the
> wrong tree by looking a the drivers?
>
> On Mon, Jan 14, 2013 at 12:56:57PM +0100, Soeren Moch wrote:
>> On 20.11.2012 15:31, Marek Szyprowski wrote:
>>> dmapool always calls dma_alloc_coherent() with GFP_ATOMIC flag,
>>> regardless the flags provided by the caller. This causes excessive
>>> pruning of emergency memory pools without any good reason. Additionaly,
>>> on ARM architecture any driver which is using dmapools will sooner or
>>> later  trigger the following error:
>>> "ERROR: 256 KiB atomic DMA coherent pool is too small!
>>> Please increase it with coherent_pool= kernel parameter!".
>>> Increasing the coherent pool size usually doesn't help much and only
>>> delays such error, because all GFP_ATOMIC DMA allocations are always
>>> served from the special, very limited memory pool.
>>>
>>> This patch changes the dmapool code to correctly use gfp flags provided
>>> by the dmapool caller.
>>>
>>> Reported-by: Soeren Moch<smoch@web.de>
>>> Reported-by: Thomas Petazzoni<thomas.petazzoni@free-electrons.com>
>>> Signed-off-by: Marek Szyprowski<m.szyprowski@samsung.com>
>>> Tested-by: Andrew Lunn<andrew@lunn.ch>
>>> Tested-by: Soeren Moch<smoch@web.de>
>>
>> Now I tested linux-3.7.1 (this patch is included there) on my Marvell
>> Kirkwood system. I still see
>>
>>    ERROR: 1024 KiB atomic DMA coherent pool is too small!
>>    Please increase it with coherent_pool= kernel parameter!
>>
>> after several hours of runtime under heavy load with SATA and
>> DVB-Sticks (em28xx / drxk and dib0700).
>
> Could you try running the system w/o the em28xx stick and see how it
> goes with v3.7.1?

Jason,

can you point out what you think we should be looking for?

I grep'd for 'GFP_' in  drivers/media/usb and especially for dvb-usb
(dib0700) it looks like most of the buffers in usb-urb.c are allocated
GFP_ATOMIC. em28xx also allocates some of the buffers atomic.

If we look for a mem leak in one of the above drivers (including sata_mv),
is there an easy way to keep track of allocated and freed kernel memory?

Sebastian
Jason Cooper Jan. 15, 2013, 8:16 p.m. UTC | #12
On Tue, Jan 15, 2013 at 09:50:20AM -0800, Greg KH wrote:
> On Tue, Jan 15, 2013 at 11:56:42AM -0500, Jason Cooper wrote:
> > Greg,
> > 
> > I've added you to the this thread hoping for a little insight into USB
> > drivers and their use of coherent and GFP_ATOMIC.  Am I barking up the
> > wrong tree by looking a the drivers?
> 
> I don't understand, which drivers are you referring to?  USB host
> controller drivers, or the "normal" drivers?

Sorry I wasn't clear, I was referring specifically to the usb dvb
drivers em28xx, drxk and dib0700.  These are the drivers reported to be
in heavy use when the error occurs.

sata_mv is also in use, however no other users of sata_mv have reported
problems.  Including myself. ;-)

> Most USB drivers use GFP_ATOMIC if they are creating memory during
> their URB callback path, as that is interrupt context.  But it
> shouldn't be all that bad, and the USB core hasn't changed in a while,
> so something else must be causing this.

Agreed, so I went and did more reading.  The key piece of the puzzle
that I was missing was in arch/arm/mm/dma-mapping.c 660-684.

/*
 * Allocate DMA-coherent memory space and return both the kernel
 * remapped
 * virtual and bus address for that space.
 */
void *arm_dma_alloc(struct device *dev, size_t size, dma_addr_t *handle,
                    gfp_t gfp, struct dma_attrs *attrs)
{
        pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
        void *memory;

        if (dma_alloc_from_coherent(dev, size, handle, &memory))
                return memory;

        return __dma_alloc(dev, size, handle, gfp, prot, false,
                           __builtin_return_address(0));
}

static void *arm_coherent_dma_alloc(struct device *dev, size_t size,
        dma_addr_t *handle, gfp_t gfp, struct dma_attrs *attrs)
{
        pgprot_t prot = __get_dma_pgprot(attrs, pgprot_kernel);
        void *memory;

        if (dma_alloc_from_coherent(dev, size, handle, &memory))
                return memory;

        return __dma_alloc(dev, size, handle, gfp, prot, true,
                           __builtin_return_address(0));
}


My understanding of this code is that when a driver requests dma memory,
we will first try to alloc from the per-driver pool.  If that fails, we
will then attempt to allocate from the atomic_pool.

Once the atomic_pool is exhausted, we get the error:

  ERROR: 1024 KiB atomic DMA coherent pool is too small!
  Please increase it with coherent_pool= kernel parameter!

If my understanding is correct, one of the drivers (most likely one)
either asks for too small of a dma buffer, or is not properly
deallocating blocks from the per-device pool.  Either case leads to
exhaustion, and falling back to the atomic pool.  Which subsequently
gets wiped out as well.

Am I on the right track?

thx,

Jason.
Jason Cooper Jan. 15, 2013, 8:19 p.m. UTC | #13
On Tue, Jan 15, 2013 at 09:05:50PM +0100, Sebastian Hesselbarth wrote:
> If we look for a mem leak in one of the above drivers (including sata_mv),
> is there an easy way to keep track of allocated and freed kernel memory?

I'm inclined to think sata_mv is not the cause here, as there are many
heavy users of it without error reports.  The only thing different here
are the three usb dvb dongles.

thx,

Jason.
Jason Cooper Jan. 15, 2013, 9:56 p.m. UTC | #14
Soeren,

On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
> If my understanding is correct, one of the drivers (most likely one)
> either asks for too small of a dma buffer, or is not properly
> deallocating blocks from the per-device pool.  Either case leads to
> exhaustion, and falling back to the atomic pool.  Which subsequently
> gets wiped out as well.

If my hunch is right, could you please try each of the three dvb drivers
in turn and see which one (or more than one) causes the error?

thx,

Jason.
Soeren Moch Jan. 16, 2013, 12:17 a.m. UTC | #15
On 15.01.2013 22:56, Jason Cooper wrote:
> Soeren,
>
> On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
>> If my understanding is correct, one of the drivers (most likely one)
>> either asks for too small of a dma buffer, or is not properly
>> deallocating blocks from the per-device pool.  Either case leads to
>> exhaustion, and falling back to the atomic pool.  Which subsequently
>> gets wiped out as well.
>
> If my hunch is right, could you please try each of the three dvb drivers
> in turn and see which one (or more than one) causes the error?
>

In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk
demodulator, and dib0700 usb bridge plus dib7000p demod.

I would bet for em28xx causing the error, but this is not thoroughly
tested. Unfortunately testing with removed sticks is not easy, because
this is a production system and disabling some services for the long
time we need to trigger this error will certainly result in unhappy
users.

I will see what I can do here. Is there an easy way to track the buffer
usage without having to wait for complete exhaustion?

In linux-3.5.x there is no such problem. Can we use all available memory
for dma buffers here on armv5 architectures, in contrast to newer
kernels?

Regards,
Soeren
Jason Cooper Jan. 16, 2013, 2:40 a.m. UTC | #16
Soeren,

On Wed, Jan 16, 2013 at 01:17:59AM +0100, Soeren Moch wrote:
> On 15.01.2013 22:56, Jason Cooper wrote:
> >On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
> >>If my understanding is correct, one of the drivers (most likely one)
> >>either asks for too small of a dma buffer, or is not properly
> >>deallocating blocks from the per-device pool.  Either case leads to
> >>exhaustion, and falling back to the atomic pool.  Which subsequently
> >>gets wiped out as well.
> >
> >If my hunch is right, could you please try each of the three dvb drivers
> >in turn and see which one (or more than one) causes the error?
> 
> In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk
> demodulator, and dib0700 usb bridge plus dib7000p demod.
> 
> I would bet for em28xx causing the error, but this is not thoroughly
> tested. Unfortunately testing with removed sticks is not easy, because
> this is a production system and disabling some services for the long
> time we need to trigger this error will certainly result in unhappy
> users.

Just out of curiosity, what board is it?

> I will see what I can do here. Is there an easy way to track the buffer
> usage without having to wait for complete exhaustion?

DMA_API_DEBUG

> In linux-3.5.x there is no such problem. Can we use all available memory
> for dma buffers here on armv5 architectures, in contrast to newer
> kernels?

Were the loads exactly the same when you tested 3.5.x?  I looked at the
changes from v3.5 to v3.7.1 for all four drivers you mentioned as well
as sata_mv.

The biggest thing I see is that all of the media drivers got shuffled
around into their own subdirectories after v3.5.  'git show -M 0c0d06c'
shows it was a clean copy of all the files.

What would be most helpful is if you could do a git bisect between
v3.5.x (working) and the oldest version where you know it started
failing (v3.7.1 or earlier if you know it).

thx,

Jason.
Soeren Moch Jan. 16, 2013, 3:24 a.m. UTC | #17
On 16.01.2013 03:40, Jason Cooper wrote:
> Soeren,
>
> On Wed, Jan 16, 2013 at 01:17:59AM +0100, Soeren Moch wrote:
>> On 15.01.2013 22:56, Jason Cooper wrote:
>>> On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
>>>> If my understanding is correct, one of the drivers (most likely one)
>>>> either asks for too small of a dma buffer, or is not properly
>>>> deallocating blocks from the per-device pool.  Either case leads to
>>>> exhaustion, and falling back to the atomic pool.  Which subsequently
>>>> gets wiped out as well.
>>>
>>> If my hunch is right, could you please try each of the three dvb drivers
>>> in turn and see which one (or more than one) causes the error?
>>
>> In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk
>> demodulator, and dib0700 usb bridge plus dib7000p demod.
>>
>> I would bet for em28xx causing the error, but this is not thoroughly
>> tested. Unfortunately testing with removed sticks is not easy, because
>> this is a production system and disabling some services for the long
>> time we need to trigger this error will certainly result in unhappy
>> users.
>
> Just out of curiosity, what board is it?

The kirkwood board? A modified Guruplug Server Plus.
>
>> I will see what I can do here. Is there an easy way to track the buffer
>> usage without having to wait for complete exhaustion?
>
> DMA_API_DEBUG

OK, maybe I can try this.
>
>> In linux-3.5.x there is no such problem. Can we use all available memory
>> for dma buffers here on armv5 architectures, in contrast to newer
>> kernels?
>
> Were the loads exactly the same when you tested 3.5.x?

Exactly the same, yes.

>I looked at the
> changes from v3.5 to v3.7.1 for all four drivers you mentioned as well
> as sata_mv.
>
> The biggest thing I see is that all of the media drivers got shuffled
> around into their own subdirectories after v3.5.  'git show -M 0c0d06c'
> shows it was a clean copy of all the files.
>
> What would be most helpful is if you could do a git bisect between
> v3.5.x (working) and the oldest version where you know it started
> failing (v3.7.1 or earlier if you know it).
>
I did not bisect it, but Marek mentioned earlier that commit
e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced
new code for dma allocations. This is probably the root cause for the
new (mis-)behavior (due to my tests 3.6.0 is not working anymore).
I'm not very familiar with arm mm code, and from the patch itself I
cannot understand what's different. Maybe CONFIG_CMA is default
also for armv5 (not only v6) now? But I might be totally wrong here,
maybe someone of the mm experts can explain the difference?

Regards,
Soeren
Soeren Moch Jan. 16, 2013, 8:55 a.m. UTC | #18
On 16.01.2013 04:24, Soeren Moch wrote:
> On 16.01.2013 03:40, Jason Cooper wrote:
>> Soeren,
>>
>> On Wed, Jan 16, 2013 at 01:17:59AM +0100, Soeren Moch wrote:
>>> On 15.01.2013 22:56, Jason Cooper wrote:
>>>> On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
>>>>> If my understanding is correct, one of the drivers (most likely one)
>>>>> either asks for too small of a dma buffer, or is not properly
>>>>> deallocating blocks from the per-device pool.  Either case leads to
>>>>> exhaustion, and falling back to the atomic pool.  Which subsequently
>>>>> gets wiped out as well.
>>>>
>>>> If my hunch is right, could you please try each of the three dvb
>>>> drivers
>>>> in turn and see which one (or more than one) causes the error?
>>>
>>> In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk
>>> demodulator, and dib0700 usb bridge plus dib7000p demod.
>>>
>>> I would bet for em28xx causing the error, but this is not thoroughly
>>> tested. Unfortunately testing with removed sticks is not easy, because
>>> this is a production system and disabling some services for the long
>>> time we need to trigger this error will certainly result in unhappy
>>> users.
>>
OK, I could trigger the error
   ERROR: 1024 KiB atomic DMA coherent pool is too small!
   Please increase it with coherent_pool= kernel parameter!
only with em28xx sticks and sata, dib0700 sticks removed.

>> Just out of curiosity, what board is it?
>
> The kirkwood board? A modified Guruplug Server Plus.
em28xx sticks: "TerraTec Cinergy HTC Stick HD" and "PCTV Quatro Stick"
dib0700 sticks: "WinTV-NOVA-TD Stick"
>>
>>> I will see what I can do here. Is there an easy way to track the buffer
>>> usage without having to wait for complete exhaustion?
>>
>> DMA_API_DEBUG
>
> OK, maybe I can try this.
>>
>>> In linux-3.5.x there is no such problem. Can we use all available memory
>>> for dma buffers here on armv5 architectures, in contrast to newer
>>> kernels?
>>
>> Were the loads exactly the same when you tested 3.5.x?
>
> Exactly the same, yes.
>
>> I looked at the
>> changes from v3.5 to v3.7.1 for all four drivers you mentioned as well
>> as sata_mv.
>>
>> The biggest thing I see is that all of the media drivers got shuffled
>> around into their own subdirectories after v3.5.  'git show -M 0c0d06c'
>> shows it was a clean copy of all the files.
>>
>> What would be most helpful is if you could do a git bisect between
>> v3.5.x (working) and the oldest version where you know it started
>> failing (v3.7.1 or earlier if you know it).
>>
> I did not bisect it, but Marek mentioned earlier that commit
> e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced
> new code for dma allocations. This is probably the root cause for the
> new (mis-)behavior (due to my tests 3.6.0 is not working anymore).

I don't want to say that Mareks patch is wrong, probably it triggers a
bug somewhere else! (in em28xx?)

> I'm not very familiar with arm mm code, and from the patch itself I
> cannot understand what's different. Maybe CONFIG_CMA is default
> also for armv5 (not only v6) now? But I might be totally wrong here,
> maybe someone of the mm experts can explain the difference?
>
Regards,
Soeren
Soeren Moch Jan. 16, 2013, 5:32 p.m. UTC | #19
On 16.01.2013 09:55, Soeren Moch wrote:
> On 16.01.2013 04:24, Soeren Moch wrote:
>> On 16.01.2013 03:40, Jason Cooper wrote:
>>> Soeren,
>>>
>>> On Wed, Jan 16, 2013 at 01:17:59AM +0100, Soeren Moch wrote:
>>>> On 15.01.2013 22:56, Jason Cooper wrote:
>>>>> On Tue, Jan 15, 2013 at 03:16:17PM -0500, Jason Cooper wrote:
>>>>>> If my understanding is correct, one of the drivers (most likely one)
>>>>>> either asks for too small of a dma buffer, or is not properly
>>>>>> deallocating blocks from the per-device pool.  Either case leads to
>>>>>> exhaustion, and falling back to the atomic pool.  Which subsequently
>>>>>> gets wiped out as well.
>>>>>
>>>>> If my hunch is right, could you please try each of the three dvb
>>>>> drivers
>>>>> in turn and see which one (or more than one) causes the error?
>>>>
>>>> In fact I use only 2 types of DVB sticks: em28xx usb bridge plus drxk
>>>> demodulator, and dib0700 usb bridge plus dib7000p demod.
>>>>
>>>> I would bet for em28xx causing the error, but this is not thoroughly
>>>> tested. Unfortunately testing with removed sticks is not easy, because
>>>> this is a production system and disabling some services for the long
>>>> time we need to trigger this error will certainly result in unhappy
>>>> users.
>>>
> OK, I could trigger the error
>    ERROR: 1024 KiB atomic DMA coherent pool is too small!
>    Please increase it with coherent_pool= kernel parameter!
> only with em28xx sticks and sata, dib0700 sticks removed.
>
>>> Just out of curiosity, what board is it?
>>
>> The kirkwood board? A modified Guruplug Server Plus.
> em28xx sticks: "TerraTec Cinergy HTC Stick HD" and "PCTV Quatro Stick"
> dib0700 sticks: "WinTV-NOVA-TD Stick"
>>>
>>>> I will see what I can do here. Is there an easy way to track the buffer
>>>> usage without having to wait for complete exhaustion?
>>>
>>> DMA_API_DEBUG
>>
>> OK, maybe I can try this.
>>>
>>>> In linux-3.5.x there is no such problem. Can we use all available
>>>> memory
>>>> for dma buffers here on armv5 architectures, in contrast to newer
>>>> kernels?
>>>
>>> Were the loads exactly the same when you tested 3.5.x?
>>
>> Exactly the same, yes.
>>
>>> I looked at the
>>> changes from v3.5 to v3.7.1 for all four drivers you mentioned as well
>>> as sata_mv.
>>>
>>> The biggest thing I see is that all of the media drivers got shuffled
>>> around into their own subdirectories after v3.5.  'git show -M 0c0d06c'
>>> shows it was a clean copy of all the files.
>>>
>>> What would be most helpful is if you could do a git bisect between
>>> v3.5.x (working) and the oldest version where you know it started
>>> failing (v3.7.1 or earlier if you know it).
>>>
>> I did not bisect it, but Marek mentioned earlier that commit
>> e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced
>> new code for dma allocations. This is probably the root cause for the
>> new (mis-)behavior (due to my tests 3.6.0 is not working anymore).
>
> I don't want to say that Mareks patch is wrong, probably it triggers a
> bug somewhere else! (in em28xx?)

The em28xx sticks are using isochronous usb transfers. Is there a
special handling for that?

>> I'm not very familiar with arm mm code, and from the patch itself I
>> cannot understand what's different. Maybe CONFIG_CMA is default
>> also for armv5 (not only v6) now? But I might be totally wrong here,
>> maybe someone of the mm experts can explain the difference?
>>
Regards,
Soeren
Jason Cooper Jan. 16, 2013, 5:47 p.m. UTC | #20
On Wed, Jan 16, 2013 at 06:32:09PM +0100, Soeren Moch wrote:
> On 16.01.2013 09:55, Soeren Moch wrote:
> >On 16.01.2013 04:24, Soeren Moch wrote:
> >>I did not bisect it, but Marek mentioned earlier that commit
> >>e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced
> >>new code for dma allocations. This is probably the root cause for the
> >>new (mis-)behavior (due to my tests 3.6.0 is not working anymore).
> >
> >I don't want to say that Mareks patch is wrong, probably it triggers a
> >bug somewhere else! (in em28xx?)
> 
> The em28xx sticks are using isochronous usb transfers. Is there a
> special handling for that?

I'm looking at that now.  It looks like the em28xx wants (as a maximum)
655040 bytes (em28xx-core.c:1088).  There are 5 transfer buffers, with
64 max packets and 2047 max packet size (runtime reported max & 0x7ff).

If it actually needs all of that, then the answer may be to just
increase coherent_pool= when using that driver.  I'll keep digging.

thx,

Jason.
Soeren Moch Jan. 16, 2013, 10:36 p.m. UTC | #21
On 16.01.2013 18:47, Jason Cooper wrote:
> On Wed, Jan 16, 2013 at 06:32:09PM +0100, Soeren Moch wrote:
>> On 16.01.2013 09:55, Soeren Moch wrote:
>>> On 16.01.2013 04:24, Soeren Moch wrote:
>>>> I did not bisect it, but Marek mentioned earlier that commit
>>>> e9da6e9905e639b0f842a244bc770b48ad0523e9 in Linux v3.6-rc1 introduced
>>>> new code for dma allocations. This is probably the root cause for the
>>>> new (mis-)behavior (due to my tests 3.6.0 is not working anymore).
>>>
>>> I don't want to say that Mareks patch is wrong, probably it triggers a
>>> bug somewhere else! (in em28xx?)
>>
>> The em28xx sticks are using isochronous usb transfers. Is there a
>> special handling for that?
>
> I'm looking at that now.  It looks like the em28xx wants (as a maximum)
> 655040 bytes (em28xx-core.c:1088).  There are 5 transfer buffers, with
> 64 max packets and 2047 max packet size (runtime reported max & 0x7ff).
>
> If it actually needs all of that, then the answer may be to just
> increase coherent_pool= when using that driver.  I'll keep digging.

I already tested with 4M coherent pool size and could not see
significant improvement. Would it make sense to further increase the
buffer size?

Regards,
Soeren
Arnd Bergmann Jan. 17, 2013, 10:49 a.m. UTC | #22
On Wednesday 16 January 2013, Soeren Moch wrote:
> >> I will see what I can do here. Is there an easy way to track the buffer
> >> usage without having to wait for complete exhaustion?
> >
> > DMA_API_DEBUG
> 
> OK, maybe I can try this.
> >

Any success with this? It should at least tell you if there is a
memory leak in one of the drivers.

	Arnd
Soeren Moch Jan. 17, 2013, 1:47 p.m. UTC | #23
On 17.01.2013 11:49, Arnd Bergmann wrote:
> On Wednesday 16 January 2013, Soeren Moch wrote:
>>>> I will see what I can do here. Is there an easy way to track the buffer
>>>> usage without having to wait for complete exhaustion?
>>>
>>> DMA_API_DEBUG
>>
>> OK, maybe I can try this.
>>>
>
> Any success with this? It should at least tell you if there is a
> memory leak in one of the drivers.

Not yet, sorry. I have to do all the tests in my limited spare time.
Can you tell me what to search for in the debug output?

Soeren
diff mbox

Patch

diff --git a/mm/dmapool.c b/mm/dmapool.c
index c5ab33b..da1b0f0 100644
--- a/mm/dmapool.c
+++ b/mm/dmapool.c
@@ -50,7 +50,6 @@  struct dma_pool {		/* the pool */
 	size_t allocation;
 	size_t boundary;
 	char name[32];
-	wait_queue_head_t waitq;
 	struct list_head pools;
 };
 
@@ -62,8 +61,6 @@  struct dma_page {		/* cacheable header for 'allocation' bytes */
 	unsigned int offset;
 };
 
-#define	POOL_TIMEOUT_JIFFIES	((100 /* msec */ * HZ) / 1000)
-
 static DEFINE_MUTEX(pools_lock);
 
 static ssize_t
@@ -172,7 +169,6 @@  struct dma_pool *dma_pool_create(const char *name, struct device *dev,
 	retval->size = size;
 	retval->boundary = boundary;
 	retval->allocation = allocation;
-	init_waitqueue_head(&retval->waitq);
 
 	if (dev) {
 		int ret;
@@ -227,7 +223,6 @@  static struct dma_page *pool_alloc_page(struct dma_pool *pool, gfp_t mem_flags)
 		memset(page->vaddr, POOL_POISON_FREED, pool->allocation);
 #endif
 		pool_initialise_page(pool, page);
-		list_add(&page->page_list, &pool->page_list);
 		page->in_use = 0;
 		page->offset = 0;
 	} else {
@@ -315,30 +310,21 @@  void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 	might_sleep_if(mem_flags & __GFP_WAIT);
 
 	spin_lock_irqsave(&pool->lock, flags);
- restart:
 	list_for_each_entry(page, &pool->page_list, page_list) {
 		if (page->offset < pool->allocation)
 			goto ready;
 	}
-	page = pool_alloc_page(pool, GFP_ATOMIC);
-	if (!page) {
-		if (mem_flags & __GFP_WAIT) {
-			DECLARE_WAITQUEUE(wait, current);
 
-			__set_current_state(TASK_UNINTERRUPTIBLE);
-			__add_wait_queue(&pool->waitq, &wait);
-			spin_unlock_irqrestore(&pool->lock, flags);
+	/* pool_alloc_page() might sleep, so temporarily drop &pool->lock */
+	spin_unlock_irqrestore(&pool->lock, flags);
 
-			schedule_timeout(POOL_TIMEOUT_JIFFIES);
+	page = pool_alloc_page(pool, mem_flags);
+	if (!page)
+		return NULL;
 
-			spin_lock_irqsave(&pool->lock, flags);
-			__remove_wait_queue(&pool->waitq, &wait);
-			goto restart;
-		}
-		retval = NULL;
-		goto done;
-	}
+	spin_lock_irqsave(&pool->lock, flags);
 
+	list_add(&page->page_list, &pool->page_list);
  ready:
 	page->in_use++;
 	offset = page->offset;
@@ -348,7 +334,6 @@  void *dma_pool_alloc(struct dma_pool *pool, gfp_t mem_flags,
 #ifdef	DMAPOOL_DEBUG
 	memset(retval, POOL_POISON_ALLOCATED, pool->size);
 #endif
- done:
 	spin_unlock_irqrestore(&pool->lock, flags);
 	return retval;
 }
@@ -435,8 +420,6 @@  void dma_pool_free(struct dma_pool *pool, void *vaddr, dma_addr_t dma)
 	page->in_use--;
 	*(int *)vaddr = page->offset;
 	page->offset = offset;
-	if (waitqueue_active(&pool->waitq))
-		wake_up_locked(&pool->waitq);
 	/*
 	 * Resist a temptation to do
 	 *    if (!is_page_busy(page)) pool_free_page(pool, page);