[PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.

Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
 include/linux/ww_mutex.h               | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index eaa3cb0c3ad1..4c04b560e358 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
 	*map = mapping;
 
 	/* Double check that the BO is reserved by this CS */
-	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
+	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
+				  &parser->ticket))
 		return -EINVAL;
 
 	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..dd580db289e8 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
 	return mutex_is_locked(&lock->base);
 }
 
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @task: the task structure to check
+ * @ctx: the w/w acquire context to test
+ *
+ * Returns true if the mutex is locked in the context by the given task, false
+ * otherwise.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+					struct task_struct *task,
+					struct ww_acquire_ctx *ctx)
+{
+	return likely(__mutex_owner(&lock->base) == task) &&
+		READ_ONCE(lock->ctx) == ctx;
+}
+
 #endif
-- 
2.14.1

[PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

amdgpu needs to verify if userspace sends us valid addresses and the simplest
way of doing this is to check if the buffer object is locked with the ticket
of the current submission.

Clean up the access to the ww_mutex internals by providing a function
for this and extend the check to the thread owning the underlying mutex.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
 include/linux/ww_mutex.h               | 17 +++++++++++++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index eaa3cb0c3ad1..4c04b560e358 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
 	*map = mapping;
 
 	/* Double check that the BO is reserved by this CS */
-	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
+	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
+				  &parser->ticket))
 		return -EINVAL;
 
 	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 39fda195bf78..dd580db289e8 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
 	return mutex_is_locked(&lock->base);
 }
 
+/**
+ * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
+ * @lock: the mutex to be queried
+ * @task: the task structure to check
+ * @ctx: the w/w acquire context to test
+ *
+ * Returns true if the mutex is locked in the context by the given task, false
+ * otherwise.
+ */
+static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
+					struct task_struct *task,
+					struct ww_acquire_ctx *ctx)
+{
+	return likely(__mutex_owner(&lock->base) == task) &&
+		READ_ONCE(lock->ctx) == ctx;
+}
+
 #endif
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/3] drm/ttm: handle already locked BOs during eviction and swapout.

[Christian König]

This solves the problem that when we swapout a BO from a domain we
sometimes couldn't make room for it because holding the lock blocks all
other BOs with this reservation object.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 33 ++++++++++++++++-----------------
 1 file changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index d90b1cf10b27..fba40e22d088 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -713,31 +713,30 @@ bool ttm_bo_eviction_valuable(struct ttm_buffer_object *bo,
 EXPORT_SYMBOL(ttm_bo_eviction_valuable);
 
 /**
- * Check the target bo is allowable to be evicted or swapout, including cases:
- *
- * a. if share same reservation object with ctx->resv, have assumption
- * reservation objects should already be locked, so not lock again and
- * return true directly when either the opreation allow_reserved_eviction
- * or the target bo already is in delayed free list;
- *
- * b. Otherwise, trylock it.
+ * Check if the target bo is allowed to be evicted or swapedout.
  */
 static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
-			struct ttm_operation_ctx *ctx, bool *locked)
+					   struct ttm_operation_ctx *ctx,
+					   bool *locked)
 {
-	bool ret = false;
+	/* First check if we can lock it */
+	*locked = reservation_object_trylock(bo->resv);
+	if (*locked)
+		return true;
 
-	*locked = false;
+	/* Check if it's locked because it is part of the current operation */
 	if (bo->resv == ctx->resv) {
 		reservation_object_assert_held(bo->resv);
-		if (ctx->allow_reserved_eviction || !list_empty(&bo->ddestroy))
-			ret = true;
-	} else {
-		*locked = reservation_object_trylock(bo->resv);
-		ret = *locked;
+		return ctx->allow_reserved_eviction ||
+			!list_empty(&bo->ddestroy);
 	}
 
-	return ret;
+	/* Check if it's locked because it was already evicted */
+	if (ww_mutex_is_owned_by(&bo->resv->lock, current, NULL))
+		return true;
+
+	/* Some other thread is using it, don't touch it */
+	return false;
 }
 
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
-- 
2.14.1

[PATCH 3/3] drm/ttm: keep BOs reserved until end of eviction

[Christian König]

This avoids problems when BOs are evicted but directly moved back into
the domain from other threads.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index fba40e22d088..568cf216b374 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -742,7 +742,8 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 			       uint32_t mem_type,
 			       const struct ttm_place *place,
-			       struct ttm_operation_ctx *ctx)
+			       struct ttm_operation_ctx *ctx,
+			       struct list_head *evicted)
 {
 	struct ttm_bo_global *glob = bdev->glob;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
@@ -792,17 +793,28 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
 	ret = ttm_bo_evict(bo, ctx);
 	if (locked) {
-		ttm_bo_unreserve(bo);
+		list_add_tail(&bo->lru, evicted);
 	} else {
 		spin_lock(&glob->lru_lock);
 		ttm_bo_add_to_lru(bo);
 		spin_unlock(&glob->lru_lock);
+		kref_put(&bo->list_kref, ttm_bo_release_list);
 	}
 
-	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
 }
 
+static void ttm_mem_evict_cleanup(struct list_head *evicted)
+{
+	struct ttm_buffer_object *bo, *tmp;
+
+	list_for_each_entry_safe(bo, tmp, evicted, lru) {
+		list_del_init(&bo->lru);
+		ttm_bo_unreserve(bo);
+		kref_put(&bo->list_kref, ttm_bo_release_list);
+	}
+}
+
 void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem)
 {
 	struct ttm_mem_type_manager *man = &bo->bdev->man[mem->mem_type];
@@ -852,20 +864,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
+	struct list_head evicted;
 	int ret;
 
+	INIT_LIST_HEAD(&evicted);
 	do {
 		ret = (*man->func->get_node)(man, bo, place, mem);
 		if (unlikely(ret != 0))
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
+		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx, &evicted);
 		if (unlikely(ret != 0))
-			return ret;
+			goto error;
 	} while (1);
 	mem->mem_type = mem_type;
-	return ttm_bo_add_move_fence(bo, man, mem);
+	ret = ttm_bo_add_move_fence(bo, man, mem);
+
+error:
+	ttm_mem_evict_cleanup(&evicted);
+	return ret;
 }
 
 static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man,
@@ -1345,6 +1363,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	struct ttm_operation_ctx ctx = { false, false };
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_bo_global *glob = bdev->glob;
+	struct list_head evicted;
 	struct dma_fence *fence;
 	int ret;
 	unsigned i;
@@ -1352,18 +1371,20 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	/*
 	 * Can't use standard list traversal since we're unlocking.
 	 */
-
+	INIT_LIST_HEAD(&evicted);
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		while (!list_empty(&man->lru[i])) {
 			spin_unlock(&glob->lru_lock);
-			ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx);
+			ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx,
+						  &evicted);
 			if (ret)
 				return ret;
 			spin_lock(&glob->lru_lock);
 		}
 	}
 	spin_unlock(&glob->lru_lock);
+	ttm_mem_evict_cleanup(&evicted);
 
 	spin_lock(&man->move_lock);
 	fence = dma_fence_get(man->move);
-- 
2.14.1

[PATCH 3/3] drm/ttm: keep BOs reserved until end of eviction

[Christian König]

This avoids problems when BOs are evicted but directly moved back into
the domain from other threads.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/ttm/ttm_bo.c | 37 +++++++++++++++++++++++++++++--------
 1 file changed, 29 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c
