Re: [PATCH WW 04/13] drm/ttm: convert to the reservation api

On Thu, Jun 27, 2013 at 01:48:19PM +0200, Maarten Lankhorst wrote:

Now that the code is compatible in semantics, flip the switch. Use ww_mutex instead of the homegrown implementation.

ww_mutex uses -EDEADLK to signal that the caller has to back off, and -EALREADY to indicate this buffer is already held by the caller.

ttm used -EAGAIN and -EDEADLK for those, respectively. So some changes were needed to handle this correctly.

Signed-off-by: Maarten Lankhorst maarten.lankhorst@canonical.com

This one change the radeon cs kernel API. We will now return -EALREADY if userspace try to send a cs with same bo two time on the list. Which is bug in itself, but a bug that might have been abuse by some to force trigger gpu reset (even if i am convince that i sent a patch at one point to avoid that). I am not against that change just thought i should point it off.

Reviewed-by: Jerome Glisse jglisse@redhat.com

---

drivers/gpu/drm/nouveau/nouveau_gem.c | 2 +- drivers/gpu/drm/qxl/qxl_object.h | 5 - drivers/gpu/drm/ttm/ttm_bo.c | 190 +++++++++------------------------ drivers/gpu/drm/ttm/ttm_bo_util.c | 6 +- drivers/gpu/drm/ttm/ttm_execbuf_util.c | 43 +++----- include/drm/ttm/ttm_bo_api.h | 25 ++--- include/drm/ttm/ttm_execbuf_util.h | 1 - 7 files changed, 79 insertions(+), 193 deletions(-)

