ww_mutex deadlock handling cleanup

ww_mutex deadlock handling cleanup

[Christian König]

The ww_mutex implementation allows drivers to acquire locks on a set of locking
primitives with detection of deadlocks. If a deadlock happens we can then
backoff and reacquire the contended lock until we should finally be able to
acquire all necessary locks for an operation.

The problem is that handling those deadlocks was the burden of the user of
the ww_mutex implementation and at least some users didn't got that right
on the first try.

So this patch adds two macros to lock and unlock multiple ww_mutex instances
with the necessary deadlock handling. I'm not a big fan of macros, but it still
better then implementing the same logic at least halve a dozen times.

Please note that only the first two patches are more than compile tested.
And this only cleans up the tip of the iceberg, just enough to show the
potential of those two macros.

Please review and/or comment,
Christian.

[PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

[Christian König]

The ww_mutex implementation allows for detection deadlocks when multiple
threads try to acquire the same set of locks in different order.

The problem is that handling those deadlocks was the burden of the user of
the ww_mutex implementation and at least some users didn't got that right
on the first try.

I'm not a big fan of macros, but it still better then implementing the same
logic at least halve a dozen times. So this patch adds two macros to lock
and unlock multiple ww_mutex instances with the necessary deadlock handling.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
index 3af7c0e03be5..a0d893b5b114 100644
--- a/include/linux/ww_mutex.h
+++ b/include/linux/ww_mutex.h
@@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
 	return mutex_is_locked(&lock->base);
 }
 
+/**
+ * ww_mutex_unlock_for_each - cleanup after error or contention
+ * @loop: for loop code fragment iterating over all the locks
+ * @pos: code fragment returning the currently handled lock
+ * @contended: the last contended ww_mutex or NULL or ERR_PTR
+ *
+ * Helper to make cleanup after error or lock contention easier.
+ * First unlock the last contended lock and then all other locked ones.
+ */
+#define ww_mutex_unlock_for_each(loop, pos, contended)	\
+	if (!IS_ERR(contended)) {			\
+		if (contended)				\
+			ww_mutex_unlock(contended);	\
+		contended = (pos);			\
+		loop {					\
+			if (contended == (pos))		\
+				break;			\
+			ww_mutex_unlock(pos);		\
+		}					\
+	}
+
+/**
+ * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
+ * @loop: for loop code fragment iterating over all the locks
+ * @pos: code fragment returning the currently handled lock
+ * @contended: ww_mutex pointer variable for state handling
+ * @ret: int variable to store the return value of ww_mutex_lock()
+ * @intr: If true ww_mutex_lock_interruptible() is used
+ * @ctx: ww_acquire_ctx pointer for the locking
+ *
+ * This macro implements the necessary logic to lock multiple ww_mutex
+ * instances. Lock contention with backoff and re-locking is handled by the
+ * macro so that the loop body only need to handle other errors and
+ * successfully acquired locks.
+ *
+ * With the @loop and @pos code fragment it is possible to apply this logic
+ * to all kind of containers (array, list, tree, etc...) holding multiple
+ * ww_mutex instances.
+ *
+ * @contended is used to hold the current state we are in. -ENOENT is used to
+ * signal that we are just starting the handling. -EDEADLK means that the
+ * current position is contended and we need to restart the loop after locking
+ * it. NULL means that there is no contention to be handled. Any other value is
+ * the last contended ww_mutex.
+ */
+#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
+	for (contended = ERR_PTR(-ENOENT); ({				\
+		__label__ relock, next;					\
+		ret = -ENOENT;						\
+		if (contended == ERR_PTR(-ENOENT))			\
+			contended = NULL;				\
+		else if (contended == ERR_PTR(-EDEADLK))		\
+			contended = (pos);				\
+		else							\
+			goto next;					\
+		loop {							\
+			if (contended == (pos))	{			\
+				contended = NULL;			\
+				continue;				\
+			}						\
+relock:									\
+			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
+				ww_mutex_lock_interruptible(pos, ctx);	\
+			if (ret == -EDEADLK) {				\
+				ww_mutex_unlock_for_each(loop, pos,	\
+							 contended);	\
+				contended = ERR_PTR(-EDEADLK);		\
+				goto relock;				\
+			}						\
+			break;						\
+next:									\
+			continue;					\
+		}							\
+	}), ret != -ENOENT;)
+
 #endif
-- 
2.17.1

[PATCH 2/6] drm/ttm: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

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

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
index 957ec375a4ba..3c3ac6c94d7f 100644
--- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c
+++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c
@@ -33,16 +33,6 @@
 #include <linux/sched.h>
 #include <linux/module.h>
 
-static void ttm_eu_backoff_reservation_reverse(struct list_head *list,
-					      struct ttm_validate_buffer *entry)
-{
-	list_for_each_entry_continue_reverse(entry, list, head) {
-		struct ttm_buffer_object *bo = entry->bo;
-
-		reservation_object_unlock(bo->resv);
-	}
-}
-
 static void ttm_eu_del_from_lru_locked(struct list_head *list)
 {
 	struct ttm_validate_buffer *entry;
@@ -96,8 +86,9 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
 			   struct list_head *list, bool intr,
 			   struct list_head *dups, bool del_lru)
 {
-	struct ttm_bo_global *glob;
 	struct ttm_validate_buffer *entry;
+	struct ww_mutex *contended;
+	struct ttm_bo_global *glob;
 	int ret;
 
 	if (list_empty(list))
@@ -109,68 +100,39 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
 	if (ticket)
 		ww_acquire_init(ticket, &reservation_ww_class);
 
-	list_for_each_entry(entry, list, head) {
+	ww_mutex_lock_for_each(list_for_each_entry(entry, list, head),
+			       &entry->bo->resv->lock, contended, ret,
+			       intr, ticket)
+	{
 		struct ttm_buffer_object *bo = entry->bo;
 
-		ret = __ttm_bo_reserve(bo, intr, (ticket == NULL), ticket);
 		if (!ret && unlikely(atomic_read(&bo->cpu_writers) > 0)) {
 			reservation_object_unlock(bo->resv);
-
 			ret = -EBUSY;
+			goto error;
+		}
 
-		} else if (ret == -EALREADY && dups) {
+		if (ret == -EALREADY && dups) {
 			struct ttm_validate_buffer *safe = entry;
+
 			entry = list_prev_entry(entry, head);
 			list_del(&safe->head);
 			list_add(&safe->head, dups);
 			continue;
 		}
 
-		if (!ret) {
-			if (!entry->num_shared)
-				continue;
-
-			ret = reservation_object_reserve_shared(bo->resv,
-								entry->num_shared);
-			if (!ret)
-				continue;
-		}
+		if (unlikely(ret))
+			goto error;
 
-		/* uh oh, we lost out, drop every reservation and try
-		 * to only reserve this buffer, then start over if
-		 * this succeeds.
-		 */
-		ttm_eu_backoff_reservation_reverse(list, entry);
-
-		if (ret == -EDEADLK) {
-			if (intr) {
-				ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock,
-								       ticket);
-			} else {
-				ww_mutex_lock_slow(&bo->resv->lock, ticket);
-				ret = 0;
-			}
-		}
+		if (!entry->num_shared)
+			continue;
 
-		if (!ret && entry->num_shared)
-			ret = reservation_object_reserve_shared(bo->resv,
-								entry->num_shared);
-
-		if (unlikely(ret != 0)) {
-			if (ret == -EINTR)
-				ret = -ERESTARTSYS;
-			if (ticket) {
-				ww_acquire_done(ticket);
-				ww_acquire_fini(ticket);
-			}
-			return ret;
+		ret = reservation_object_reserve_shared(bo->resv,
+							entry->num_shared);
+		if (unlikely(ret)) {
+			reservation_object_unlock(bo->resv);
+			goto error;
 		}
-
-		/* move this item to the front of the list,
-		 * forces correct iteration of the loop without keeping track
-		 */
-		list_del(&entry->head);
-		list_add(&entry->head, list);
 	}
 
 	if (del_lru) {
@@ -179,6 +141,17 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket,
 		spin_unlock(&glob->lru_lock);
 	}
 	return 0;
+
+error:
+	ww_mutex_unlock_for_each(list_for_each_entry(entry, list, head),
+				 &entry->bo->resv->lock, contended);
+	if (ret == -EINTR)
+		ret = -ERESTARTSYS;
+	if (ticket) {
+		ww_acquire_done(ticket);
+		ww_acquire_fini(ticket);
+	}
+	return ret;
 }
 EXPORT_SYMBOL(ttm_eu_reserve_buffers);
 
-- 
2.17.1

[PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

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

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 50de138c89e0..6e4623d3bee2 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1307,51 +1307,26 @@ int
 drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 			  struct ww_acquire_ctx *acquire_ctx)
 {
-	int contended = -1;
+	struct ww_mutex *contended;
 	int i, ret;
 
 	ww_acquire_init(acquire_ctx, &reservation_ww_class);
 
-retry:
-	if (contended != -1) {
-		struct drm_gem_object *obj = objs[contended];
-
-		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
-						       acquire_ctx);
-		if (ret) {
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
-
-	for (i = 0; i < count; i++) {
-		if (i == contended)
-			continue;
-
-		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
-						  acquire_ctx);
-		if (ret) {
-			int j;
-
-			for (j = 0; j < i; j++)
-				ww_mutex_unlock(&objs[j]->resv->lock);
-
-			if (contended != -1 && contended >= i)
-				ww_mutex_unlock(&objs[contended]->resv->lock);
-
-			if (ret == -EDEADLK) {
-				contended = i;
-				goto retry;
-			}
-
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
+	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
+			       &objs[i]->resv->lock, contended, ret, true,
+			       acquire_ctx)
+		if (ret)
+			goto error;
 
 	ww_acquire_done(acquire_ctx);
 
 	return 0;
+
+error:
+	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
+				 &objs[i]->resv->lock, contended);
+	ww_acquire_done(acquire_ctx);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_lock_reservations);
 
-- 
2.17.1

[PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

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

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 50de138c89e0..6e4623d3bee2 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1307,51 +1307,26 @@ int
 drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
 			  struct ww_acquire_ctx *acquire_ctx)
 {
-	int contended = -1;
+	struct ww_mutex *contended;
 	int i, ret;
 
 	ww_acquire_init(acquire_ctx, &reservation_ww_class);
 
-retry:
-	if (contended != -1) {
-		struct drm_gem_object *obj = objs[contended];
-
-		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
-						       acquire_ctx);
-		if (ret) {
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
-
-	for (i = 0; i < count; i++) {
-		if (i == contended)
-			continue;
-
-		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
-						  acquire_ctx);
-		if (ret) {
-			int j;
-
-			for (j = 0; j < i; j++)
-				ww_mutex_unlock(&objs[j]->resv->lock);
-
-			if (contended != -1 && contended >= i)
-				ww_mutex_unlock(&objs[contended]->resv->lock);
-
-			if (ret == -EDEADLK) {
-				contended = i;
-				goto retry;
-			}
-
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
+	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
+			       &objs[i]->resv->lock, contended, ret, true,
+			       acquire_ctx)
+		if (ret)
+			goto error;
 
 	ww_acquire_done(acquire_ctx);
 
 	return 0;
+
+error:
+	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
+				 &objs[i]->resv->lock, contended);
+	ww_acquire_done(acquire_ctx);
+	return ret;
 }
 EXPORT_SYMBOL(drm_gem_lock_reservations);
 
-- 
2.17.1

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

[PATCH 4/6] drm/etnaviv: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 55 +++++---------------
 1 file changed, 14 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index e054f09ac828..923b8a71f80d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -118,55 +118,28 @@ static void submit_unlock_object(struct etnaviv_gem_submit *submit, int i)
 static int submit_lock_objects(struct etnaviv_gem_submit *submit,
 		struct ww_acquire_ctx *ticket)
 {
-	int contended, slow_locked = -1, i, ret = 0;
-
-retry:
-	for (i = 0; i < submit->nr_bos; i++) {
-		struct drm_gem_object *obj = &submit->bos[i].obj->base;
-
-		if (slow_locked == i)
-			slow_locked = -1;
-
-		contended = i;
+	struct ww_mutex *contended;
+	int i, ret = 0;
 
-		if (!(submit->bos[i].flags & BO_LOCKED)) {
-			ret = ww_mutex_lock_interruptible(&obj->resv->lock,
-							  ticket);
-			if (ret == -EALREADY)
-				DRM_ERROR("BO at index %u already on submit list\n",
-					  i);
-			if (ret)
-				goto fail;
-			submit->bos[i].flags |= BO_LOCKED;
-		}
+	ww_mutex_lock_for_each(for (i = 0; i < submit->nr_bos; i++),
+			       &submit->bos[i].obj->base.resv->lock,
+			       contended, ret, true, ticket) {
+		if (ret == -EALREADY)
+			DRM_ERROR("BO at index %u already on submit list\n", i);
+		if (ret)
+			goto fail;
 	}
 
+	for (i = 0; i < submit->nr_bos; i++)
+		submit->bos[i].flags |= BO_LOCKED;
 	ww_acquire_done(ticket);
 
 	return 0;
 
 fail:
-	for (; i >= 0; i--)
-		submit_unlock_object(submit, i);
-
-	if (slow_locked > 0)
-		submit_unlock_object(submit, slow_locked);
-
-	if (ret == -EDEADLK) {
-		struct drm_gem_object *obj;
-
-		obj = &submit->bos[contended].obj->base;
-
-		/* we lost out in a seqno race, lock and retry.. */
-		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
-						       ticket);
-		if (!ret) {
-			submit->bos[contended].flags |= BO_LOCKED;
-			slow_locked = contended;
-			goto retry;
-		}
-	}
-
+	ww_mutex_unlock_for_each(for (i = 0; i < submit->nr_bos; i++),
+				 &submit->bos[i].obj->base.resv->lock,
+				 contended);
 	return ret;
 }
 
-- 
2.17.1

[PATCH 4/6] drm/etnaviv: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 55 +++++---------------
 1 file changed, 14 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
index e054f09ac828..923b8a71f80d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c
@@ -118,55 +118,28 @@ static void submit_unlock_object(struct etnaviv_gem_submit *submit, int i)
 static int submit_lock_objects(struct etnaviv_gem_submit *submit,
 		struct ww_acquire_ctx *ticket)
 {
-	int contended, slow_locked = -1, i, ret = 0;
-
-retry:
-	for (i = 0; i < submit->nr_bos; i++) {
-		struct drm_gem_object *obj = &submit->bos[i].obj->base;
-
-		if (slow_locked == i)
-			slow_locked = -1;
-
-		contended = i;
+	struct ww_mutex *contended;
+	int i, ret = 0;
 
-		if (!(submit->bos[i].flags & BO_LOCKED)) {
-			ret = ww_mutex_lock_interruptible(&obj->resv->lock,
-							  ticket);
-			if (ret == -EALREADY)
-				DRM_ERROR("BO at index %u already on submit list\n",
-					  i);
-			if (ret)
-				goto fail;
-			submit->bos[i].flags |= BO_LOCKED;
-		}
+	ww_mutex_lock_for_each(for (i = 0; i < submit->nr_bos; i++),
+			       &submit->bos[i].obj->base.resv->lock,
+			       contended, ret, true, ticket) {
+		if (ret == -EALREADY)
+			DRM_ERROR("BO at index %u already on submit list\n", i);
+		if (ret)
+			goto fail;
 	}
 
+	for (i = 0; i < submit->nr_bos; i++)
+		submit->bos[i].flags |= BO_LOCKED;
 	ww_acquire_done(ticket);
 
 	return 0;
 
 fail:
-	for (; i >= 0; i--)
-		submit_unlock_object(submit, i);
-
-	if (slow_locked > 0)
-		submit_unlock_object(submit, slow_locked);
-
-	if (ret == -EDEADLK) {
-		struct drm_gem_object *obj;
-
-		obj = &submit->bos[contended].obj->base;
-
-		/* we lost out in a seqno race, lock and retry.. */
-		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
-						       ticket);
-		if (!ret) {
-			submit->bos[contended].flags |= BO_LOCKED;
-			slow_locked = contended;
-			goto retry;
-		}
-	}
-
+	ww_mutex_unlock_for_each(for (i = 0; i < submit->nr_bos; i++),
+				 &submit->bos[i].obj->base.resv->lock,
+				 contended);
 	return ret;
 }
 
-- 
2.17.1

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

[PATCH 5/6] drm/lima: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/lima/lima_gem.c | 41 +++++++++------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 477c0f766663..6a51f100c29e 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -151,43 +151,24 @@ static int lima_gem_sync_bo(struct lima_sched_task *task, struct lima_bo *bo,
 static int lima_gem_lock_bos(struct lima_bo **bos, u32 nr_bos,
 			     struct ww_acquire_ctx *ctx)
 {
-	int i, ret = 0, contended, slow_locked = -1;
+	struct ww_mutex *contended;
+	int i, ret = 0;
 
 	ww_acquire_init(ctx, &reservation_ww_class);
 
-retry:
-	for (i = 0; i < nr_bos; i++) {
-		if (i == slow_locked) {
-			slow_locked = -1;
-			continue;
-		}
-
-		ret = ww_mutex_lock_interruptible(&bos[i]->gem.resv->lock, ctx);
-		if (ret < 0) {
-			contended = i;
-			goto err;
-		}
-	}
+	ww_mutex_lock_for_each(for (i = 0; i < nr_bos; i++),
+			       &bos[i]->gem.resv->lock, contended, ret, true,
+			       ctx)
+		if (ret)
+			goto error;
 
 	ww_acquire_done(ctx);
-	return 0;
 
-err:
-	for (i--; i >= 0; i--)
-		ww_mutex_unlock(&bos[i]->gem.resv->lock);
-
-	if (slow_locked >= 0)
-		ww_mutex_unlock(&bos[slow_locked]->gem.resv->lock);
+	return 0;
 
-	if (ret == -EDEADLK) {
-		/* we lost out in a seqno race, lock and retry.. */
-		ret = ww_mutex_lock_slow_interruptible(
-			&bos[contended]->gem.resv->lock, ctx);
-		if (!ret) {
-			slow_locked = contended;
-			goto retry;
-		}
-	}
+error:
+	ww_mutex_unlock_for_each(for (i = 0; i < nr_bos; i++),
+				 &bos[i]->gem.resv->lock, contended);
 	ww_acquire_fini(ctx);
 
 	return ret;
-- 
2.17.1

[PATCH 5/6] drm/lima: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/lima/lima_gem.c | 41 +++++++++------------------------
 1 file changed, 11 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c
index 477c0f766663..6a51f100c29e 100644
--- a/drivers/gpu/drm/lima/lima_gem.c
+++ b/drivers/gpu/drm/lima/lima_gem.c
@@ -151,43 +151,24 @@ static int lima_gem_sync_bo(struct lima_sched_task *task, struct lima_bo *bo,
 static int lima_gem_lock_bos(struct lima_bo **bos, u32 nr_bos,
 			     struct ww_acquire_ctx *ctx)
 {
-	int i, ret = 0, contended, slow_locked = -1;
+	struct ww_mutex *contended;
+	int i, ret = 0;
 
 	ww_acquire_init(ctx, &reservation_ww_class);
 
-retry:
-	for (i = 0; i < nr_bos; i++) {
-		if (i == slow_locked) {
-			slow_locked = -1;
-			continue;
-		}
-
-		ret = ww_mutex_lock_interruptible(&bos[i]->gem.resv->lock, ctx);
-		if (ret < 0) {
-			contended = i;
-			goto err;
-		}
-	}
+	ww_mutex_lock_for_each(for (i = 0; i < nr_bos; i++),
+			       &bos[i]->gem.resv->lock, contended, ret, true,
+			       ctx)
+		if (ret)
+			goto error;
 
 	ww_acquire_done(ctx);
-	return 0;
 
-err:
-	for (i--; i >= 0; i--)
-		ww_mutex_unlock(&bos[i]->gem.resv->lock);
-
-	if (slow_locked >= 0)
-		ww_mutex_unlock(&bos[slow_locked]->gem.resv->lock);
+	return 0;
 
-	if (ret == -EDEADLK) {
-		/* we lost out in a seqno race, lock and retry.. */
-		ret = ww_mutex_lock_slow_interruptible(
-			&bos[contended]->gem.resv->lock, ctx);
-		if (!ret) {
-			slow_locked = contended;
-			goto retry;
-		}
-	}
+error:
+	ww_mutex_unlock_for_each(for (i = 0; i < nr_bos; i++),
+				 &bos[i]->gem.resv->lock, contended);
 	ww_acquire_fini(ctx);
 
 	return ret;
-- 
2.17.1

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

[PATCH 6/6] drm/vc4: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Use the provided macros instead of implementing deadlock handling on our own.

Signed-off-by: Christian König <christian.koenig@amd.com>
---
 drivers/gpu/drm/vc4/vc4_gem.c | 56 ++++++++---------------------------
 1 file changed, 13 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c
index d9311be32a4f..628b3a8bcf6a 100644
--- a/drivers/gpu/drm/vc4/vc4_gem.c
+++ b/drivers/gpu/drm/vc4/vc4_gem.c
@@ -584,53 +584,17 @@ vc4_lock_bo_reservations(struct drm_device *dev,
 			 struct vc4_exec_info *exec,
 			 struct ww_acquire_ctx *acquire_ctx)
 {
-	int contended_lock = -1;
-	int i, ret;
+	struct ww_mutex *contended;
 	struct drm_gem_object *bo;
+	int i, ret;
 
 	ww_acquire_init(acquire_ctx, &reservation_ww_class);
 
-retry:
-	if (contended_lock != -1) {
-		bo = &exec->bo[contended_lock]->base;
-		ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock,
-						       acquire_ctx);
-		if (ret) {
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
-
-	for (i = 0; i < exec->bo_count; i++) {
-		if (i == contended_lock)
-			continue;
-
-		bo = &exec->bo[i]->base;
-
-		ret = ww_mutex_lock_interruptible(&bo->resv->lock, acquire_ctx);
-		if (ret) {
-			int j;
-
-			for (j = 0; j < i; j++) {
-				bo = &exec->bo[j]->base;
-				ww_mutex_unlock(&bo->resv->lock);
-			}
-
-			if (contended_lock != -1 && contended_lock >= i) {
-				bo = &exec->bo[contended_lock]->base;
-
-				ww_mutex_unlock(&bo->resv->lock);
-			}
-
-			if (ret == -EDEADLK) {
-				contended_lock = i;
-				goto retry;
-			}
-
-			ww_acquire_done(acquire_ctx);
-			return ret;
-		}
-	}
+	ww_mutex_lock_for_each(for (i = 0; i < exec->bo_count; i++),
+			       &exec->bo[i]->base.resv->lock, contended, ret,
+			       true, acquire_ctx)
+		if (ret)
+			goto error;
 
 	ww_acquire_done(acquire_ctx);
 
@@ -648,6 +612,12 @@ vc4_lock_bo_reservations(struct drm_device *dev,
 	}
 
 	return 0;
+
+error:
+	ww_mutex_unlock_for_each(for (i = 0; i < exec->bo_count; i++),
+				 &exec->bo[i]->base.resv->lock, contended);
+	ww_acquire_done(acquire_ctx);
+	return ret;
 }
 
 /* Queues a struct vc4_exec_info for execution.  If no job is
-- 
2.17.1

Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 02:41:20PM +0200, Christian König wrote:
> The ww_mutex implementation allows for detection deadlocks when multiple
> threads try to acquire the same set of locks in different order.
> 
> The problem is that handling those deadlocks was the burden of the user of
> the ww_mutex implementation and at least some users didn't got that right
> on the first try.
> 
> I'm not a big fan of macros, but it still better then implementing the same
> logic at least halve a dozen times. So this patch adds two macros to lock
> and unlock multiple ww_mutex instances with the necessary deadlock handling.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 3af7c0e03be5..a0d893b5b114 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_unlock_for_each - cleanup after error or contention
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: the last contended ww_mutex or NULL or ERR_PTR
> + *
> + * Helper to make cleanup after error or lock contention easier.
> + * First unlock the last contended lock and then all other locked ones.
> + */
> +#define ww_mutex_unlock_for_each(loop, pos, contended)	\
> +	if (!IS_ERR(contended)) {			\
> +		if (contended)				\
> +			ww_mutex_unlock(contended);	\
> +		contended = (pos);			\
> +		loop {					\
> +			if (contended == (pos))		\
> +				break;			\
> +			ww_mutex_unlock(pos);		\
> +		}					\
> +	}
> +
> +/**
> + * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: ww_mutex pointer variable for state handling
> + * @ret: int variable to store the return value of ww_mutex_lock()
> + * @intr: If true ww_mutex_lock_interruptible() is used
> + * @ctx: ww_acquire_ctx pointer for the locking
> + *
> + * This macro implements the necessary logic to lock multiple ww_mutex
> + * instances. Lock contention with backoff and re-locking is handled by the
> + * macro so that the loop body only need to handle other errors and
> + * successfully acquired locks.
> + *
> + * With the @loop and @pos code fragment it is possible to apply this logic
> + * to all kind of containers (array, list, tree, etc...) holding multiple
> + * ww_mutex instances.
> + *
> + * @contended is used to hold the current state we are in. -ENOENT is used to
> + * signal that we are just starting the handling. -EDEADLK means that the
> + * current position is contended and we need to restart the loop after locking
> + * it. NULL means that there is no contention to be handled. Any other value is
> + * the last contended ww_mutex.
> + */
> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
> +	for (contended = ERR_PTR(-ENOENT); ({				\
> +		__label__ relock, next;					\
> +		ret = -ENOENT;						\
> +		if (contended == ERR_PTR(-ENOENT))			\
> +			contended = NULL;				\
> +		else if (contended == ERR_PTR(-EDEADLK))		\
> +			contended = (pos);				\
> +		else							\
> +			goto next;					\
> +		loop {							\
> +			if (contended == (pos))	{			\
> +				contended = NULL;			\
> +				continue;				\
> +			}						\
> +relock:									\
> +			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
> +				ww_mutex_lock_interruptible(pos, ctx);	\
> +			if (ret == -EDEADLK) {				\
> +				ww_mutex_unlock_for_each(loop, pos,	\
> +							 contended);	\
> +				contended = ERR_PTR(-EDEADLK);		\
> +				goto relock;				\
> +			}						\
> +			break;						\
> +next:									\
> +			continue;					\
> +		}							\
> +	}), ret != -ENOENT;)
> +

Yea gawds, that's eyebleeding bad, even for macros :/

It also breaks with pretty much all other *for_each*() macros in
existence by not actually including the loop itself but farming that out
to an argument statement (@loop), suggesting these really should not be
called for_each.

Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 02:41:20PM +0200, Christian König wrote:
> The ww_mutex implementation allows for detection deadlocks when multiple
> threads try to acquire the same set of locks in different order.
> 
> The problem is that handling those deadlocks was the burden of the user of
> the ww_mutex implementation and at least some users didn't got that right
> on the first try.
> 
> I'm not a big fan of macros, but it still better then implementing the same
> logic at least halve a dozen times. So this patch adds two macros to lock
> and unlock multiple ww_mutex instances with the necessary deadlock handling.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
> index 3af7c0e03be5..a0d893b5b114 100644
> --- a/include/linux/ww_mutex.h
> +++ b/include/linux/ww_mutex.h
> @@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>  	return mutex_is_locked(&lock->base);
>  }
>  
> +/**
> + * ww_mutex_unlock_for_each - cleanup after error or contention
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: the last contended ww_mutex or NULL or ERR_PTR
> + *
> + * Helper to make cleanup after error or lock contention easier.
> + * First unlock the last contended lock and then all other locked ones.
> + */
> +#define ww_mutex_unlock_for_each(loop, pos, contended)	\
> +	if (!IS_ERR(contended)) {			\
> +		if (contended)				\
> +			ww_mutex_unlock(contended);	\
> +		contended = (pos);			\
> +		loop {					\
> +			if (contended == (pos))		\
> +				break;			\
> +			ww_mutex_unlock(pos);		\
> +		}					\
> +	}
> +
> +/**
> + * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
> + * @loop: for loop code fragment iterating over all the locks
> + * @pos: code fragment returning the currently handled lock
> + * @contended: ww_mutex pointer variable for state handling
> + * @ret: int variable to store the return value of ww_mutex_lock()
> + * @intr: If true ww_mutex_lock_interruptible() is used
> + * @ctx: ww_acquire_ctx pointer for the locking
> + *
> + * This macro implements the necessary logic to lock multiple ww_mutex
> + * instances. Lock contention with backoff and re-locking is handled by the
> + * macro so that the loop body only need to handle other errors and
> + * successfully acquired locks.
> + *
> + * With the @loop and @pos code fragment it is possible to apply this logic
> + * to all kind of containers (array, list, tree, etc...) holding multiple
> + * ww_mutex instances.
> + *
> + * @contended is used to hold the current state we are in. -ENOENT is used to
> + * signal that we are just starting the handling. -EDEADLK means that the
> + * current position is contended and we need to restart the loop after locking
> + * it. NULL means that there is no contention to be handled. Any other value is
> + * the last contended ww_mutex.
> + */
> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
> +	for (contended = ERR_PTR(-ENOENT); ({				\
> +		__label__ relock, next;					\
> +		ret = -ENOENT;						\
> +		if (contended == ERR_PTR(-ENOENT))			\
> +			contended = NULL;				\
> +		else if (contended == ERR_PTR(-EDEADLK))		\
> +			contended = (pos);				\
> +		else							\
> +			goto next;					\
> +		loop {							\
> +			if (contended == (pos))	{			\
> +				contended = NULL;			\
> +				continue;				\
> +			}						\
> +relock:									\
> +			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
> +				ww_mutex_lock_interruptible(pos, ctx);	\
> +			if (ret == -EDEADLK) {				\
> +				ww_mutex_unlock_for_each(loop, pos,	\
> +							 contended);	\
> +				contended = ERR_PTR(-EDEADLK);		\
> +				goto relock;				\
> +			}						\
> +			break;						\
> +next:									\
> +			continue;					\
> +		}							\
> +	}), ret != -ENOENT;)
> +

Yea gawds, that's eyebleeding bad, even for macros :/

It also breaks with pretty much all other *for_each*() macros in
existence by not actually including the loop itself but farming that out
to an argument statement (@loop), suggesting these really should not be
called for_each.


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

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> Use the provided macros instead of implementing deadlock handling on our own.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 50de138c89e0..6e4623d3bee2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1307,51 +1307,26 @@ int
>  drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  			  struct ww_acquire_ctx *acquire_ctx)
>  {
> -	int contended = -1;
> +	struct ww_mutex *contended;
>  	int i, ret;
>  
>  	ww_acquire_init(acquire_ctx, &reservation_ww_class);
>  
> -retry:
> -	if (contended != -1) {
> -		struct drm_gem_object *obj = objs[contended];
> -
> -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> -						       acquire_ctx);
> -		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}
> -
> -	for (i = 0; i < count; i++) {
> -		if (i == contended)
> -			continue;
> -
> -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> -						  acquire_ctx);
> -		if (ret) {
> -			int j;
> -
> -			for (j = 0; j < i; j++)
> -				ww_mutex_unlock(&objs[j]->resv->lock);
> -
> -			if (contended != -1 && contended >= i)
> -				ww_mutex_unlock(&objs[contended]->resv->lock);
> -
> -			if (ret == -EDEADLK) {
> -				contended = i;
> -				goto retry;

retry here, starts the whole locking loop.

> -			}
> -
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}

+#define ww_mutex_unlock_for_each(loop, pos, contended) \
+       if (!IS_ERR(contended)) {                       \
+               if (contended)                          \
+                       ww_mutex_unlock(contended);     \
+               contended = (pos);                      \
+               loop {                                  \
+                       if (contended == (pos))         \
+                               break;                  \
+                       ww_mutex_unlock(pos);           \
+               }                                       \
+       }
+

+#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)   \
+       for (contended = ERR_PTR(-ENOENT); ({                           \
+               __label__ relock, next;                                 \
+               ret = -ENOENT;                                          \
+               if (contended == ERR_PTR(-ENOENT))                      \
+                       contended = NULL;                               \
+               else if (contended == ERR_PTR(-EDEADLK))                \
+                       contended = (pos);                              \
+               else                                                    \
+                       goto next;                                      \
+               loop {                                                  \
+                       if (contended == (pos)) {                       \
+                               contended = NULL;                       \
+                               continue;                               \
+                       }                                               \
+relock:                                                                        \
+                       ret = !(intr) ? ww_mutex_lock(pos, ctx) :       \
+                               ww_mutex_lock_interruptible(pos, ctx);  \
+                       if (ret == -EDEADLK) {                          \
+                               ww_mutex_unlock_for_each(loop, pos,     \
+                                                        contended);    \
+                               contended = ERR_PTR(-EDEADLK);          \
+                               goto relock;                            \

while relock here continues where it left of and doesn't restart @loop.
Or am I reading this monstrosity the wrong way?

+                       }                                               \
+                       break;                                          \
+next:                                                                  \
+                       continue;                                       \
+               }                                                       \
+       }), ret != -ENOENT;)
+

> +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> +			       &objs[i]->resv->lock, contended, ret, true,
> +			       acquire_ctx)
> +		if (ret)
> +			goto error;
>  
>  	ww_acquire_done(acquire_ctx);
>  
>  	return 0;
> +
> +error:
> +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> +				 &objs[i]->resv->lock, contended);
> +	ww_acquire_done(acquire_ctx);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_lock_reservations);

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> Use the provided macros instead of implementing deadlock handling on our own.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 50de138c89e0..6e4623d3bee2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1307,51 +1307,26 @@ int
>  drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  			  struct ww_acquire_ctx *acquire_ctx)
>  {
> -	int contended = -1;
> +	struct ww_mutex *contended;
>  	int i, ret;
>  
>  	ww_acquire_init(acquire_ctx, &reservation_ww_class);
>  
> -retry:
> -	if (contended != -1) {
> -		struct drm_gem_object *obj = objs[contended];
> -
> -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> -						       acquire_ctx);
> -		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}
> -
> -	for (i = 0; i < count; i++) {
> -		if (i == contended)
> -			continue;
> -
> -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> -						  acquire_ctx);
> -		if (ret) {
> -			int j;
> -
> -			for (j = 0; j < i; j++)
> -				ww_mutex_unlock(&objs[j]->resv->lock);
> -
> -			if (contended != -1 && contended >= i)
> -				ww_mutex_unlock(&objs[contended]->resv->lock);
> -
> -			if (ret == -EDEADLK) {
> -				contended = i;
> -				goto retry;

retry here, starts the whole locking loop.

> -			}
> -
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}