index fba40e22d088..568cf216b374 100644
--- a/drivers/gpu/drm/ttm/ttm_bo.c
+++ b/drivers/gpu/drm/ttm/ttm_bo.c
@@ -742,7 +742,8 @@ static bool ttm_bo_evict_swapout_allowable(struct ttm_buffer_object *bo,
 static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 			       uint32_t mem_type,
 			       const struct ttm_place *place,
-			       struct ttm_operation_ctx *ctx)
+			       struct ttm_operation_ctx *ctx,
+			       struct list_head *evicted)
 {
 	struct ttm_bo_global *glob = bdev->glob;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
@@ -792,17 +793,28 @@ static int ttm_mem_evict_first(struct ttm_bo_device *bdev,
 
 	ret = ttm_bo_evict(bo, ctx);
 	if (locked) {
-		ttm_bo_unreserve(bo);
+		list_add_tail(&bo->lru, evicted);
 	} else {
 		spin_lock(&glob->lru_lock);
 		ttm_bo_add_to_lru(bo);
 		spin_unlock(&glob->lru_lock);
+		kref_put(&bo->list_kref, ttm_bo_release_list);
 	}
 
-	kref_put(&bo->list_kref, ttm_bo_release_list);
 	return ret;
 }
 
+static void ttm_mem_evict_cleanup(struct list_head *evicted)
+{
+	struct ttm_buffer_object *bo, *tmp;
+
+	list_for_each_entry_safe(bo, tmp, evicted, lru) {
+		list_del_init(&bo->lru);
+		ttm_bo_unreserve(bo);
+		kref_put(&bo->list_kref, ttm_bo_release_list);
+	}
+}
+
 void ttm_bo_mem_put(struct ttm_buffer_object *bo, struct ttm_mem_reg *mem)
 {
 	struct ttm_mem_type_manager *man = &bo->bdev->man[mem->mem_type];
@@ -852,20 +864,26 @@ static int ttm_bo_mem_force_space(struct ttm_buffer_object *bo,
 {
 	struct ttm_bo_device *bdev = bo->bdev;
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
+	struct list_head evicted;
 	int ret;
 
+	INIT_LIST_HEAD(&evicted);
 	do {
 		ret = (*man->func->get_node)(man, bo, place, mem);
 		if (unlikely(ret != 0))
 			return ret;
 		if (mem->mm_node)
 			break;
-		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx);
+		ret = ttm_mem_evict_first(bdev, mem_type, place, ctx, &evicted);
 		if (unlikely(ret != 0))
-			return ret;
+			goto error;
 	} while (1);
 	mem->mem_type = mem_type;
-	return ttm_bo_add_move_fence(bo, man, mem);
+	ret = ttm_bo_add_move_fence(bo, man, mem);
+
+error:
+	ttm_mem_evict_cleanup(&evicted);
+	return ret;
 }
 
 static uint32_t ttm_bo_select_caching(struct ttm_mem_type_manager *man,
@@ -1345,6 +1363,7 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	struct ttm_operation_ctx ctx = { false, false };
 	struct ttm_mem_type_manager *man = &bdev->man[mem_type];
 	struct ttm_bo_global *glob = bdev->glob;
+	struct list_head evicted;
 	struct dma_fence *fence;
 	int ret;
 	unsigned i;
@@ -1352,18 +1371,20 @@ static int ttm_bo_force_list_clean(struct ttm_bo_device *bdev,
 	/*
 	 * Can't use standard list traversal since we're unlocking.
 	 */
-
+	INIT_LIST_HEAD(&evicted);
 	spin_lock(&glob->lru_lock);
 	for (i = 0; i < TTM_MAX_BO_PRIORITY; ++i) {
 		while (!list_empty(&man->lru[i])) {
 			spin_unlock(&glob->lru_lock);
-			ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx);
+			ret = ttm_mem_evict_first(bdev, mem_type, NULL, &ctx,
+						  &evicted);
 			if (ret)
 				return ret;
 			spin_lock(&glob->lru_lock);
 		}
 	}
 	spin_unlock(&glob->lru_lock);
+	ttm_mem_evict_cleanup(&evicted);
 
 	spin_lock(&man->move_lock);
 	fence = dma_fence_get(man->move);
-- 
2.14.1

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Alex Deucher]

On Thu, Feb 15, 2018 at 9:19 AM, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
>
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Shouldn't this be two patches?  One to add the new ww_mutex code and
one to update amdgpu?

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>  include/linux/ww_mutex.h               | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index eaa3cb0c3ad1..4c04b560e358 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>         *map = mapping;
>
>         /* Double check that the BO is reserved by this CS */
> -       if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> +       if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> +                                 &parser->ticket))
>                 return -EINVAL;
>
>         if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>         return mutex_is_locked(&lock->base);
>  }
>
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +                                       struct task_struct *task,
> +                                       struct ww_acquire_ctx *ctx)
> +{
> +       return likely(__mutex_owner(&lock->base) == task) &&
> +               READ_ONCE(lock->ctx) == ctx;
> +}
> +
>  #endif
> --
> 2.14.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Alex Deucher]

On Thu, Feb 15, 2018 at 9:19 AM, Christian König
<ckoenig.leichtzumerken@gmail.com> wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
>
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
>
> Signed-off-by: Christian König <christian.koenig@amd.com>

Shouldn't this be two patches?  One to add the new ww_mutex code and
one to update amdgpu?