diff --git a/drivers/gpu/drm/nouveau/nouveau_gem.c b/drivers/gpu/drm/nouveau/nouveau_gem.c index e35d468..2b2077d 100644 --- a/drivers/gpu/drm/nouveau/nouveau_gem.c +++ b/drivers/gpu/drm/nouveau/nouveau_gem.c @@ -368,7 +368,7 @@ retry: ret = ttm_bo_reserve(&nvbo->bo, true, false, true, &op->ticket); if (ret) { validate_fini_no_ticket(op, NULL);

• 	if (unlikely(ret == -EAGAIN)) {
  

• 	if (unlikely(ret == -EDEADLK)) {
  	ret = ttm_bo_reserve_slowpath(&nvbo->bo, true,
  				      &op->ticket);
  	if (!ret)
  

diff --git a/drivers/gpu/drm/qxl/qxl_object.h b/drivers/gpu/drm/qxl/qxl_object.h index b4fd89f..ee7ad79 100644 --- a/drivers/gpu/drm/qxl/qxl_object.h +++ b/drivers/gpu/drm/qxl/qxl_object.h @@ -57,11 +57,6 @@ static inline unsigned long qxl_bo_size(struct qxl_bo *bo) return bo->tbo.num_pages << PAGE_SHIFT; }

-static inline bool qxl_bo_is_reserved(struct qxl_bo *bo) -{

• return !!atomic_read(&bo->tbo.reserved);

-}

static inline u64 qxl_bo_mmap_offset(struct qxl_bo *bo) { return bo->tbo.addr_space_offset; diff --git a/drivers/gpu/drm/ttm/ttm_bo.c b/drivers/gpu/drm/ttm/ttm_bo.c index b912375..5f9fe80 100644 --- a/drivers/gpu/drm/ttm/ttm_bo.c +++ b/drivers/gpu/drm/ttm/ttm_bo.c @@ -150,6 +150,9 @@ static void ttm_bo_release_list(struct kref *list_kref) if (bo->ttm) ttm_tt_destroy(bo->ttm); atomic_dec(&bo->glob->bo_count);

• if (bo->resv == &bo->ttm_resv)

• reservation_object_fini(&bo->ttm_resv);
  

• if (bo->destroy) bo->destroy(bo); else {

@@ -158,18 +161,6 @@ static void ttm_bo_release_list(struct kref *list_kref) ttm_mem_global_free(bdev->glob->mem_glob, acc_size); }

-static int ttm_bo_wait_unreserved(struct ttm_buffer_object *bo,

• 		  bool interruptible)
  

-{

• if (interruptible) {

• return wait_event_interruptible(bo->event_queue,
  

• 			       !ttm_bo_is_reserved(bo));
  

• } else {

• wait_event(bo->event_queue, !ttm_bo_is_reserved(bo));
  

• return 0;
  

• }

-}

void ttm_bo_add_to_lru(struct ttm_buffer_object *bo) { struct ttm_bo_device *bdev = bo->bdev; @@ -218,65 +209,27 @@ int ttm_bo_reserve_nolru(struct ttm_buffer_object *bo, bool no_wait, bool use_ticket, struct ww_acquire_ctx *ticket) {

• int ret;

• int ret = 0;

• while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) {

• /**
  

•  * Deadlock avoidance for multi-bo reserving.
  

•  */
  

• if (use_ticket && bo->seq_valid) {
  

• 	/**
  

• 	 * We've already reserved this one.
  

• 	 */
  

• 	if (unlikely(ticket->stamp == bo->val_seq))
  

• 		return -EDEADLK;
  

• 	/**
  

• 	 * Already reserved by a thread that will not back
  

• 	 * off for us. We need to back off.
  

• 	 */
  

• 	if (unlikely(ticket->stamp - bo->val_seq <= LONG_MAX))
  

• 		return -EAGAIN;
  

• }
  

• if (no_wait) {

• bool success;
  
  

• if (no_wait)
  

• /* not valid any more, fix your locking! */
  

• if (WARN_ON(ticket))
  return -EBUSY;
  
  

• ret = ttm_bo_wait_unreserved(bo, interruptible);
  

• if (unlikely(ret))
  

• 	return ret;
  

• }
• if (use_ticket) {

• bool wake_up = false;
  

• /**
  

•  * Wake up waiters that may need to recheck for deadlock,
  

•  * if we decreased the sequence number.
  

•  */
  

• if (unlikely((bo->val_seq - ticket->stamp <= LONG_MAX)
  

• 	     || !bo->seq_valid))
  

• 	wake_up = true;
  

• /*
  

•  * In the worst case with memory ordering these values can be
  

•  * seen in the wrong order. However since we call wake_up_all
  

•  * in that case, this will hopefully not pose a problem,
  

•  * and the worst case would only cause someone to accidentally
  

•  * hit -EAGAIN in ttm_bo_reserve when they see old value of
  

•  * val_seq. However this would only happen if seq_valid was
  

•  * written before val_seq was, and just means some slightly
  

•  * increased cpu usage
  

•  */
  

• bo->val_seq = ticket->stamp;
  

• bo->seq_valid = true;
  

• if (wake_up)
  

• 	wake_up_all(&bo->event_queue);
  

• } else {

• bo->seq_valid = false;
  

• success = ww_mutex_trylock(&bo->resv->lock);
  

• return success ? 0 : -EBUSY;
  

  }

• return 0;

• if (interruptible)

• ret = ww_mutex_lock_interruptible(&bo->resv->lock,
  

• 				  ticket);
  

• else

• ret = ww_mutex_lock(&bo->resv->lock, ticket);
  

• if (ret == -EINTR)

• return -ERESTARTSYS;
  

• return ret;

} EXPORT_SYMBOL(ttm_bo_reserve);

@@ -313,50 +266,27 @@ int ttm_bo_reserve(struct ttm_buffer_object *bo, return ret; }

-int ttm_bo_reserve_slowpath_nolru(struct ttm_buffer_object *bo,

• 		  bool interruptible,
  

• 		  struct ww_acquire_ctx *ticket)
  

-{

• bool wake_up = false;
• int ret;
• while (unlikely(atomic_xchg(&bo->reserved, 1) != 0)) {

• WARN_ON(bo->seq_valid && ticket->stamp == bo->val_seq);
  

• ret = ttm_bo_wait_unreserved(bo, interruptible);
  

• if (unlikely(ret))
  

• 	return ret;
  

• }
• if (bo->val_seq - ticket->stamp < LONG_MAX || !bo->seq_valid)

• wake_up = true;
  

• /**

• * Wake up waiters that may need to recheck for deadlock,
  

• * if we decreased the sequence number.
  

• */
  

• bo->val_seq = ticket->stamp;
• bo->seq_valid = true;
• if (wake_up)

• wake_up_all(&bo->event_queue);
  

• return 0;

-}

int ttm_bo_reserve_slowpath(struct ttm_buffer_object *bo, bool interruptible, struct ww_acquire_ctx *ticket) { struct ttm_bo_global *glob = bo->glob;

• int put_count, ret;

• int put_count = 0;
• int ret = 0;
• if (interruptible)

• ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock,
  

• 				       ticket);
  

• else

• ww_mutex_lock_slow(&bo->resv->lock, ticket);
  
  

• ret = ttm_bo_reserve_slowpath_nolru(bo, interruptible, ticket);
• if (likely(!ret)) {

• if (likely(ret == 0)) { spin_lock(&glob->lru_lock); put_count = ttm_bo_del_from_lru(bo); spin_unlock(&glob->lru_lock); ttm_bo_list_ref_sub(bo, put_count, true);

• }

• } else if (ret == -EINTR)

• ret = -ERESTARTSYS;
  

• return ret;

} EXPORT_SYMBOL(ttm_bo_reserve_slowpath); @@ -364,8 +294,7 @@ EXPORT_SYMBOL(ttm_bo_reserve_slowpath); void ttm_bo_unreserve_ticket_locked(struct ttm_buffer_object *bo, struct ww_acquire_ctx *ticket) { ttm_bo_add_to_lru(bo);

• atomic_set(&bo->reserved, 0);
• wake_up_all(&bo->event_queue);

• ww_mutex_unlock(&bo->resv->lock);

}

void ttm_bo_unreserve(struct ttm_buffer_object *bo) @@ -558,17 +487,7 @@ static void ttm_bo_cleanup_memtype_use(struct ttm_buffer_object *bo) } ttm_bo_mem_put(bo, &bo->mem);

• atomic_set(&bo->reserved, 0);
• wake_up_all(&bo->event_queue);
• /*

• * Since the final reference to this bo may not be dropped by
  

• * the current task we have to put a memory barrier here to make
  

• * sure the changes done in this function are always visible.
  

• *
  

• * This function only needs protection against the final kref_put.
  

• */
  

• smp_mb__before_atomic_dec();

• ww_mutex_unlock (&bo->resv->lock);

}

static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) @@ -600,10 +519,8 @@ static void ttm_bo_cleanup_refs_or_queue(struct ttm_buffer_object *bo) sync_obj = driver->sync_obj_ref(bo->sync_obj); spin_unlock(&bdev->fence_lock);