+#define ww_mutex_unlock_for_each(loop, pos, contended) \
+       if (!IS_ERR(contended)) {                       \
+               if (contended)                          \
+                       ww_mutex_unlock(contended);     \
+               contended = (pos);                      \
+               loop {                                  \
+                       if (contended == (pos))         \
+                               break;                  \
+                       ww_mutex_unlock(pos);           \
+               }                                       \
+       }
+

+#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)   \
+       for (contended = ERR_PTR(-ENOENT); ({                           \
+               __label__ relock, next;                                 \
+               ret = -ENOENT;                                          \
+               if (contended == ERR_PTR(-ENOENT))                      \
+                       contended = NULL;                               \
+               else if (contended == ERR_PTR(-EDEADLK))                \
+                       contended = (pos);                              \
+               else                                                    \
+                       goto next;                                      \
+               loop {                                                  \
+                       if (contended == (pos)) {                       \
+                               contended = NULL;                       \
+                               continue;                               \
+                       }                                               \
+relock:                                                                        \
+                       ret = !(intr) ? ww_mutex_lock(pos, ctx) :       \
+                               ww_mutex_lock_interruptible(pos, ctx);  \
+                       if (ret == -EDEADLK) {                          \
+                               ww_mutex_unlock_for_each(loop, pos,     \
+                                                        contended);    \
+                               contended = ERR_PTR(-EDEADLK);          \
+                               goto relock;                            \

while relock here continues where it left of and doesn't restart @loop.
Or am I reading this monstrosity the wrong way?

+                       }                                               \
+                       break;                                          \
+next:                                                                  \
+                       continue;                                       \
+               }                                                       \
+       }), ret != -ENOENT;)
+