Alex

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>  include/linux/ww_mutex.h               | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index eaa3cb0c3ad1..4c04b560e358 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>         *map = mapping;
>
>         /* Double check that the BO is reserved by this CS */
> -       if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> +       if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> +                                 &parser->ticket))
>                 return -EINVAL;
>
>         if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>         return mutex_is_locked(&lock->base);
>  }
>
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +                                       struct task_struct *task,
> +                                       struct ww_acquire_ctx *ctx)
> +{
> +       return likely(__mutex_owner(&lock->base) == task) &&
> +               READ_ONCE(lock->ctx) == ctx;
> +}
> +
>  #endif
> --
> 2.14.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
> 
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>  include/linux/ww_mutex.h               | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index eaa3cb0c3ad1..4c04b560e358 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>  	*map = mapping;
>  
>  	/* Double check that the BO is reserved by this CS */
> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> +				  &parser->ticket))
>  		return -EINVAL;
>  
>  	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +					struct task_struct *task,
> +					struct ww_acquire_ctx *ctx)
> +{
> +	return likely(__mutex_owner(&lock->base) == task) &&
> +		READ_ONCE(lock->ctx) == ctx;

Just comparing the context should be good enough. If you ever pass a
ww_acquire_ctx which does not belong to your own thread your seriously
wreaking things much worse already (and if we do catch that, should
probably lock the ctx to a given task when ww-mutex debugging is enabled).

That also simplifies the function signature.

Of course that means if you don't have a ctx, you can't test ownership of
a ww_mute, but I think that's not a really valid use-case. And not needed
for cmd submission, where you need the ctx anyway.

Besides this interface nit looks all good. With the task check&parameter
removed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-Daniel

> +}
> +
>  #endif
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> amdgpu needs to verify if userspace sends us valid addresses and the simplest
> way of doing this is to check if the buffer object is locked with the ticket
> of the current submission.
> 
> Clean up the access to the ww_mutex internals by providing a function
> for this and extend the check to the thread owning the underlying mutex.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>  include/linux/ww_mutex.h               | 17 +++++++++++++++++
>  2 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> index eaa3cb0c3ad1..4c04b560e358 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>  	*map = mapping;
>  
>  	/* Double check that the BO is reserved by this CS */
> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> +				  &parser->ticket))
>  		return -EINVAL;
>  
>  	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +					struct task_struct *task,
> +					struct ww_acquire_ctx *ctx)
> +{
> +	return likely(__mutex_owner(&lock->base) == task) &&
> +		READ_ONCE(lock->ctx) == ctx;

Just comparing the context should be good enough. If you ever pass a
ww_acquire_ctx which does not belong to your own thread your seriously
wreaking things much worse already (and if we do catch that, should
probably lock the ctx to a given task when ww-mutex debugging is enabled).

That also simplifies the function signature.

Of course that means if you don't have a ctx, you can't test ownership of
a ww_mute, but I think that's not a really valid use-case. And not needed
for cmd submission, where you need the ctx anyway.

Besides this interface nit looks all good. With the task check&parameter
removed:

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

-Daniel

> +}
> +
>  #endif
> -- 
> 2.14.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>>   include/linux/ww_mutex.h               | 17 +++++++++++++++++
>>   2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index eaa3cb0c3ad1..4c04b560e358 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>>   	*map = mapping;
>>   
>>   	/* Double check that the BO is reserved by this CS */
>> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
>> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
>> +				  &parser->ticket))
>>   		return -EINVAL;
>>   
>>   	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 39fda195bf78..dd580db289e8 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>> + * @lock: the mutex to be queried
>> + * @task: the task structure to check
>> + * @ctx: the w/w acquire context to test
>> + *
>> + * Returns true if the mutex is locked in the context by the given task, false
>> + * otherwise.
>> + */
>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>> +					struct task_struct *task,
>> +					struct ww_acquire_ctx *ctx)
>> +{
>> +	return likely(__mutex_owner(&lock->base) == task) &&
>> +		READ_ONCE(lock->ctx) == ctx;
> Just comparing the context should be good enough. If you ever pass a
> ww_acquire_ctx which does not belong to your own thread your seriously
> wreaking things much worse already (and if we do catch that, should
> probably lock the ctx to a given task when ww-mutex debugging is enabled).
>
> That also simplifies the function signature.
>
> Of course that means if you don't have a ctx, you can't test ownership of
> a ww_mute, but I think that's not a really valid use-case.

Well exactly that is the use case in TTM, see patch #3 in this series.

In TTM the evicted BOs are trylocked and so we need a way of testing for 
ownership without a context.

Christian.

>   And not needed
> for cmd submission, where you need the ctx anyway.
>
> Besides this interface nit looks all good. With the task check&parameter
> removed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> -Daniel
>
>> +}
>> +
>>   #endif
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>> way of doing this is to check if the buffer object is locked with the ticket
>> of the current submission.
>>
>> Clean up the access to the ww_mutex internals by providing a function
>> for this and extend the check to the thread owning the underlying mutex.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>>   include/linux/ww_mutex.h               | 17 +++++++++++++++++
>>   2 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> index eaa3cb0c3ad1..4c04b560e358 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>>   	*map = mapping;
>>   
>>   	/* Double check that the BO is reserved by this CS */
>> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
>> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
>> +				  &parser->ticket))
>>   		return -EINVAL;
>>   
>>   	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 39fda195bf78..dd580db289e8 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>> + * @lock: the mutex to be queried
>> + * @task: the task structure to check
>> + * @ctx: the w/w acquire context to test
>> + *
>> + * Returns true if the mutex is locked in the context by the given task, false
>> + * otherwise.
>> + */
>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>> +					struct task_struct *task,
>> +					struct ww_acquire_ctx *ctx)
>> +{
>> +	return likely(__mutex_owner(&lock->base) == task) &&
>> +		READ_ONCE(lock->ctx) == ctx;
> Just comparing the context should be good enough. If you ever pass a
> ww_acquire_ctx which does not belong to your own thread your seriously
> wreaking things much worse already (and if we do catch that, should
> probably lock the ctx to a given task when ww-mutex debugging is enabled).
>
> That also simplifies the function signature.
>
> Of course that means if you don't have a ctx, you can't test ownership of
> a ww_mute, but I think that's not a really valid use-case.

Well exactly that is the use case in TTM, see patch #3 in this series.

In TTM the evicted BOs are trylocked and so we need a way of testing for 
ownership without a context.

Christian.

>   And not needed
> for cmd submission, where you need the ctx anyway.
>
> Besides this interface nit looks all good. With the task check&parameter
> removed:
>
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> -Daniel
>
>> +}
>> +
>>   #endif
>> -- 
>> 2.14.1
>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
> Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> > On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> > > amdgpu needs to verify if userspace sends us valid addresses and the simplest
> > > way of doing this is to check if the buffer object is locked with the ticket
> > > of the current submission.
> > > 
> > > Clean up the access to the ww_mutex internals by providing a function
> > > for this and extend the check to the thread owning the underlying mutex.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
> > >   include/linux/ww_mutex.h               | 17 +++++++++++++++++
> > >   2 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > index eaa3cb0c3ad1..4c04b560e358 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
> > >   	*map = mapping;
> > >   	/* Double check that the BO is reserved by this CS */
> > > -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> > > +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> > > +				  &parser->ticket))
> > >   		return -EINVAL;
> > >   	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> > > diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> > > index 39fda195bf78..dd580db289e8 100644
> > > --- a/include/linux/ww_mutex.h
> > > +++ b/include/linux/ww_mutex.h
> > > @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
> > >   	return mutex_is_locked(&lock->base);
> > >   }
> > > +/**
> > > + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> > > + * @lock: the mutex to be queried
> > > + * @task: the task structure to check
> > > + * @ctx: the w/w acquire context to test
> > > + *
> > > + * Returns true if the mutex is locked in the context by the given task, false
> > > + * otherwise.
> > > + */
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > +					struct task_struct *task,
> > > +					struct ww_acquire_ctx *ctx)
> > > +{
> > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > +		READ_ONCE(lock->ctx) == ctx;
> > Just comparing the context should be good enough. If you ever pass a
> > ww_acquire_ctx which does not belong to your own thread your seriously
> > wreaking things much worse already (and if we do catch that, should
> > probably lock the ctx to a given task when ww-mutex debugging is enabled).
> > 
> > That also simplifies the function signature.
> > 
> > Of course that means if you don't have a ctx, you can't test ownership of
> > a ww_mute, but I think that's not a really valid use-case.
> 
> Well exactly that is the use case in TTM, see patch #3 in this series.
> 
> In TTM the evicted BOs are trylocked and so we need a way of testing for
> ownership without a context.

I don't think your final patch to keep ww_mutex locked until the end
works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
trylock bypasses the entire deadlock avoidance).

If this is really what you want to do, then we need a
ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
threads can correctly resolve deadlocks when you hold that lock while
trying to grab additional locks). In which case you really don't need the
task pointer.

Yes it's a disappointment that lockdep doesn't correctly track trylocks,
it just does basic sanity checks, but then drops them on the floor wrt
depency tracking. Just in case you wonder why you're not getting a
lockdeps splat for this. Unfortunately I don't understand lockdep enough
to be able to fix this gap.
-Daniel

> 
> Christian.
> 
> >   And not needed
> > for cmd submission, where you need the ctx anyway.
> > 
> > Besides this interface nit looks all good. With the task check&parameter
> > removed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > -Daniel
> > 
> > > +}
> > > +
> > >   #endif
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
> Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> > On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> > > amdgpu needs to verify if userspace sends us valid addresses and the simplest
> > > way of doing this is to check if the buffer object is locked with the ticket
> > > of the current submission.
> > > 
> > > Clean up the access to the ww_mutex internals by providing a function
> > > for this and extend the check to the thread owning the underlying mutex.
> > > 
> > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
> > >   include/linux/ww_mutex.h               | 17 +++++++++++++++++
> > >   2 files changed, 19 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > index eaa3cb0c3ad1..4c04b560e358 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
> > >   	*map = mapping;
> > >   	/* Double check that the BO is reserved by this CS */
> > > -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> > > +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> > > +				  &parser->ticket))
> > >   		return -EINVAL;
> > >   	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> > > diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> > > index 39fda195bf78..dd580db289e8 100644
> > > --- a/include/linux/ww_mutex.h
> > > +++ b/include/linux/ww_mutex.h
> > > @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
> > >   	return mutex_is_locked(&lock->base);
> > >   }
> > > +/**
> > > + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> > > + * @lock: the mutex to be queried
> > > + * @task: the task structure to check
> > > + * @ctx: the w/w acquire context to test
> > > + *
> > > + * Returns true if the mutex is locked in the context by the given task, false
> > > + * otherwise.
> > > + */
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > +					struct task_struct *task,
> > > +					struct ww_acquire_ctx *ctx)
> > > +{
> > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > +		READ_ONCE(lock->ctx) == ctx;
> > Just comparing the context should be good enough. If you ever pass a
> > ww_acquire_ctx which does not belong to your own thread your seriously
> > wreaking things much worse already (and if we do catch that, should
> > probably lock the ctx to a given task when ww-mutex debugging is enabled).
> > 
> > That also simplifies the function signature.
> > 
> > Of course that means if you don't have a ctx, you can't test ownership of
> > a ww_mute, but I think that's not a really valid use-case.
> 
> Well exactly that is the use case in TTM, see patch #3 in this series.
> 
> In TTM the evicted BOs are trylocked and so we need a way of testing for
> ownership without a context.

I don't think your final patch to keep ww_mutex locked until the end
works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
trylock bypasses the entire deadlock avoidance).

If this is really what you want to do, then we need a
ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
threads can correctly resolve deadlocks when you hold that lock while
trying to grab additional locks). In which case you really don't need the
task pointer.

Yes it's a disappointment that lockdep doesn't correctly track trylocks,
it just does basic sanity checks, but then drops them on the floor wrt
depency tracking. Just in case you wonder why you're not getting a
lockdeps splat for this. Unfortunately I don't understand lockdep enough
to be able to fix this gap.
-Daniel

> 
> Christian.
> 
> >   And not needed
> > for cmd submission, where you need the ctx anyway.
> > 
> > Besides this interface nit looks all good. With the task check&parameter
> > removed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > 
> > -Daniel
> > 
> > > +}
> > > +
> > >   #endif
> > > -- 
> > > 2.14.1
> > > 
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 17:15 schrieb Daniel Vetter:
> On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
>> Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
>>> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>>>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>>>> way of doing this is to check if the buffer object is locked with the ticket
>>>> of the current submission.
>>>>
>>>> Clean up the access to the ww_mutex internals by providing a function
>>>> for this and extend the check to the thread owning the underlying mutex.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>>>>    include/linux/ww_mutex.h               | 17 +++++++++++++++++
>>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index eaa3cb0c3ad1..4c04b560e358 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>>>>    	*map = mapping;
>>>>    	/* Double check that the BO is reserved by this CS */
>>>> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
>>>> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
>>>> +				  &parser->ticket))
>>>>    		return -EINVAL;
>>>>    	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
>>>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>>>> index 39fda195bf78..dd580db289e8 100644
>>>> --- a/include/linux/ww_mutex.h
>>>> +++ b/include/linux/ww_mutex.h
>>>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>>>    	return mutex_is_locked(&lock->base);
>>>>    }
>>>> +/**
>>>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>>>> + * @lock: the mutex to be queried
>>>> + * @task: the task structure to check
>>>> + * @ctx: the w/w acquire context to test
>>>> + *
>>>> + * Returns true if the mutex is locked in the context by the given task, false
>>>> + * otherwise.
>>>> + */
>>>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>>>> +					struct task_struct *task,
>>>> +					struct ww_acquire_ctx *ctx)
>>>> +{
>>>> +	return likely(__mutex_owner(&lock->base) == task) &&
>>>> +		READ_ONCE(lock->ctx) == ctx;
>>> Just comparing the context should be good enough. If you ever pass a
>>> ww_acquire_ctx which does not belong to your own thread your seriously
>>> wreaking things much worse already (and if we do catch that, should
>>> probably lock the ctx to a given task when ww-mutex debugging is enabled).
>>>
>>> That also simplifies the function signature.
>>>
>>> Of course that means if you don't have a ctx, you can't test ownership of
>>> a ww_mute, but I think that's not a really valid use-case.
>> Well exactly that is the use case in TTM, see patch #3 in this series.
>>
>> In TTM the evicted BOs are trylocked and so we need a way of testing for
>> ownership without a context.
> I don't think your final patch to keep ww_mutex locked until the end
> works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
> trylock bypasses the entire deadlock avoidance).

Well that is not a problem at all. See we don't nest trylock with normal 
lock acquiring, cause that would indeed bypass the whole deadlock detection.

Instead we first use ww_mutex_acquire to lock all BOs which are needed 
for a command submission, including the deadlock detection.

Then all additional BOs which needed to be evicted to fulfill the 
current request are trylocked.

> If this is really what you want to do, then we need a
> ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> threads can correctly resolve deadlocks when you hold that lock while
> trying to grab additional locks). In which case you really don't need the
> task pointer.

Actually considered that as well, but it turned out that this is exactly 
what I don't want.

Cause then we wouldn't be able to distinct ww_mutex locked with a 
context (because they are part of the submission) and without (because 
TTM trylocked them).

> Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> it just does basic sanity checks, but then drops them on the floor wrt
> depency tracking. Just in case you wonder why you're not getting a
> lockdeps splat for this. Unfortunately I don't understand lockdep enough
> to be able to fix this gap.

Sorry to disappoint you, but lockdep is indeed capable to correctly 
track those trylocked BOs.

Got quite a bunch of warning before I was able to resolve to this solution.

Christian.

> -Daniel
>
>> Christian.
>>
>>>    And not needed
>>> for cmd submission, where you need the ctx anyway.
>>>
>>> Besides this interface nit looks all good. With the task check&parameter
>>> removed:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> -Daniel
>>>
>>>> +}
>>>> +
>>>>    #endif
>>>> -- 
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 17:15 schrieb Daniel Vetter:
> On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
>> Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
>>> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>>>> amdgpu needs to verify if userspace sends us valid addresses and the simplest
>>>> way of doing this is to check if the buffer object is locked with the ticket
>>>> of the current submission.
>>>>
>>>> Clean up the access to the ww_mutex internals by providing a function
>>>> for this and extend the check to the thread owning the underlying mutex.
>>>>
>>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>>> ---
>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
>>>>    include/linux/ww_mutex.h               | 17 +++++++++++++++++
>>>>    2 files changed, 19 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index eaa3cb0c3ad1..4c04b560e358 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
>>>>    	*map = mapping;
>>>>    	/* Double check that the BO is reserved by this CS */
>>>> -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
>>>> +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
>>>> +				  &parser->ticket))
>>>>    		return -EINVAL;
>>>>    	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
>>>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>>>> index 39fda195bf78..dd580db289e8 100644
>>>> --- a/include/linux/ww_mutex.h
>>>> +++ b/include/linux/ww_mutex.h
>>>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>>>    	return mutex_is_locked(&lock->base);
>>>>    }
>>>> +/**
>>>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>>>> + * @lock: the mutex to be queried
>>>> + * @task: the task structure to check
>>>> + * @ctx: the w/w acquire context to test
>>>> + *
>>>> + * Returns true if the mutex is locked in the context by the given task, false
>>>> + * otherwise.
>>>> + */
>>>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>>>> +					struct task_struct *task,
>>>> +					struct ww_acquire_ctx *ctx)
>>>> +{
>>>> +	return likely(__mutex_owner(&lock->base) == task) &&
>>>> +		READ_ONCE(lock->ctx) == ctx;
>>> Just comparing the context should be good enough. If you ever pass a
>>> ww_acquire_ctx which does not belong to your own thread your seriously
>>> wreaking things much worse already (and if we do catch that, should
>>> probably lock the ctx to a given task when ww-mutex debugging is enabled).
>>>
>>> That also simplifies the function signature.
>>>
>>> Of course that means if you don't have a ctx, you can't test ownership of
>>> a ww_mute, but I think that's not a really valid use-case.
>> Well exactly that is the use case in TTM, see patch #3 in this series.
>>
>> In TTM the evicted BOs are trylocked and so we need a way of testing for
>> ownership without a context.
> I don't think your final patch to keep ww_mutex locked until the end
> works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
> trylock bypasses the entire deadlock avoidance).

Well that is not a problem at all. See we don't nest trylock with normal 
lock acquiring, cause that would indeed bypass the whole deadlock detection.

Instead we first use ww_mutex_acquire to lock all BOs which are needed 
for a command submission, including the deadlock detection.

Then all additional BOs which needed to be evicted to fulfill the 
current request are trylocked.

> If this is really what you want to do, then we need a
> ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> threads can correctly resolve deadlocks when you hold that lock while
> trying to grab additional locks). In which case you really don't need the
> task pointer.

Actually considered that as well, but it turned out that this is exactly 
what I don't want.

Cause then we wouldn't be able to distinct ww_mutex locked with a 
context (because they are part of the submission) and without (because 
TTM trylocked them).

> Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> it just does basic sanity checks, but then drops them on the floor wrt
> depency tracking. Just in case you wonder why you're not getting a
> lockdeps splat for this. Unfortunately I don't understand lockdep enough
> to be able to fix this gap.

