Re: CONFIG_DMA_CMA causes ttm performance problems/hangs.

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On 08/11/2014 05:17 PM, Jerome Glisse wrote:
> On Mon, Aug 11, 2014 at 12:11:21PM +0200, Thomas Hellstrom wrote:
>> On 08/10/2014 08:02 PM, Mario Kleiner wrote:
>>> On 08/10/2014 01:03 PM, Thomas Hellstrom wrote:
>>>> On 08/10/2014 05:11 AM, Mario Kleiner wrote:
>>>>> Resent this time without HTML formatting which lkml doesn't like.
>>>>> Sorry.
>>>>>
>>>>> On 08/09/2014 03:58 PM, Thomas Hellstrom wrote:
>>>>>> On 08/09/2014 03:33 PM, Konrad Rzeszutek Wilk wrote:
>>>>>>> On August 9, 2014 1:39:39 AM EDT, Thomas
>>>>>>> Hellstrom<thellstrom@vmware.com>  wrote:
>>>>>>>> Hi.
>>>>>>>>
>>>>>>> Hey Thomas!
>>>>>>>
>>>>>>>> IIRC I don't think the TTM DMA pool allocates coherent pages more
>>>>>>>> than
>>>>>>>> one page at a time, and _if that's true_ it's pretty unnecessary for
>>>>>>>> the
>>>>>>>> dma subsystem to route those allocations to CMA. Maybe Konrad could
>>>>>>>> shed
>>>>>>>> some light over this?
>>>>>>> It should allocate in batches and keep them in the TTM DMA pool for
>>>>>>> some time to be reused.
>>>>>>>
>>>>>>> The pages that it gets are in 4kb granularity though.
>>>>>> Then I feel inclined to say this is a DMA subsystem bug. Single page
>>>>>> allocations shouldn't get routed to CMA.
>>>>>>
>>>>>> /Thomas
>>>>> Yes, seems you're both right. I read through the code a bit more and
>>>>> indeed the TTM DMA pool allocates only one page during each
>>>>> dma_alloc_coherent() call, so it doesn't need CMA memory. The current
>>>>> allocators don't check for single page CMA allocations and therefore
>>>>> try to get it from the CMA area anyway, instead of skipping to the
>>>>> much cheaper fallback.
>>>>>
>>>>> So the callers of dma_alloc_from_contiguous() could need that little
>>>>> optimization of skipping it if only one page is requested. For
>>>>>
>>>>> dma_generic_alloc_coherent
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3Ddma_generic_alloc_coherent&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=d1852625e2ab2ff07eb34a7f33fc1f55f7f13959912d5a6ce9316d23070ce939>
>>>>>
>>>>> andintel_alloc_coherent
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3Dintel_alloc_coherent&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=82d587e9b6aeced5cf9a7caefa91bf47fba809f3522b7379d22e45a2d5d35ebd>
>>>>> this
>>>>> seems easy to do. Looking at the arm arch variants, e.g.,
>>>>>
>>>>> https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/source/arch/arm/mm/dma-mapping.c%23L1194&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=4c178257eab9b5d7ca650dedba76cf27abeb49ddc7aebb9433f52b6c8bb3bbac
>>>>>
>>>>>
>>>>> and
>>>>>
>>>>> https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/source/arch/arm64/mm/dma-mapping.c%23L44&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=5f62f4cbe8cee1f1dd4cbba656354efe6867bcdc664cf90e9719e2f42a85de08
>>>>>
>>>>>
>>>>> i'm not sure if it is that easily done, as there aren't any fallbacks
>>>>> for such a case and the code looks to me as if that's at least
>>>>> somewhat intentional.
>>>>>
>>>>> As far as TTM goes, one quick one-line fix to prevent it from using
>>>>> the CMA at least on SWIOTLB, NOMMU and Intel IOMMU (when using the
>>>>> above methods) would be to clear the __GFP_WAIT
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3D__GFP_WAIT&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=d56d076770d3416264be6c9ea2829ac0d6951203696fa3ad04144f13307577bc>
>>>>> flag from the
>>>>> passed gfp_t flags. That would trigger the well working fallback.
>>>>> So, is
>>>>>
>>>>> __GFP_WAIT
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3D__GFP_WAIT&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=d56d076770d3416264be6c9ea2829ac0d6951203696fa3ad04144f13307577bc>
>>>>> needed
>>>>> for those single page allocations that go through__ttm_dma_alloc_page
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3D__ttm_dma_alloc_page&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=7898522bba274e4dcc332735fbcf0c96e48918f60c2ee8e9a3e9c73ab3487bd0>?
>>>>>
>>>>>
>>>>> It would be nice to have such a simple, non-intrusive one-line patch
>>>>> that we still could get into 3.17 and then backported to older stable
>>>>> kernels to avoid the same desktop hangs there if CMA is enabled. It
>>>>> would be also nice for actual users of CMA to not use up lots of CMA
>>>>> space for gpu's which don't need it. I think DMA_CMA was introduced
>>>>> around 3.12.
>>>>>
>>>> I don't think that's a good idea. Omitting __GFP_WAIT would cause
>>>> unnecessary memory allocation errors on systems under stress.
>>>> I think this should be filed as a DMA subsystem kernel bug / regression
>>>> and an appropriate solution should be worked out together with the DMA
>>>> subsystem maintainers and then backported.
>>> Ok, so it is needed. I'll file a bug report.
>>>
>>>>> The other problem is that probably TTM does not reuse pages from the
>>>>> DMA pool. If i trace the __ttm_dma_alloc_page
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3D__ttm_dma_alloc_page&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=7898522bba274e4dcc332735fbcf0c96e48918f60c2ee8e9a3e9c73ab3487bd0>
>>>>> and
>>>>> __ttm_dma_free_page
>>>>> <https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/ident?i%3D__ttm_dma_alloc_page&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=7898522bba274e4dcc332735fbcf0c96e48918f60c2ee8e9a3e9c73ab3487bd0>
>>>>> calls for
>>>>> those single page allocs/frees, then over a 20 second interval of
>>>>> tracing and switching tabs in firefox, scrolling things around etc. i
>>>>> find about as many alloc's as i find free's, e.g., 1607 allocs vs.
>>>>> 1648 frees.
>>>> This is because historically the pools have been designed to keep only
>>>> pages with nonstandard caching attributes since changing page caching
>>>> attributes have been very slow but the kernel page allocators have been
>>>> reasonably fast.
>>>>
>>>> /Thomas
>>> Ok. A bit more ftraceing showed my hang problem case goes through the
>>> "if (is_cached)" paths, so the pool doesn't recycle anything and i see
>>> it bouncing up and down by 4 pages all the time.
>>>
>>> But for the non-cached case, which i don't hit with my problem, could
>>> one of you look at line 954...
>>>
>>> https://urldefense.proofpoint.com/v1/url?u=http://lxr.free-electrons.com/source/drivers/gpu/drm/ttm/ttm_page_alloc_dma.c%23L954&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=l5Ago9ekmVFZ3c4M6eauqrJWGwjf6fTb%2BP3CxbBFkVM%3D%0A&m=QQSN6uVpEiw6RuWLAfK%2FKWBFV5HspJUfDh4Y2mUz%2FH4%3D%0A&s=e15c51805d429ee6d8960d6b88035e9811a1cdbfbf13168eec2fbb2214b99c60
>>>
>>>
>>> ... and tell me why that unconditional npages = count; assignment
>>> makes sense? It seems to essentially disable all recycling for the dma
>>> pool whenever the pool isn't filled up to/beyond its maximum with free
>>> pages? When the pool is filled up, lots of stuff is recycled, but when
>>> it is already somewhat below capacity, it gets "punished" by not
>>> getting refilled? I'd just like to understand the logic behind that line.
>>>
>>> thanks,
>>> -mario
>> I'll happily forward that question to Konrad who wrote the code (or it
>> may even stem from the ordinary page pool code which IIRC has Dave
>> Airlie / Jerome Glisse as authors)
> This is effectively bogus code, i now wonder how it came to stay alive.
> Attached patch will fix that.

Yes, that makes sense to me. Fwiw,

Reviewed-by: Mario Kleiner <mario.kleiner.de@gmail.com>

-mario