Please ignore this one, I've send out the wrong version without Daniels latest comment nit picks fixed.

The interesting one in this series is the last patch.

Regards, Christian.

Am 17.02.20 um 16:45 schrieb Christian König:

On the exporter side we add optional explicit pinning callbacks. Which are called when the importer doesn't implement dynamic handling, move notification or need the DMA-buf locked in place for its use case.

On the importer side we add an optional move_notify callback. This callback is used by the exporter to inform the importers that their mappings should be destroyed as soon as possible.

This allows the exporter to provide the mappings without the need to pin the backing store.

v2: don't try to invalidate mappings when the callback is NULL, lock the reservation obj while using the attachments, add helper to set the callback v3: move flag for invalidation support into the DMA-buf, use new attach_info structure to set the callback v4: use importer_priv field instead of mangling exporter priv. v5: drop invalidation_supported flag v6: squash together with pin/unpin changes v7: pin/unpin takes an attachment now v8: nuke dma_buf_attachment_(map|unmap)_locked, everything is now handled backward compatible v9: always cache when export/importer don't agree on dynamic handling v10: minimal style cleanup v11: drop automatically re-entry avoidance v12: rename callback to move_notify v13: add might_lock in appropriate places v14: rebase on separated locking change

Signed-off-by: Christian König christian.koenig@amd.com

drivers/dma-buf/dma-buf.c | 106 ++++++++++++++++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 6 +- include/linux/dma-buf.h | 78 ++++++++++++-- 3 files changed, 170 insertions(+), 20 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index d377b4ca66bf..ce293cee76ed 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -529,6 +529,10 @@ struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info) exp_info->ops->dynamic_mapping)) return ERR_PTR(-EINVAL);

• if (WARN_ON(!exp_info->ops->dynamic_mapping &&

•     (exp_info->ops->pin || exp_info->ops->unpin)))
  

• return ERR_PTR(-EINVAL);
  

• if (!try_module_get(exp_info->owner)) return ERR_PTR(-ENOENT);

@@ -653,7 +657,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put);

• calls attach() of dma_buf_ops to allow device-specific attach functionality
• @dmabuf: [in] buffer to attach device to.
• @dev: [in] device to be attached.

• • @dynamic_mapping: [in] calling convention for map/unmap

• • @importer_ops [in] importer operations for the attachment
• • @importer_priv [in] importer private pointer for the attachment
  • Returns struct dma_buf_attachment pointer for this attachment. Attachments
  • must be cleaned up by calling dma_buf_detach().

@@ -669,7 +674,8 @@ EXPORT_SYMBOL_GPL(dma_buf_put); */ struct dma_buf_attachment * dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,

•        bool dynamic_mapping)
  

•        const struct dma_buf_attach_ops *importer_ops,
  

•        void *importer_priv)
  

  { struct dma_buf_attachment *attach; int ret;

@@ -683,7 +689,8 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,

attach->dev = dev; attach->dmabuf = dmabuf;

• attach->dynamic_mapping = dynamic_mapping;

• attach->importer_ops = importer_ops;

• attach->importer_priv = importer_priv;

  if (dmabuf->ops->attach) { ret = dmabuf->ops->attach(dmabuf, attach);

@@ -702,15 +709,19 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, dma_buf_is_dynamic(dmabuf)) { struct sg_table *sgt;

• if (dma_buf_is_dynamic(attach->dmabuf))
  

• if (dma_buf_is_dynamic(attach->dmabuf)) {
  dma_resv_lock(attach->dmabuf->resv, NULL);
  

• 	ret = dma_buf_pin(attach);
  

• 	if (ret)
  

• 		goto err_unlock;
  

• }
  
  

  sgt = dmabuf->ops->map_dma_buf(attach, DMA_BIDIRECTIONAL); if (!sgt) sgt = ERR_PTR(-ENOMEM); if (IS_ERR(sgt)) { ret = PTR_ERR(sgt);

• 	goto err_unlock;
  

• 	goto err_unpin;
  

  } if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv);

@@ -724,6 +735,10 @@ dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev, kfree(attach); return ERR_PTR(ret);

+err_unpin:

• if (dma_buf_is_dynamic(attach->dmabuf))

• dma_buf_unpin(attach);
  

• err_unlock: if (dma_buf_is_dynamic(attach->dmabuf)) dma_resv_unlock(attach->dmabuf->resv);

@@ -744,7 +759,7 @@ EXPORT_SYMBOL_GPL(dma_buf_dynamic_attach); struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, struct device *dev) {

• return dma_buf_dynamic_attach(dmabuf, dev, false);

• return dma_buf_dynamic_attach(dmabuf, dev, NULL, NULL); } EXPORT_SYMBOL_GPL(dma_buf_attach);

@@ -767,8 +782,10 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach)

dmabuf->ops->unmap_dma_buf(attach, attach->sgt, attach->dir);

• if (dma_buf_is_dynamic(attach->dmabuf))
  

• if (dma_buf_is_dynamic(attach->dmabuf)) {
  

• 	dma_buf_unpin(attach);
  dma_resv_unlock(attach->dmabuf->resv);
  

• }
  

  }

  dma_resv_lock(dmabuf->resv, NULL);

@@ -781,6 +798,44 @@ void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach) } EXPORT_SYMBOL_GPL(dma_buf_detach);

+/**

• • dma_buf_pin - Lock down the DMA-buf
• • @attach: [in] attachment which should be pinned
• • Returns:
• • 0 on success, negative error code on failure.
• */

+int dma_buf_pin(struct dma_buf_attachment *attach) +{

• struct dma_buf *dmabuf = attach->dmabuf;
• int ret = 0;
• dma_resv_assert_held(dmabuf->resv);
• if (dmabuf->ops->pin)

• ret = dmabuf->ops->pin(attach);
  

• return ret;

+} +EXPORT_SYMBOL_GPL(dma_buf_pin);

+/**

• • dma_buf_unpin - Remove lock from DMA-buf
• • @attach: [in] attachment which should be unpinned
• */

+void dma_buf_unpin(struct dma_buf_attachment *attach) +{

• struct dma_buf *dmabuf = attach->dmabuf;
• dma_resv_assert_held(dmabuf->resv);
• if (dmabuf->ops->unpin)

• dmabuf->ops->unpin(attach);
  

+} +EXPORT_SYMBOL_GPL(dma_buf_unpin);

• /**
  • dma_buf_map_attachment - Returns the scatterlist table of the attachment;
  • mapped into _device_ address space. Is a wrapper for map_dma_buf() of the

@@ -800,6 +855,7 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, enum dma_data_direction direction) { struct sg_table *sg_table;

• int r;

  might_sleep();

@@ -821,13 +877,23 @@ struct sg_table *dma_buf_map_attachment(struct dma_buf_attachment *attach, return attach->sgt; }

• if (dma_buf_is_dynamic(attach->dmabuf))

• if (dma_buf_is_dynamic(attach->dmabuf)) { dma_resv_assert_held(attach->dmabuf->resv);

• if (!attach->importer_ops->move_notify) {
  

• 	r = dma_buf_pin(attach);
  

• 	if (r)
  

• 		return ERR_PTR(r);
  

• }
  

• }

  sg_table = attach->dmabuf->ops->map_dma_buf(attach, direction); if (!sg_table) sg_table = ERR_PTR(-ENOMEM);

• if (IS_ERR(sg_table) && dma_buf_is_dynamic(attach->dmabuf) &&

•    !attach->importer_ops->move_notify)
  

• dma_buf_unpin(attach);
  

• if (!IS_ERR(sg_table) && attach->dmabuf->ops->cache_sgt_mapping) { attach->sgt = sg_table; attach->dir = direction;

@@ -866,9 +932,33 @@ void dma_buf_unmap_attachment(struct dma_buf_attachment *attach, dma_resv_assert_held(attach->dmabuf->resv);

attach->dmabuf->ops->unmap_dma_buf(attach, sg_table, direction);

• if (dma_buf_is_dynamic(attach->dmabuf) &&

•    !attach->importer_ops->move_notify)
  

• dma_buf_unpin(attach);
  

  } EXPORT_SYMBOL_GPL(dma_buf_unmap_attachment);

+/**

• • dma_buf_move_notify - notify attachments that DMA-buf is moving
• • @dmabuf: [in] buffer which is moving
• • Informs all attachmenst that they need to destroy and recreated all their
• • mappings.
• */

+void dma_buf_move_notify(struct dma_buf *dmabuf) +{

• struct dma_buf_attachment *attach;
• dma_resv_assert_held(dmabuf->resv);
• list_for_each_entry(attach, &dmabuf->attachments, node)

• if (attach->importer_ops && attach->importer_ops->move_notify)
  

• 	attach->importer_ops->move_notify(attach);
  

+} +EXPORT_SYMBOL_GPL(dma_buf_move_notify);

• /**
  • DOC: cpu access

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index e2eec7b66334..b2ca78b6abce 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -414,6 +414,9 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf) return ERR_PTR(ret); }

+static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops = { +};

• /**
  • amdgpu_gem_prime_import - &drm_driver.gem_prime_import implementation
  • @dev: DRM device

@@ -446,7 +449,8 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, if (IS_ERR(obj)) return obj;

• attach = dma_buf_dynamic_attach(dma_buf, dev->dev, true);

• attach = dma_buf_dynamic_attach(dma_buf, dev->dev,

• 			&amdgpu_dma_buf_attach_ops, NULL);
  

  if (IS_ERR(attach)) { drm_gem_object_put(obj); return ERR_CAST(attach);

diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index af73f835c51c..7456bb937635 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -93,14 +93,40 @@ struct dma_buf_ops { */ void (*detach)(struct dma_buf *, struct dma_buf_attachment *);

• /**

• * @pin:
  

• *
  

• * This is called by dma_buf_pin and lets the exporter know that the
  

• * DMA-buf can't be moved any more.
  

• *
  

• * This is called with the dmabuf->resv object locked.
  

• *
  

• * This callback is optional.
  

• *
  

• * Returns:
  

• *
  

• * 0 on success, negative error code on failure.
  

• */
  

• int (*pin)(struct dma_buf_attachment *attach);
• /**

• * @unpin:
  

• *
  

• * This is called by dma_buf_unpin and lets the exporter know that the
  

• * DMA-buf can be moved again.
  

• *
  

• * This is called with the dmabuf->resv object locked.
  

• *
  

• * This callback is optional.
  

• */
  

• void (*unpin)(struct dma_buf_attachment *attach);
• /**
  • @map_dma_buf:
  • This is called by dma_buf_map_attachment() and is used to map a
  • shared &dma_buf into device address space, and it is mandatory. It

• * can only be called if @attach has been called successfully. This
  

• * essentially pins the DMA buffer into place, and it cannot be moved
  

• * any more
  

• * can only be called if @attach has been called successfully.
  

  • This call may sleep, e.g. when the backing storage first needs to be
  • allocated, or moved to a location suitable for all currently attached

@@ -141,9 +167,6 @@ struct dma_buf_ops { * * This is called by dma_buf_unmap_attachment() and should unmap and * release the &sg_table allocated in @map_dma_buf, and it is mandatory.

• * It should also unpin the backing storage if this is the last mapping
  

• * of the DMA buffer, it the exporter supports backing storage
  

• * migration.
  

  */ void (*unmap_dma_buf)(struct dma_buf_attachment *, struct sg_table *,

@@ -336,6 +359,34 @@ struct dma_buf { } cb_excl, cb_shared; };

+/**

• • struct dma_buf_attach_ops - importer operations for an attachment
• • @move_notify: [optional] notification that the DMA-buf is moving
• • Attachment operations implemented by the importer.
• */

+struct dma_buf_attach_ops {

• /**

• * @move_notify
  

• *
  

• * If this callback is provided the framework can avoid pinning the
  

• * backing store while mappings exists.
  

• *
  

• * This callback is called with the lock of the reservation object
  

• * associated with the dma_buf held and the mapping function must be
  

• * called with this lock held as well. This makes sure that no mapping
  

• * is created concurrently with an ongoing move operation.
  

• *
  

• * Mappings stay valid and are not directly affected by this callback.
  

• * But the DMA-buf can now be in a different physical location, so all
  

• * mappings should be destroyed and re-created as soon as possible.
  

• *
  

• * New mappings can be created after this callback returns, and will
  

• * point to the new location of the DMA-buf.
  

• */
  

• void (*move_notify)(struct dma_buf_attachment *attach);

+};

• /**
  • struct dma_buf_attachment - holds device-buffer attachment data
  • @dmabuf: buffer for this attachment.

@@ -344,8 +395,9 @@ struct dma_buf {

• @sgt: cached mapping.
• @dir: direction of cached mapping.
• @priv: exporter specific attachment data.

• • @dynamic_mapping: true if dma_buf_map/unmap_attachment() is called with the
• • dma_resv lock held.

• • @importer_ops: importer operations for this attachment, if provided
• • dma_buf_map/unmap_attachment() must be called with the dma_resv lock held.
• • @importer_priv: importer specific attachment data.
  • This structure holds the attachment information between the dma_buf buffer
  • and its user device(s). The list contains one attachment struct per device

@@ -362,7 +414,8 @@ struct dma_buf_attachment { struct list_head node; struct sg_table *sgt; enum dma_data_direction dir;

• bool dynamic_mapping;

• const struct dma_buf_attach_ops *importer_ops;
• void *importer_priv; void *priv; };

@@ -438,16 +491,19 @@ static inline bool dma_buf_is_dynamic(struct dma_buf *dmabuf) static inline bool dma_buf_attachment_is_dynamic(struct dma_buf_attachment *attach) {

• return attach->dynamic_mapping;

• return !!attach->importer_ops; }

  struct dma_buf_attachment *dma_buf_attach(struct dma_buf *dmabuf, struct device *dev); struct dma_buf_attachment * dma_buf_dynamic_attach(struct dma_buf *dmabuf, struct device *dev,

•        bool dynamic_mapping);
  

•        const struct dma_buf_attach_ops *importer_ops,
  

•        void *importer_priv);
  

  void dma_buf_detach(struct dma_buf *dmabuf, struct dma_buf_attachment *attach);

+int dma_buf_pin(struct dma_buf_attachment *attach); +void dma_buf_unpin(struct dma_buf_attachment *attach);

struct dma_buf *dma_buf_export(const struct dma_buf_export_info *exp_info);

Am 17.02.20 um 18:55 schrieb Daniel Vetter:

On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:

...

Implement the importer side of unpinned DMA-buf handling.

v2: update page tables immediately

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 ++++++++++++++++++++- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 770baba621b3..48de7624d49c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf) return ERR_PTR(ret); }

+/**

• • amdgpu_dma_buf_move_notify - &attach.move_notify implementation
• • @attach: the DMA-buf attachment
• • Invalidate the DMA-buf attachment, making sure that the we re-create the
• • mapping before the next use.
• */

+static void +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) +{

• struct drm_gem_object *obj = attach->importer_priv;
• struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
• struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
• struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
• struct ttm_operation_ctx ctx = { false, false };
• struct ttm_placement placement = {};
• struct amdgpu_vm_bo_base *bo_base;
• int r;
• if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)

• return;
  

• r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
• if (r) {

• DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
  

• return;
  

• }
• for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {

• struct amdgpu_vm *vm = bo_base->vm;
  

• struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
  

• if (ticket) {
  

Yeah so this is kinda why I've been a total pain about the exact semantics of the move_notify hook. I think we should flat-out require that importers _always_ have a ticket attach when they call this, and that they can cope with additional locks being taken (i.e. full EDEADLCK) handling.

That is pretty much exactly my thinking as well.

And is also the sole reason why I started looking into the ww_mutex cursor handling a while back (e.g. the initial version with the horrible macro hack).

But this is really really hard to get right. So my thinking for now is to push this series upstream to at least unblock my ongoing P2P work.

Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to the dma_resv object, which we then can take #ifdef CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).

Now the real disaster is how we handle deadlocks. Two issues:

• Ideally we'd keep any lock we've taken locked until the end, it helps needless backoffs. I've played around a bit with that but not even poc level, just an idea:

https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27...

Idea is essentially to track a list of objects we had to lock as part of the ttm_bo_validate of the main object.

• Second one is if we get a EDEADLCK on one of these sublocks (like the one here). We need to pass that up the entire callchain, including a temporary reference (we have to drop locks to do the ww_mutex_lock_slow call), and need a custom callback to drop that temporary reference (since that's all driver specific, might even be internal ww_mutex and not anything remotely looking like a normal dma_buf). This probably needs the exec util helpers from ttm, but at the dma_resv level, so that we can do something like this:

struct dma_resv_ticket { struct ww_acquire_ctx base;

/* can be set by anyone (including other drivers) that got hold of * this ticket and had to acquire some new lock. This lock might * protect anything, including driver-internal stuff, and isn't * required to be a dma_buf or even just a dma_resv. */ struct ww_mutex *contended_lock;

/* callback which the driver (which might be a dma-buf exporter * and not matching the driver that started this locking ticket) * sets together with @contended_lock, for the main driver to drop * when it calls dma_resv_unlock on the contended_lock. */ void (drop_ref*)(struct ww_mutex *contended_lock); };

My initial thinking was to make all of this part of the core ww_mutex implementation, but then I quickly found that this won't work.

This is all supremely nasty (also ttm_bo_validate would need to be improved to handle these sublocks and random new objects that could force a ww_mutex_lock_slow).

The next idea was to have it based on dma_resv objects, but as you also figured out you then need to drop the reference to the contended lock somehow...

So my current working plan was to use GEM object to avoid the callback...

Plan B would be to throw our hands into and declare that "move_notify is best effort only and can fail for any reason". Exactly like ttm eviction currently does, even with all your hacks to do at least some dma_resv_lock (but not the full slowpath).

I would seriously NAK that. We have tried that with TTM and the whole idea is just braindead.

You can use trylock based eviction for things like best effort shrinker callbacks. But that's pretty much it.

Given how much "fun" you have with all the low memory handling and ttm fundamentally being best-effort only (despite that dma_resv would allow us to do this right, with some work) I'm not sure that's a good idea to extend to a cross-driver interface. Personally I'd lean towards fixing this first fully (in ttm/amdgpu), and then using that to implement move_notify correctly.

Well have you seen this: https://fosdem.org/2020/schedule/event/ttm/ :)

Or just add an int return value here and mandate that importers must handle eviction failures. Exactly like ttm_mem_evict_first can currently still fail for various reasons.

(Sorry this isn't exactly the mail you hoped for)

Well it's actually the mail I expected. I'm thinking about exactly those problems for over a year now as well.

For the rather specific amdgpu case I could work around that by utilizing the HMM work to invalidate page tables on the fly, but that doesn't really help with memory management in general.

So YES, I totally agree that we need some sort of GEM execution context or something like this to lock buffers on the fly as we try to make room for others.

Regards, Christian.

Cheers, Daniel

...

• 	/* When we get an error here it means that somebody
  

• 	 * else is holding the VM lock and updating page tables
  

• 	 * So we can just continue here.
  

• 	 */
  

• 	r = dma_resv_lock(resv, ticket);
  

• 	if (r)
  

• 		continue;
  

• } else {
  

• 	/* TODO: This is more problematic and we actually need
  

• 	 * to allow page tables updates without holding the
  

• 	 * lock.
  

• 	 */
  

• 	if (!dma_resv_trylock(resv))
  

• 		continue;
  

• }
  

• r = amdgpu_vm_clear_freed(adev, vm, NULL);
  

• if (!r)
  

• 	r = amdgpu_vm_handle_moved(adev, vm);
  

• if (r && r != -EBUSY)
  

• 	DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
  

• 		  r);
  

• dma_resv_unlock(resv);
  

• }

+}

• static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops = {

• .move_notify = amdgpu_dma_buf_move_notify };

  /**

@@ -489,7 +553,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, return obj;

attach = dma_buf_dynamic_attach(dma_buf, dev->dev,

• 			&amdgpu_dma_buf_attach_ops, NULL);
  

• 			&amdgpu_dma_buf_attach_ops, obj);
  

  if (IS_ERR(attach)) { drm_gem_object_put(obj); return ERR_CAST(attach);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 8ae260822908..8c480c898b0d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -926,6 +926,9 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, return 0; }

• if (bo->tbo.base.import_attach)

• dma_buf_pin(bo->tbo.base.import_attach);
  

• bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS; /* force to pin into visible video ram */ if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))

@@ -1009,6 +1012,9 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)

amdgpu_bo_subtract_pin_size(bo);

• if (bo->tbo.base.import_attach)

• dma_buf_unpin(bo->tbo.base.import_attach);
  

• for (i = 0; i < bo->placement.num_placement; i++) { bo->placements[i].lpfn = 0; bo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;

-- 2.17.1

Am 17.02.20 um 20:38 schrieb Daniel Vetter:

On Mon, Feb 17, 2020 at 7:58 PM Christian König ckoenig.leichtzumerken@gmail.com wrote:

...

Am 17.02.20 um 18:55 schrieb Daniel Vetter:

...

On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: [SNIP]

And is also the sole reason why I started looking into the ww_mutex cursor handling a while back (e.g. the initial version with the horrible macro hack).

But this is really really hard to get right. So my thinking for now is to push this series upstream to at least unblock my ongoing P2P work.

Hm, but at least the move_notify stuff and the locking nightmare around that feels rushed if we just push that. Otoh it's indeed getting painful, and we'll probably have another few rounds of headaches to sort this all out. What about a

config EXPERIMENTAL_DYNAMIC_DMA_BUF depends on BROKEN

Wrapped around the new ->move_notify hook, plus all relevant code?

Oh, that is a really good idea and trivial to to do.

...

My initial thinking was to make all of this part of the core ww_mutex implementation, but then I quickly found that this won't work.

...

This is all supremely nasty (also ttm_bo_validate would need to be improved to handle these sublocks and random new objects that could force a ww_mutex_lock_slow).

The next idea was to have it based on dma_resv objects, but as you also figured out you then need to drop the reference to the contended lock somehow...

So my current working plan was to use GEM object to avoid the callback...

I've heard noise that someone is looking into adding dynamic dma-buf support to stuff like rdma drivers. Because interconnects and big machines. Plus feels a bit awkward to mandate a gem library if you want to use dynamic dma-buf support in your driver. Hence why I think something around dma_resv (but with enough flexibility that it doesn't insist that the contending lock must be a dma_resv itself).

Ok, good to know. So to hell with the idea of using a GEM object.

But this also means that we can't do this with a single drop_ref() callback in the context because the context might contains different objects of all kind.....

[SNIP]

Oh cool, I was waiting for the upload. Will watch asap. btw slides somewhere?

Attached.

...

[SNIP] For the rather specific amdgpu case I could work around that by utilizing the HMM work to invalidate page tables on the fly, but that doesn't really help with memory management in general.

Yeah, so move_notify is maybe solveable with better hw and hmm,

You don't even need better hardware and HMM.

All you need to do is clever locking because in this case you will never export page tables to other devices.

but there's other scenarios where I think the cross-driver ww_mutex locking will be needed, for fundamental reasons. Scenario:

• a bunch of gpus in pcie slots, all in the same machine
• because pcie is slot a nice interconnect (iirc you guys call yours

xgmi or something like that)

• working sets that are bigger than vram of a single gpu
• lots of buffer sharing ofc

Yeah, completely agree. The issue with the page tables is actually a rather specific use case.

1. So driver has an imported dma-buf, currently not mapped anywhere

because ti got thrown out (or first use). 2. Importer calls dma_buf_map_attachement 3. Exporter realizes there's a nice xgmi link and p2p would be much better if that object is in vram. 4. Exporter does ttm_bo_validate or equivalent to get the bo into vram, including eviction and lots of locking 5. In turn this might bite back to the importer through some move_notify of objects still mapped, but at the end of the lru.

So ->move_notify might not be the worst, eventually I think we'll need the full locking dance across drivers (or at least across drm_device instances, there might be internal upcasting going on so you get your buffers placed in the right vram and all that directly).

YES, EXACTLY! That's the reason why I'm working on this stuff and not try to get P2P/XGMI/etc.. upstream directly without it.

I mean using P2P without all this is certainly possible, but sooner or later your memory management will just fall apart.

...

So YES, I totally agree that we need some sort of GEM execution context or something like this to lock buffers on the fly as we try to make room for others.

So what's the plan? Merge current series (with the bikesheds address) under this CONFIG_EXPERIMENTAL_DYN_DMABUF and then see where we land from there? Trying to get all the pieces lined up out of tree feels like it's going to be too much :-/

At least I hoped for something like that.

Developing this out of tree and especially since I have this only as a background task turned out to be delaying things over and over again.

This way I can get it upstream (even when still under experimental flag) and start to convince our internal team/customers that we should work on investing time into this.

Thanks, Christian.

-Daniel

...

Regards, Christian.

...

Cheers, Daniel

...

•                /* When we get an error here it means that somebody
  

•                 * else is holding the VM lock and updating page tables
  

•                 * So we can just continue here.
  

•                 */
  

•                r = dma_resv_lock(resv, ticket);
  

•                if (r)
  

•                        continue;
  

•        } else {
  

•                /* TODO: This is more problematic and we actually need
  

•                 * to allow page tables updates without holding the
  

•                 * lock.
  

•                 */
  

•                if (!dma_resv_trylock(resv))
  

•                        continue;
  

•        }
  

•        r = amdgpu_vm_clear_freed(adev, vm, NULL);
  

•        if (!r)
  

•                r = amdgpu_vm_handle_moved(adev, vm);
  

•        if (r && r != -EBUSY)
  

•                DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
  

•                          r);
  

•        dma_resv_unlock(resv);
  

• }

+}

• static const struct dma_buf_attach_ops amdgpu_dma_buf_attach_ops = {

• .move_notify = amdgpu_dma_buf_move_notify };

  /**

@@ -489,7 +553,7 @@ struct drm_gem_object *amdgpu_gem_prime_import(struct drm_device *dev, return obj;

  attach = dma_buf_dynamic_attach(dma_buf, dev->dev,

•                                &amdgpu_dma_buf_attach_ops, NULL);
  

•                                &amdgpu_dma_buf_attach_ops, obj);
  if (IS_ERR(attach)) {
          drm_gem_object_put(obj);
          return ERR_CAST(attach);
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c index 8ae260822908..8c480c898b0d 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c @@ -926,6 +926,9 @@ int amdgpu_bo_pin_restricted(struct amdgpu_bo *bo, u32 domain, return 0; }

• if (bo->tbo.base.import_attach)

•        dma_buf_pin(bo->tbo.base.import_attach);
  

• bo->flags |= AMDGPU_GEM_CREATE_VRAM_CONTIGUOUS;
  /* force to pin into visible video ram */
  if (!(bo->flags & AMDGPU_GEM_CREATE_NO_CPU_ACCESS))
  

@@ -1009,6 +1012,9 @@ int amdgpu_bo_unpin(struct amdgpu_bo *bo)

  amdgpu_bo_subtract_pin_size(bo);

• if (bo->tbo.base.import_attach)

•        dma_buf_unpin(bo->tbo.base.import_attach);
  

• for (i = 0; i < bo->placement.num_placement; i++) {
          bo->placements[i].lpfn = 0;
          bo->placements[i].flags &= ~TTM_PL_FLAG_NO_EVICT;
  

-- 2.17.1

On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) thomas_os@shipmail.org wrote:

On 2/17/20 6:55 PM, Daniel Vetter wrote:

...

On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:

...

Implement the importer side of unpinned DMA-buf handling.

v2: update page tables immediately

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 ++++++++++++++++++++- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 6 ++ 2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 770baba621b3..48de7624d49c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf) return ERR_PTR(ret); }

+/**

• • amdgpu_dma_buf_move_notify - &attach.move_notify implementation
• • @attach: the DMA-buf attachment
• • Invalidate the DMA-buf attachment, making sure that the we re-create the
• • mapping before the next use.
• */

+static void +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) +{

• struct drm_gem_object *obj = attach->importer_priv;
• struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv);
• struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj);
• struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev);
• struct ttm_operation_ctx ctx = { false, false };
• struct ttm_placement placement = {};
• struct amdgpu_vm_bo_base *bo_base;
• int r;
• if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM)

•        return;
  

• r = ttm_bo_validate(&bo->tbo, &placement, &ctx);
• if (r) {

•        DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r);
  

•        return;
  

• }
• for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) {

•        struct amdgpu_vm *vm = bo_base->vm;
  

•        struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;
  

•        if (ticket) {
  

Yeah so this is kinda why I've been a total pain about the exact semantics of the move_notify hook. I think we should flat-out require that importers _always_ have a ticket attach when they call this, and that they can cope with additional locks being taken (i.e. full EDEADLCK) handling.

Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to the dma_resv object, which we then can take #ifdef CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).

Now the real disaster is how we handle deadlocks. Two issues:

• Ideally we'd keep any lock we've taken locked until the end, it helps needless backoffs. I've played around a bit with that but not even poc level, just an idea:

https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27...

Idea is essentially to track a list of objects we had to lock as part of the ttm_bo_validate of the main object.

• Second one is if we get a EDEADLCK on one of these sublocks (like the one here). We need to pass that up the entire callchain, including a temporary reference (we have to drop locks to do the ww_mutex_lock_slow call), and need a custom callback to drop that temporary reference (since that's all driver specific, might even be internal ww_mutex and not anything remotely looking like a normal dma_buf). This probably needs the exec util helpers from ttm, but at the dma_resv level, so that we can do something like this:

struct dma_resv_ticket { struct ww_acquire_ctx base;

  /* can be set by anyone (including other drivers) that got hold of
   * this ticket and had to acquire some new lock. This lock might
   * protect anything, including driver-internal stuff, and isn't
   * required to be a dma_buf or even just a dma_resv. */
  struct ww_mutex *contended_lock;

  /* callback which the driver (which might be a dma-buf exporter
   * and not matching the driver that started this locking ticket)
   * sets together with @contended_lock, for the main driver to drop
   * when it calls dma_resv_unlock on the contended_lock. */
  void (drop_ref*)(struct ww_mutex *contended_lock);

};

This is all supremely nasty (also ttm_bo_validate would need to be improved to handle these sublocks and random new objects that could force a ww_mutex_lock_slow).

Just a short comment on this:

Neither the currently used wait-die or the wound-wait algorithm *strictly* requires a slow lock on the contended lock. For wait-die it's just very convenient since it makes us sleep instead of spinning with -EDEADLK on the contended lock. For wound-wait IIRC one could just immediately restart the whole locking transaction after an -EDEADLK, and the transaction would automatically end up waiting on the contended lock, provided the mutex lock stealing is not allowed. There is however a possibility that the transaction will be wounded again on another lock, taken before the contended lock, but I think there are ways to improve the wound-wait algorithm to reduce that probability.

So in short, choosing the wound-wait algorithm instead of wait-die and perhaps modifying the ww mutex code somewhat would probably help passing an -EDEADLK up the call chain without requiring passing the contended lock, as long as each locker releases its own locks when receiving an -EDEADLK.

Hm this is kinda tempting, since rolling out the full backoff tricker across driver boundaries is going to be real painful.

What I'm kinda worried about is the debug/validation checks we're losing with this. The required backoff has this nice property that ww_mutex debug code can check that we've fully unwound everything when we should, that we've blocked on the right lock, and that we're restarting everything without keeling over. Without that I think we could end up with situations where a driver in the middle feels like handling the EDEADLCK, which might go well most of the times (the deadlock will probably be mostly within a given driver, not across). Right up to the point where someone creates a deadlock across drivers, and the lack of full rollback will be felt.

So not sure whether we can still keep all these debug/validation checks, or whether this is a step too far towards clever tricks.

But definitely a neat idea ... -Daniel

On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:

On 2/18/20 10:01 PM, Daniel Vetter wrote:

...

On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) thomas_os@shipmail.org wrote:

...

On 2/17/20 6:55 PM, Daniel Vetter wrote:

...

On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote:

...

Implement the importer side of unpinned DMA-buf handling.

v2: update page tables immediately

Signed-off-by: Christian König christian.koenig@amd.com

drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 ++++++++++++++++++++-    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  6 ++    2 files changed, 71 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c index 770baba621b3..48de7624d49c 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct drm_device *dev, struct dma_buf *dma_buf)       return ERR_PTR(ret);    }

+/**

• • amdgpu_dma_buf_move_notify - &attach.move_notify implementation
• • @attach: the DMA-buf attachment
• • Invalidate the DMA-buf attachment, making sure that the we

re-create the

• • mapping before the next use.
• */

+static void +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) +{ +    struct drm_gem_object *obj = attach->importer_priv; +    struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); +    struct ttm_operation_ctx ctx = { false, false }; +    struct ttm_placement placement = {}; +    struct amdgpu_vm_bo_base *bo_base; +    int r;

+    if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) +            return;

+    r = ttm_bo_validate(&bo->tbo, &placement, &ctx); +    if (r) { +            DRM_ERROR("Failed to invalidate DMA-buf import (%d))\n", r); +            return; +    }

+    for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { +            struct amdgpu_vm *vm = bo_base->vm; +            struct dma_resv *resv = vm->root.base.bo->tbo.base.resv;

+            if (ticket) {

Yeah so this is kinda why I've been a total pain about the exact semantics of the move_notify hook. I think we should flat-out require that importers _always_ have a ticket attach when they call this, and that they can cope with additional locks being taken (i.e. full EDEADLCK) handling.

Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to the dma_resv object, which we then can take #ifdef CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).

Now the real disaster is how we handle deadlocks. Two issues:

• Ideally we'd keep any lock we've taken locked until the end, it

helps     needless backoffs. I've played around a bit with that but not even poc     level, just an idea:

https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27...

Idea is essentially to track a list of objects we had to lock as part of     the ttm_bo_validate of the main object.

• Second one is if we get a EDEADLCK on one of these sublocks (like

the     one here). We need to pass that up the entire callchain, including a     temporary reference (we have to drop locks to do the ww_mutex_lock_slow     call), and need a custom callback to drop that temporary reference     (since that's all driver specific, might even be internal ww_mutex and     not anything remotely looking like a normal dma_buf). This probably     needs the exec util helpers from ttm, but at the dma_resv level, so that     we can do something like this:

struct dma_resv_ticket {        struct ww_acquire_ctx base;

/* can be set by anyone (including other drivers) that got hold of         * this ticket and had to acquire some new lock. This lock might         * protect anything, including driver-internal stuff, and isn't         * required to be a dma_buf or even just a dma_resv. */        struct ww_mutex *contended_lock;

/* callback which the driver (which might be a dma-buf exporter         * and not matching the driver that started this locking ticket)         * sets together with @contended_lock, for the main driver to drop         * when it calls dma_resv_unlock on the contended_lock. */        void (drop_ref*)(struct ww_mutex *contended_lock); };

This is all supremely nasty (also ttm_bo_validate would need to be improved to handle these sublocks and random new objects that could force a ww_mutex_lock_slow).

Just a short comment on this:

Neither the currently used wait-die or the wound-wait algorithm *strictly* requires a slow lock on the contended lock. For wait-die it's just very convenient since it makes us sleep instead of spinning with -EDEADLK on the contended lock. For wound-wait IIRC one could just immediately restart the whole locking transaction after an -EDEADLK, and the transaction would automatically end up waiting on the contended lock, provided the mutex lock stealing is not allowed. There is however a possibility that the transaction will be wounded again on another lock, taken before the contended lock, but I think there are ways to improve the wound-wait algorithm to reduce that probability.

So in short, choosing the wound-wait algorithm instead of wait-die and perhaps modifying the ww mutex code somewhat would probably help passing an -EDEADLK up the call chain without requiring passing the contended lock, as long as each locker releases its own locks when receiving an -EDEADLK.

Hm this is kinda tempting, since rolling out the full backoff tricker across driver boundaries is going to be real painful.

What I'm kinda worried about is the debug/validation checks we're losing with this. The required backoff has this nice property that ww_mutex debug code can check that we've fully unwound everything when we should, that we've blocked on the right lock, and that we're restarting everything without keeling over. Without that I think we could end up with situations where a driver in the middle feels like handling the EDEADLCK, which might go well most of the times (the deadlock will probably be mostly within a given driver, not across). Right up to the point where someone creates a deadlock across drivers, and the lack of full rollback will be felt.

So not sure whether we can still keep all these debug/validation checks, or whether this is a step too far towards clever tricks.

I think we could definitely find a way to keep debugging to make sure everything is unwound before attempting to restart the locking transaction. But the debug check that we're restarting on the contended lock only really makes sense for wait-die, (and we could easily keep it for wait-die). The lock returning -EDEADLK for wound-wait may actually not be the contending lock but an arbitrary lock that the wounded transaction attempts to take after it is wounded.

So in the end IMO this is a tradeoff between added (possibly severe) locking complexity into dma-buf and not being able to switch back to wait-die efficiently if we need / want to do that.

/Thomas

And as a consequence an interface *could* be:

*) We introduce functions

void ww_acquire_relax(struct ww_acquire_ctx *ctx); int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);

that can be used instead of ww_mutex_lock_slow() in the absence of a contending lock to avoid spinning on -EDEADLK. While trying to take the contending lock is probably the best choice there are various second best approaches that can be explored, for example waiting on the contending acquire to finish or in the wound-wait case, perhaps do nothing. These functions will also help us keep the debugging.

*) A function returning -EDEADLK to a caller *must* have already released its own locks.

*) move_notify() explicitly takes a struct ww_acquire_ctx * to make sure there is no ambiguity. (I think it would be valuable if we could do the same for ttm_bo_validate()).

/Thomas

On 2/20/20 7:04 PM, Daniel Vetter wrote:

On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote:

...

On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:

...

On 2/18/20 10:01 PM, Daniel Vetter wrote:

...

On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) thomas_os@shipmail.org wrote:

...

On 2/17/20 6:55 PM, Daniel Vetter wrote:

...

On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: > Implement the importer side of unpinned DMA-buf handling. > > v2: update page tables immediately > > Signed-off-by: Christian König christian.koenig@amd.com > --- >    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 > ++++++++++++++++++++- >    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  6 ++ >    2 files changed, 71 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > index 770baba621b3..48de7624d49c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c > @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct > drm_device *dev, struct dma_buf *dma_buf) >       return ERR_PTR(ret); >    } > > +/** > + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation > + * > + * @attach: the DMA-buf attachment > + * > + * Invalidate the DMA-buf attachment, making sure that > the we re-create the > + * mapping before the next use. > + */ > +static void > +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) > +{ > +    struct drm_gem_object *obj = attach->importer_priv; > +    struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); > +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); > +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); > +    struct ttm_operation_ctx ctx = { false, false }; > +    struct ttm_placement placement = {}; > +    struct amdgpu_vm_bo_base *bo_base; > +    int r; > + > +    if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) > +            return; > + > +    r = ttm_bo_validate(&bo->tbo, &placement, &ctx); > +    if (r) { > +            DRM_ERROR("Failed to invalidate DMA-buf > import (%d))\n", r); > +            return; > +    } > + > +    for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { > +            struct amdgpu_vm *vm = bo_base->vm; > +            struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; > + > +            if (ticket) { Yeah so this is kinda why I've been a total pain about the exact semantics of the move_notify hook. I think we should flat-out require that importers _always_ have a ticket attach when they call this, and that they can cope with additional locks being taken (i.e. full EDEADLCK) handling.

Simplest way to force that contract is to add a dummy 2nd ww_mutex lock to the dma_resv object, which we then can take #ifdef CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket).

Now the real disaster is how we handle deadlocks. Two issues:

• Ideally we'd keep any lock we've taken locked until the

end, it helps     needless backoffs. I've played around a bit with that but not even poc     level, just an idea:

https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27...

Idea is essentially to track a list of objects we had to lock as part of     the ttm_bo_validate of the main object.

• Second one is if we get a EDEADLCK on one of these

sublocks (like the     one here). We need to pass that up the entire callchain, including a     temporary reference (we have to drop locks to do the ww_mutex_lock_slow     call), and need a custom callback to drop that temporary reference     (since that's all driver specific, might even be internal ww_mutex and     not anything remotely looking like a normal dma_buf). This probably     needs the exec util helpers from ttm, but at the dma_resv level, so that     we can do something like this:

struct dma_resv_ticket {        struct ww_acquire_ctx base;

/* can be set by anyone (including other drivers) that got hold of         * this ticket and had to acquire some new lock. This lock might         * protect anything, including driver-internal stuff, and isn't         * required to be a dma_buf or even just a dma_resv. */        struct ww_mutex *contended_lock;

/* callback which the driver (which might be a dma-buf exporter         * and not matching the driver that started this locking ticket)         * sets together with @contended_lock, for the main driver to drop         * when it calls dma_resv_unlock on the contended_lock. */        void (drop_ref*)(struct ww_mutex *contended_lock); };

This is all supremely nasty (also ttm_bo_validate would need to be improved to handle these sublocks and random new objects that could force a ww_mutex_lock_slow).

Just a short comment on this:

Neither the currently used wait-die or the wound-wait algorithm *strictly* requires a slow lock on the contended lock. For wait-die it's just very convenient since it makes us sleep instead of spinning with -EDEADLK on the contended lock. For wound-wait IIRC one could just immediately restart the whole locking transaction after an -EDEADLK, and the transaction would automatically end up waiting on the contended lock, provided the mutex lock stealing is not allowed. There is however a possibility that the transaction will be wounded again on another lock, taken before the contended lock, but I think there are ways to improve the wound-wait algorithm to reduce that probability.

So in short, choosing the wound-wait algorithm instead of wait-die and perhaps modifying the ww mutex code somewhat would probably help passing an -EDEADLK up the call chain without requiring passing the contended lock, as long as each locker releases its own locks when receiving an -EDEADLK.

Hm this is kinda tempting, since rolling out the full backoff tricker across driver boundaries is going to be real painful.

What I'm kinda worried about is the debug/validation checks we're losing with this. The required backoff has this nice property that ww_mutex debug code can check that we've fully unwound everything when we should, that we've blocked on the right lock, and that we're restarting everything without keeling over. Without that I think we could end up with situations where a driver in the middle feels like handling the EDEADLCK, which might go well most of the times (the deadlock will probably be mostly within a given driver, not across). Right up to the point where someone creates a deadlock across drivers, and the lack of full rollback will be felt.

So not sure whether we can still keep all these debug/validation checks, or whether this is a step too far towards clever tricks.

I think we could definitely find a way to keep debugging to make sure everything is unwound before attempting to restart the locking transaction. But the debug check that we're restarting on the contended lock only really makes sense for wait-die, (and we could easily keep it for wait-die). The lock returning -EDEADLK for wound-wait may actually not be the contending lock but an arbitrary lock that the wounded transaction attempts to take after it is wounded.

So in the end IMO this is a tradeoff between added (possibly severe) locking complexity into dma-buf and not being able to switch back to wait-die efficiently if we need / want to do that.

/Thomas

And as a consequence an interface *could* be:

*) We introduce functions

void ww_acquire_relax(struct ww_acquire_ctx *ctx); int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);

that can be used instead of ww_mutex_lock_slow() in the absence of a contending lock to avoid spinning on -EDEADLK. While trying to take the contending lock is probably the best choice there are various second best approaches that can be explored, for example waiting on the contending acquire to finish or in the wound-wait case, perhaps do nothing. These functions will also help us keep the debugging.

Hm ... I guess this could work. Trouble is, it only gets rid of the slowpath locking book-keeping headaches, we still have quite a few others.

...

*) A function returning -EDEADLK to a caller *must* have already released its own locks.

