Re: [Intel-gfx] [RFC] Cross-driver ww transaction lock lists

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Apr 13, 2021 at 09:57:06AM +0200, Christian König wrote:
> 
> 
> Am 13.04.21 um 09:50 schrieb Thomas Hellström:
> > Hi!
> > 
> > During the dma_resv conversion of the i915 driver, we've been using ww
> > transaction lock lists to keep track of ww_mutexes that are locked
> > during the transaction so that they can be batch unlocked at suitable
> > locations. Including also the LMEM/VRAM eviction we've ended up with
> > two static lists per transaction context; one typically unlocked at the
> > end of transaction and one initialized before and unlocked after each
> > buffer object validate. This enables us to do sleeping locking at
> > eviction and keep objects locked on the eviction list until we
> > eventually succeed allocating memory (modulo minor flaws of course).
> > 
> > It would be beneficial with the i915 TTM conversion to be able to
> > introduce a similar functionality that would work in ttm but also
> > cross-driver in, for example move_notify. It would also be beneficial
> > to be able to put any dma_resv ww mutex on the lists, and not require
> > it to be embedded in a particular object type.
> > 
> > I started scetching on some utilities for this. For TTM, for example,
> > the idea would be to pass a list head for the ww transaction lock list
> > in the ttm_operation_ctx. A function taking a ww_mutex could then
> > either attach a grabbed lock to the list for batch unlocking, or be
> > responsible for unlocking when the lock's scope is exited. It's also
> > possible to create sublists if so desired. I believe the below would be
> > sufficient to cover the i915 functionality.
> > 
> > Any comments and suggestions appreciated!
> 
> ah yes Daniel and I haven been discussing something like this for years.
> 
> I also came up with rough implementation, but we always ran into lifetime
> issues.
> 
> In other words the ww_mutexes which are on the list would need to be kept
> alive until unlocked.
> 
> Because of this we kind of backed up and said we would need this on the GEM
> level instead of working with dma_resv objects.

Yeah there's a few funny concerns here that make this awkward:
- For simplicity doing these helpers at the gem level should make things a
  bit easier, because then we have a standard way to drop the reference.
  It does mean that the only thing you can lock like this are gem objects,
  but I think that's fine. At least for a first cut.

- This is a bit awkward for vmwgfx, but a) Zack has mentioned he's looking
  into adopting gem bo internally to be able to drop a pile of code and
  stop making vmwgfx the only special-case we have b) drivers which don't
  need this won't need this, so should be fine.

  The other awkward thing I guess is that ttm would need to use the
  embedded kref from the gem bo, but that should be transparent I think.

- Next up is dma-buf: For i915 we'd like to do the same eviction trick
  also through p2p dma-buf callbacks, so that this works the same as
  eviction/reservation within a gpu. But for these internal bo you might
  not have a dma-buf, so we can't just lift the trick to the dma-buf
  level. But I think if we pass e.g. a struct list_head and a callback to
  unreference/unlock all the buffers in there to the exporter, plus
  similar for the slowpath lock, then that should be doable without
  glorious layering inversions between dma-buf and gem.

  I think for dma-buf it should even be ok if this requires that we
  allocate an entire structure with kmalloc or something, allocating
  memory while holding dma_resv is ok.

- Another reason for doing it at the gem level is that the SoC drivers
  should probably use dma_resv more, so having some competent cs/eviction
  helpers derived from what we have in ttm would be nice I think.

But also I never got anywhere with anything, since like Christian said if
we just link up ww_mutex, or dma_resv, it always dies on some lifetime
handling issues.
-Daniel

> 
> Regards,
> Christian.
> 
> > 
> > 8<------------------------------------------------------
> > 
> > #ifndef _TRANSACTION_LOCKLIST_H_
> > #define _TRANSACTION_LOCKLIST_H_
> > 
> > struct trans_lockitem;
> > 
> > /**
> >   * struct trans_locklist_ops - Ops structure for the ww locklist
> > functionality.
> >   *
> >   * Typically a const struct trans_locklist_ops is defined for each type
> > that
> >   * embeds a struct trans_lockitem, or hav a struct trans_lockitem
> > pointing
> >   * at it using the private pointer. It can be a buffer object,
> > reservation
> >   * object, a single ww_mutex or even a sublist of trans_lockitems.
> >   */
> > struct trans_locklist_ops {
> > 	/**
> > 	 * slow_lock: Slow lock to relax the transaction. Only used by
> > 	 * a contending lock item.
> > 	 * @item: The struct trans_lockitem to lock
> > 	 * @intr: Whether to lock interruptible
> > 	 *
> > 	 * Return: -ERESTARTSYS: Hit a signal when relaxing,
> > 	 * -EAGAIN, relaxing succesful, but the contending lock
> > 	 * remains unlocked.
> > 	 */
> > 	int (*slow_lock) (struct trans_lockitem *item, bool intr);
> > 
> > 	/**
> > 	 * unlock: Unlock.
> > 	 * @item: The struct trans_lockitem to unlock.
> > 	 */
> > 	void (*unlock) (struct trans_lockitem *item);
> > 
> > 	/**
> > 	 * unlock: Unlock.
> > 	 * @item: The struct trans_lockitem to put. This function may
> > be NULL.
> > 	 */
> > 	void (*put) (struct trans_lockitem *item);
> > };
> > 
> > /**
> >   * struct trans_lockitem
> >   * @ops: Pointer to an Ops structure for this lockitem.
> >   * @link: List link for the transaction locklist.
> >   * @private: Private info.
> >   * @relax_unlock: Unlock contending lock after relaxation since it is
> >   * likely not needed after a transaction restart.
> >   *
> >   * A struct trans_lockitem typically represents a single lock, but is
> >   * generic enough to represent a sublist of locks. It can either be
> >   * embedded, or allocated on demand. A kmem_cache might be beneficial.
> >   */
> > struct trans_lockitem {
> > 	const struct trans_locklist_ops *ops;
> > 	struct list_head link;
> > 	u32 relax_unlock:1;
> > 	void *private;
> > };
> > 
> > /* unlock example */
> > static inline void trans_unlock_put_all(struct list_head *list)
> > {
> > 	struct trans_lockitem *lock, *next;
> > 
> > 	list_for_each_entry_safe(lock, next, typeof(*lock), link) {
> > 		lock->ops->unlock(lock);
> > 		list_del_init(&lock->link);
> > 		if (lock->ops_put)
> > 			lock->ops->put(lock);
> > 	}
> > }
> > 
> > /* Backoff example */
> > static inline int __must_check trans_backoff(struct list_head *list,
> > bool intr,
> > 					     struct trans_lockitem
> > *contending)
> > {
> > 	int ret = 0;
> > 
> > 	trans_unlock_put_all(list);
> > 	if (contending) {
> > 		ret = contending->ops->slow_lock(contending, intr);
> > 		if (!ret && contending->relax_unlock)
> > 			contending->unlock(contending);
> > 		if (ret == -EAGAIN)
> > 			ret = 0;
> > 		contending->ops->put(contending);
> > 	}
> > 
> > 	return ret;
> > }
> > 		
> > 		
> > #endif _TRANSACTION_LOCKLIST_
> > 
> > 
> 

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