Re: [Intel-xe] [PATCH 2/5] drm/xe: Reduce the number list links in xe_vma

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

Hi, Matthew,

On 7/11/23 23:27, Matthew Brost wrote:
> Combine the userptr, rebind, and destroy links into a union as
> the lists these links belong to are mutually exclusive.
>
> v2: Adjust which lists are combined (Thomas H)
>
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>   drivers/gpu/drm/xe/xe_bo.c       |  6 +++--
>   drivers/gpu/drm/xe/xe_exec.c     |  2 +-
>   drivers/gpu/drm/xe/xe_pt.c       |  2 +-
>   drivers/gpu/drm/xe/xe_vm.c       | 45 ++++++++++++++++----------------
>   drivers/gpu/drm/xe/xe_vm_types.h | 28 +++++++++-----------
>   5 files changed, 41 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_bo.c b/drivers/gpu/drm/xe/xe_bo.c
> index 6353afa8d846..ca7f74ecb753 100644
> --- a/drivers/gpu/drm/xe/xe_bo.c
> +++ b/drivers/gpu/drm/xe/xe_bo.c
> @@ -474,8 +474,10 @@ static int xe_bo_trigger_rebind(struct xe_device *xe, struct xe_bo *bo,
>   			}
>   
>   			xe_vm_assert_held(vm);
> -			if (list_empty(&vma->rebind_link) && vma->tile_present)
> -				list_add_tail(&vma->rebind_link, &vm->rebind_list);
> +			if (list_empty(&vma->combined_links.rebind) &&
> +			    vma->tile_present)
> +				list_add_tail(&vma->combined_links.rebind,
> +					      &vm->rebind_list);
>   
>   			if (vm_resv_locked)
>   				dma_resv_unlock(&vm->resv);
> diff --git a/drivers/gpu/drm/xe/xe_exec.c b/drivers/gpu/drm/xe/xe_exec.c
> index ba13d20ed348..bc5bd4bdc582 100644
> --- a/drivers/gpu/drm/xe/xe_exec.c
> +++ b/drivers/gpu/drm/xe/xe_exec.c
> @@ -120,7 +120,7 @@ static int xe_exec_begin(struct xe_engine *e, struct ww_acquire_ctx *ww,
>   	 * BOs have valid placements possibly moving an evicted BO back
>   	 * to a location where the GPU can access it).
>   	 */
> -	list_for_each_entry(vma, &vm->rebind_list, rebind_link) {
> +	list_for_each_entry(vma, &vm->rebind_list, combined_links.rebind) {
>   		XE_WARN_ON(xe_vma_is_null(vma));
>   
>   		if (xe_vma_is_userptr(vma))
> diff --git a/drivers/gpu/drm/xe/xe_pt.c b/drivers/gpu/drm/xe/xe_pt.c
> index a8d96cbd53e3..3b43b32f4a33 100644
> --- a/drivers/gpu/drm/xe/xe_pt.c
> +++ b/drivers/gpu/drm/xe/xe_pt.c
> @@ -1690,7 +1690,7 @@ __xe_pt_unbind_vma(struct xe_tile *tile, struct xe_vma *vma, struct xe_engine *e
>   	}
>   
>   	if (!vma->tile_present)
> -		list_del_init(&vma->rebind_link);
> +		list_del_init(&vma->combined_links.rebind);
>   
>   	if (unbind_pt_update.locked) {
>   		XE_WARN_ON(!xe_vma_is_userptr(vma));
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 5f4f6dab270a..b2847be6de6a 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -467,7 +467,8 @@ int xe_vm_lock_dma_resv(struct xe_vm *vm, struct ww_acquire_ctx *ww,
>   
>   		list_del_init(&vma->notifier.rebind_link);
>   		if (vma->tile_present && !(vma->gpuva.flags & XE_VMA_DESTROYED))
> -			list_move_tail(&vma->rebind_link, &vm->rebind_list);
> +			list_move_tail(&vma->combined_links.rebind,
> +				       &vm->rebind_list);
>   	}
>   	spin_unlock(&vm->notifier.list_lock);
>   
> @@ -608,7 +609,7 @@ static void preempt_rebind_work_func(struct work_struct *w)
>   	if (err)
>   		goto out_unlock;
>   
> -	list_for_each_entry(vma, &vm->rebind_list, rebind_link) {
> +	list_for_each_entry(vma, &vm->rebind_list, combined_links.rebind) {
>   		if (xe_vma_has_no_bo(vma) ||
>   		    vma->gpuva.flags & XE_VMA_DESTROYED)
>   			continue;
> @@ -780,17 +781,20 @@ int xe_vm_userptr_pin(struct xe_vm *vm)
>   	list_for_each_entry_safe(vma, next, &vm->userptr.invalidated,
>   				 userptr.invalidate_link) {
>   		list_del_init(&vma->userptr.invalidate_link);
> -		list_move_tail(&vma->userptr_link, &vm->userptr.repin_list);
> +		if (list_empty(&vma->combined_links.userptr))
> +			list_move_tail(&vma->combined_links.userptr,
> +				       &vm->userptr.repin_list);
>   	}
>   	spin_unlock(&vm->userptr.invalidated_lock);
>   
>   	/* Pin and move to temporary list */
> -	list_for_each_entry_safe(vma, next, &vm->userptr.repin_list, userptr_link) {
> +	list_for_each_entry_safe(vma, next, &vm->userptr.repin_list,
> +				 combined_links.userptr) {
>   		err = xe_vma_userptr_pin_pages(vma);
>   		if (err < 0)
>   			goto out_err;
>   
> -		list_move_tail(&vma->userptr_link, &tmp_evict);
> +		list_move_tail(&vma->combined_links.userptr, &tmp_evict);
>   	}
>   
>   	/* Take lock and move to rebind_list for rebinding. */
> @@ -798,10 +802,8 @@ int xe_vm_userptr_pin(struct xe_vm *vm)
>   	if (err)
>   		goto out_err;
>   
> -	list_for_each_entry_safe(vma, next, &tmp_evict, userptr_link) {
> -		list_del_init(&vma->userptr_link);
> -		list_move_tail(&vma->rebind_link, &vm->rebind_list);
> -	}
> +	list_for_each_entry_safe(vma, next, &tmp_evict, combined_links.userptr)
> +		list_move_tail(&vma->combined_links.rebind, &vm->rebind_list);
>   
>   	dma_resv_unlock(&vm->resv);
>   
> @@ -845,10 +847,11 @@ struct dma_fence *xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
>   		return NULL;
>   
>   	xe_vm_assert_held(vm);
> -	list_for_each_entry_safe(vma, next, &vm->rebind_list, rebind_link) {
> +	list_for_each_entry_safe(vma, next, &vm->rebind_list,
> +				 combined_links.rebind) {
>   		XE_WARN_ON(!vma->tile_present);
>   
> -		list_del_init(&vma->rebind_link);
> +		list_del_init(&vma->combined_links.rebind);
>   		dma_fence_put(fence);
>   		if (rebind_worker)
>   			trace_xe_vma_rebind_worker(vma);
> @@ -883,9 +886,7 @@ static struct xe_vma *xe_vma_create(struct xe_vm *vm,
>   		return vma;
>   	}
>   
> -	INIT_LIST_HEAD(&vma->rebind_link);
> -	INIT_LIST_HEAD(&vma->unbind_link);
> -	INIT_LIST_HEAD(&vma->userptr_link);
> +	INIT_LIST_HEAD(&vma->combined_links.rebind);
>   	INIT_LIST_HEAD(&vma->userptr.invalidate_link);
>   	INIT_LIST_HEAD(&vma->notifier.rebind_link);
>   	INIT_LIST_HEAD(&vma->extobj.link);
> @@ -1058,15 +1059,14 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence)
>   	struct xe_vm *vm = xe_vma_vm(vma);
>   
>   	lockdep_assert_held_write(&vm->lock);
> -	XE_BUG_ON(!list_empty(&vma->unbind_link));
> +	XE_BUG_ON(!list_empty(&vma->combined_links.destroy));
>   
>   	if (xe_vma_is_userptr(vma)) {
>   		XE_WARN_ON(!(vma->gpuva.flags & XE_VMA_DESTROYED));
>   
>   		spin_lock(&vm->userptr.invalidated_lock);
> -		list_del_init(&vma->userptr.invalidate_link);
> +		list_del(&vma->userptr.invalidate_link);
>   		spin_unlock(&vm->userptr.invalidated_lock);
> -		list_del(&vma->userptr_link);
>   	} else if (!xe_vma_is_null(vma)) {
>   		xe_bo_assert_held(xe_vma_bo(vma));
>   
> @@ -1087,9 +1087,6 @@ static void xe_vma_destroy(struct xe_vma *vma, struct dma_fence *fence)
>   	}
>   
>   	xe_vm_assert_held(vm);
> -	if (!list_empty(&vma->rebind_link))
> -		list_del(&vma->rebind_link);
> -
>   	if (fence) {
>   		int ret = dma_fence_add_callback(fence, &vma->destroy_cb,
>   						 vma_destroy_cb);
> @@ -1443,11 +1440,12 @@ void xe_vm_close_and_put(struct xe_vm *vm)
>   
>   		/* easy case, remove from VMA? */
>   		if (xe_vma_has_no_bo(vma) || xe_vma_bo(vma)->vm) {
> +			list_del_init(&vma->combined_links.rebind);
>   			xe_vma_destroy(vma, NULL);
>   			continue;
>   		}
>   
> -		list_add_tail(&vma->unbind_link, &contested);
> +		list_move_tail(&vma->combined_links.destroy, &contested);
>   	}
>   
>   	/*
> @@ -1475,8 +1473,9 @@ void xe_vm_close_and_put(struct xe_vm *vm)
>   	 * Since we hold a refcount to the bo, we can remove and free
>   	 * the members safely without locking.
>   	 */
> -	list_for_each_entry_safe(vma, next_vma, &contested, unbind_link) {
> -		list_del_init(&vma->unbind_link);
> +	list_for_each_entry_safe(vma, next_vma, &contested,
> +				 combined_links.destroy) {
> +		list_del_init(&vma->combined_links.destroy);
>   		xe_vma_destroy_unlocked(vma);
>   	}
>   
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index f17dc5d7370e..84bf1620214c 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -49,21 +49,19 @@ struct xe_vma {
>   	 */
>   	u64 tile_present;
>   
> -	/** @userptr_link: link into VM repin list if userptr */
> -	struct list_head userptr_link;
> -
> -	/**
> -	 * @rebind_link: link into VM if this VMA needs rebinding, and
> -	 * if it's a bo (not userptr) needs validation after a possible
> -	 * eviction. Protected by the vm's resv lock.
> -	 */
> -	struct list_head rebind_link;

The rebind_link and the userptr_link are not combinable. To be 
combinable they need to either be protected by the same lock or be kept 
mutually exclusive by some other *simple* argument, like construction / 
destruction, used for different subclasses etc.

The userptr_link is protected by the userptr.invalidated_lock and the 
rebind_link by the vm resv, and no simple argument can be made. In fact 
let's say that a userptr sits on or is being moved to the rebind list, 
and then the invalidation notifier gets called, which will try to move 
it to the userptr list again using the userptr link. In the best case it 
will fail the move since the list is not empty, in the worst case list 
corruption due to the list head is currently being modified.

So IMO for any list head being combined like this, we need to kerneldoc 
why this is possible, using either the lock argument or another simple 
argument like detailed above.

I *think* the notifier.rebind_list and the userptr_list are combinable, 
though, using the different vma subclass (userptr/bo) argument.

Thanks,

Thomas


> -
> -	/**
> -	 * @unbind_link: link or list head if an unbind of multiple VMAs, in
> -	 * single unbind op, is being done.
> -	 */
> -	struct list_head unbind_link;
> +	/** @combined_links: links into lists which are mutually exclusive */
> +	union {
> +		/** @userptr: link into VM repin list if userptr */
> +		struct list_head userptr;
> +		/**
> +		 * @rebind: link into VM if this VMA needs rebinding, and
> +		 * if it's a bo (not userptr) needs validation after a possible
> +		 * eviction. Protected by the vm's resv lock.
> +		 */
> +		struct list_head rebind;
> +		/** @destroy: link to contested list when VM is being closed */
> +		struct list_head destroy;
> +	} combined_links;
>   
>   	/** @destroy_cb: callback to destroy VMA when unbind job is done */
>   	struct dma_fence_cb destroy_cb;