So this ties to another question, as in should these callbacks have to drops the locks thei acquire (much simpler code) or not (less thrashing, if we drop locks we might end up in a situation where threads thrash around instead of realizing quicker that they're actually deadlocking and one of them should stop and back off).

Hmm.. Could you describe such a thrashing case with an example?

/Thomas

On 2/20/20 9:08 PM, Daniel Vetter wrote:

On Thu, Feb 20, 2020 at 08:46:27PM +0100, Thomas Hellström (VMware) wrote:

...

On 2/20/20 7:04 PM, Daniel Vetter wrote:

...

On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote:

...

On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote:

...

On 2/18/20 10:01 PM, Daniel Vetter wrote:

...

On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) thomas_os@shipmail.org wrote: > On 2/17/20 6:55 PM, Daniel Vetter wrote: >> On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: >>> Implement the importer side of unpinned DMA-buf handling. >>> >>> v2: update page tables immediately >>> >>> Signed-off-by: Christian König christian.koenig@amd.com >>> --- >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 >>> ++++++++++++++++++++- >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  6 ++ >>>    2 files changed, 71 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> index 770baba621b3..48de7624d49c 100644 >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct >>> drm_device *dev, struct dma_buf *dma_buf) >>>       return ERR_PTR(ret); >>>    } >>> >>> +/** >>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation >>> + * >>> + * @attach: the DMA-buf attachment >>> + * >>> + * Invalidate the DMA-buf attachment, making sure that >>> the we re-create the >>> + * mapping before the next use. >>> + */ >>> +static void >>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) >>> +{ >>> +    struct drm_gem_object *obj = attach->importer_priv; >>> +    struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); >>> +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >>> +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >>> +    struct ttm_operation_ctx ctx = { false, false }; >>> +    struct ttm_placement placement = {}; >>> +    struct amdgpu_vm_bo_base *bo_base; >>> +    int r; >>> + >>> +    if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) >>> +            return; >>> + >>> +    r = ttm_bo_validate(&bo->tbo, &placement, &ctx); >>> +    if (r) { >>> +            DRM_ERROR("Failed to invalidate DMA-buf >>> import (%d))\n", r); >>> +            return; >>> +    } >>> + >>> +    for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { >>> +            struct amdgpu_vm *vm = bo_base->vm; >>> +            struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; >>> + >>> +            if (ticket) { >> Yeah so this is kinda why I've been a total pain about the >> exact semantics >> of the move_notify hook. I think we should flat-out require >> that importers >> _always_ have a ticket attach when they call this, and that >> they can cope >> with additional locks being taken (i.e. full EDEADLCK) handling. >> >> Simplest way to force that contract is to add a dummy 2nd >> ww_mutex lock to >> the dma_resv object, which we then can take #ifdef >> CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). >> >> Now the real disaster is how we handle deadlocks. Two issues: >> >> - Ideally we'd keep any lock we've taken locked until the >> end, it helps >>     needless backoffs. I've played around a bit with that >> but not even poc >>     level, just an idea: >> >> https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27... >> >> >>     Idea is essentially to track a list of objects we had to >> lock as part of >>     the ttm_bo_validate of the main object. >> >> - Second one is if we get a EDEADLCK on one of these >> sublocks (like the >>     one here). We need to pass that up the entire callchain, >> including a >>     temporary reference (we have to drop locks to do the >> ww_mutex_lock_slow >>     call), and need a custom callback to drop that temporary reference >>     (since that's all driver specific, might even be >> internal ww_mutex and >>     not anything remotely looking like a normal dma_buf). >> This probably >>     needs the exec util helpers from ttm, but at the >> dma_resv level, so that >>     we can do something like this: >> >> struct dma_resv_ticket { >>        struct ww_acquire_ctx base; >> >>        /* can be set by anyone (including other drivers) >> that got hold of >>         * this ticket and had to acquire some new lock. This >> lock might >>         * protect anything, including driver-internal stuff, and isn't >>         * required to be a dma_buf or even just a dma_resv. */ >>        struct ww_mutex *contended_lock; >> >>        /* callback which the driver (which might be a dma-buf exporter >>         * and not matching the driver that started this >> locking ticket) >>         * sets together with @contended_lock, for the main >> driver to drop >>         * when it calls dma_resv_unlock on the contended_lock. */ >>        void (drop_ref*)(struct ww_mutex *contended_lock); >> }; >> >> This is all supremely nasty (also ttm_bo_validate would need to be >> improved to handle these sublocks and random new objects >> that could force >> a ww_mutex_lock_slow). >> > Just a short comment on this: > > Neither the currently used wait-die or the wound-wait algorithm > *strictly* requires a slow lock on the contended lock. For > wait-die it's > just very convenient since it makes us sleep instead of spinning with > -EDEADLK on the contended lock. For wound-wait IIRC one could just > immediately restart the whole locking transaction after an > -EDEADLK, and > the transaction would automatically end up waiting on the contended > lock, provided the mutex lock stealing is not allowed. There is however > a possibility that the transaction will be wounded again on another > lock, taken before the contended lock, but I think there are ways to > improve the wound-wait algorithm to reduce that probability. > > So in short, choosing the wound-wait algorithm instead of wait-die and > perhaps modifying the ww mutex code somewhat would probably help > passing > an -EDEADLK up the call chain without requiring passing the contended > lock, as long as each locker releases its own locks when receiving an > -EDEADLK. Hm this is kinda tempting, since rolling out the full backoff tricker across driver boundaries is going to be real painful.

What I'm kinda worried about is the debug/validation checks we're losing with this. The required backoff has this nice property that ww_mutex debug code can check that we've fully unwound everything when we should, that we've blocked on the right lock, and that we're restarting everything without keeling over. Without that I think we could end up with situations where a driver in the middle feels like handling the EDEADLCK, which might go well most of the times (the deadlock will probably be mostly within a given driver, not across). Right up to the point where someone creates a deadlock across drivers, and the lack of full rollback will be felt.

So not sure whether we can still keep all these debug/validation checks, or whether this is a step too far towards clever tricks.

I think we could definitely find a way to keep debugging to make sure everything is unwound before attempting to restart the locking transaction. But the debug check that we're restarting on the contended lock only really makes sense for wait-die, (and we could easily keep it for wait-die). The lock returning -EDEADLK for wound-wait may actually not be the contending lock but an arbitrary lock that the wounded transaction attempts to take after it is wounded.

So in the end IMO this is a tradeoff between added (possibly severe) locking complexity into dma-buf and not being able to switch back to wait-die efficiently if we need / want to do that.

/Thomas

And as a consequence an interface *could* be:

*) We introduce functions

void ww_acquire_relax(struct ww_acquire_ctx *ctx); int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);

