Re: [PATCH 4/6] drm/ttm: rework BO delayed delete.

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



comments inline

> 2020年2月11日 13:26,Pan, Xinhui <Xinhui.Pan@xxxxxxx> 写道:
> 
> comments inline.
> [xh]
> 
> 
>> 2020年2月10日 23:09,Christian König <ckoenig.leichtzumerken@xxxxxxxxx> 写道:
>> 
>> This patch reworks the whole delayed deletion of BOs which aren't idle.
>> 
>> Instead of having two counters for the BO structure we resurrect the BO
>> when we find that a deleted BO is not idle yet.
>> 
>> This has many advantages, especially that we don't need to
>> increment/decrement the BOs reference counter any more when it
>> moves on the LRUs.
>> 
>> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
>> ---
>> drivers/gpu/drm/ttm/ttm_bo.c      | 217 +++++++++++++-----------------
>> drivers/gpu/drm/ttm/ttm_bo_util.c |   1 -
>> include/drm/ttm/ttm_bo_api.h      |  11 +-
>> 3 files changed, 97 insertions(+), 132 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
>> index e12fc2c2d165..d0624685f5d2 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo.c
>> @@ -145,26 +145,6 @@ static inline uint32_t ttm_bo_type_flags(unsigned type)
>> 	return 1 << (type);
>> }
>> 
>> -static void ttm_bo_release_list(struct kref *list_kref)
>> -{
>> -	struct ttm_buffer_object *bo =
>> -	    container_of(list_kref, struct ttm_buffer_object, list_kref);
>> -	size_t acc_size = bo->acc_size;
>> -
>> -	BUG_ON(kref_read(&bo->list_kref));
>> -	BUG_ON(kref_read(&bo->kref));
>> -	BUG_ON(bo->mem.mm_node != NULL);
>> -	BUG_ON(!list_empty(&bo->lru));
>> -	BUG_ON(!list_empty(&bo->ddestroy));
>> -	ttm_tt_destroy(bo->ttm);
>> -	atomic_dec(&ttm_bo_glob.bo_count);
>> -	dma_fence_put(bo->moving);
>> -	if (!ttm_bo_uses_embedded_gem_object(bo))
>> -		dma_resv_fini(&bo->base._resv);
>> -	bo->destroy(bo);
>> -	ttm_mem_global_free(&ttm_mem_glob, acc_size);
>> -}
>> -
>> static void ttm_bo_add_mem_to_lru(struct ttm_buffer_object *bo,
>> 				  struct ttm_mem_reg *mem)
>> {
>> @@ -181,21 +161,14 @@ static void ttm_bo_add_mem_to_lru(struct ttm_buffer_object *bo,
>> 
>> 	man = &bdev->man[mem->mem_type];
>> 	list_add_tail(&bo->lru, &man->lru[bo->priority]);
>> -	kref_get(&bo->list_kref);
>> 
>> 	if (!(man->flags & TTM_MEMTYPE_FLAG_FIXED) && bo->ttm &&
>> 	    !(bo->ttm->page_flags & (TTM_PAGE_FLAG_SG |
>> 				     TTM_PAGE_FLAG_SWAPPED))) {
>> 		list_add_tail(&bo->swap, &ttm_bo_glob.swap_lru[bo->priority]);
>> -		kref_get(&bo->list_kref);
>> 	}
>> }
>> 
>> -static void ttm_bo_ref_bug(struct kref *list_kref)
>> -{
>> -	BUG();
>> -}
>> -
>> static void ttm_bo_del_from_lru(struct ttm_buffer_object *bo)
>> {
>> 	struct ttm_bo_device *bdev = bo->bdev;
>> @@ -203,12 +176,10 @@ static void ttm_bo_del_from_lru(struct ttm_buffer_object *bo)
>> 
>> 	if (!list_empty(&bo->swap)) {
>> 		list_del_init(&bo->swap);
>> -		kref_put(&bo->list_kref, ttm_bo_ref_bug);
>> 		notify = true;
>> 	}
>> 	if (!list_empty(&bo->lru)) {
>> 		list_del_init(&bo->lru);
>> -		kref_put(&bo->list_kref, ttm_bo_ref_bug);
>> 		notify = true;
>> 	}
>> 
>> @@ -421,8 +392,7 @@ static int ttm_bo_individualize_resv(struct ttm_buffer_object *bo)
>> 	BUG_ON(!dma_resv_trylock(&bo->base._resv));
>> 
>> 	r = dma_resv_copy_fences(&bo->base._resv, bo->base.resv);
>> -	if (r)
>> -		dma_resv_unlock(&bo->base._resv);
>> +	dma_resv_unlock(&bo->base._resv);
>> 
>> 	return r;
>> }
>> @@ -449,68 +419,10 @@ static void ttm_bo_flush_all_fences(struct ttm_buffer_object *bo)
>> 	rcu_read_unlock();
>> }
>> 
>> -static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo)
>> -{
>> -	struct ttm_bo_device *bdev = bo->bdev;
>> -	int ret;
>> -
>> -	ret = ttm_bo_individualize_resv(bo);
>> -	if (ret) {
>> -		/* Last resort, if we fail to allocate memory for the
>> -		 * fences block for the BO to become idle
>> -		 */
>> -		dma_resv_wait_timeout_rcu(bo->base.resv, true, false,
>> -						    30 * HZ);
>> -		spin_lock(&ttm_bo_glob.lru_lock);
>> -		goto error;
>> -	}
>> -
>> -	spin_lock(&ttm_bo_glob.lru_lock);
>> -	ret = dma_resv_trylock(bo->base.resv) ? 0 : -EBUSY;
>> -	if (!ret) {
>> -		if (dma_resv_test_signaled_rcu(&bo->base._resv, true)) {
>> -			ttm_bo_del_from_lru(bo);
>> -			spin_unlock(&ttm_bo_glob.lru_lock);
>> -			if (bo->base.resv != &bo->base._resv)
>> -				dma_resv_unlock(&bo->base._resv);
>> -
>> -			ttm_bo_cleanup_memtype_use(bo);
>> -			dma_resv_unlock(bo->base.resv);
>> -			return;
>> -		}
>> -
>> -		ttm_bo_flush_all_fences(bo);
>> -
>> -		/*
>> -		 * Make NO_EVICT bos immediately available to
>> -		 * shrinkers, now that they are queued for
>> -		 * destruction.
>> -		 */
>> -		if (bo->mem.placement & TTM_PL_FLAG_NO_EVICT) {
>> -			bo->mem.placement &= ~TTM_PL_FLAG_NO_EVICT;
>> -			ttm_bo_move_to_lru_tail(bo, NULL);
>> -		}
>> -
>> -		dma_resv_unlock(bo->base.resv);
>> -	}
>> -	if (bo->base.resv != &bo->base._resv) {
>> -		ttm_bo_flush_all_fences(bo);
>> -		dma_resv_unlock(&bo->base._resv);
>> -	}
>> -
>> -error:
>> -	kref_get(&bo->list_kref);
>> -	list_add_tail(&bo->ddestroy, &bdev->ddestroy);
>> -	spin_unlock(&ttm_bo_glob.lru_lock);
>> -
>> -	schedule_delayed_work(&bdev->wq,
>> -			      ((HZ / 100) < 1) ? 1 : HZ / 100);
>> -}
>> -
>> /**
>> * function ttm_bo_cleanup_refs
>> - * If bo idle, remove from delayed- and lru lists, and unref.
>> - * If not idle, do nothing.
>> + * If bo idle, remove from lru lists, and unref.
>> + * If not idle, block if possible.
>> *
>> * Must be called with lru_lock and reservation held, this function
>> * will drop the lru lock and optionally the reservation lock before returning.
>> @@ -572,14 +484,14 @@ static int ttm_bo_cleanup_refs(struct ttm_buffer_object *bo,
>> 
>> 	ttm_bo_del_from_lru(bo);
>> 	list_del_init(&bo->ddestroy);
>> -	kref_put(&bo->list_kref, ttm_bo_ref_bug);
>> -
>> 	spin_unlock(&ttm_bo_glob.lru_lock);
>> 	ttm_bo_cleanup_memtype_use(bo);
>> 
>> 	if (unlock_resv)
>> 		dma_resv_unlock(bo->base.resv);
>> 
>> +	ttm_bo_put(bo);
>> +
>> 	return 0;
>> }
>> 
>> @@ -601,8 +513,9 @@ static bool ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
>> 
>> 		bo = list_first_entry(&bdev->ddestroy, struct ttm_buffer_object,
>> 				      ddestroy);
>> -		kref_get(&bo->list_kref);
>> 		list_move_tail(&bo->ddestroy, &removed);
>> +		if (!ttm_bo_get_unless_zero(bo))
>> +			continue;
>> 
>> 		if (remove_all || bo->base.resv != &bo->base._resv) {
>> 			spin_unlock(&glob->lru_lock);
>> @@ -617,7 +530,7 @@ static bool ttm_bo_delayed_delete(struct ttm_bo_device *bdev, bool remove_all)
>> 			spin_unlock(&glob->lru_lock);
>> 		}
>> 
>> -		kref_put(&bo->list_kref, ttm_bo_release_list);
>> +		ttm_bo_put(bo);
>> 		spin_lock(&glob->lru_lock);
>> 	}
>> 	list_splice_tail(&removed, &bdev->ddestroy);
>> @@ -643,16 +556,68 @@ static void ttm_bo_release(struct kref *kref)
>> 	    container_of(kref, struct ttm_buffer_object, kref);
>> 	struct ttm_bo_device *bdev = bo->bdev;
>> 	struct ttm_mem_type_manager *man = &bdev->man[bo->mem.mem_type];
>> +	size_t acc_size = bo->acc_size;
>> +	int ret;
>> 
>> -	if (bo->bdev->driver->release_notify)
>> -		bo->bdev->driver->release_notify(bo);
>> +	if (!bo->deleted) {
>> +		if (bo->bdev->driver->release_notify)
>> +			bo->bdev->driver->release_notify(bo);
>> 
>> -	drm_vma_offset_remove(bdev->vma_manager, &bo->base.vma_node);
>> -	ttm_mem_io_lock(man, false);
>> -	ttm_mem_io_free_vm(bo);
>> -	ttm_mem_io_unlock(man);
>> -	ttm_bo_cleanup_refs_or_queue(bo);
>> -	kref_put(&bo->list_kref, ttm_bo_release_list);
>> +		drm_vma_offset_remove(bdev->vma_manager, &bo->base.vma_node);
>> +		ttm_mem_io_lock(man, false);
>> +		ttm_mem_io_free_vm(bo);
>> +		ttm_mem_io_unlock(man);
>> +
>> +		ret = ttm_bo_individualize_resv(bo);
>> +		if (ret) {
>> +			/* Last resort, if we fail to allocate memory for the
>> +			 * fences block for the BO to become idle
>> +			 */
>> +			dma_resv_wait_timeout_rcu(bo->base.resv, true, false,
>> +						  30 * HZ);
>> +		}
>> +	}
>> +
>> +	if (!dma_resv_test_signaled_rcu(bo->base.resv, true)) {
>> +		/* The BO is not idle, resurrect it for delayed destroy */
>> +		ttm_bo_flush_all_fences(bo);
>> +		bo->deleted = true;
>> +
>> +		/*
>> +		 * Make NO_EVICT bos immediately available to
>> +		 * shrinkers, now that they are queued for
>> +		 * destruction.
>> +		 */
>> +		if (bo->mem.placement & TTM_PL_FLAG_NO_EVICT) {
>> +			bo->mem.placement &= ~TTM_PL_FLAG_NO_EVICT;
>> +			ttm_bo_move_to_lru_tail(bo, NULL);
> 
> [xh] this should be under lru lock.
> 
>> +		}
>> +
>> +		spin_lock(&ttm_bo_glob.lru_lock);
>> +		kref_init(&bo->kref);
>> +		list_add_tail(&bo->ddestroy, &bdev->ddestroy);
>> +		spin_unlock(&ttm_bo_glob.lru_lock);
>> +
>> +		schedule_delayed_work(&bdev->wq,
>> +				      ((HZ / 100) < 1) ? 1 : HZ / 100);
>> +		return;
>> +	}
>> +
>> +	spin_lock(&ttm_bo_glob.lru_lock);
>> +	ttm_bo_del_from_lru(bo);
>> +	list_del(&bo->ddestroy);
>> +	spin_unlock(&ttm_bo_glob.lru_lock);
>> +
>> +	ttm_bo_cleanup_memtype_use(bo);
>> +
>> +	BUG_ON(bo->mem.mm_node != NULL);
>> +	ttm_tt_destroy(bo->ttm);
> 
> [xh] already destroy it in ttm_bo_cleanup_memtype_use.
> 
> 
>> +	atomic_dec(&ttm_bo_glob.bo_count);
>> +	dma_fence_put(bo->moving);
>> +	if (!ttm_bo_uses_embedded_gem_object(bo))
>> +		dma_resv_fini(&bo->base._resv);
>> +	bo->destroy(bo);
>> +	ttm_mem_global_free(&ttm_mem_glob, acc_size);
>> }
>> 
>> void ttm_bo_put(struct ttm_buffer_object *bo)
>> @@ -755,8 +720,7 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
>> 
>> 	if (bo->base.resv == ctx->resv) {
>> 		dma_resv_assert_held(bo->base.resv);
>> -		if (ctx->flags & TTM_OPT_FLAG_ALLOW_RES_EVICT
>> -		    || !list_empty(&bo->ddestroy))
>> +		if (ctx->flags & TTM_OPT_FLAG_ALLOW_RES_EVICT || bo->deleted)
>> 			ret = true;
>> 		*locked = false;
>> 		if (busy)
>> @@ -837,6 +801,11 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>> 					dma_resv_unlock(bo->base.resv);
>> 				continue;
>> 			}
>> +			if (!ttm_bo_get_unless_zero(bo)) {
>> +				if (locked)
>> +					dma_resv_unlock(bo->base.resv);
>> +				continue;
>> +			}
>> 			break;
>> 		}
>> 
>> @@ -848,21 +817,19 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>> 	}
>> 
>> 	if (!bo) {
>> -		if (busy_bo)
>> -			kref_get(&busy_bo->list_kref);
>> +		if (busy_bo && !ttm_bo_get_unless_zero(busy_bo))
>> +			busy_bo = NULL;
>> 		spin_unlock(&ttm_bo_glob.lru_lock);
>> 		ret = ttm_mem_evict_wait_busy(busy_bo, ctx, ticket);
>> 		if (busy_bo)
>> -			kref_put(&busy_bo->list_kref, ttm_bo_release_list);
>> +			ttm_bo_put(busy_bo);
>> 		return ret;
>> 	}
>> 
>> -	kref_get(&bo->list_kref);
>> -
>> -	if (!list_empty(&bo->ddestroy)) {
>> +	if (bo->deleted) {
>> 		ret = ttm_bo_cleanup_refs(bo, ctx->interruptible,
>> 					  ctx->no_wait_gpu, locked);
>> -		kref_put(&bo->list_kref, ttm_bo_release_list);
>> +		ttm_bo_put(bo);
>> 		return ret;
>> 	}
>> 
>> @@ -872,7 +839,7 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
>> 	if (locked)
>> 		ttm_bo_unreserve(bo);
>> 
>> -	kref_put(&bo->list_kref, ttm_bo_release_list);
>> +	ttm_bo_put(bo);
>> 	return ret;
>> }
>> 
>> @@ -1284,7 +1251,6 @@ int ttm_bo_init_reserved(struct ttm_bo_device *bdev,
>> 	bo->destroy = destroy ? destroy : ttm_bo_default_destroy;
>> 
>> 	kref_init(&bo->kref);
>> -	kref_init(&bo->list_kref);
>> 	INIT_LIST_HEAD(&bo->lru);
>> 	INIT_LIST_HEAD(&bo->ddestroy);
>> 	INIT_LIST_HEAD(&bo->swap);
>> @@ -1804,11 +1770,18 @@ int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx *ctx)
>> 	spin_lock(&glob->lru_lock);
>> 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
>> 		list_for_each_entry(bo, &glob->swap_lru[i], swap) {
>> -			if (ttm_bo_evict_swapout_allowable(bo, ctx, &locked,
>> -							   NULL)) {
>> -				ret = 0;
>> -				break;
>> +			if (!ttm_bo_evict_swapout_allowable(bo, ctx, &locked,
>> +							    NULL))
>> +				continue;
>> +
>> +			if (!ttm_bo_get_unless_zero(bo)) {
>> +				if (locked)
>> +					dma_resv_unlock(bo->base.resv);
>> +				continue;
>> 			}
> 
> [xh] As you get the bo. when you put it?
> 
> You only put one bo just before return. BUT you get the bos in the for loop. Am I missing somethings?
> 
> thanks
> xinhui
> 
> 
[xh] Never mind.
I missed the break;

thanks
xinhui

>> +
>> +			ret = 0;
>> +			break;
>> 		}
>> 		if (!ret)
>> 			break;
>> @@ -1819,11 +1792,9 @@ int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx *ctx)
>> 		return ret;
>> 	}
>> 
>> -	kref_get(&bo->list_kref);
>> -
>> -	if (!list_empty(&bo->ddestroy)) {
>> +	if (bo->deleted) {
>> 		ret = ttm_bo_cleanup_refs(bo, false, false, locked);
>> -		kref_put(&bo->list_kref, ttm_bo_release_list);
>> +		ttm_bo_put(bo);
>> 		return ret;
>> 	}
>> 
>> @@ -1877,7 +1848,7 @@ int ttm_bo_swapout(struct ttm_bo_global *glob, struct ttm_operation_ctx *ctx)
>> 	 */
>> 	if (locked)
>> 		dma_resv_unlock(bo->base.resv);
>> -	kref_put(&bo->list_kref, ttm_bo_release_list);
>> +	ttm_bo_put(bo);
>> 	return ret;
>> }
>> EXPORT_SYMBOL(ttm_bo_swapout);
>> diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> index 86d152472f38..c8e359ded1df 100644
>> --- a/drivers/gpu/drm/ttm/ttm_bo_util.c
>> +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c
>> @@ -507,7 +507,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo,
>> 	fbo->base.moving = NULL;
>> 	drm_vma_node_reset(&fbo->base.base.vma_node);
>> 
>> -	kref_init(&fbo->base.list_kref);
>> 	kref_init(&fbo->base.kref);
>> 	fbo->base.destroy = &ttm_transfered_destroy;
>> 	fbo->base.acc_size = 0;
>> diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h
>> index 66ca49db9633..b9bc1b00142e 100644
>> --- a/include/drm/ttm/ttm_bo_api.h
>> +++ b/include/drm/ttm/ttm_bo_api.h
>> @@ -135,18 +135,14 @@ struct ttm_tt;
>> * @num_pages: Actual number of pages.
>> * @acc_size: Accounted size for this object.
>> * @kref: Reference count of this buffer object. When this refcount reaches
>> - * zero, the object is put on the delayed delete list.
>> - * @list_kref: List reference count of this buffer object. This member is
>> - * used to avoid destruction while the buffer object is still on a list.
>> - * Lru lists may keep one refcount, the delayed delete list, and kref != 0
>> - * keeps one refcount. When this refcount reaches zero,
>> - * the object is destroyed.
>> + * zero, the object is destroyed or put on the delayed delete list.
>> * @mem: structure describing current placement.
>> * @persistent_swap_storage: Usually the swap storage is deleted for buffers
>> * pinned in physical memory. If this behaviour is not desired, this member
>> * holds a pointer to a persistent shmem object.
>> * @ttm: TTM structure holding system pages.
>> * @evicted: Whether the object was evicted without user-space knowing.
>> + * @deleted: True if the object is only a zombie and already deleted.
>> * @lru: List head for the lru list.
>> * @ddestroy: List head for the delayed destroy list.
>> * @swap: List head for swap LRU list.
>> @@ -183,9 +179,7 @@ struct ttm_buffer_object {
>> 	/**
>> 	* Members not needing protection.
>> 	*/
>> -
>> 	struct kref kref;
>> -	struct kref list_kref;
>> 
>> 	/**
>> 	 * Members protected by the bo::resv::reserved lock.
>> @@ -195,6 +189,7 @@ struct ttm_buffer_object {
>> 	struct file *persistent_swap_storage;
>> 	struct ttm_tt *ttm;
>> 	bool evicted;
>> +	bool deleted;
>> 
>> 	/**
>> 	 * Members protected by the bdev::lru_lock.
>> -- 
>> 2.17.1
>> 
> 

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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux