Re: [RFC PATCH 02/16] drm/ttm/pool: Fix ttm_pool_alloc error path

["Thomas Hellström" <thomas.hellstrom@linux.intel.com>]

On Wed, 2023-02-15 at 19:26 +0100, Christian König wrote:
> 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.

OK, I'll see if this is doable without adding a tremendous amount of
code.

/Thomas


> 
> 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@amd.com>
> > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > Cc: Madhav Chauhan <madhav.chauhan@amd.com>
> > > > Cc: Christian Koenig <christian.koenig@amd.com>
> > > > Cc: Huang Rui <ray.huang@amd.com>
> > > > Cc: dri-devel@lists.freedesktop.org
> > > > Signed-off-by: Thomas Hellström
> > > > <thomas.hellstrom@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();
>