On Tue, Jan 12, 2021 at 06:49:18PM +0800, Christian König wrote:

Ping? Ray can I get an acked-by? It's an important bug fix.

Thanks, Christian.

Am 11.01.21 um 14:57 schrieb Christian König:

...

set_pages_wb() might sleep and so we can't do this in an atomic context.

Signed-off-by: Christian König christian.koenig@amd.com Reported-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")

Sorry, missed this mail yesterday.

Patch looks good for me.

Reviewed-by: Huang Rui ray.huang@amd.com

...

---

drivers/gpu/drm/ttm/ttm_pool.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c index a00b7ab9c14c..6a6eeba423d1 100644 --- a/drivers/gpu/drm/ttm/ttm_pool.c +++ b/drivers/gpu/drm/ttm/ttm_pool.c @@ -66,7 +66,7 @@ static struct ttm_pool_type global_uncached[MAX_ORDER]; static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER]; static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];

-static spinlock_t shrinker_lock; +static struct mutex shrinker_lock; static struct list_head shrinker_list; static struct shrinker mm_shrinker;

@@ -249,9 +249,9 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool, spin_lock_init(&pt->lock); INIT_LIST_HEAD(&pt->pages);

• spin_lock(&shrinker_lock);

• mutex_lock(&shrinker_lock); list_add_tail(&pt->shrinker_list, &shrinker_list);

• spin_unlock(&shrinker_lock);

• mutex_unlock(&shrinker_lock); }

  /* Remove a pool_type from the global shrinker list and free all pages */

@@ -259,9 +259,9 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt) { struct page *p, *tmp;

• spin_lock(&shrinker_lock);

• mutex_lock(&shrinker_lock); list_del(&pt->shrinker_list);

• spin_unlock(&shrinker_lock);

• mutex_unlock(&shrinker_lock);

  list_for_each_entry_safe(p, tmp, &pt->pages, lru) ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);

@@ -302,7 +302,7 @@ static unsigned int ttm_pool_shrink(void) unsigned int num_freed; struct page *p;

• spin_lock(&shrinker_lock);

• mutex_lock(&shrinker_lock); pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);

  p = ttm_pool_type_take(pt);

@@ -314,7 +314,7 @@ static unsigned int ttm_pool_shrink(void) }

list_move_tail(&pt->shrinker_list, &shrinker_list);

• spin_unlock(&shrinker_lock);

• mutex_unlock(&shrinker_lock);

  return num_freed; }

@@ -564,7 +564,7 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m) { unsigned int i;

• spin_lock(&shrinker_lock);

• mutex_lock(&shrinker_lock);

  seq_puts(m, "\t "); for (i = 0; i < MAX_ORDER; ++i)

@@ -600,7 +600,7 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m) seq_printf(m, "\ntotal\t: %8lu of %8lu\n", atomic_long_read(&allocated_pages), page_pool_size);

• spin_unlock(&shrinker_lock);

• mutex_unlock(&shrinker_lock);

  return 0; }

@@ -644,7 +644,7 @@ int ttm_pool_mgr_init(unsigned long num_pages) if (!page_pool_size) page_pool_size = num_pages;

• spin_lock_init(&shrinker_lock);

• mutex_init(&shrinker_lock); INIT_LIST_HEAD(&shrinker_list);

  for (i = 0; i < MAX_ORDER; ++i) {

Am 12.01.21 um 15:03 schrieb Daniel Vetter:

On Mon, Jan 11, 2021 at 2:57 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:

...

set_pages_wb() might sleep and so we can't do this in an atomic context.

Signed-off-by: Christian König christian.koenig@amd.com Reported-by: Mikhail Gavrilov mikhail.v.gavrilov@gmail.com Fixes: d099fc8f540a ("drm/ttm: new TT backend allocation pool v3")

Hm I guess long term proper fix would be to pull the shrinker into two parts, first part takes the right amount of entries of the list, holding the lock. 2nd part does the releasing (or maybe at least in batches). lru locks should be cheap, doing expensive stuff like flushing or rewriting ptes might not be the best idea.

Yeah, agree. It should actually be trivial, but I didn't wanted the churn in fixes.

So just going for the easy fix now and the optimal later on.

Regards, Christian.

-Daniel

...

---

drivers/gpu/drm/ttm/ttm_pool.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_pool.c b/drivers/gpu/drm/ttm/ttm_pool.c index a00b7ab9c14c..6a6eeba423d1 100644 --- a/drivers/gpu/drm/ttm/ttm_pool.c +++ b/drivers/gpu/drm/ttm/ttm_pool.c @@ -66,7 +66,7 @@ static struct ttm_pool_type global_uncached[MAX_ORDER]; static struct ttm_pool_type global_dma32_write_combined[MAX_ORDER]; static struct ttm_pool_type global_dma32_uncached[MAX_ORDER];

-static spinlock_t shrinker_lock; +static struct mutex shrinker_lock; static struct list_head shrinker_list; static struct shrinker mm_shrinker;

@@ -249,9 +249,9 @@ static void ttm_pool_type_init(struct ttm_pool_type *pt, struct ttm_pool *pool, spin_lock_init(&pt->lock); INIT_LIST_HEAD(&pt->pages);

•   spin_lock(&shrinker_lock);
  

•   mutex_lock(&shrinker_lock);
     list_add_tail(&pt->shrinker_list, &shrinker_list);
  

•   spin_unlock(&shrinker_lock);
  

•   mutex_unlock(&shrinker_lock);
  

  }

  /* Remove a pool_type from the global shrinker list and free all pages */

@@ -259,9 +259,9 @@ static void ttm_pool_type_fini(struct ttm_pool_type *pt) { struct page *p, *tmp;

•   spin_lock(&shrinker_lock);
  

•   mutex_lock(&shrinker_lock);
     list_del(&pt->shrinker_list);
  

•   spin_unlock(&shrinker_lock);
  

•   mutex_unlock(&shrinker_lock);
  
     list_for_each_entry_safe(p, tmp, &pt->pages, lru)
             ttm_pool_free_page(pt->pool, pt->caching, pt->order, p);
  

@@ -302,7 +302,7 @@ static unsigned int ttm_pool_shrink(void) unsigned int num_freed; struct page *p;

•   spin_lock(&shrinker_lock);
  

•   mutex_lock(&shrinker_lock);
     pt = list_first_entry(&shrinker_list, typeof(*pt), shrinker_list);
  
     p = ttm_pool_type_take(pt);
  

@@ -314,7 +314,7 @@ static unsigned int ttm_pool_shrink(void) }

     list_move_tail(&pt->shrinker_list, &shrinker_list);

•   spin_unlock(&shrinker_lock);
  

•   mutex_unlock(&shrinker_lock);
  
     return num_freed;
  

  }

@@ -564,7 +564,7 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m) { unsigned int i;

•   spin_lock(&shrinker_lock);
  

•   mutex_lock(&shrinker_lock);
  
     seq_puts(m, "\t ");
     for (i = 0; i < MAX_ORDER; ++i)
  

@@ -600,7 +600,7 @@ int ttm_pool_debugfs(struct ttm_pool *pool, struct seq_file *m) seq_printf(m, "\ntotal\t: %8lu of %8lu\n", atomic_long_read(&allocated_pages), page_pool_size);

•   spin_unlock(&shrinker_lock);
  

•   mutex_unlock(&shrinker_lock);
  
     return 0;
  

  }

@@ -644,7 +644,7 @@ int ttm_pool_mgr_init(unsigned long num_pages) if (!page_pool_size) page_pool_size = num_pages;

•   spin_lock_init(&shrinker_lock);
  

•   mutex_init(&shrinker_lock);
     INIT_LIST_HEAD(&shrinker_list);
  
     for (i = 0; i < MAX_ORDER; ++i) {
  

-- 2.25.1

---

dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel