Re: [PATCH drm-misc-next v3 6/7] drm/gpuvm: generalize dma_resv/extobj handling and GEM validation

[Boris Brezillon <boris.brezillon@collabora.com>]

Hello Danilo,

On Sat,  9 Sep 2023 17:31:13 +0200
Danilo Krummrich <dakr@redhat.com> wrote:


> @@ -632,6 +661,131 @@
>   *	}
>   */
>  
> +/**
> + * get_next_vm_bo_from_list() - get the next vm_bo element
> + * @__gpuvm: The GPU VM
> + * @__list_name: The name of the list we're iterating on
> + * @__local_list: A pointer to the local list used to store already iterated items
> + * @__prev_vm_bo: The previous element we got from drm_gpuvm_get_next_cached_vm_bo()
> + *
> + * This helper is here to provide lockless list iteration. Lockless as in, the
> + * iterator releases the lock immediately after picking the first element from
> + * the list, so list insertion deletion can happen concurrently.
> + *
> + * Elements popped from the original list are kept in a local list, so removal
> + * and is_empty checks can still happen while we're iterating the list.
> + */
> +#define get_next_vm_bo_from_list(__gpuvm, __list_name, __local_list, __prev_vm_bo)	\
> +	({										\
> +		struct drm_gpuvm_bo *__vm_bo;						\
> +											\
> +		drm_gpuvm_bo_put(__prev_vm_bo);						\
> +											\
> +		spin_lock(&(__gpuvm)->__list_name.lock);				\

I'm tempted to add a drm_gpuvm::<list_name>::local_list field, so we
can catch concurrent iterations with something like:

		if (!(__gpuvm)->__list_name.local_list)
			(__gpuvm)->__list_name.local_list = __local_list;
		else
			WARN_ON((__gpuvm)->__list_name.local_list != __local_list);

with (__gpuvm)->__list_name.local_list being restored to NULL
in restore_vm_bo_list().

> +		while (!list_empty(&(__gpuvm)->__list_name.list)) {			\
> +			__vm_bo = list_first_entry(&(__gpuvm)->__list_name.list,	\
> +						   struct drm_gpuvm_bo,			\
> +						   list.entry.__list_name);		\
> +			if (drm_gpuvm_bo_get_unless_zero(__vm_bo)) {			\
> +				list_move_tail(&(__vm_bo)->list.entry.__list_name,	\
> +					       __local_list);				\
> +				break;							\
> +			} else {							\
> +				list_del_init(&(__vm_bo)->list.entry.__list_name);	\
> +				__vm_bo = NULL;						\
> +			}								\
> +		}									\
> +		spin_unlock(&(__gpuvm)->__list_name.lock);				\
> +											\
> +		__vm_bo;								\
> +	})
> +
> +/**
> + * for_each_vm_bo_in_list() - internal vm_bo list iterator
> + *
> + * This helper is here to provide lockless list iteration. Lockless as in, the
> + * iterator releases the lock immediately after picking the first element from the
> + * list, so list insertion and deletion can happen concurrently.
> + *
> + * Typical use:
> + *
> + *	struct drm_gpuvm_bo *vm_bo;
> + *	LIST_HEAD(my_local_list);
> + *
> + *	ret = 0;
> + *	drm_gpuvm_for_each_vm_bo(gpuvm, <list_name>, &my_local_list, vm_bo) {
> + *		ret = do_something_with_vm_bo(..., vm_bo);
> + *		if (ret)
> + *			break;
> + *	}
> + *	drm_gpuvm_bo_put(vm_bo);
> + *	drm_gpuvm_restore_vm_bo_list(gpuvm, <list_name>, &my_local_list);

The names in this example and the helper names don't match.

> + *
> + *
> + * Only used for internal list iterations, not meant to be exposed to the outside
> + * world.
> + */
> +#define for_each_vm_bo_in_list(__gpuvm, __list_name, __local_list, __vm_bo)	\
> +	for (__vm_bo = get_next_vm_bo_from_list(__gpuvm, __list_name,		\
> +						__local_list, NULL);		\
> +	     __vm_bo;								\
> +	     __vm_bo = get_next_vm_bo_from_list(__gpuvm, __list_name,		\
> +						__local_list, __vm_bo))		\
> +
> +/**
> + * restore_vm_bo_list() - move vm_bo elements back to their original list
> + * @__gpuvm: The GPU VM
> + * @__list_name: The name of the list we're iterating on
> + * @__local_list: A pointer to the local list used to store already iterated items
> + *
> + * When we're done iterating a vm_bo list, we should call restore_vm_bo_list()
> + * to restore the original state and let new iterations take place.
> + */
> +#define restore_vm_bo_list(__gpuvm, __list_name, __local_list)				\
> +	do {										\
> +		/* Merge back the two lists, moving local list elements to the		\
> +		 * head to preserve previous ordering, in case it matters.		\
> +		 */									\
> +		spin_lock(&(__gpuvm)->__list_name.lock);				\
> +		list_splice(__local_list, &(__gpuvm)->__list_name.list);		\
> +		spin_unlock(&(__gpuvm)->__list_name.lock);				\
> +	} while (0)
> +/**
> + * drm_gpuvm_bo_list_add() - insert a vm_bo into the given list
> + * @__vm_bo: the &drm_gpuvm_bo
> + * @__list_name: the name of the list to insert into
> + *
> + * Inserts the given @__vm_bo into the list specified by @__list_name and
> + * increases the vm_bo's reference count.
> + */
> +#define drm_gpuvm_bo_list_add(__vm_bo, __list_name)				\
> +	do {									\
> +		spin_lock(&(__vm_bo)->vm->__list_name.lock);			\
> +		if (list_empty(&(__vm_bo)->list.entry.__list_name))		\
> +			list_add_tail(&(__vm_bo)->list.entry.__list_name,	\
> +				      &(__vm_bo)->vm->__list_name.list);	\
> +		spin_unlock(&(__vm_bo)->vm->__list_name.lock);			\
> +	} while (0)
> +
> +/**
> + * drm_gpuvm_bo_list_del() - remove a vm_bo from the given list
> + * @__vm_bo: the &drm_gpuvm_bo
> + * @__list_name: the name of the list to insert into
> + *
> + * Removes the given @__vm_bo from the list specified by @__list_name and
> + * decreases the vm_bo's reference count.
> + */
> +#define drm_gpuvm_bo_list_del(__vm_bo, __list_name)				\
> +	do {									\
> +		spin_lock(&(__vm_bo)->vm->__list_name.lock);			\
> +		if (!list_empty(&(__vm_bo)->list.entry.__list_name))		\
> +			list_del_init(&(__vm_bo)->list.entry.__list_name);	\
> +		spin_unlock(&(__vm_bo)->vm->__list_name.lock);			\
> +	} while (0)
> +
> +static int __must_check
> +drm_gpuvm_bo_get_unless_zero(struct drm_gpuvm_bo *vm_bo);

I see no obvious reason to have a forward declaration for this helper,
if we decide to keep it, let's at least move the declaration here.


> @@ -807,6 +1262,14 @@ drm_gpuvm_bo_destroy(struct kref *kref)
>  
>  	drm_gem_gpuva_assert_lock_held(vm_bo->obj);
>  
> +	spin_lock(&gpuvm->extobj.lock);
> +	list_del(&vm_bo->list.entry.extobj);
> +	spin_unlock(&gpuvm->extobj.lock);
> +
> +	spin_lock(&gpuvm->evict.lock);
> +	list_del(&vm_bo->list.entry.evict);
> +	spin_unlock(&gpuvm->evict.lock);
> +
>  	list_del(&vm_bo->list.entry.gem);
>  
>  	drm_gem_object_put(obj);
> @@ -822,6 +1285,11 @@ drm_gpuvm_bo_destroy(struct kref *kref)
>   * @vm_bo: the &drm_gpuvm_bo to release the reference of
>   *
>   * This releases a reference to @vm_bo.
> + *
> + * If the reference count drops to zero, the &gpuvm_bo is destroyed, which
> + * includes removing it from the GEMs gpuva list. Hence, if a call to this
> + * function can potentially let the reference count to zero the caller must
> + * hold the dma-resv or driver specific GEM gpuva lock.

Looks like this should have been part of the previous patch. I hate
the fact we have to worry about GEM gpuva lock being held when we call
_put() only if the ref drops to zero though. I think I'd feel more
comfortable if the function was named differently. Maybe _return() or
_release() to match the _obtain() function, where the object is inserted
in the GEM vm_bo list. I would also do the lock_is_held() check
unconditionally, move the list removal in this function with a del_init(),
and have a WARN_ON(!list_empty) in vm_bo_destroy().

>   */
>  void
>  drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo)
> @@ -831,6 +1299,12 @@ drm_gpuvm_bo_put(struct drm_gpuvm_bo *vm_bo)
>  }
>  EXPORT_SYMBOL_GPL(drm_gpuvm_bo_put);
>  
> +static int __must_check
> +drm_gpuvm_bo_get_unless_zero(struct drm_gpuvm_bo *vm_bo)
> +{
> +	return kref_get_unless_zero(&vm_bo->kref);

Not convinced this helper is needed. It's only used once, and I
don't think we'll need it elsewhere.

> +}
> +
>  static struct drm_gpuvm_bo *
>  __drm_gpuvm_bo_find(struct drm_gpuvm *gpuvm,
>  		    struct drm_gem_object *obj)


Regards,

Boris