[RFC PATCH 02/16] drm/ttm/pool: Fix ttm_pool_alloc error path
Christian König
christian.koenig at amd.com
Wed Feb 15 18:26:23 UTC 2023
Am 15.02.23 um 19:02 schrieb Thomas Hellström:
> On Wed, 2023-02-15 at 18:31 +0100, Christian König wrote:
>> Am 15.02.23 um 17:13 schrieb Thomas Hellström:
>>> When hitting an error, the error path forgot to unmap dma mappings
>>> and
>> I don't see where this happens?
> From what I can tell, ttm_pool_page_allocated() maps the page for dma,
> If we later hit an error, ttm_pool_free_page() will leak the mapping.
Ah, I see. Good point.
>
>>> could call set_pages_wb() on already uncached pages.
>> Yeah, but what's the problem?
> Umm, at least if you try to set WC on an already WC'd page, the
> set_pages_ code will spam dmesg with warnings.
> Not sure if set_pages_wb() on WB pages does the same, nor if it
> issues unnecessary global cache / tlb flushes or whether that will
> change in the future.
> The point of avoiding the set_pages_wb() when already WB is you don't
> have to check, and you don't have to care.
Please just open code the error handling then. That helper function
looks horrible complicated to me.
Alternatively we could have a free function for a range of pages.
Regards,
Christian.
>
> That said, the __ttm_pool_free() is used also in upcoming patches.
>
> /Thomas
>
>
>> Regards,
>> Christian.
>>
>>> Fix this by introducing a common __ttm_pool_free() function that
>>> does the right thing.
>>>
>>> Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Dave Airlie <airlied at redhat.com>
>>> Cc: Madhav Chauhan <madhav.chauhan at amd.com>
>>> Cc: Christian Koenig <christian.koenig at amd.com>
>>> Cc: Huang Rui <ray.huang at amd.com>
>>> Cc: dri-devel at lists.freedesktop.org
>>> Signed-off-by: Thomas Hellström <thomas.hellstrom at linux.intel.com>
>>> ---
>>> drivers/gpu/drm/ttm/ttm_pool.c | 74 +++++++++++++++++++++--------
>>> -----
>>> 1 file changed, 45 insertions(+), 29 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ttm/ttm_pool.c
>>> b/drivers/gpu/drm/ttm/ttm_pool.c
>>> index aa116a7bbae3..1cc7591a9542 100644
>>> --- a/drivers/gpu/drm/ttm/ttm_pool.c
>>> +++ b/drivers/gpu/drm/ttm/ttm_pool.c
>>> @@ -367,6 +367,39 @@ static int ttm_pool_page_allocated(struct
>>> ttm_pool *pool, unsigned int order,
>>> return 0;
>>> }
>>>
>>> +static void __ttm_pool_free(struct ttm_pool *pool, struct ttm_tt
>>> *tt,
>>> + struct page **caching_divide,
>>> + enum ttm_caching initial_caching,
>>> + enum ttm_caching subseq_caching,
>>> + pgoff_t num_pages)
>>> +{
>>> + enum ttm_caching caching = subseq_caching;
>>> + struct page **pages = tt->pages;
>>> + unsigned int order;
>>> + pgoff_t i, nr;
>>> +
>>> + if (pool && caching_divide)
>>> + caching = initial_caching;
>>> +
>>> + for (i = 0; i < num_pages; i += nr, pages += nr) {
>>> + struct ttm_pool_type *pt = NULL;
>>> +
>>> + if (unlikely(caching_divide == pages))
>>> + caching = subseq_caching;
>>> +
>>> + order = ttm_pool_page_order(pool, *pages);
>>> + nr = (1UL << order);
>>> + if (tt->dma_address)
>>> + ttm_pool_unmap(pool, tt->dma_address[i],
>>> nr);
>>> +
>>> + pt = ttm_pool_select_type(pool, caching, order);
>>> + if (pt)
>>> + ttm_pool_type_give(pt, *pages);
>>> + else
>>> + ttm_pool_free_page(pool, caching, order,
>>> *pages);
>>> + }
>>> +}
>>> +
>>> /**
>>> * ttm_pool_alloc - Fill a ttm_tt object
>>> *
>>> @@ -386,8 +419,9 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> dma_addr_t *dma_addr = tt->dma_address;
>>> struct page **caching = tt->pages;
>>> struct page **pages = tt->pages;
>>> + enum ttm_caching page_caching;
>>> gfp_t gfp_flags = GFP_USER;
>>> - unsigned int i, order;
>>> + unsigned int order;
>>> struct page *p;
>>> int r;
>>>
>>> @@ -410,6 +444,7 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> order = min_t(unsigned int, order, __fls(num_pages)))
>>> {
>>> struct ttm_pool_type *pt;
>>>
>>> + page_caching = tt->caching;
>>> pt = ttm_pool_select_type(pool, tt->caching,
>>> order);
>>> p = pt ? ttm_pool_type_take(pt) : NULL;
>>> if (p) {
>>> @@ -418,6 +453,7 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> if (r)
>>> goto error_free_page;
>>>
>>> + caching = pages;
>>> do {
>>> r = ttm_pool_page_allocated(pool,
>>> order, p,
>>>
>>> &dma_addr,
>>> @@ -426,14 +462,15 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> if (r)
>>> goto error_free_page;
>>>
>>> + caching = pages;
>>> if (num_pages < (1 << order))
>>> break;
>>>
>>> p = ttm_pool_type_take(pt);
>>> } while (p);
>>> - caching = pages;
>>> }
>>>
>>> + page_caching = ttm_cached;
>>> while (num_pages >= (1 << order) &&
>>> (p = ttm_pool_alloc_page(pool, gfp_flags,
>>> order))) {
>>>
>>> @@ -442,6 +479,7 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> tt-
>>>> caching);
>>> if (r)
>>> goto error_free_page;
>>> + caching = pages;
>>> }
>>> r = ttm_pool_page_allocated(pool, order, p,
>>> &dma_addr,
>>> &num_pages,
>>> &pages);
>>> @@ -468,15 +506,12 @@ int ttm_pool_alloc(struct ttm_pool *pool,
>>> struct ttm_tt *tt,
>>> return 0;
>>>
>>> error_free_page:
>>> - ttm_pool_free_page(pool, tt->caching, order, p);
>>> + ttm_pool_free_page(pool, page_caching, order, p);
>>>
>>> error_free_all:
>>> num_pages = tt->num_pages - num_pages;
>>> - for (i = 0; i < num_pages; ) {
>>> - order = ttm_pool_page_order(pool, tt->pages[i]);
>>> - ttm_pool_free_page(pool, tt->caching, order, tt-
>>>> pages[i]);
>>> - i += 1 << order;
>>> - }
>>> + __ttm_pool_free(pool, tt, caching, tt->caching, ttm_cached,
>>> + num_pages);
>>>
>>> return r;
>>> }
>>> @@ -492,27 +527,8 @@ EXPORT_SYMBOL(ttm_pool_alloc);
>>> */
>>> void ttm_pool_free(struct ttm_pool *pool, struct ttm_tt *tt)
>>> {
>>> - unsigned int i;
>>> -
>>> - for (i = 0; i < tt->num_pages; ) {
>>> - struct page *p = tt->pages[i];
>>> - unsigned int order, num_pages;
>>> - struct ttm_pool_type *pt;
>>> -
>>> - order = ttm_pool_page_order(pool, p);
>>> - num_pages = 1ULL << order;
>>> - if (tt->dma_address)
>>> - ttm_pool_unmap(pool, tt->dma_address[i],
>>> num_pages);
>>> -
>>> - pt = ttm_pool_select_type(pool, tt->caching,
>>> order);
>>> - if (pt)
>>> - ttm_pool_type_give(pt, tt->pages[i]);
>>> - else
>>> - ttm_pool_free_page(pool, tt->caching,
>>> order,
>>> - tt->pages[i]);
>>> -
>>> - i += num_pages;
>>> - }
>>> + __ttm_pool_free(pool, tt, NULL, tt->caching, tt->caching,
>>> + tt->num_pages);
>>>
>>> while (atomic_long_read(&allocated_pages) > page_pool_size)
>>> ttm_pool_shrink();
More information about the dri-devel
mailing list