• if (!ret) {

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

• }

• if (!ret)

• ww_mutex_unlock(&bo->resv->lock);
  
  

  kref_get(&bo->list_kref); list_add_tail(&bo->ddestroy, &bdev->ddestroy);

@@ -653,8 +570,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, sync_obj = driver->sync_obj_ref(bo->sync_obj); spin_unlock(&bdev->fence_lock);

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

• ww_mutex_unlock(&bo->resv->lock);
  

  spin_unlock(&glob->lru_lock);

  ret = driver->sync_obj_wait(sync_obj, false, interruptible);

@@ -692,8 +608,7 @@ static int ttm_bo_cleanup_refs_and_unlock(struct ttm_buffer_object *bo, spin_unlock(&bdev->fence_lock);

if (ret || unlikely(list_empty(&bo->ddestroy))) {

• atomic_set(&bo->reserved, 0);
  

• wake_up_all(&bo->event_queue);
  

• ww_mutex_unlock(&bo->resv->lock);
  

  spin_unlock(&glob->lru_lock); return ret; }

@@ -1253,6 +1168,7 @@ int ttm_bo_init(struct ttm_bo_device *bdev, int ret = 0; unsigned long num_pages; struct ttm_mem_global *mem_glob = bdev->glob->mem_glob;

• bool locked;

  ret = ttm_mem_global_alloc(mem_glob, acc_size, false, false); if (ret) {

@@ -1279,8 +1195,6 @@ int ttm_bo_init(struct ttm_bo_device *bdev, kref_init(&bo->kref); kref_init(&bo->list_kref); atomic_set(&bo->cpu_writers, 0);

• atomic_set(&bo->reserved, 1);
• init_waitqueue_head(&bo->event_queue); INIT_LIST_HEAD(&bo->lru); INIT_LIST_HEAD(&bo->ddestroy); INIT_LIST_HEAD(&bo->swap);

@@ -1298,37 +1212,34 @@ int ttm_bo_init(struct ttm_bo_device *bdev, bo->mem.bus.io_reserved_count = 0; bo->priv_flags = 0; bo->mem.placement = (TTM_PL_FLAG_SYSTEM | TTM_PL_FLAG_CACHED);

• bo->seq_valid = false; bo->persistent_swap_storage = persistent_swap_storage; bo->acc_size = acc_size; bo->sg = sg;

• bo->resv = &bo->ttm_resv;

• reservation_object_init(bo->resv); atomic_inc(&bo->glob->bo_count);

  ret = ttm_bo_check_placement(bo, placement);

• if (unlikely(ret != 0))

• goto out_err;
  
  

  /*

  • For ttm_bo_type_device buffers, allocate
  • address space from the device.

  */

• if (bo->type == ttm_bo_type_device ||

•    bo->type == ttm_bo_type_sg) {
  

• if (likely(!ret) &&

•    (bo->type == ttm_bo_type_device ||
  

•     bo->type == ttm_bo_type_sg))
  

  ret = ttm_bo_setup_vm(bo);

• if (ret)
  

• 	goto out_err;
  

• }

• ret = ttm_bo_validate(bo, placement, interruptible, false);

• if (ret)

• goto out_err;
  

• locked = ww_mutex_trylock(&bo->resv->lock);
• WARN_ON(!locked);

• ttm_bo_unreserve(bo);
• return 0;

• if (likely(!ret))

• ret = ttm_bo_validate(bo, placement, interruptible, false);
  
  

-out_err: ttm_bo_unreserve(bo);

• ttm_bo_unref(&bo);

• if (unlikely(ret))

• ttm_bo_unref(&bo);
  
  

  return ret;

} @@ -1941,8 +1852,7 @@ out: * already swapped buffer. */

• atomic_set(&bo->reserved, 0);
• wake_up_all(&bo->event_queue);

• ww_mutex_unlock(&bo->resv->lock); kref_put(&bo->list_kref, ttm_bo_release_list); return ret;

} diff --git a/drivers/gpu/drm/ttm/ttm_bo_util.c b/drivers/gpu/drm/ttm/ttm_bo_util.c index af89458..319cf41 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_util.c +++ b/drivers/gpu/drm/ttm/ttm_bo_util.c @@ -433,6 +433,7 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, struct ttm_buffer_object *fbo; struct ttm_bo_device *bdev = bo->bdev; struct ttm_bo_driver *driver = bdev->driver;

• int ret;

  fbo = kmalloc(sizeof(*fbo), GFP_KERNEL); if (!fbo)

@@ -445,7 +446,6 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, * TODO: Explicit member copy would probably be better here. */

• init_waitqueue_head(&fbo->event_queue); INIT_LIST_HEAD(&fbo->ddestroy); INIT_LIST_HEAD(&fbo->lru); INIT_LIST_HEAD(&fbo->swap);

@@ -463,6 +463,10 @@ static int ttm_buffer_object_transfer(struct ttm_buffer_object *bo, kref_init(&fbo->kref); fbo->destroy = &ttm_transfered_destroy; fbo->acc_size = 0;

• fbo->resv = &fbo->ttm_resv;

• reservation_object_init(fbo->resv);

• ret = ww_mutex_trylock(&fbo->resv->lock);

• WARN_ON(!ret);

  *new_obj = fbo; return 0;

diff --git a/drivers/gpu/drm/ttm/ttm_execbuf_util.c b/drivers/gpu/drm/ttm/ttm_execbuf_util.c index efcb734..7392da5 100644 --- a/drivers/gpu/drm/ttm/ttm_execbuf_util.c +++ b/drivers/gpu/drm/ttm/ttm_execbuf_util.c @@ -48,8 +48,7 @@ static void ttm_eu_backoff_reservation_locked(struct list_head *list, entry->removed = false;

} else {

• 	atomic_set(&bo->reserved, 0);
  

• 	wake_up_all(&bo->event_queue);
  

• 	ww_mutex_unlock(&bo->resv->lock);
  

  } }

} @@ -134,8 +133,6 @@ int ttm_eu_reserve_buffers(struct ww_acquire_ctx *ticket, glob = entry->bo->glob;

ww_acquire_init(ticket, &reservation_ww_class);

• spin_lock(&glob->lru_lock);

retry: list_for_each_entry(entry, list, head) { struct ttm_buffer_object *bo = entry->bo; @@ -144,42 +141,34 @@ retry: if (entry->reserved) continue;

• ret = ttm_bo_reserve_nolru(bo, true, true, true, ticket);
  

• switch (ret) {
  

• case 0:
  

• 	break;
  

• case -EBUSY:
  

• 	ttm_eu_del_from_lru_locked(list);
  

• 	spin_unlock(&glob->lru_lock);
  

• 	ret = ttm_bo_reserve_nolru(bo, true, false,
  

• 				   true, ticket);
  

• 	spin_lock(&glob->lru_lock);
  

• 	if (!ret)
  

• 		break;
  
  

• 	if (unlikely(ret != -EAGAIN))
  

• 		goto err;
  

• ret = ttm_bo_reserve_nolru(bo, true, false, true, ticket);
  
  

• 	/* fallthrough */
  

• case -EAGAIN:
  

• if (ret == -EDEADLK) {
  

• 	/* uh oh, we lost out, drop every reservation and try
  

• 	 * to only reserve this buffer, then start over if
  

• 	 * this succeeds.
  

• 	 */
  

• 	spin_lock(&glob->lru_lock);
  ttm_eu_backoff_reservation_locked(list, ticket);
  spin_unlock(&glob->lru_lock);
  ttm_eu_list_ref_sub(list);
  

• 	ret = ttm_bo_reserve_slowpath_nolru(bo, true, ticket);
  

• 	if (unlikely(ret != 0))
  

• 	ret = ww_mutex_lock_slow_interruptible(&bo->resv->lock,
  

• 					       ticket);
  

• 	if (unlikely(ret != 0)) {
  

• 		if (ret == -EINTR)
  

• 			ret = -ERESTARTSYS;
  	goto err_fini;
  

• 	}
  
  

• 	spin_lock(&glob->lru_lock);
  entry->reserved = true;
  if (unlikely(atomic_read(&bo->cpu_writers) > 0)) {
  	ret = -EBUSY;
  	goto err;
  }
  goto retry;
  

• default:
  

• } else if (ret)
  goto err;
  

• }
  
  

  entry->reserved = true; if (unlikely(atomic_read(&bo->cpu_writers) > 0)) {

@@ -189,12 +178,14 @@ retry: }

ww_acquire_done(ticket);

• spin_lock(&glob->lru_lock); ttm_eu_del_from_lru_locked(list); spin_unlock(&glob->lru_lock); ttm_eu_list_ref_sub(list); return 0;

err:

• spin_lock(&glob->lru_lock); ttm_eu_backoff_reservation_locked(list, ticket); spin_unlock(&glob->lru_lock); ttm_eu_list_ref_sub(list);

diff --git a/include/drm/ttm/ttm_bo_api.h b/include/drm/ttm/ttm_bo_api.h index 0a992b0..31ad860 100644 --- a/include/drm/ttm/ttm_bo_api.h +++ b/include/drm/ttm/ttm_bo_api.h @@ -39,6 +39,7 @@ #include <linux/mm.h> #include <linux/rbtree.h> #include <linux/bitmap.h> +#include <linux/reservation.h>

struct ttm_bo_device;

@@ -153,7 +154,6 @@ struct ttm_tt;

• Lru lists may keep one refcount, the delayed delete list, and kref != 0
• keeps one refcount. When this refcount reaches zero,
• the object is destroyed.

• • @event_queue: Queue for processes waiting on buffer object status change.
  • @mem: structure describing current placement.
  • @persistent_swap_storage: Usually the swap storage is deleted for buffers
  • pinned in physical memory. If this behaviour is not desired, this member

@@ -164,12 +164,6 @@ struct ttm_tt;

• @lru: List head for the lru list.
• @ddestroy: List head for the delayed destroy list.
• @swap: List head for swap LRU list.

• • @val_seq: Sequence of the validation holding the @reserved lock.
• • Used to avoid starvation when many processes compete to validate the
• • buffer. This member is protected by the bo_device::lru_lock.
• • @seq_valid: The value of @val_seq is valid. This value is protected by
• • the bo_device::lru_lock.
• • @reserved: Deadlock-free lock used for synchronization state transitions.
  • @sync_obj: Pointer to a synchronization object.
  • @priv_flags: Flags describing buffer object internal state.
  • @vm_rb: Rb node for the vm rb tree.

@@ -209,10 +203,9 @@ struct ttm_buffer_object {

struct kref kref; struct kref list_kref;

• wait_queue_head_t event_queue;

  /**

• * Members protected by the bo::reserved lock.
  

• * Members protected by the bo::resv::reserved lock.
  

  */

  struct ttm_mem_reg mem;

@@ -234,15 +227,6 @@ struct ttm_buffer_object { struct list_head ddestroy; struct list_head swap; struct list_head io_reserve_lru;

• unsigned long val_seq;

• bool seq_valid;

• /**

• * Members protected by the bdev::lru_lock
  

• * only when written to.
  

• */
  

• atomic_t reserved;

  /**

  • Members protected by struct buffer_object_device::fence_lock

@@ -272,6 +256,9 @@ struct ttm_buffer_object { uint32_t cur_placement;

struct sg_table *sg;

• struct reservation_object *resv;
• struct reservation_object ttm_resv;

};

/** @@ -736,7 +723,7 @@ extern void ttm_bo_swapout_all(struct ttm_bo_device *bdev); */ static inline bool ttm_bo_is_reserved(struct ttm_buffer_object *bo) {

• return atomic_read(&bo->reserved);

• return ww_mutex_is_locked(&bo->resv->lock);

}

#endif diff --git a/include/drm/ttm/ttm_execbuf_util.h b/include/drm/ttm/ttm_execbuf_util.h index ba71ef9..ec8a1d3 100644 --- a/include/drm/ttm/ttm_execbuf_util.h +++ b/include/drm/ttm/ttm_execbuf_util.h @@ -33,7 +33,6 @@

#include <ttm/ttm_bo_api.h> #include <linux/list.h> -#include <linux/reservation.h>

/**

• struct ttm_validate_buffer

-- 1.8.3.1

---

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