Sorry to disappoint you, but lockdep is indeed capable to correctly 
track those trylocked BOs.

Got quite a bunch of warning before I was able to resolve to this solution.

Christian.

> -Daniel
>
>> Christian.
>>
>>>    And not needed
>>> for cmd submission, where you need the ctx anyway.
>>>
>>> Besides this interface nit looks all good. With the task check&parameter
>>> removed:
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> -Daniel
>>>
>>>> +}
>>>> +
>>>>    #endif
>>>> -- 
>>>> 2.14.1
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
> Am 19.02.2018 um 17:15 schrieb Daniel Vetter:
> > On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
> > > Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> > > > On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> > > > > amdgpu needs to verify if userspace sends us valid addresses and the simplest
> > > > > way of doing this is to check if the buffer object is locked with the ticket
> > > > > of the current submission.
> > > > > 
> > > > > Clean up the access to the ww_mutex internals by providing a function
> > > > > for this and extend the check to the thread owning the underlying mutex.
> > > > > 
> > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > ---
> > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
> > > > >    include/linux/ww_mutex.h               | 17 +++++++++++++++++
> > > > >    2 files changed, 19 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > index eaa3cb0c3ad1..4c04b560e358 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
> > > > >    	*map = mapping;
> > > > >    	/* Double check that the BO is reserved by this CS */
> > > > > -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> > > > > +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> > > > > +				  &parser->ticket))
> > > > >    		return -EINVAL;
> > > > >    	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> > > > > diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> > > > > index 39fda195bf78..dd580db289e8 100644
> > > > > --- a/include/linux/ww_mutex.h
> > > > > +++ b/include/linux/ww_mutex.h
> > > > > @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
> > > > >    	return mutex_is_locked(&lock->base);
> > > > >    }
> > > > > +/**
> > > > > + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> > > > > + * @lock: the mutex to be queried
> > > > > + * @task: the task structure to check
> > > > > + * @ctx: the w/w acquire context to test
> > > > > + *
> > > > > + * Returns true if the mutex is locked in the context by the given task, false
> > > > > + * otherwise.
> > > > > + */
> > > > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > > > +					struct task_struct *task,
> > > > > +					struct ww_acquire_ctx *ctx)
> > > > > +{
> > > > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > > > +		READ_ONCE(lock->ctx) == ctx;
> > > > Just comparing the context should be good enough. If you ever pass a
> > > > ww_acquire_ctx which does not belong to your own thread your seriously
> > > > wreaking things much worse already (and if we do catch that, should
> > > > probably lock the ctx to a given task when ww-mutex debugging is enabled).
> > > > 
> > > > That also simplifies the function signature.
> > > > 
> > > > Of course that means if you don't have a ctx, you can't test ownership of
> > > > a ww_mute, but I think that's not a really valid use-case.
> > > Well exactly that is the use case in TTM, see patch #3 in this series.
> > > 
> > > In TTM the evicted BOs are trylocked and so we need a way of testing for
> > > ownership without a context.
> > I don't think your final patch to keep ww_mutex locked until the end
> > works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
> > trylock bypasses the entire deadlock avoidance).
> 
> Well that is not a problem at all. See we don't nest trylock with normal
> lock acquiring, cause that would indeed bypass the whole deadlock detection.
> 
> Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
> command submission, including the deadlock detection.
> 
> Then all additional BOs which needed to be evicted to fulfill the current
> request are trylocked.

Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
catches that one (and not some other recursion combo) then I think we
don't have to worry about holding tons of trylock'ed locks.
> 
> > If this is really what you want to do, then we need a
> > ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> > threads can correctly resolve deadlocks when you hold that lock while
> > trying to grab additional locks). In which case you really don't need the
> > task pointer.
> 
> Actually considered that as well, but it turned out that this is exactly
> what I don't want.
> 
> Cause then we wouldn't be able to distinct ww_mutex locked with a context
> (because they are part of the submission) and without (because TTM trylocked
> them).

Out of curiosity, why do you need to know that?

> > Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> > it just does basic sanity checks, but then drops them on the floor wrt
> > depency tracking. Just in case you wonder why you're not getting a
> > lockdeps splat for this. Unfortunately I don't understand lockdep enough
> > to be able to fix this gap.
> 
> Sorry to disappoint you, but lockdep is indeed capable to correctly track
> those trylocked BOs.
> 
> Got quite a bunch of warning before I was able to resolve to this solution.

Hm, I thought it didn't detect a lock; trylock; lock combo because the
trylock didn't show up in the dependency stack. Maybe that got fixed
meanwhile.

Assuming that we indeed need both, could we split up the two use-cases for
clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
forgoes checking for a task, since that's implied) and requires a non-NULL
ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
if ww-mutex debugging is enabled).

Or does that hit another requirement of your use-case?
-Daniel

> 
> Christian.
> 
> > -Daniel
> > 
> > > Christian.
> > > 
> > > >    And not needed
> > > > for cmd submission, where you need the ctx anyway.
> > > > 
> > > > Besides this interface nit looks all good. With the task check&parameter
> > > > removed:
> > > > 
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > -Daniel
> > > > 
> > > > > +}
> > > > > +
> > > > >    #endif
> > > > > -- 
> > > > > 2.14.1
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
> Am 19.02.2018 um 17:15 schrieb Daniel Vetter:
> > On Mon, Feb 19, 2018 at 04:41:55PM +0100, Christian König wrote:
> > > Am 19.02.2018 um 16:24 schrieb Daniel Vetter:
> > > > On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> > > > > amdgpu needs to verify if userspace sends us valid addresses and the simplest
> > > > > way of doing this is to check if the buffer object is locked with the ticket
> > > > > of the current submission.
> > > > > 
> > > > > Clean up the access to the ww_mutex internals by providing a function
> > > > > for this and extend the check to the thread owning the underlying mutex.
> > > > > 
> > > > > Signed-off-by: Christian König <christian.koenig@amd.com>
> > > > > ---
> > > > >    drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c |  3 ++-
> > > > >    include/linux/ww_mutex.h               | 17 +++++++++++++++++
> > > > >    2 files changed, 19 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > index eaa3cb0c3ad1..4c04b560e358 100644
> > > > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > > > @@ -1594,7 +1594,8 @@ int amdgpu_cs_find_mapping(struct amdgpu_cs_parser *parser,
> > > > >    	*map = mapping;
> > > > >    	/* Double check that the BO is reserved by this CS */
> > > > > -	if (READ_ONCE((*bo)->tbo.resv->lock.ctx) != &parser->ticket)
> > > > > +	if (!ww_mutex_is_owned_by(&(*bo)->tbo.resv->lock, current,
> > > > > +				  &parser->ticket))
> > > > >    		return -EINVAL;
> > > > >    	if (!((*bo)->flags & AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS)) {
> > > > > diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> > > > > index 39fda195bf78..dd580db289e8 100644
> > > > > --- a/include/linux/ww_mutex.h
> > > > > +++ b/include/linux/ww_mutex.h
> > > > > @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
> > > > >    	return mutex_is_locked(&lock->base);
> > > > >    }
> > > > > +/**
> > > > > + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> > > > > + * @lock: the mutex to be queried
> > > > > + * @task: the task structure to check
> > > > > + * @ctx: the w/w acquire context to test
> > > > > + *
> > > > > + * Returns true if the mutex is locked in the context by the given task, false
> > > > > + * otherwise.
> > > > > + */
> > > > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > > > +					struct task_struct *task,
> > > > > +					struct ww_acquire_ctx *ctx)
> > > > > +{
> > > > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > > > +		READ_ONCE(lock->ctx) == ctx;
> > > > Just comparing the context should be good enough. If you ever pass a
> > > > ww_acquire_ctx which does not belong to your own thread your seriously
> > > > wreaking things much worse already (and if we do catch that, should
> > > > probably lock the ctx to a given task when ww-mutex debugging is enabled).
> > > > 
> > > > That also simplifies the function signature.
> > > > 
> > > > Of course that means if you don't have a ctx, you can't test ownership of
> > > > a ww_mute, but I think that's not a really valid use-case.
> > > Well exactly that is the use case in TTM, see patch #3 in this series.
> > > 
> > > In TTM the evicted BOs are trylocked and so we need a way of testing for
> > > ownership without a context.
> > I don't think your final patch to keep ww_mutex locked until the end
> > works. You can't really nest ww_mutex_trylock with ww_mutex at will (since
> > trylock bypasses the entire deadlock avoidance).
> 
> Well that is not a problem at all. See we don't nest trylock with normal
> lock acquiring, cause that would indeed bypass the whole deadlock detection.
> 
> Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
> command submission, including the deadlock detection.
> 
> Then all additional BOs which needed to be evicted to fulfill the current
> request are trylocked.

Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
catches that one (and not some other recursion combo) then I think we
don't have to worry about holding tons of trylock'ed locks.
> 
> > If this is really what you want to do, then we need a
> > ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> > threads can correctly resolve deadlocks when you hold that lock while
> > trying to grab additional locks). In which case you really don't need the
> > task pointer.
> 
> Actually considered that as well, but it turned out that this is exactly
> what I don't want.
> 
> Cause then we wouldn't be able to distinct ww_mutex locked with a context
> (because they are part of the submission) and without (because TTM trylocked
> them).