that can be used instead of ww_mutex_lock_slow() in the absence of a contending lock to avoid spinning on -EDEADLK. While trying to take the contending lock is probably the best choice there are various second best approaches that can be explored, for example waiting on the contending acquire to finish or in the wound-wait case, perhaps do nothing. These functions will also help us keep the debugging.

Hm ... I guess this could work. Trouble is, it only gets rid of the slowpath locking book-keeping headaches, we still have quite a few others.

...

*) A function returning -EDEADLK to a caller *must* have already released its own locks.

So this ties to another question, as in should these callbacks have to drops the locks thei acquire (much simpler code) or not (less thrashing, if we drop locks we might end up in a situation where threads thrash around instead of realizing quicker that they're actually deadlocking and one of them should stop and back off).

Hmm.. Could you describe such a thrashing case with an example?

Ignoring cross device fun and all that, just a simplified example of why holding onto locks you've acquired for eviction is useful, at least in a slow path.

• one thread trying to do an execbuf with a huge bo

vs.

• an entire pile of thread that try to do execbuf with just a few small bo

First thread is in the eviction loop, selects a bo, wins against all the other thread since it's been doing this forever already, gets the bo moved out, unlocks.

Since it's competing against lots of other threads with small bo, it'll have to do that a lot of times. Often enough to create a contiguous hole. If you have a smarter allocator that tries to create that hole more actively, just assume that the single huge bo is a substantial part of total vram.

The other threads will be quicker in cramming new stuff in, even if they occasionally lose the ww dance against the single thread. So the big thread livelocks.

If otoh the big thread would keep onto all the locks, eventually it have the entire vram locked, and every other thread is guaranteed to lose against it in the ww dance and queue up behind. And it could finally but its huge bo into vram and execute.

Hmm, yes this indeed explains why it's beneficial in some cases to keep a number of  locks held across certain operations, but I still fail to see why we would like *all* locks held across the entire transaction? In the above case I'd envision us ending up with something like:

int validate(ctx, bo) {

for_each_suitable_bo_to_evict(ebo) {         r = lock(ctx, ebo);         if (r == EDEADLK)             goto out_unlock;

r = move_notify(ctx, ebo);// locks and unlocks GPU VM bo.         if (r == EDEADLK)             goto out_unlock;         evict();     }

place_bo(bo);     //Repeat until success.

out_unlock:     for_each_locked_bo(ebo)         unlock(ctx, ebo); }

void command_submission() {     acquire_init(ctx);

restart:     for_each_bo_in_cs(bo) {         r = lock(ctx, bo);         if (r == -EDEADLK)             goto out_unreserve;     }

for_each_bo_in_cs(bo) {         r = validate(ctx, bo);         if (r == -EDEADLK)             goto out_unreserve;     };

cs();

for_each_bo_in_cs(bo)         unlock(ctx, bo);

acquire_fini(ctx);     return 0;

out_unreserve:     for_each_locked_bo()         unlock(ctx, bo);

acquire_relax();     goto restart; }

Vary example for multi-gpu and more realism, but that's roughly it.

Aside, a lot of the stuff Christian has been doing in ttm is to improve the chances that the competing threads will hit one of the locked objects of the big thread, and at least back off a bit. That's at least my understanding of what's been happening. -Daniel

OK unserstood. For vmwgfx the crude simplistic idea to avoid that situation has been to have an rwsem around command submission: When the thread with the big bo has run a full LRU worth of eviction without succeeding it would get annoyed and take the rwsem in write mode, blocking competing threads. But that would of course never work in a dma-buf setting, and IIRC the implementation is not complete either....

/Thomas

On 2/21/20 6:12 PM, Daniel Vetter wrote:

On Thu, Feb 20, 2020 at 11:51:07PM +0100, Thomas Hellström (VMware) wrote:

...

On 2/20/20 9:08 PM, Daniel Vetter wrote:

...

On Thu, Feb 20, 2020 at 08:46:27PM +0100, Thomas Hellström (VMware) wrote:

...

On 2/20/20 7:04 PM, Daniel Vetter wrote:

...

On Thu, Feb 20, 2020 at 10:39:06AM +0100, Thomas Hellström (VMware) wrote:

...

On 2/19/20 7:42 AM, Thomas Hellström (VMware) wrote: > On 2/18/20 10:01 PM, Daniel Vetter wrote: >> On Tue, Feb 18, 2020 at 9:17 PM Thomas Hellström (VMware) >> thomas_os@shipmail.org wrote: >>> On 2/17/20 6:55 PM, Daniel Vetter wrote: >>>> On Mon, Feb 17, 2020 at 04:45:09PM +0100, Christian König wrote: >>>>> Implement the importer side of unpinned DMA-buf handling. >>>>> >>>>> v2: update page tables immediately >>>>> >>>>> Signed-off-by: Christian König christian.koenig@amd.com >>>>> --- >>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c | 66 >>>>> ++++++++++++++++++++- >>>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_object.c  |  6 ++ >>>>>    2 files changed, 71 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>>>> index 770baba621b3..48de7624d49c 100644 >>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c >>>>> @@ -453,7 +453,71 @@ amdgpu_dma_buf_create_obj(struct >>>>> drm_device *dev, struct dma_buf *dma_buf) >>>>>       return ERR_PTR(ret); >>>>>    } >>>>> >>>>> +/** >>>>> + * amdgpu_dma_buf_move_notify - &attach.move_notify implementation >>>>> + * >>>>> + * @attach: the DMA-buf attachment >>>>> + * >>>>> + * Invalidate the DMA-buf attachment, making sure that >>>>> the we re-create the >>>>> + * mapping before the next use. >>>>> + */ >>>>> +static void >>>>> +amdgpu_dma_buf_move_notify(struct dma_buf_attachment *attach) >>>>> +{ >>>>> +    struct drm_gem_object *obj = attach->importer_priv; >>>>> +    struct ww_acquire_ctx *ticket = dma_resv_locking_ctx(obj->resv); >>>>> +    struct amdgpu_bo *bo = gem_to_amdgpu_bo(obj); >>>>> +    struct amdgpu_device *adev = amdgpu_ttm_adev(bo->tbo.bdev); >>>>> +    struct ttm_operation_ctx ctx = { false, false }; >>>>> +    struct ttm_placement placement = {}; >>>>> +    struct amdgpu_vm_bo_base *bo_base; >>>>> +    int r; >>>>> + >>>>> +    if (bo->tbo.mem.mem_type == TTM_PL_SYSTEM) >>>>> +            return; >>>>> + >>>>> +    r = ttm_bo_validate(&bo->tbo, &placement, &ctx); >>>>> +    if (r) { >>>>> +            DRM_ERROR("Failed to invalidate DMA-buf >>>>> import (%d))\n", r); >>>>> +            return; >>>>> +    } >>>>> + >>>>> +    for (bo_base = bo->vm_bo; bo_base; bo_base = bo_base->next) { >>>>> +            struct amdgpu_vm *vm = bo_base->vm; >>>>> +            struct dma_resv *resv = vm->root.base.bo->tbo.base.resv; >>>>> + >>>>> +            if (ticket) { >>>> Yeah so this is kinda why I've been a total pain about the >>>> exact semantics >>>> of the move_notify hook. I think we should flat-out require >>>> that importers >>>> _always_ have a ticket attach when they call this, and that >>>> they can cope >>>> with additional locks being taken (i.e. full EDEADLCK) handling. >>>> >>>> Simplest way to force that contract is to add a dummy 2nd >>>> ww_mutex lock to >>>> the dma_resv object, which we then can take #ifdef >>>> CONFIG_WW_MUTEX_SLOWPATH_DEBUG. Plus mabye a WARN_ON(!ticket). >>>> >>>> Now the real disaster is how we handle deadlocks. Two issues: >>>> >>>> - Ideally we'd keep any lock we've taken locked until the >>>> end, it helps >>>>     needless backoffs. I've played around a bit with that >>>> but not even poc >>>>     level, just an idea: >>>> >>>> https://cgit.freedesktop.org/~danvet/drm/commit/?id=b1799c5a0f02df9e1bb08d27... >>>> >>>> >>>>     Idea is essentially to track a list of objects we had to >>>> lock as part of >>>>     the ttm_bo_validate of the main object. >>>> >>>> - Second one is if we get a EDEADLCK on one of these >>>> sublocks (like the >>>>     one here). We need to pass that up the entire callchain, >>>> including a >>>>     temporary reference (we have to drop locks to do the >>>> ww_mutex_lock_slow >>>>     call), and need a custom callback to drop that temporary reference >>>>     (since that's all driver specific, might even be >>>> internal ww_mutex and >>>>     not anything remotely looking like a normal dma_buf). >>>> This probably >>>>     needs the exec util helpers from ttm, but at the >>>> dma_resv level, so that >>>>     we can do something like this: >>>> >>>> struct dma_resv_ticket { >>>>        struct ww_acquire_ctx base; >>>> >>>>        /* can be set by anyone (including other drivers) >>>> that got hold of >>>>         * this ticket and had to acquire some new lock. This >>>> lock might >>>>         * protect anything, including driver-internal stuff, and isn't >>>>         * required to be a dma_buf or even just a dma_resv. */ >>>>        struct ww_mutex *contended_lock; >>>> >>>>        /* callback which the driver (which might be a dma-buf exporter >>>>         * and not matching the driver that started this >>>> locking ticket) >>>>         * sets together with @contended_lock, for the main >>>> driver to drop >>>>         * when it calls dma_resv_unlock on the contended_lock. */ >>>>        void (drop_ref*)(struct ww_mutex *contended_lock); >>>> }; >>>> >>>> This is all supremely nasty (also ttm_bo_validate would need to be >>>> improved to handle these sublocks and random new objects >>>> that could force >>>> a ww_mutex_lock_slow). >>>> >>> Just a short comment on this: >>> >>> Neither the currently used wait-die or the wound-wait algorithm >>> *strictly* requires a slow lock on the contended lock. For >>> wait-die it's >>> just very convenient since it makes us sleep instead of spinning with >>> -EDEADLK on the contended lock. For wound-wait IIRC one could just >>> immediately restart the whole locking transaction after an >>> -EDEADLK, and >>> the transaction would automatically end up waiting on the contended >>> lock, provided the mutex lock stealing is not allowed. There is however >>> a possibility that the transaction will be wounded again on another >>> lock, taken before the contended lock, but I think there are ways to >>> improve the wound-wait algorithm to reduce that probability. >>> >>> So in short, choosing the wound-wait algorithm instead of wait-die and >>> perhaps modifying the ww mutex code somewhat would probably help >>> passing >>> an -EDEADLK up the call chain without requiring passing the contended >>> lock, as long as each locker releases its own locks when receiving an >>> -EDEADLK. >> Hm this is kinda tempting, since rolling out the full backoff tricker >> across driver boundaries is going to be real painful. >> >> What I'm kinda worried about is the debug/validation checks we're >> losing with this. The required backoff has this nice property that >> ww_mutex debug code can check that we've fully unwound everything when >> we should, that we've blocked on the right lock, and that we're >> restarting everything without keeling over. Without that I think we >> could end up with situations where a driver in the middle feels like >> handling the EDEADLCK, which might go well most of the times (the >> deadlock will probably be mostly within a given driver, not across). >> Right up to the point where someone creates a deadlock across drivers, >> and the lack of full rollback will be felt. >> >> So not sure whether we can still keep all these debug/validation >> checks, or whether this is a step too far towards clever tricks. > I think we could definitely find a way to keep debugging to make sure > everything is unwound before attempting to restart the locking > transaction. But the debug check that we're restarting on the contended > lock only really makes sense for wait-die, (and we could easily keep it > for wait-die). The lock returning -EDEADLK for wound-wait may actually > not be the contending lock but an arbitrary lock that the wounded > transaction attempts to take after it is wounded. > > So in the end IMO this is a tradeoff between added (possibly severe) > locking complexity into dma-buf and not being able to switch back to > wait-die efficiently if we need / want to do that. > > /Thomas And as a consequence an interface *could* be:

*) We introduce functions

void ww_acquire_relax(struct ww_acquire_ctx *ctx); int ww_acquire_relax_interruptible(struct ww_acquire_ctx *ctx);

that can be used instead of ww_mutex_lock_slow() in the absence of a contending lock to avoid spinning on -EDEADLK. While trying to take the contending lock is probably the best choice there are various second best approaches that can be explored, for example waiting on the contending acquire to finish or in the wound-wait case, perhaps do nothing. These functions will also help us keep the debugging.

Hm ... I guess this could work. Trouble is, it only gets rid of the slowpath locking book-keeping headaches, we still have quite a few others.

...

*) A function returning -EDEADLK to a caller *must* have already released its own locks.

So this ties to another question, as in should these callbacks have to drops the locks thei acquire (much simpler code) or not (less thrashing, if we drop locks we might end up in a situation where threads thrash around instead of realizing quicker that they're actually deadlocking and one of them should stop and back off).

Hmm.. Could you describe such a thrashing case with an example?

Ignoring cross device fun and all that, just a simplified example of why holding onto locks you've acquired for eviction is useful, at least in a slow path.

• one thread trying to do an execbuf with a huge bo

vs.

• an entire pile of thread that try to do execbuf with just a few small bo

First thread is in the eviction loop, selects a bo, wins against all the other thread since it's been doing this forever already, gets the bo moved out, unlocks.

Since it's competing against lots of other threads with small bo, it'll have to do that a lot of times. Often enough to create a contiguous hole. If you have a smarter allocator that tries to create that hole more actively, just assume that the single huge bo is a substantial part of total vram.

The other threads will be quicker in cramming new stuff in, even if they occasionally lose the ww dance against the single thread. So the big thread livelocks.

If otoh the big thread would keep onto all the locks, eventually it have the entire vram locked, and every other thread is guaranteed to lose against it in the ww dance and queue up behind. And it could finally but its huge bo into vram and execute.

Hmm, yes this indeed explains why it's beneficial in some cases to keep a number of  locks held across certain operations, but I still fail to see why we would like *all* locks held across the entire transaction? In the above case I'd envision us ending up with something like:

int validate(ctx, bo) {

for_each_suitable_bo_to_evict(ebo) {         r = lock(ctx, ebo);         if (r == EDEADLK)             goto out_unlock;

r = move_notify(ctx, ebo);// locks and unlocks GPU VM bo.

Yeah I think for move_notify the "keep the locks" thing is probably not what we want. That's more for when you have to evict stuff and similar things like that (which hopefully no driver needs to do in their ->move_notify). But for placing buffers we kinda want to keep things, and that's also a cross-driver thing (eventually at least I think).

...

if (r == EDEADLK)             goto out_unlock;         evict();     }

place_bo(bo);     //Repeat until success.

out_unlock:     for_each_locked_bo(ebo)         unlock(ctx, ebo);

So that this unlock loop would need to be moved up to higher levels perhaps. This here would solve the example of a single big bo, but if you have multiple then you still end up with a lot of thrashing until the younger thread realizes that it needs to back off.

Ah, so we hold on to resources we don't really need to attempt to block threads likely to be thrashing?

Yeah the Great Plan (tm) is to fully rely on ww_mutex slowly degenerating into essentially a global lock. But only when there's actual contention and thrashing.

OK.

/Thomas

-Daniel