| Message ID | 1353421905-3112-1-git-send-email-m.szyprowski@samsung.com |
|---|---|
| State | Accepted |
| Commit | 387870f2d6d679746020fa8e25ef786ff338dc98 |
| Headers | show |
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?
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
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
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.
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
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.
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
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
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
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
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
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.
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.
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.
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
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.
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
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
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
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.
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
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
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 --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);