Out of curiosity, why do you need to know that?

> > Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> > it just does basic sanity checks, but then drops them on the floor wrt
> > depency tracking. Just in case you wonder why you're not getting a
> > lockdeps splat for this. Unfortunately I don't understand lockdep enough
> > to be able to fix this gap.
> 
> Sorry to disappoint you, but lockdep is indeed capable to correctly track
> those trylocked BOs.
> 
> Got quite a bunch of warning before I was able to resolve to this solution.

Hm, I thought it didn't detect a lock; trylock; lock combo because the
trylock didn't show up in the dependency stack. Maybe that got fixed
meanwhile.

Assuming that we indeed need both, could we split up the two use-cases for
clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
forgoes checking for a task, since that's implied) and requires a non-NULL
ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
if ww-mutex debugging is enabled).

Or does that hit another requirement of your use-case?
-Daniel

> 
> Christian.
> 
> > -Daniel
> > 
> > > Christian.
> > > 
> > > >    And not needed
> > > > for cmd submission, where you need the ctx anyway.
> > > > 
> > > > Besides this interface nit looks all good. With the task check&parameter
> > > > removed:
> > > > 
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > 
> > > > -Daniel
> > > > 
> > > > > +}
> > > > > +
> > > > >    #endif
> > > > > -- 
> > > > > 2.14.1
> > > > > 
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 17:43 schrieb Daniel Vetter:
> On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
>> [SNIP]
>> Well that is not a problem at all. See we don't nest trylock with normal
>> lock acquiring, cause that would indeed bypass the whole deadlock detection.
>>
>> Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
>> command submission, including the deadlock detection.
>>
>> Then all additional BOs which needed to be evicted to fulfill the current
>> request are trylocked.
> Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
> catches that one (and not some other recursion combo) then I think we
> don't have to worry about holding tons of trylock'ed locks.

Well I haven't explicitly tested the lock; trylock; lock case, but you 
get a warning anyway in the lock; ... anything ...; lock case.

See the first and the second lock can't use the same acquire context, 
because that isn't known down the call stack and lockdep warns about 
that quite intensively.

What is a problem is that lockdep sometimes runs out of space to keep 
track of all the trylocked mutexes, but that could have happened before 
as well if I'm not completely mistaken.

>>> If this is really what you want to do, then we need a
>>> ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
>>> threads can correctly resolve deadlocks when you hold that lock while
>>> trying to grab additional locks). In which case you really don't need the
>>> task pointer.
>> Actually considered that as well, but it turned out that this is exactly
>> what I don't want.
>>
>> Cause then we wouldn't be able to distinct ww_mutex locked with a context
>> (because they are part of the submission) and without (because TTM trylocked
>> them).
> Out of curiosity, why do you need to know that?

The control flow in TTM is that when you trylocked a BO you start to 
evict it.

Now sometimes it happens that we evict it from VRAM to GTT, but then 
find that we don't have enough GTT space and need to evict something 
from there to the SYSTEM domain.

The problem is now since the reservation object is trylocked because of 
the VRAM to GTT eviction we can't lock it again because of the GTT to 
the SYSTEM domain.

>>> Yes it's a disappointment that lockdep doesn't correctly track trylocks,
>>> it just does basic sanity checks, but then drops them on the floor wrt
>>> depency tracking. Just in case you wonder why you're not getting a
>>> lockdeps splat for this. Unfortunately I don't understand lockdep enough
>>> to be able to fix this gap.
>> Sorry to disappoint you, but lockdep is indeed capable to correctly track
>> those trylocked BOs.
>>
>> Got quite a bunch of warning before I was able to resolve to this solution.
> Hm, I thought it didn't detect a lock; trylock; lock combo because the
> trylock didn't show up in the dependency stack. Maybe that got fixed
> meanwhile.

Yeah I can confirm that this indeed got fixed.

> Assuming that we indeed need both, could we split up the two use-cases for
> clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
> forgoes checking for a task, since that's implied) and requires a non-NULL
> ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
> we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
> if ww-mutex debugging is enabled).
>
> Or does that hit another requirement of your use-case?

Well we could add two tests, one which only checks the context and one 
which checks that the context is NULL and then checks the mutex owner.

But to me it actually looks more like that makes it unnecessary 
complicated. The use case in amdgpu which could only check the context 
isn't performance critical.

Christian.

> -Daniel
>

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 19.02.2018 um 17:43 schrieb Daniel Vetter:
> On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
>> [SNIP]
>> Well that is not a problem at all. See we don't nest trylock with normal
>> lock acquiring, cause that would indeed bypass the whole deadlock detection.
>>
>> Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
>> command submission, including the deadlock detection.
>>
>> Then all additional BOs which needed to be evicted to fulfill the current
>> request are trylocked.
> Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
> catches that one (and not some other recursion combo) then I think we
> don't have to worry about holding tons of trylock'ed locks.

Well I haven't explicitly tested the lock; trylock; lock case, but you 
get a warning anyway in the lock; ... anything ...; lock case.

See the first and the second lock can't use the same acquire context, 
because that isn't known down the call stack and lockdep warns about 
that quite intensively.

What is a problem is that lockdep sometimes runs out of space to keep 
track of all the trylocked mutexes, but that could have happened before 
as well if I'm not completely mistaken.

>>> If this is really what you want to do, then we need a
>>> ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
>>> threads can correctly resolve deadlocks when you hold that lock while
>>> trying to grab additional locks). In which case you really don't need the
>>> task pointer.
>> Actually considered that as well, but it turned out that this is exactly
>> what I don't want.
>>
>> Cause then we wouldn't be able to distinct ww_mutex locked with a context
>> (because they are part of the submission) and without (because TTM trylocked
>> them).
> Out of curiosity, why do you need to know that?

The control flow in TTM is that when you trylocked a BO you start to 
evict it.

Now sometimes it happens that we evict it from VRAM to GTT, but then 
find that we don't have enough GTT space and need to evict something 
from there to the SYSTEM domain.

The problem is now since the reservation object is trylocked because of 
the VRAM to GTT eviction we can't lock it again because of the GTT to 
the SYSTEM domain.

>>> Yes it's a disappointment that lockdep doesn't correctly track trylocks,
>>> it just does basic sanity checks, but then drops them on the floor wrt
>>> depency tracking. Just in case you wonder why you're not getting a
>>> lockdeps splat for this. Unfortunately I don't understand lockdep enough
>>> to be able to fix this gap.
>> Sorry to disappoint you, but lockdep is indeed capable to correctly track
>> those trylocked BOs.
>>
>> Got quite a bunch of warning before I was able to resolve to this solution.
> Hm, I thought it didn't detect a lock; trylock; lock combo because the
> trylock didn't show up in the dependency stack. Maybe that got fixed
> meanwhile.