> +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> +			       &objs[i]->resv->lock, contended, ret, true,
> +			       acquire_ctx)
> +		if (ret)
> +			goto error;
>  
>  	ww_acquire_done(acquire_ctx);
>  
>  	return 0;
> +
> +error:
> +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> +				 &objs[i]->resv->lock, contended);
> +	ww_acquire_done(acquire_ctx);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_lock_reservations);

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

Re: [PATCH 1/6] locking: add ww_mutex_(un)lock_for_each helpers

[Christian König]

Am 14.06.19 um 14:56 schrieb Peter Zijlstra:
> On Fri, Jun 14, 2019 at 02:41:20PM +0200, Christian König wrote:
>> The ww_mutex implementation allows for detection deadlocks when multiple
>> threads try to acquire the same set of locks in different order.
>>
>> The problem is that handling those deadlocks was the burden of the user of
>> the ww_mutex implementation and at least some users didn't got that right
>> on the first try.
>>
>> I'm not a big fan of macros, but it still better then implementing the same
>> logic at least halve a dozen times. So this patch adds two macros to lock
>> and unlock multiple ww_mutex instances with the necessary deadlock handling.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   include/linux/ww_mutex.h | 75 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/include/linux/ww_mutex.h b/include/linux/ww_mutex.h
>> index 3af7c0e03be5..a0d893b5b114 100644
>> --- a/include/linux/ww_mutex.h
>> +++ b/include/linux/ww_mutex.h
>> @@ -369,4 +369,79 @@ static inline bool ww_mutex_is_locked(struct ww_mutex *lock)
>>   	return mutex_is_locked(&lock->base);
>>   }
>>   
>> +/**
>> + * ww_mutex_unlock_for_each - cleanup after error or contention
>> + * @loop: for loop code fragment iterating over all the locks
>> + * @pos: code fragment returning the currently handled lock
>> + * @contended: the last contended ww_mutex or NULL or ERR_PTR
>> + *
>> + * Helper to make cleanup after error or lock contention easier.
>> + * First unlock the last contended lock and then all other locked ones.
>> + */
>> +#define ww_mutex_unlock_for_each(loop, pos, contended)	\
>> +	if (!IS_ERR(contended)) {			\
>> +		if (contended)				\
>> +			ww_mutex_unlock(contended);	\
>> +		contended = (pos);			\
>> +		loop {					\
>> +			if (contended == (pos))		\
>> +				break;			\
>> +			ww_mutex_unlock(pos);		\
>> +		}					\
>> +	}
>> +
>> +/**
>> + * ww_mutex_lock_for_each - implement ww_mutex deadlock handling
>> + * @loop: for loop code fragment iterating over all the locks
>> + * @pos: code fragment returning the currently handled lock
>> + * @contended: ww_mutex pointer variable for state handling
>> + * @ret: int variable to store the return value of ww_mutex_lock()
>> + * @intr: If true ww_mutex_lock_interruptible() is used
>> + * @ctx: ww_acquire_ctx pointer for the locking
>> + *
>> + * This macro implements the necessary logic to lock multiple ww_mutex
>> + * instances. Lock contention with backoff and re-locking is handled by the
>> + * macro so that the loop body only need to handle other errors and
>> + * successfully acquired locks.
>> + *
>> + * With the @loop and @pos code fragment it is possible to apply this logic
>> + * to all kind of containers (array, list, tree, etc...) holding multiple
>> + * ww_mutex instances.
>> + *
>> + * @contended is used to hold the current state we are in. -ENOENT is used to
>> + * signal that we are just starting the handling. -EDEADLK means that the
>> + * current position is contended and we need to restart the loop after locking
>> + * it. NULL means that there is no contention to be handled. Any other value is
>> + * the last contended ww_mutex.
>> + */
>> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)	\
>> +	for (contended = ERR_PTR(-ENOENT); ({				\
>> +		__label__ relock, next;					\
>> +		ret = -ENOENT;						\
>> +		if (contended == ERR_PTR(-ENOENT))			\
>> +			contended = NULL;				\
>> +		else if (contended == ERR_PTR(-EDEADLK))		\
>> +			contended = (pos);				\
>> +		else							\
>> +			goto next;					\
>> +		loop {							\
>> +			if (contended == (pos))	{			\
>> +				contended = NULL;			\
>> +				continue;				\
>> +			}						\
>> +relock:									\
>> +			ret = !(intr) ? ww_mutex_lock(pos, ctx) :	\
>> +				ww_mutex_lock_interruptible(pos, ctx);	\
>> +			if (ret == -EDEADLK) {				\
>> +				ww_mutex_unlock_for_each(loop, pos,	\
>> +							 contended);	\
>> +				contended = ERR_PTR(-EDEADLK);		\
>> +				goto relock;				\
>> +			}						\
>> +			break;						\
>> +next:									\
>> +			continue;					\
>> +		}							\
>> +	}), ret != -ENOENT;)
>> +
> Yea gawds, that's eyebleeding bad, even for macros :/

Yeah, I actually don't like it that much either.

But we have at least 8 implementations of this and I'm going to add 
number 9 rather soon.

> It also breaks with pretty much all other *for_each*() macros in
> existence by not actually including the loop itself but farming that out
> to an argument statement (@loop), suggesting these really should not be
> called for_each.

How about ww_mutex_lock_multiple? If not feel free to suggest anything.

Also I'm open to suggestions how to approach that differently, but my 
other tries (callback functions mostly) just somehow suck badly as well.

Thanks,
Christian.

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Am 14.06.19 um 14:59 schrieb Peter Zijlstra:
> On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
>> Use the provided macros instead of implementing deadlock handling on our own.
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
>> ---
>>   drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
>>   1 file changed, 12 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>> index 50de138c89e0..6e4623d3bee2 100644
>> --- a/drivers/gpu/drm/drm_gem.c
>> +++ b/drivers/gpu/drm/drm_gem.c
>> @@ -1307,51 +1307,26 @@ int
>>   drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>   			  struct ww_acquire_ctx *acquire_ctx)
>>   {
>> -	int contended = -1;
>> +	struct ww_mutex *contended;
>>   	int i, ret;
>>   
>>   	ww_acquire_init(acquire_ctx, &reservation_ww_class);
>>   
>> -retry:
>> -	if (contended != -1) {
>> -		struct drm_gem_object *obj = objs[contended];
>> -
>> -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
>> -						       acquire_ctx);
>> -		if (ret) {
>> -			ww_acquire_done(acquire_ctx);
>> -			return ret;
>> -		}
>> -	}
>> -
>> -	for (i = 0; i < count; i++) {
>> -		if (i == contended)
>> -			continue;
>> -
>> -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
>> -						  acquire_ctx);
>> -		if (ret) {
>> -			int j;
>> -
>> -			for (j = 0; j < i; j++)
>> -				ww_mutex_unlock(&objs[j]->resv->lock);
>> -
>> -			if (contended != -1 && contended >= i)
>> -				ww_mutex_unlock(&objs[contended]->resv->lock);
>> -
>> -			if (ret == -EDEADLK) {
>> -				contended = i;
>> -				goto retry;
> retry here, starts the whole locking loop.
>
>> -			}
>> -
>> -			ww_acquire_done(acquire_ctx);
>> -			return ret;
>> -		}
>> -	}
> +#define ww_mutex_unlock_for_each(loop, pos, contended) \
> +       if (!IS_ERR(contended)) {                       \
> +               if (contended)                          \
> +                       ww_mutex_unlock(contended);     \
> +               contended = (pos);                      \
> +               loop {                                  \
> +                       if (contended == (pos))         \
> +                               break;                  \
> +                       ww_mutex_unlock(pos);           \
> +               }                                       \
> +       }
> +
>
> +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)   \
> +       for (contended = ERR_PTR(-ENOENT); ({                           \
> +               __label__ relock, next;                                 \
> +               ret = -ENOENT;                                          \
> +               if (contended == ERR_PTR(-ENOENT))                      \
> +                       contended = NULL;                               \
> +               else if (contended == ERR_PTR(-EDEADLK))                \
> +                       contended = (pos);                              \
> +               else                                                    \
> +                       goto next;                                      \
> +               loop {                                                  \
> +                       if (contended == (pos)) {                       \
> +                               contended = NULL;                       \
> +                               continue;                               \
> +                       }                                               \
> +relock:                                                                        \
> +                       ret = !(intr) ? ww_mutex_lock(pos, ctx) :       \
> +                               ww_mutex_lock_interruptible(pos, ctx);  \
> +                       if (ret == -EDEADLK) {                          \
> +                               ww_mutex_unlock_for_each(loop, pos,     \
> +                                                        contended);    \
> +                               contended = ERR_PTR(-EDEADLK);          \
> +                               goto relock;                            \
>
> while relock here continues where it left of and doesn't restart @loop.
> Or am I reading this monstrosity the wrong way?

contended = ERR_PTR(-EDEADLK) makes sure that the whole loop is 
restarted after retrying to acquire the lock.

See the "if" above "loop".

Christian.

>
> +                       }                                               \
> +                       break;                                          \
> +next:                                                                  \
> +                       continue;                                       \
> +               }                                                       \
> +       }), ret != -ENOENT;)
> +
>
>> +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
>> +			       &objs[i]->resv->lock, contended, ret, true,
>> +			       acquire_ctx)
>> +		if (ret)
>> +			goto error;
>>   
>>   	ww_acquire_done(acquire_ctx);
>>   
>>   	return 0;
>> +
>> +error:
>> +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
>> +				 &objs[i]->resv->lock, contended);
>> +	ww_acquire_done(acquire_ctx);
>> +	return ret;
>>   }
>>   EXPORT_SYMBOL(drm_gem_lock_reservations);

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> Use the provided macros instead of implementing deadlock handling on our own.
> 
> Signed-off-by: Christian König <christian.koenig@amd.com>
> ---
>  drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
>  1 file changed, 12 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 50de138c89e0..6e4623d3bee2 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1307,51 +1307,26 @@ int
>  drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>  			  struct ww_acquire_ctx *acquire_ctx)
>  {
> -	int contended = -1;
> +	struct ww_mutex *contended;
>  	int i, ret;
>  
>  	ww_acquire_init(acquire_ctx, &reservation_ww_class);
>  
> -retry:
> -	if (contended != -1) {
> -		struct drm_gem_object *obj = objs[contended];
> -
> -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> -						       acquire_ctx);
> -		if (ret) {
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}
> -
> -	for (i = 0; i < count; i++) {
> -		if (i == contended)
> -			continue;
> -
> -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> -						  acquire_ctx);
> -		if (ret) {
> -			int j;
> -
> -			for (j = 0; j < i; j++)
> -				ww_mutex_unlock(&objs[j]->resv->lock);
> -
> -			if (contended != -1 && contended >= i)
> -				ww_mutex_unlock(&objs[contended]->resv->lock);
> -
> -			if (ret == -EDEADLK) {
> -				contended = i;
> -				goto retry;
> -			}
> -
> -			ww_acquire_done(acquire_ctx);
> -			return ret;
> -		}
> -	}

I note all the sites you use this on are simple idx iterators; so how
about something like so:

int ww_mutex_unlock_all(int count, void *data, struct ww_mutex *(*func)(int, void *))
{
	int i;

	for (i = 0; i < count; i++) {
		lock = func(i, data);
		ww_mutex_unlock(lock);
	}
}

int ww_mutex_lock_all(int count, struct ww_acquire_context *acquire_ctx, bool intr,
		      void *data, struct ww_mutex *(*func)(int, void *))
{
	int i, ret, contended = -1;
	struct ww_mutex *lock;

retry:
	if (contended != -1) {
		lock = func(contended, data);
		if (intr)
			ret = ww_mutex_lock_slow_interruptible(lock, acquire_ctx);
		else
			ret = ww_mutex_lock_slow(lock, acquire_ctx), 0;

		if (ret) {
			ww_acquire_done(acquire_ctx);
			return ret;
		}
	}

	for (i = 0; i < count; i++) {
		if (i == contended)
			continue;

		lock = func(i, data);
		if (intr)
			ret = ww_mutex_lock_interruptible(lock, acquire_ctx);
		else
			ret = ww_mutex_lock(lock, acquire_ctx), 0;

		if (ret) {
			ww_mutex_unlock_all(i, data, func);
			if (contended > i) {
				lock = func(contended, data);
				ww_mutex_unlock(lock);
			}

			if (ret == -EDEADLK) {
				contended = i;
				goto retry;
			}

			ww_acquire_done(acquire_ctx);
			return ret;
		}
	}

	ww_acquire_done(acquire_ctx);
	return 0;
}

> +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> +			       &objs[i]->resv->lock, contended, ret, true,
> +			       acquire_ctx)
> +		if (ret)
> +			goto error;

which then becomes:

struct ww_mutex *gem_ww_mutex_func(int i, void *data)
{
	struct drm_gem_object **objs = data;
	return &objs[i]->resv->lock;
}

	ret = ww_mutex_lock_all(count, acquire_ctx, true, objs, gem_ww_mutex_func);

>  	ww_acquire_done(acquire_ctx);
>  
>  	return 0;
> +
> +error:
> +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> +				 &objs[i]->resv->lock, contended);
> +	ww_acquire_done(acquire_ctx);
> +	return ret;
>  }
>  EXPORT_SYMBOL(drm_gem_lock_reservations);
>  
> -- 
> 2.17.1
> 

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Peter Zijlstra]

On Fri, Jun 14, 2019 at 03:06:10PM +0200, Christian König wrote:
> Am 14.06.19 um 14:59 schrieb Peter Zijlstra:
> > +#define ww_mutex_lock_for_each(loop, pos, contended, ret, intr, ctx)   \
> > +       for (contended = ERR_PTR(-ENOENT); ({                           \
> > +               __label__ relock, next;                                 \
> > +               ret = -ENOENT;                                          \
> > +               if (contended == ERR_PTR(-ENOENT))                      \
> > +                       contended = NULL;                               \
> > +               else if (contended == ERR_PTR(-EDEADLK))                \
> > +                       contended = (pos);                              \
> > +               else                                                    \
> > +                       goto next;                                      \
> > +               loop {                                                  \
> > +                       if (contended == (pos)) {                       \
> > +                               contended = NULL;                       \
> > +                               continue;                               \
> > +                       }                                               \
> > +relock:                                                                        \
> > +                       ret = !(intr) ? ww_mutex_lock(pos, ctx) :       \
> > +                               ww_mutex_lock_interruptible(pos, ctx);  \
> > +                       if (ret == -EDEADLK) {                          \
> > +                               ww_mutex_unlock_for_each(loop, pos,     \
> > +                                                        contended);    \
> > +                               contended = ERR_PTR(-EDEADLK);          \
> > +                               goto relock;                            \
> > 
> > while relock here continues where it left of and doesn't restart @loop.
> > Or am I reading this monstrosity the wrong way?
> 
> contended = ERR_PTR(-EDEADLK) makes sure that the whole loop is restarted
> after retrying to acquire the lock.
> 
> See the "if" above "loop".

ARGH, the loop inside the loop... Yeah, maybe, brain hurts.

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 03:19:16PM +0200, Peter Zijlstra wrote:
> On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> > Use the provided macros instead of implementing deadlock handling on our own.
> > 
> > Signed-off-by: Christian König <christian.koenig@amd.com>
> > ---
> >  drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
> >  1 file changed, 12 insertions(+), 37 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> > index 50de138c89e0..6e4623d3bee2 100644
> > --- a/drivers/gpu/drm/drm_gem.c
> > +++ b/drivers/gpu/drm/drm_gem.c
> > @@ -1307,51 +1307,26 @@ int
> >  drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >  			  struct ww_acquire_ctx *acquire_ctx)
> >  {
> > -	int contended = -1;
> > +	struct ww_mutex *contended;
> >  	int i, ret;
> >  
> >  	ww_acquire_init(acquire_ctx, &reservation_ww_class);
> >  
> > -retry:
> > -	if (contended != -1) {
> > -		struct drm_gem_object *obj = objs[contended];
> > -
> > -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> > -						       acquire_ctx);
> > -		if (ret) {
> > -			ww_acquire_done(acquire_ctx);
> > -			return ret;
> > -		}
> > -	}
> > -
> > -	for (i = 0; i < count; i++) {
> > -		if (i == contended)
> > -			continue;
> > -
> > -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> > -						  acquire_ctx);
> > -		if (ret) {
> > -			int j;
> > -
> > -			for (j = 0; j < i; j++)
> > -				ww_mutex_unlock(&objs[j]->resv->lock);
> > -
> > -			if (contended != -1 && contended >= i)
> > -				ww_mutex_unlock(&objs[contended]->resv->lock);
> > -
> > -			if (ret == -EDEADLK) {
> > -				contended = i;
> > -				goto retry;
> > -			}
> > -
> > -			ww_acquire_done(acquire_ctx);
> > -			return ret;
> > -		}
> > -	}
> 
> I note all the sites you use this on are simple idx iterators; so how
> about something like so:
> 
> int ww_mutex_unlock_all(int count, void *data, struct ww_mutex *(*func)(int, void *))
> {
> 	int i;
> 
> 	for (i = 0; i < count; i++) {
> 		lock = func(i, data);
> 		ww_mutex_unlock(lock);
> 	}
> }
> 
> int ww_mutex_lock_all(int count, struct ww_acquire_context *acquire_ctx, bool intr,
> 		      void *data, struct ww_mutex *(*func)(int, void *))
> {
> 	int i, ret, contended = -1;
> 	struct ww_mutex *lock;
> 
> retry:
> 	if (contended != -1) {
> 		lock = func(contended, data);
> 		if (intr)
> 			ret = ww_mutex_lock_slow_interruptible(lock, acquire_ctx);
> 		else
> 			ret = ww_mutex_lock_slow(lock, acquire_ctx), 0;
> 
> 		if (ret) {
> 			ww_acquire_done(acquire_ctx);
> 			return ret;
> 		}
> 	}
> 
> 	for (i = 0; i < count; i++) {
> 		if (i == contended)
> 			continue;
> 
> 		lock = func(i, data);
> 		if (intr)
> 			ret = ww_mutex_lock_interruptible(lock, acquire_ctx);
> 		else
> 			ret = ww_mutex_lock(lock, acquire_ctx), 0;
> 
> 		if (ret) {
> 			ww_mutex_unlock_all(i, data, func);
> 			if (contended > i) {
> 				lock = func(contended, data);
> 				ww_mutex_unlock(lock);
> 			}
> 
> 			if (ret == -EDEADLK) {
> 				contended = i;
> 				goto retry;
> 			}
> 
> 			ww_acquire_done(acquire_ctx);
> 			return ret;
> 		}
> 	}
> 
> 	ww_acquire_done(acquire_ctx);
> 	return 0;
> }
> 
> > +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> > +			       &objs[i]->resv->lock, contended, ret, true,
> > +			       acquire_ctx)
> > +		if (ret)
> > +			goto error;
> 
> which then becomes:
> 
> struct ww_mutex *gem_ww_mutex_func(int i, void *data)
> {
> 	struct drm_gem_object **objs = data;
> 	return &objs[i]->resv->lock;
> }
> 
> 	ret = ww_mutex_lock_all(count, acquire_ctx, true, objs, gem_ww_mutex_func);
> 
> >  	ww_acquire_done(acquire_ctx);
> >  
> >  	return 0;
> > +
> > +error:
> > +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> > +				 &objs[i]->resv->lock, contended);
> > +	ww_acquire_done(acquire_ctx);
> > +	return ret;
> >  }
> >  EXPORT_SYMBOL(drm_gem_lock_reservations);

Another idea, entirely untested (I guess making sure that we can use the
same iterator for both locking and unlocking in the contended case will be
fun), but maybe something like this:

	WW_MUTEX_LOCK_BEGIN();
	driver_for_each_loop (iter, pos) {
		WW_MUTEX_LOCK(&pos->ww_mutex);
	}
	WW_MUTEX_LOCK_END();

That way we can reuse any and all iterators that'll ever show up at least.
It's still horrible because the macros need to jump around between all of
them. Would also make this useful for more cases, where maybe you need to
trylock some lru lock to get at your next ww_mutex, or do some
kref_get_unless_zero. Buffer eviction loops tend to acquire these, and
that would all get ugly real fast if we'd need to stuff it into some
iterator argument.

This is kinda what we went with for modeset locks with
DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
pair at least. But it's a lot more limited use-cases, maybe too fragile an
idea for ww_mutex in full generality.

Not going to type this out because too much w/e mode here already, but I
can give it a stab next week.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Am 14.06.19 um 17:22 schrieb Daniel Vetter:
> On Fri, Jun 14, 2019 at 03:19:16PM +0200, Peter Zijlstra wrote:
>> On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
>>> Use the provided macros instead of implementing deadlock handling on our own.
>>>
>>> Signed-off-by: Christian König <christian.koenig@amd.com>
>>> ---
>>>   drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
>>>   1 file changed, 12 insertions(+), 37 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
>>> index 50de138c89e0..6e4623d3bee2 100644
>>> --- a/drivers/gpu/drm/drm_gem.c
>>> +++ b/drivers/gpu/drm/drm_gem.c
>>> @@ -1307,51 +1307,26 @@ int
>>>   drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
>>>   			  struct ww_acquire_ctx *acquire_ctx)
>>>   {
>>> -	int contended = -1;
>>> +	struct ww_mutex *contended;
>>>   	int i, ret;
>>>   
>>>   	ww_acquire_init(acquire_ctx, &reservation_ww_class);
>>>   
>>> -retry:
>>> -	if (contended != -1) {
>>> -		struct drm_gem_object *obj = objs[contended];
>>> -
>>> -		ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
>>> -						       acquire_ctx);
>>> -		if (ret) {
>>> -			ww_acquire_done(acquire_ctx);
>>> -			return ret;
>>> -		}
>>> -	}
>>> -
>>> -	for (i = 0; i < count; i++) {
>>> -		if (i == contended)
>>> -			continue;
>>> -
>>> -		ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
>>> -						  acquire_ctx);
>>> -		if (ret) {
>>> -			int j;
>>> -
>>> -			for (j = 0; j < i; j++)
>>> -				ww_mutex_unlock(&objs[j]->resv->lock);
>>> -
>>> -			if (contended != -1 && contended >= i)
>>> -				ww_mutex_unlock(&objs[contended]->resv->lock);
>>> -
>>> -			if (ret == -EDEADLK) {
>>> -				contended = i;
>>> -				goto retry;
>>> -			}
>>> -
>>> -			ww_acquire_done(acquire_ctx);
>>> -			return ret;
>>> -		}
>>> -	}
>> I note all the sites you use this on are simple idx iterators; so how
>> about something like so:
>>
>> int ww_mutex_unlock_all(int count, void *data, struct ww_mutex *(*func)(int, void *))
>> {
>> 	int i;
>>
>> 	for (i = 0; i < count; i++) {
>> 		lock = func(i, data);
>> 		ww_mutex_unlock(lock);
>> 	}
>> }
>>
>> int ww_mutex_lock_all(int count, struct ww_acquire_context *acquire_ctx, bool intr,
>> 		      void *data, struct ww_mutex *(*func)(int, void *))
>> {
>> 	int i, ret, contended = -1;
>> 	struct ww_mutex *lock;
>>
>> retry:
>> 	if (contended != -1) {
>> 		lock = func(contended, data);
>> 		if (intr)
>> 			ret = ww_mutex_lock_slow_interruptible(lock, acquire_ctx);
>> 		else
>> 			ret = ww_mutex_lock_slow(lock, acquire_ctx), 0;
>>
>> 		if (ret) {
>> 			ww_acquire_done(acquire_ctx);
>> 			return ret;
>> 		}
>> 	}
>>
>> 	for (i = 0; i < count; i++) {
>> 		if (i == contended)
>> 			continue;
>>
>> 		lock = func(i, data);
>> 		if (intr)
>> 			ret = ww_mutex_lock_interruptible(lock, acquire_ctx);
>> 		else
>> 			ret = ww_mutex_lock(lock, acquire_ctx), 0;
>>
>> 		if (ret) {
>> 			ww_mutex_unlock_all(i, data, func);
>> 			if (contended > i) {
>> 				lock = func(contended, data);
>> 				ww_mutex_unlock(lock);
>> 			}
>>
>> 			if (ret == -EDEADLK) {
>> 				contended = i;
>> 				goto retry;
>> 			}
>>
>> 			ww_acquire_done(acquire_ctx);
>> 			return ret;
>> 		}
>> 	}
>>
>> 	ww_acquire_done(acquire_ctx);
>> 	return 0;
>> }
>>
>>> +	ww_mutex_lock_for_each(for (i = 0; i < count; i++),
>>> +			       &objs[i]->resv->lock, contended, ret, true,
>>> +			       acquire_ctx)
>>> +		if (ret)
>>> +			goto error;
>> which then becomes:
>>
>> struct ww_mutex *gem_ww_mutex_func(int i, void *data)
>> {
>> 	struct drm_gem_object **objs = data;
>> 	return &objs[i]->resv->lock;
>> }
>>
>> 	ret = ww_mutex_lock_all(count, acquire_ctx, true, objs, gem_ww_mutex_func);
>>
>>>   	ww_acquire_done(acquire_ctx);
>>>   
>>>   	return 0;
>>> +
>>> +error:
>>> +	ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
>>> +				 &objs[i]->resv->lock, contended);
>>> +	ww_acquire_done(acquire_ctx);
>>> +	return ret;
>>>   }
>>>   EXPORT_SYMBOL(drm_gem_lock_reservations);
> Another idea, entirely untested (I guess making sure that we can use the
> same iterator for both locking and unlocking in the contended case will be
> fun), but maybe something like this:
>
> 	WW_MUTEX_LOCK_BEGIN();
> 	driver_for_each_loop (iter, pos) {
> 		WW_MUTEX_LOCK(&pos->ww_mutex);
> 	}
> 	WW_MUTEX_LOCK_END();
>
> That way we can reuse any and all iterators that'll ever show up at least.
> It's still horrible because the macros need to jump around between all of
> them.

Yeah, I tried this as well and that's exactly the reason why I discarded 
this approach.

There is this hack with goto *void we could use, but I'm pretty sure 
that is actually not part of any C standard.

> Would also make this useful for more cases, where maybe you need to
> trylock some lru lock to get at your next ww_mutex, or do some
> kref_get_unless_zero. Buffer eviction loops tend to acquire these, and
> that would all get ugly real fast if we'd need to stuff it into some
> iterator argument.

Well I don't see a use case with eviction in general. The dance there 
requires something different as far as I can see.

Christian.

> This is kinda what we went with for modeset locks with
> DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> pair at least. But it's a lot more limited use-cases, maybe too fragile an
> idea for ww_mutex in full generality.
>
> Not going to type this out because too much w/e mode here already, but I
> can give it a stab next week.
> -Daniel

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.06.19 um 17:22 schrieb Daniel Vetter:
> > On Fri, Jun 14, 2019 at 03:19:16PM +0200, Peter Zijlstra wrote:
> >> On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> >>> Use the provided macros instead of implementing deadlock handling on our own.
> >>>
> >>> Signed-off-by: Christian König <christian.koenig@amd.com>
> >>> ---
> >>>   drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
> >>>   1 file changed, 12 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index 50de138c89e0..6e4623d3bee2 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1307,51 +1307,26 @@ int
> >>>   drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >>>                       struct ww_acquire_ctx *acquire_ctx)
> >>>   {
> >>> -   int contended = -1;
> >>> +   struct ww_mutex *contended;
> >>>     int i, ret;
> >>>  
> >>>     ww_acquire_init(acquire_ctx, &reservation_ww_class);
> >>>  
> >>> -retry:
> >>> -   if (contended != -1) {
> >>> -           struct drm_gem_object *obj = objs[contended];
> >>> -
> >>> -           ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> >>> -                                                  acquire_ctx);
> >>> -           if (ret) {
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> -                   return ret;
> >>> -           }
> >>> -   }
> >>> -
> >>> -   for (i = 0; i < count; i++) {
> >>> -           if (i == contended)
> >>> -                   continue;
> >>> -
> >>> -           ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> >>> -                                             acquire_ctx);
> >>> -           if (ret) {
> >>> -                   int j;
> >>> -
> >>> -                   for (j = 0; j < i; j++)
> >>> -                           ww_mutex_unlock(&objs[j]->resv->lock);
> >>> -
> >>> -                   if (contended != -1 && contended >= i)
> >>> -                           ww_mutex_unlock(&objs[contended]->resv->lock);
> >>> -
> >>> -                   if (ret == -EDEADLK) {
> >>> -                           contended = i;
> >>> -                           goto retry;
> >>> -                   }
> >>> -
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> -                   return ret;
> >>> -           }
> >>> -   }
> >> I note all the sites you use this on are simple idx iterators; so how
> >> about something like so:
> >>
> >> int ww_mutex_unlock_all(int count, void *data, struct ww_mutex *(*func)(int, void *))
> >> {
> >>      int i;
> >>
> >>      for (i = 0; i < count; i++) {
> >>              lock = func(i, data);
> >>              ww_mutex_unlock(lock);
> >>      }
> >> }
> >>
> >> int ww_mutex_lock_all(int count, struct ww_acquire_context *acquire_ctx, bool intr,
> >>                    void *data, struct ww_mutex *(*func)(int, void *))
> >> {
> >>      int i, ret, contended = -1;
> >>      struct ww_mutex *lock;
> >>
> >> retry:
> >>      if (contended != -1) {
> >>              lock = func(contended, data);
> >>              if (intr)
> >>                      ret = ww_mutex_lock_slow_interruptible(lock, acquire_ctx);
> >>              else
> >>                      ret = ww_mutex_lock_slow(lock, acquire_ctx), 0;
> >>
> >>              if (ret) {
> >>                      ww_acquire_done(acquire_ctx);
> >>                      return ret;
> >>              }
> >>      }
> >>
> >>      for (i = 0; i < count; i++) {
> >>              if (i == contended)
> >>                      continue;
> >>
> >>              lock = func(i, data);
> >>              if (intr)
> >>                      ret = ww_mutex_lock_interruptible(lock, acquire_ctx);
> >>              else
> >>                      ret = ww_mutex_lock(lock, acquire_ctx), 0;
> >>
> >>              if (ret) {
> >>                      ww_mutex_unlock_all(i, data, func);
> >>                      if (contended > i) {
> >>                              lock = func(contended, data);
> >>                              ww_mutex_unlock(lock);
> >>                      }
> >>
> >>                      if (ret == -EDEADLK) {
> >>                              contended = i;
> >>                              goto retry;
> >>                      }
> >>
> >>                      ww_acquire_done(acquire_ctx);
> >>                      return ret;
> >>              }
> >>      }
> >>
> >>      ww_acquire_done(acquire_ctx);
> >>      return 0;
> >> }
> >>
> >>> +   ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> >>> +                          &objs[i]->resv->lock, contended, ret, true,
> >>> +                          acquire_ctx)
> >>> +           if (ret)
> >>> +                   goto error;
> >> which then becomes:
> >>
> >> struct ww_mutex *gem_ww_mutex_func(int i, void *data)
> >> {
> >>      struct drm_gem_object **objs = data;
> >>      return &objs[i]->resv->lock;
> >> }
> >>
> >>      ret = ww_mutex_lock_all(count, acquire_ctx, true, objs, gem_ww_mutex_func);
> >>
> >>>     ww_acquire_done(acquire_ctx);
> >>>  
> >>>     return 0;
> >>> +
> >>> +error:
> >>> +   ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> >>> +                            &objs[i]->resv->lock, contended);
> >>> +   ww_acquire_done(acquire_ctx);
> >>> +   return ret;
> >>>   }
> >>>   EXPORT_SYMBOL(drm_gem_lock_reservations);
> > Another idea, entirely untested (I guess making sure that we can use the
> > same iterator for both locking and unlocking in the contended case will be
> > fun), but maybe something like this:
> >
> >       WW_MUTEX_LOCK_BEGIN();
> >       driver_for_each_loop (iter, pos) {
> >               WW_MUTEX_LOCK(&pos->ww_mutex);
> >       }
> >       WW_MUTEX_LOCK_END();
> >
> > That way we can reuse any and all iterators that'll ever show up at least.
> > It's still horrible because the macros need to jump around between all of
> > them.
>
> Yeah, I tried this as well and that's exactly the reason why I discarded
> this approach.
>
> There is this hack with goto *void we could use, but I'm pretty sure
> that is actually not part of any C standard.

Nah, just invisible jump labels + the all uppercase macro names to scream
that into your face. You essentially trade one set of horrors for another,
and this one allows you to open-code any kind of loop or other algorithim
to find all the ww_mutexes you need.

> > Would also make this useful for more cases, where maybe you need to
> > trylock some lru lock to get at your next ww_mutex, or do some
> > kref_get_unless_zero. Buffer eviction loops tend to acquire these, and
> > that would all get ugly real fast if we'd need to stuff it into some
> > iterator argument.
>
> Well I don't see a use case with eviction in general. The dance there
> requires something different as far as I can see.

Current ttm doesn't really bother with multi-threaded contention for all
of memory. You're fixing that, but I think in the end we need a
slow-reclaim which eats up the entire lru using the full ww_mutex dance.
Rougly

WW_MUTEX_LOCK_BEGIN()

lock(lru_lock);

while (bo = list_first(lru)) {
	if (kref_get_unless_zero(bo)) {
		unlock(lru_lock);
		WW_MUTEX_LOCK(bo->ww_mutex);
		lock(lru_lock);
	} else {
		/* bo is getting freed, steal it from the freeing process
		 * or just ignore */
	}
}
unlock(lru_lock)

WW_MUTEX_LOCK_END;


Also I think if we allow this we could perhaps use this to implement the
modeset macros too.
-Daniel




> > This is kinda what we went with for modeset locks with
> > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > idea for ww_mutex in full generality.
> >
> > Not going to type this out because too much w/e mode here already, but I
> > can give it a stab next week.
> > -Daniel
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>
> Am 14.06.19 um 17:22 schrieb Daniel Vetter:
> > On Fri, Jun 14, 2019 at 03:19:16PM +0200, Peter Zijlstra wrote:
> >> On Fri, Jun 14, 2019 at 02:41:22PM +0200, Christian König wrote:
> >>> Use the provided macros instead of implementing deadlock handling on our own.
> >>>
> >>> Signed-off-by: Christian König <christian.koenig@amd.com>
> >>> ---
> >>>   drivers/gpu/drm/drm_gem.c | 49 ++++++++++-----------------------------
> >>>   1 file changed, 12 insertions(+), 37 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> >>> index 50de138c89e0..6e4623d3bee2 100644
> >>> --- a/drivers/gpu/drm/drm_gem.c
> >>> +++ b/drivers/gpu/drm/drm_gem.c
> >>> @@ -1307,51 +1307,26 @@ int
> >>>   drm_gem_lock_reservations(struct drm_gem_object **objs, int count,
> >>>                       struct ww_acquire_ctx *acquire_ctx)
> >>>   {
> >>> -   int contended = -1;
> >>> +   struct ww_mutex *contended;
> >>>     int i, ret;
> >>>  
> >>>     ww_acquire_init(acquire_ctx, &reservation_ww_class);
> >>>  
> >>> -retry:
> >>> -   if (contended != -1) {
> >>> -           struct drm_gem_object *obj = objs[contended];
> >>> -
> >>> -           ret = ww_mutex_lock_slow_interruptible(&obj->resv->lock,
> >>> -                                                  acquire_ctx);
> >>> -           if (ret) {
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> -                   return ret;
> >>> -           }
> >>> -   }
> >>> -
> >>> -   for (i = 0; i < count; i++) {
> >>> -           if (i == contended)
> >>> -                   continue;
> >>> -
> >>> -           ret = ww_mutex_lock_interruptible(&objs[i]->resv->lock,
> >>> -                                             acquire_ctx);
> >>> -           if (ret) {
> >>> -                   int j;
> >>> -
> >>> -                   for (j = 0; j < i; j++)
> >>> -                           ww_mutex_unlock(&objs[j]->resv->lock);
> >>> -
> >>> -                   if (contended != -1 && contended >= i)
> >>> -                           ww_mutex_unlock(&objs[contended]->resv->lock);
> >>> -
> >>> -                   if (ret == -EDEADLK) {
> >>> -                           contended = i;
> >>> -                           goto retry;
> >>> -                   }
> >>> -
> >>> -                   ww_acquire_done(acquire_ctx);
> >>> -                   return ret;
> >>> -           }
> >>> -   }
> >> I note all the sites you use this on are simple idx iterators; so how
> >> about something like so:
> >>
> >> int ww_mutex_unlock_all(int count, void *data, struct ww_mutex *(*func)(int, void *))
> >> {
> >>      int i;
> >>
> >>      for (i = 0; i < count; i++) {
> >>              lock = func(i, data);
> >>              ww_mutex_unlock(lock);
> >>      }
> >> }
> >>
> >> int ww_mutex_lock_all(int count, struct ww_acquire_context *acquire_ctx, bool intr,
> >>                    void *data, struct ww_mutex *(*func)(int, void *))
> >> {
> >>      int i, ret, contended = -1;
> >>      struct ww_mutex *lock;
> >>
> >> retry:
> >>      if (contended != -1) {
> >>              lock = func(contended, data);
> >>              if (intr)
> >>                      ret = ww_mutex_lock_slow_interruptible(lock, acquire_ctx);
> >>              else
> >>                      ret = ww_mutex_lock_slow(lock, acquire_ctx), 0;
> >>
> >>              if (ret) {
> >>                      ww_acquire_done(acquire_ctx);
> >>                      return ret;
> >>              }
> >>      }
> >>
> >>      for (i = 0; i < count; i++) {
> >>              if (i == contended)
> >>                      continue;
> >>
> >>              lock = func(i, data);
> >>              if (intr)
> >>                      ret = ww_mutex_lock_interruptible(lock, acquire_ctx);
> >>              else
> >>                      ret = ww_mutex_lock(lock, acquire_ctx), 0;
> >>
> >>              if (ret) {
> >>                      ww_mutex_unlock_all(i, data, func);
> >>                      if (contended > i) {
> >>                              lock = func(contended, data);
> >>                              ww_mutex_unlock(lock);
> >>                      }
> >>
> >>                      if (ret == -EDEADLK) {
> >>                              contended = i;
> >>                              goto retry;
> >>                      }
> >>
> >>                      ww_acquire_done(acquire_ctx);
> >>                      return ret;
> >>              }
> >>      }
> >>
> >>      ww_acquire_done(acquire_ctx);
> >>      return 0;
> >> }
> >>
> >>> +   ww_mutex_lock_for_each(for (i = 0; i < count; i++),
> >>> +                          &objs[i]->resv->lock, contended, ret, true,
> >>> +                          acquire_ctx)
> >>> +           if (ret)
> >>> +                   goto error;
> >> which then becomes:
> >>
> >> struct ww_mutex *gem_ww_mutex_func(int i, void *data)
> >> {
> >>      struct drm_gem_object **objs = data;
> >>      return &objs[i]->resv->lock;
> >> }
> >>
> >>      ret = ww_mutex_lock_all(count, acquire_ctx, true, objs, gem_ww_mutex_func);
> >>
> >>>     ww_acquire_done(acquire_ctx);
> >>>  
> >>>     return 0;
> >>> +
> >>> +error:
> >>> +   ww_mutex_unlock_for_each(for (i = 0; i < count; i++),
> >>> +                            &objs[i]->resv->lock, contended);
> >>> +   ww_acquire_done(acquire_ctx);
> >>> +   return ret;
> >>>   }
> >>>   EXPORT_SYMBOL(drm_gem_lock_reservations);
> > Another idea, entirely untested (I guess making sure that we can use the
> > same iterator for both locking and unlocking in the contended case will be
> > fun), but maybe something like this:
> >
> >       WW_MUTEX_LOCK_BEGIN();
> >       driver_for_each_loop (iter, pos) {
> >               WW_MUTEX_LOCK(&pos->ww_mutex);
> >       }
> >       WW_MUTEX_LOCK_END();
> >
> > That way we can reuse any and all iterators that'll ever show up at least.
> > It's still horrible because the macros need to jump around between all of
> > them.
>
> Yeah, I tried this as well and that's exactly the reason why I discarded
> this approach.
>
> There is this hack with goto *void we could use, but I'm pretty sure
> that is actually not part of any C standard.

Nah, just invisible jump labels + the all uppercase macro names to scream
that into your face. You essentially trade one set of horrors for another,
and this one allows you to open-code any kind of loop or other algorithim
to find all the ww_mutexes you need.

> > Would also make this useful for more cases, where maybe you need to
> > trylock some lru lock to get at your next ww_mutex, or do some
> > kref_get_unless_zero. Buffer eviction loops tend to acquire these, and
> > that would all get ugly real fast if we'd need to stuff it into some
> > iterator argument.
>
> Well I don't see a use case with eviction in general. The dance there
> requires something different as far as I can see.

Current ttm doesn't really bother with multi-threaded contention for all
of memory. You're fixing that, but I think in the end we need a
slow-reclaim which eats up the entire lru using the full ww_mutex dance.
Rougly

WW_MUTEX_LOCK_BEGIN()

lock(lru_lock);

while (bo = list_first(lru)) {
	if (kref_get_unless_zero(bo)) {
		unlock(lru_lock);
		WW_MUTEX_LOCK(bo->ww_mutex);
		lock(lru_lock);
	} else {
		/* bo is getting freed, steal it from the freeing process
		 * or just ignore */
	}
}
unlock(lru_lock)

WW_MUTEX_LOCK_END;


Also I think if we allow this we could perhaps use this to implement the
modeset macros too.
-Daniel




> > This is kinda what we went with for modeset locks with
> > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > idea for ww_mutex in full generality.
> >
> > Not going to type this out because too much w/e mode here already, but I
> > can give it a stab next week.
> > -Daniel
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Am 14.06.19 um 20:24 schrieb Daniel Vetter:
>
> On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>> [SNIP]
>> WW_MUTEX_LOCK_BEGIN()
>>
>> lock(lru_lock);
>>
>> while (bo = list_first(lru)) {
>> 	if (kref_get_unless_zero(bo)) {
>> 		unlock(lru_lock);
>> 		WW_MUTEX_LOCK(bo->ww_mutex);
>> 		lock(lru_lock);
>> 	} else {
>> 		/* bo is getting freed, steal it from the freeing process
>> 		 * or just ignore */
>> 	}
>> }
>> unlock(lru_lock)
>>
>> WW_MUTEX_LOCK_END;

Ah, now I know what you mean. And NO, that approach doesn't work.

See for the correct ww_mutex dance we need to use the iterator multiple 
times.

Once to give us the BOs which needs to be locked and another time to 
give us the BOs which needs to be unlocked in case of a contention.

That won't work with the approach you suggest here.

Regards,
Christian.

>
>
> Also I think if we allow this we could perhaps use this to implement the
> modeset macros too.
> -Daniel
>
>
>
>
>>> This is kinda what we went with for modeset locks with
>>> DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
>>> pair at least. But it's a lot more limited use-cases, maybe too fragile an
>>> idea for ww_mutex in full generality.
>>>
>>> Not going to type this out because too much w/e mode here already, but I
>>> can give it a stab next week.
>>> -Daniel
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
> Am 14.06.19 um 20:24 schrieb Daniel Vetter:
> > 
> > On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> > > [SNIP]
> > > WW_MUTEX_LOCK_BEGIN()
> > > 
> > > lock(lru_lock);
> > > 
> > > while (bo = list_first(lru)) {
> > > 	if (kref_get_unless_zero(bo)) {
> > > 		unlock(lru_lock);
> > > 		WW_MUTEX_LOCK(bo->ww_mutex);
> > > 		lock(lru_lock);
> > > 	} else {
> > > 		/* bo is getting freed, steal it from the freeing process
> > > 		 * or just ignore */
> > > 	}
> > > }
> > > unlock(lru_lock)
> > > 
> > > WW_MUTEX_LOCK_END;
> 
> Ah, now I know what you mean. And NO, that approach doesn't work.
> 
> See for the correct ww_mutex dance we need to use the iterator multiple
> times.
> 
> Once to give us the BOs which needs to be locked and another time to give us
> the BOs which needs to be unlocked in case of a contention.
> 
> That won't work with the approach you suggest here.

A right, drat.

Maybe give up on the idea to make this work for ww_mutex in general, and
just for drm_gem_buffer_object? I'm just about to send out a patch series
which makes sure that a lot more drivers set gem_bo.resv correctly (it
will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
list_head to gem_bo (won't really matter, but not something we can do with
ww_mutex really), so that the unlock walking doesn't need to reuse the
same iterator. That should work I think ...

Also, it would almost cover everything you want to do. For ttm we'd need
to make ttm_bo a subclass of gem_bo (and maybe not initialize that
embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).

Just some ideas, since copypasting the ww_mutex dance into all drivers is
indeed not great.
-Daniel

> 
> Regards,
> Christian.
> 
> > 
> > 
> > Also I think if we allow this we could perhaps use this to implement the
> > modeset macros too.
> > -Daniel
> > 
> > 
> > 
> > 
> > > > This is kinda what we went with for modeset locks with
> > > > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > > > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > > > idea for ww_mutex in full generality.
> > > > 
> > > > Not going to type this out because too much w/e mode here already, but I
> > > > can give it a stab next week.
> > > > -Daniel
> > > _______________________________________________
> > > 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 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
> Am 14.06.19 um 20:24 schrieb Daniel Vetter:
> > 
> > On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> > > [SNIP]
> > > WW_MUTEX_LOCK_BEGIN()
> > > 
> > > lock(lru_lock);
> > > 
> > > while (bo = list_first(lru)) {
> > > 	if (kref_get_unless_zero(bo)) {
> > > 		unlock(lru_lock);
> > > 		WW_MUTEX_LOCK(bo->ww_mutex);
> > > 		lock(lru_lock);
> > > 	} else {
> > > 		/* bo is getting freed, steal it from the freeing process
> > > 		 * or just ignore */
> > > 	}
> > > }
> > > unlock(lru_lock)
> > > 
> > > WW_MUTEX_LOCK_END;
> 
> Ah, now I know what you mean. And NO, that approach doesn't work.
> 
> See for the correct ww_mutex dance we need to use the iterator multiple
> times.
> 
> Once to give us the BOs which needs to be locked and another time to give us
> the BOs which needs to be unlocked in case of a contention.
> 
> That won't work with the approach you suggest here.

A right, drat.

Maybe give up on the idea to make this work for ww_mutex in general, and
just for drm_gem_buffer_object? I'm just about to send out a patch series
which makes sure that a lot more drivers set gem_bo.resv correctly (it
will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
list_head to gem_bo (won't really matter, but not something we can do with
ww_mutex really), so that the unlock walking doesn't need to reuse the
same iterator. That should work I think ...

Also, it would almost cover everything you want to do. For ttm we'd need
to make ttm_bo a subclass of gem_bo (and maybe not initialize that
embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).

Just some ideas, since copypasting the ww_mutex dance into all drivers is
indeed not great.
-Daniel

> 
> Regards,
> Christian.
> 
> > 
> > 
> > Also I think if we allow this we could perhaps use this to implement the
> > modeset macros too.
> > -Daniel
> > 
> > 
> > 
> > 
> > > > This is kinda what we went with for modeset locks with
> > > > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > > > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > > > idea for ww_mutex in full generality.
> > > > 
> > > > Not going to type this out because too much w/e mode here already, but I
> > > > can give it a stab next week.
> > > > -Daniel
> > > _______________________________________________
> > > 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 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
> > Am 14.06.19 um 20:24 schrieb Daniel Vetter:
> > >
> > > On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > [SNIP]
> > > > WW_MUTEX_LOCK_BEGIN()
> > > >
> > > > lock(lru_lock);
> > > >
> > > > while (bo = list_first(lru)) {
> > > >   if (kref_get_unless_zero(bo)) {
> > > >           unlock(lru_lock);
> > > >           WW_MUTEX_LOCK(bo->ww_mutex);
> > > >           lock(lru_lock);
> > > >   } else {
> > > >           /* bo is getting freed, steal it from the freeing process
> > > >            * or just ignore */
> > > >   }
> > > > }
> > > > unlock(lru_lock)
> > > >
> > > > WW_MUTEX_LOCK_END;
> >
> > Ah, now I know what you mean. And NO, that approach doesn't work.
> >
> > See for the correct ww_mutex dance we need to use the iterator multiple
> > times.
> >
> > Once to give us the BOs which needs to be locked and another time to give us
> > the BOs which needs to be unlocked in case of a contention.
> >
> > That won't work with the approach you suggest here.
>
> A right, drat.
>
> Maybe give up on the idea to make this work for ww_mutex in general, and
> just for drm_gem_buffer_object? I'm just about to send out a patch series
> which makes sure that a lot more drivers set gem_bo.resv correctly (it
> will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
> list_head to gem_bo (won't really matter, but not something we can do with
> ww_mutex really), so that the unlock walking doesn't need to reuse the
> same iterator. That should work I think ...
>
> Also, it would almost cover everything you want to do. For ttm we'd need
> to make ttm_bo a subclass of gem_bo (and maybe not initialize that
> embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).
>
> Just some ideas, since copypasting the ww_mutex dance into all drivers is
> indeed not great.

Even better we don't need to force everyone to use drm_gem_object, the
hard work is already done with the reservation_object. We could add a
list_head there for unwinding, and then the locking helpers would look
a lot cleaner and simpler imo. reservation_unlock_all() would even be
a real function! And if we do this then I think we should also have a
reservation_acquire_ctx, to store the list_head and maybe anything
else.

Plus all the code you want to touch is dealing with
reservation_object, so that's all covered. And it mirros quite a bit
what we've done with struct drm_modeset_lock, to wrap ww_mutex is
something easier to deal with for kms.
-Daniel

> -Daniel
>
> >
> > Regards,
> > Christian.
> >
> > >
> > >
> > > Also I think if we allow this we could perhaps use this to implement the
> > > modeset macros too.
> > > -Daniel
> > >
> > >
> > >
> > >
> > > > > This is kinda what we went with for modeset locks with
> > > > > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > > > > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > > > > idea for ww_mutex in full generality.
> > > > >
> > > > > Not going to type this out because too much w/e mode here already, but I
> > > > > can give it a stab next week.
> > > > > -Daniel
> > > > _______________________________________________
> > > > 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Daniel Vetter]

On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
> > Am 14.06.19 um 20:24 schrieb Daniel Vetter:
> > >
> > > On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
> > > > [SNIP]
> > > > WW_MUTEX_LOCK_BEGIN()
> > > >
> > > > lock(lru_lock);
> > > >
> > > > while (bo = list_first(lru)) {
> > > >   if (kref_get_unless_zero(bo)) {
> > > >           unlock(lru_lock);
> > > >           WW_MUTEX_LOCK(bo->ww_mutex);
> > > >           lock(lru_lock);
> > > >   } else {
> > > >           /* bo is getting freed, steal it from the freeing process
> > > >            * or just ignore */
> > > >   }
> > > > }
> > > > unlock(lru_lock)
> > > >
> > > > WW_MUTEX_LOCK_END;
> >
> > Ah, now I know what you mean. And NO, that approach doesn't work.
> >
> > See for the correct ww_mutex dance we need to use the iterator multiple
> > times.
> >
> > Once to give us the BOs which needs to be locked and another time to give us
> > the BOs which needs to be unlocked in case of a contention.
> >
> > That won't work with the approach you suggest here.
>
> A right, drat.
>
> Maybe give up on the idea to make this work for ww_mutex in general, and
> just for drm_gem_buffer_object? I'm just about to send out a patch series
> which makes sure that a lot more drivers set gem_bo.resv correctly (it
> will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
> list_head to gem_bo (won't really matter, but not something we can do with
> ww_mutex really), so that the unlock walking doesn't need to reuse the
> same iterator. That should work I think ...
>
> Also, it would almost cover everything you want to do. For ttm we'd need
> to make ttm_bo a subclass of gem_bo (and maybe not initialize that
> embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).
>
> Just some ideas, since copypasting the ww_mutex dance into all drivers is
> indeed not great.

Even better we don't need to force everyone to use drm_gem_object, the
hard work is already done with the reservation_object. We could add a
list_head there for unwinding, and then the locking helpers would look
a lot cleaner and simpler imo. reservation_unlock_all() would even be
a real function! And if we do this then I think we should also have a
reservation_acquire_ctx, to store the list_head and maybe anything
else.

Plus all the code you want to touch is dealing with
reservation_object, so that's all covered. And it mirros quite a bit
what we've done with struct drm_modeset_lock, to wrap ww_mutex is
something easier to deal with for kms.
-Daniel

> -Daniel
>
> >
> > Regards,
> > Christian.
> >
> > >
> > >
> > > Also I think if we allow this we could perhaps use this to implement the
> > > modeset macros too.
> > > -Daniel
> > >
> > >
> > >
> > >
> > > > > This is kinda what we went with for modeset locks with
> > > > > DRM_MODESET_LOCK_ALL_BEGIN/END, you can grab more locks in between the
> > > > > pair at least. But it's a lot more limited use-cases, maybe too fragile an
> > > > > idea for ww_mutex in full generality.
> > > > >
> > > > > Not going to type this out because too much w/e mode here already, but I
> > > > > can give it a stab next week.
> > > > > -Daniel
> > > > _______________________________________________
> > > > 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/6] drm/gem: use new ww_mutex_(un)lock_for_each macros

[Christian König]

Am 15.06.19 um 15:56 schrieb Daniel Vetter:
> On Fri, Jun 14, 2019 at 10:30 PM Daniel Vetter <daniel@ffwll.ch> wrote:
>> On Fri, Jun 14, 2019 at 08:51:11PM +0200, Christian König wrote:
>>> Am 14.06.19 um 20:24 schrieb Daniel Vetter:
>>>> On Fri, Jun 14, 2019 at 8:10 PM Christian König <ckoenig.leichtzumerken@gmail.com> wrote:
>>>>> [SNIP]
>>>>> WW_MUTEX_LOCK_BEGIN()
>>>>>
>>>>> lock(lru_lock);
>>>>>
>>>>> while (bo = list_first(lru)) {
>>>>>    if (kref_get_unless_zero(bo)) {
>>>>>            unlock(lru_lock);
>>>>>            WW_MUTEX_LOCK(bo->ww_mutex);
>>>>>            lock(lru_lock);
>>>>>    } else {
>>>>>            /* bo is getting freed, steal it from the freeing process
>>>>>             * or just ignore */
>>>>>    }
>>>>> }
>>>>> unlock(lru_lock)
>>>>>
>>>>> WW_MUTEX_LOCK_END;
>>> Ah, now I know what you mean. And NO, that approach doesn't work.
>>>
>>> See for the correct ww_mutex dance we need to use the iterator multiple
>>> times.
>>>
>>> Once to give us the BOs which needs to be locked and another time to give us
>>> the BOs which needs to be unlocked in case of a contention.
>>>
>>> That won't work with the approach you suggest here.
>> A right, drat.
>>
>> Maybe give up on the idea to make this work for ww_mutex in general, and
>> just for drm_gem_buffer_object? I'm just about to send out a patch series
>> which makes sure that a lot more drivers set gem_bo.resv correctly (it
>> will alias with ttm_bo.resv for ttm drivers ofc). Then we could add a
>> list_head to gem_bo (won't really matter, but not something we can do with
>> ww_mutex really), so that the unlock walking doesn't need to reuse the
>> same iterator. That should work I think ...
>>
>> Also, it would almost cover everything you want to do. For ttm we'd need
>> to make ttm_bo a subclass of gem_bo (and maybe not initialize that
>> embedded gem_bo for vmwgfx and shadow bo and driver internal stuff).
>>
>> Just some ideas, since copypasting the ww_mutex dance into all drivers is
>> indeed not great.
> Even better we don't need to force everyone to use drm_gem_object, the
> hard work is already done with the reservation_object. We could add a
> list_head there for unwinding, and then the locking helpers would look
> a lot cleaner and simpler imo. reservation_unlock_all() would even be
> a real function! And if we do this then I think we should also have a
> reservation_acquire_ctx, to store the list_head and maybe anything
> else.
>
> Plus all the code you want to touch is dealing with
> reservation_object, so that's all covered. And it mirros quite a bit
> what we've done with struct drm_modeset_lock, to wrap ww_mutex is
> something easier to deal with for kms.

That's a rather interesting idea.

I wouldn't use a list_head cause that has a rather horrible caching 
footprint for something you want to use during CS, but apart from that 
the idea sounds like it would also solve a bunch of problem during eviction.

Going to give that a try,
Christian.

> -Daniel
>