Yeah I can confirm that this indeed got fixed.

> Assuming that we indeed need both, could we split up the two use-cases for
> clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
> forgoes checking for a task, since that's implied) and requires a non-NULL
> ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
> we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
> if ww-mutex debugging is enabled).
>
> Or does that hit another requirement of your use-case?

Well we could add two tests, one which only checks the context and one 
which checks that the context is NULL and then checks the mutex owner.

But to me it actually looks more like that makes it unnecessary 
complicated. The use case in amdgpu which could only check the context 
isn't performance critical.

Christian.

> -Daniel
>

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Tue, Feb 20, 2018 at 10:43:48AM +0100, Christian König wrote:
> Am 19.02.2018 um 17:43 schrieb Daniel Vetter:
> > On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
> > > [SNIP]
> > > Well that is not a problem at all. See we don't nest trylock with normal
> > > lock acquiring, cause that would indeed bypass the whole deadlock detection.
> > > 
> > > Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
> > > command submission, including the deadlock detection.
> > > 
> > > Then all additional BOs which needed to be evicted to fulfill the current
> > > request are trylocked.
> > Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
> > catches that one (and not some other recursion combo) then I think we
> > don't have to worry about holding tons of trylock'ed locks.
> 
> Well I haven't explicitly tested the lock; trylock; lock case, but you get a
> warning anyway in the lock; ... anything ...; lock case.
> 
> See the first and the second lock can't use the same acquire context,
> because that isn't known down the call stack and lockdep warns about that
> quite intensively.

Ah, so the ttm_ctx I've spotted was something entirely different and
doesn't contain the ww_acquire_ctx (I didn't check)? I'd assume you have
the same ctx passed around to everything in ttm, but if that doesn't exist
then we can indeed not annotate ww_mutex_trylock_ctx with the right ctx.

> What is a problem is that lockdep sometimes runs out of space to keep track
> of all the trylocked mutexes, but that could have happened before as well if
> I'm not completely mistaken.
> 
> > > > If this is really what you want to do, then we need a
> > > > ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> > > > threads can correctly resolve deadlocks when you hold that lock while
> > > > trying to grab additional locks). In which case you really don't need the
> > > > task pointer.
> > > Actually considered that as well, but it turned out that this is exactly
> > > what I don't want.
> > > 
> > > Cause then we wouldn't be able to distinct ww_mutex locked with a context
> > > (because they are part of the submission) and without (because TTM trylocked
> > > them).
> > Out of curiosity, why do you need to know that?
> 
> The control flow in TTM is that when you trylocked a BO you start to evict
> it.
> 
> Now sometimes it happens that we evict it from VRAM to GTT, but then find
> that we don't have enough GTT space and need to evict something from there
> to the SYSTEM domain.
> 
> The problem is now since the reservation object is trylocked because of the
> VRAM to GTT eviction we can't lock it again because of the GTT to the SYSTEM
> domain.
> 
> > > > Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> > > > it just does basic sanity checks, but then drops them on the floor wrt
> > > > depency tracking. Just in case you wonder why you're not getting a
> > > > lockdeps splat for this. Unfortunately I don't understand lockdep enough
> > > > to be able to fix this gap.
> > > Sorry to disappoint you, but lockdep is indeed capable to correctly track
> > > those trylocked BOs.
> > > 
> > > Got quite a bunch of warning before I was able to resolve to this solution.
> > Hm, I thought it didn't detect a lock; trylock; lock combo because the
> > trylock didn't show up in the dependency stack. Maybe that got fixed
> > meanwhile.
> 
> Yeah I can confirm that this indeed got fixed.
> 
> > Assuming that we indeed need both, could we split up the two use-cases for
> > clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
> > forgoes checking for a task, since that's implied) and requires a non-NULL
> > ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
> > we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
> > if ww-mutex debugging is enabled).
> > 
> > Or does that hit another requirement of your use-case?
> 
> Well we could add two tests, one which only checks the context and one which
> checks that the context is NULL and then checks the mutex owner.
> 
> But to me it actually looks more like that makes it unnecessary complicated.
> The use case in amdgpu which could only check the context isn't performance
> critical.

Oh I'm not worried about the runtime overhead at all, I'm worried about
conceptual clarity of this stuff. If you have a ctx there's no need to
also look at ->owner.

Another idea: We drop the task argument from functions and go with the
following logic:

ww_mutex_is_owner(lock, ctx)
{
	if (ctx)
		return lock->ctx == ctx;
	else
		return lock->owner == current;
}

I think that would solve your use case, and gives us the neat interface
I'm aiming for. Kerneldoc can then explain what's happening for a NULL
ctx.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Daniel Vetter]

On Tue, Feb 20, 2018 at 10:43:48AM +0100, Christian König wrote:
> Am 19.02.2018 um 17:43 schrieb Daniel Vetter:
> > On Mon, Feb 19, 2018 at 05:29:46PM +0100, Christian König wrote:
> > > [SNIP]
> > > Well that is not a problem at all. See we don't nest trylock with normal
> > > lock acquiring, cause that would indeed bypass the whole deadlock detection.
> > > 
> > > Instead we first use ww_mutex_acquire to lock all BOs which are needed for a
> > > command submission, including the deadlock detection.
> > > 
> > > Then all additional BOs which needed to be evicted to fulfill the current
> > > request are trylocked.
> > Yeah it's the lock; trylock; lock combo that deadlocks. If lockdep indeed
> > catches that one (and not some other recursion combo) then I think we
> > don't have to worry about holding tons of trylock'ed locks.
> 
> Well I haven't explicitly tested the lock; trylock; lock case, but you get a
> warning anyway in the lock; ... anything ...; lock case.
> 
> See the first and the second lock can't use the same acquire context,
> because that isn't known down the call stack and lockdep warns about that
> quite intensively.

Ah, so the ttm_ctx I've spotted was something entirely different and
doesn't contain the ww_acquire_ctx (I didn't check)? I'd assume you have
the same ctx passed around to everything in ttm, but if that doesn't exist
then we can indeed not annotate ww_mutex_trylock_ctx with the right ctx.

> What is a problem is that lockdep sometimes runs out of space to keep track
> of all the trylocked mutexes, but that could have happened before as well if
> I'm not completely mistaken.
> 
> > > > If this is really what you want to do, then we need a
> > > > ww_mutex_trylock_ctx, which also fills out the ctx value (so that other
> > > > threads can correctly resolve deadlocks when you hold that lock while
> > > > trying to grab additional locks). In which case you really don't need the
> > > > task pointer.
> > > Actually considered that as well, but it turned out that this is exactly
> > > what I don't want.
> > > 
> > > Cause then we wouldn't be able to distinct ww_mutex locked with a context
> > > (because they are part of the submission) and without (because TTM trylocked
> > > them).
> > Out of curiosity, why do you need to know that?
> 
> The control flow in TTM is that when you trylocked a BO you start to evict
> it.
> 
> Now sometimes it happens that we evict it from VRAM to GTT, but then find
> that we don't have enough GTT space and need to evict something from there
> to the SYSTEM domain.
> 
> The problem is now since the reservation object is trylocked because of the
> VRAM to GTT eviction we can't lock it again because of the GTT to the SYSTEM
> domain.
> 
> > > > Yes it's a disappointment that lockdep doesn't correctly track trylocks,
> > > > it just does basic sanity checks, but then drops them on the floor wrt
> > > > depency tracking. Just in case you wonder why you're not getting a
> > > > lockdeps splat for this. Unfortunately I don't understand lockdep enough
> > > > to be able to fix this gap.
> > > Sorry to disappoint you, but lockdep is indeed capable to correctly track
> > > those trylocked BOs.
> > > 
> > > Got quite a bunch of warning before I was able to resolve to this solution.
> > Hm, I thought it didn't detect a lock; trylock; lock combo because the
> > trylock didn't show up in the dependency stack. Maybe that got fixed
> > meanwhile.
> 
> Yeah I can confirm that this indeed got fixed.
> 
> > Assuming that we indeed need both, could we split up the two use-cases for
> > clarity? I.e. ww_mutex_is_owned_by_ctx, which only takes the ctx (and
> > forgoes checking for a task, since that's implied) and requires a non-NULL
> > ctx. And ww_mutex_is_owned_by_task, which only takes the task (and maybe
> > we should limit it to trylock and _single locks, i.e. WARN_ON(lock->ctx)
> > if ww-mutex debugging is enabled).
> > 
> > Or does that hit another requirement of your use-case?
> 
> Well we could add two tests, one which only checks the context and one which
> checks that the context is NULL and then checks the mutex owner.
> 
> But to me it actually looks more like that makes it unnecessary complicated.
> The use case in amdgpu which could only check the context isn't performance
> critical.

Oh I'm not worried about the runtime overhead at all, I'm worried about
conceptual clarity of this stuff. If you have a ctx there's no need to
also look at ->owner.

Another idea: We drop the task argument from functions and go with the
following logic:

ww_mutex_is_owner(lock, ctx)
{
	if (ctx)
		return lock->ctx == ctx;
	else
		return lock->owner == current;
}

I think that would solve your use case, and gives us the neat interface
I'm aiming for. Kerneldoc can then explain what's happening for a NULL
ctx.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 20.02.2018 um 12:33 schrieb Daniel Vetter:
> [SNIP]
> Ah, so the ttm_ctx I've spotted was something entirely different and
> doesn't contain the ww_acquire_ctx (I didn't check)? I'd assume you have
> the same ctx passed around to everything in ttm, but if that doesn't exist
> then we can indeed not annotate ww_mutex_trylock_ctx with the right ctx.

Yes, exactly.

I actually tried this approach, e.g. put the ww_acquire_context into the 
ttm_operation_context and then use that with ww_mutex_trylock_ctx.

But a) that turned out to be to much hassle, e.g. at least amdgpu 
doesn't use a ww_acquire context in most cases.

And b) it actually wasn't what I was looking for, e.g. I couldn't 
distinct between the trylocked BOs an everything else any more.

>> [SNIP]
>> But to me it actually looks more like that makes it unnecessary complicated.
>> The use case in amdgpu which could only check the context isn't performance
>> critical.
> Oh I'm not worried about the runtime overhead at all, I'm worried about
> conceptual clarity of this stuff. If you have a ctx there's no need to
> also look at ->owner.
>
> Another idea: We drop the task argument from functions and go with the
> following logic:
>
> ww_mutex_is_owner(lock, ctx)
> {
> 	if (ctx)
> 		return lock->ctx == ctx;
> 	else
> 		return lock->owner == current;
> }
>
> I think that would solve your use case, and gives us the neat interface
> I'm aiming for. Kerneldoc can then explain what's happening for a NULL
> ctx.

Good point, going to adjust the patches this way and resend.

Christian.

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 20.02.2018 um 12:33 schrieb Daniel Vetter:
> [SNIP]
> Ah, so the ttm_ctx I've spotted was something entirely different and
> doesn't contain the ww_acquire_ctx (I didn't check)? I'd assume you have
> the same ctx passed around to everything in ttm, but if that doesn't exist
> then we can indeed not annotate ww_mutex_trylock_ctx with the right ctx.

Yes, exactly.

I actually tried this approach, e.g. put the ww_acquire_context into the 
ttm_operation_context and then use that with ww_mutex_trylock_ctx.

But a) that turned out to be to much hassle, e.g. at least amdgpu 
doesn't use a ww_acquire context in most cases.

And b) it actually wasn't what I was looking for, e.g. I couldn't 
distinct between the trylocked BOs an everything else any more.

>> [SNIP]
>> But to me it actually looks more like that makes it unnecessary complicated.
>> The use case in amdgpu which could only check the context isn't performance
>> critical.
> Oh I'm not worried about the runtime overhead at all, I'm worried about
> conceptual clarity of this stuff. If you have a ctx there's no need to
> also look at ->owner.
>
> Another idea: We drop the task argument from functions and go with the
> following logic:
>
> ww_mutex_is_owner(lock, ctx)
> {
> 	if (ctx)
> 		return lock->ctx == ctx;
> 	else
> 		return lock->owner == current;
> }
>
> I think that would solve your use case, and gives us the neat interface
> I'm aiming for. Kerneldoc can then explain what's happening for a NULL
> ctx.

Good point, going to adjust the patches this way and resend.

Christian.
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Peter Zijlstra]

This really should've been Cc'ed to me.

On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +					struct task_struct *task,
> +					struct ww_acquire_ctx *ctx)
> +{
> +	return likely(__mutex_owner(&lock->base) == task) &&
> +		READ_ONCE(lock->ctx) == ctx;
> +}

Nak on that interface, that's racy and broken by design.

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Peter Zijlstra]

This really should've been Cc'ed to me.

On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 39fda195bf78..dd580db289e8 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
> + * @lock: the mutex to be queried
> + * @task: the task structure to check
> + * @ctx: the w/w acquire context to test
> + *
> + * Returns true if the mutex is locked in the context by the given task, false
> + * otherwise.
> + */
> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> +					struct task_struct *task,
> +					struct ww_acquire_ctx *ctx)
> +{
> +	return likely(__mutex_owner(&lock->base) == task) &&
> +		READ_ONCE(lock->ctx) == ctx;
> +}

Nak on that interface, that's racy and broken by design.

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

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
> This really should've been Cc'ed to me.
>
> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 39fda195bf78..dd580db289e8 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>> + * @lock: the mutex to be queried
>> + * @task: the task structure to check
>> + * @ctx: the w/w acquire context to test
>> + *
>> + * Returns true if the mutex is locked in the context by the given task, false
>> + * otherwise.
>> + */
>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>> +					struct task_struct *task,
>> +					struct ww_acquire_ctx *ctx)
>> +{
>> +	return likely(__mutex_owner(&lock->base) == task) &&
>> +		READ_ONCE(lock->ctx) == ctx;
>> +}
> Nak on that interface, that's racy and broken by design.

Why?

Regards,
Christian.

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Christian König]

Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
> This really should've been Cc'ed to me.
>
> On Thu, Feb 15, 2018 at 03:19:42PM +0100, Christian König wrote:
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 39fda195bf78..dd580db289e8 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -358,4 +358,21 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_is_owned_by - is the w/w mutex locked by this task in that context
>> + * @lock: the mutex to be queried
>> + * @task: the task structure to check
>> + * @ctx: the w/w acquire context to test
>> + *
>> + * Returns true if the mutex is locked in the context by the given task, false
>> + * otherwise.
>> + */
>> +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
>> +					struct task_struct *task,
>> +					struct ww_acquire_ctx *ctx)
>> +{
>> +	return likely(__mutex_owner(&lock->base) == task) &&
>> +		READ_ONCE(lock->ctx) == ctx;
>> +}
> Nak on that interface, that's racy and broken by design.

Why?

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

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Peter Zijlstra]

On Tue, Feb 20, 2018 at 02:08:26PM +0100, Christian König wrote:
> Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > +					struct task_struct *task,
> > > +					struct ww_acquire_ctx *ctx)
> > > +{
> > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > +		READ_ONCE(lock->ctx) == ctx;
> > > +}
> > Nak on that interface, that's racy and broken by design.
> 
> Why?

If task != current you can race with a concurrent mutex_unlock().

Re: [PATCH 1/3] locking/ww_mutex: cleanup lock->ctx usage in amdgpu

[Peter Zijlstra]

On Tue, Feb 20, 2018 at 02:08:26PM +0100, Christian König wrote:
> Am 20.02.2018 um 13:35 schrieb Peter Zijlstra:
> > > +static inline bool ww_mutex_is_owned_by(struct ww_mutex *lock,
> > > +					struct task_struct *task,
> > > +					struct ww_acquire_ctx *ctx)
> > > +{
> > > +	return likely(__mutex_owner(&lock->base) == task) &&
> > > +		READ_ONCE(lock->ctx) == ctx;
> > > +}
> > Nak on that interface, that's racy and broken by design.
> 
> Why?

If task != current you can race with a concurrent mutex_unlock().
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel