On Thu, Aug 05, 2021 at 12:47:05PM +0200, Daniel Vetter wrote:
Specifically document the new/clarified rules around how the shared fences do not have any ordering requirements against the exclusive fence.
But also document all the things a bit better, given how central struct dma_resv to dynamic buffer management the docs have been very inadequat.
Lots more links to other pieces of the puzzle. Unfortunately ttm_buffer_object has no docs, so no links :-(
Explain/complain a bit about dma_resv_locking_ctx(). I still don't like that one, but fixing the ttm call chains is going to be horrible. Plus we want to plug in real slowpath locking when we do that anyway.
Main part of the patch is some actual docs for struct dma_resv.
Overall I think we still have a lot of bad naming in this area (e.g. dma_resv.fence is singular, but contains the multiple shared fences), but I think that's more indicative of how the semantics and rules are just not great.
Another thing that's real awkard is how chaining exclusive fences right now means direct dma_resv.exclusive_fence pointer access with an rcu_assign_pointer. Not so great either.
v2:
- Fix a pile of typos (Matt, Jason)
- Hammer it in that breaking the rules leads to use-after-free issues around dma-buf sharing (Christian)
Reviewed-by: Christian König christian.koenig@amd.com Cc: Jason Ekstrand jason@jlekstrand.net Cc: Matthew Auld matthew.auld@intel.com Reviewed-by: Matthew Auld matthew.auld@intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org
Also pushed to drm-misc-next. -Daniel
drivers/dma-buf/dma-resv.c | 24 ++++++--- include/linux/dma-buf.h | 7 +++ include/linux/dma-resv.h | 104 +++++++++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 11 deletions(-)
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index e744fd87c63c..84fbe60629e3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -48,6 +48,8 @@
- write operations) or N shared fences (read operations). The RCU
- mechanism is used to protect read access to fences from locked
- write-side updates.
*/
- See struct dma_resv for more details.
DEFINE_WD_CLASS(reservation_ww_class); @@ -137,7 +139,11 @@ EXPORT_SYMBOL(dma_resv_fini);
- @num_fences: number of fences we want to add
- Should be called before dma_resv_add_shared_fence(). Must
- be called with obj->lock held.
- be called with @obj locked through dma_resv_lock().
- Note that the preallocated slots need to be re-reserved if @obj is unlocked
- at any time before calling dma_resv_add_shared_fence(). This is validated
- when CONFIG_DEBUG_MUTEXES is enabled.
- RETURNS
- Zero for success, or -errno
@@ -234,8 +240,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max);
- @obj: the reservation object
- @fence: the shared fence to add
- Add a fence to a shared slot, obj->lock must be held, and
- Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and
- dma_resv_reserve_shared() has been called.
*/
- See also &dma_resv.fence for a discussion of the semantics.
void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -278,9 +286,11 @@ EXPORT_SYMBOL(dma_resv_add_shared_fence); /**
- dma_resv_add_excl_fence - Add an exclusive fence.
- @obj: the reservation object
- @fence: the shared fence to add
- @fence: the exclusive fence to add
- Add a fence to the exclusive slot. The obj->lock must be held.
- Add a fence to the exclusive slot. @obj must be locked with dma_resv_lock().
- Note that this function replaces all fences attached to @obj, see also
*/
- &dma_resv.fence_excl for a discussion of the semantics.
void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -609,9 +619,11 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence)
- fence
- Callers are not required to hold specific locks, but maybe hold
- dma_resv_lock() already
- dma_resv_lock() already.
- RETURNS
- true if all fences signaled, else false
*/
- True if all fences signaled, else false.
bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 678b2006be78..fc62b5f9980c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -420,6 +420,13 @@ struct dma_buf { * - Dynamic importers should set fences for any access that they can't * disable immediately from their &dma_buf_attach_ops.move_notify * callback.
** IMPORTANT:** All drivers must obey the struct dma_resv rules, specifically the* rules for updating fences, see &dma_resv.fence_excl and* &dma_resv.fence. If these dependency rules are broken access tracking* can be lost resulting in use after free issues.diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index e1ca2080a1ff..9100dd3dc21f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -62,16 +62,90 @@ struct dma_resv_list {
/**
- struct dma_resv - a reservation object manages fences for a buffer
- @lock: update side lock
- @seq: sequence count for managing RCU read-side synchronization
- @fence_excl: the exclusive fence, if there is one currently
- @fence: list of current shared fences
- There are multiple uses for this, with sometimes slightly different rules in
- how the fence slots are used.
- One use is to synchronize cross-driver access to a struct dma_buf, either for
- dynamic buffer management or just to handle implicit synchronization between
- different users of the buffer in userspace. See &dma_buf.resv for a more
- in-depth discussion.
- The other major use is to manage access and locking within a driver in a
- buffer based memory manager. struct ttm_buffer_object is the canonical
- example here, since this is where reservation objects originated from. But
- use in drivers is spreading and some drivers also manage struct
*/
- drm_gem_object with the same scheme.
struct dma_resv {
/**
* @lock:** Update side lock. Don't use directly, instead use the wrapper* functions like dma_resv_lock() and dma_resv_unlock().** Drivers which use the reservation object to manage memory dynamically* also use this lock to protect buffer object state like placement,* allocation policies or throughout command submission.*/struct ww_mutex lock;
/**
* @seq:** Sequence count for managing RCU read-side synchronization, allows* read-only access to @fence_excl and @fence while ensuring we take a* consistent snapshot.*/seqcount_ww_mutex_t seq;
/**
* @fence_excl:** The exclusive fence, if there is one currently.** There are two ways to update this fence:** - First by calling dma_resv_add_excl_fence(), which replaces all* fences attached to the reservation object. To guarantee that no* fences are lost, this new fence must signal only after all previous* fences, both shared and exclusive, have signalled. In some cases it* is convenient to achieve that by attaching a struct dma_fence_array* with all the new and old fences.** - Alternatively the fence can be set directly, which leaves the* shared fences unchanged. To guarantee that no fences are lost, this* new fence must signal only after the previous exclusive fence has* signalled. Since the shared fences are staying intact, it is not* necessary to maintain any ordering against those. If semantically* only a new access is added without actually treating the previous* one as a dependency the exclusive fences can be strung together* using struct dma_fence_chain.** Note that actual semantics of what an exclusive or shared fence mean* is defined by the user, for reservation objects shared across drivers* see &dma_buf.resv.*/struct dma_fence __rcu *fence_excl;
/**
* @fence:** List of current shared fences.** There are no ordering constraints of shared fences against the* exclusive fence slot. If a waiter needs to wait for all access, it* has to wait for both sets of fences to signal.** A new fence is added by calling dma_resv_add_shared_fence(). Since* this often needs to be done past the point of no return in command* submission it cannot fail, and therefore sufficient slots need to be* reserved by calling dma_resv_reserve_shared().** Note that actual semantics of what an exclusive or shared fence mean* is defined by the user, for reservation objects shared across drivers* see &dma_buf.resv.*/struct dma_resv_list __rcu *fence;
};
@@ -98,6 +172,13 @@ static inline void dma_resv_reset_shared_max(struct dma_resv *obj) {}
- undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
- is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
- object may be locked by itself by passing NULL as @ctx.
- When a die situation is indicated by returning -EDEADLK all locks held by
- @ctx must be unlocked and then dma_resv_lock_slow() called on @obj.
- Unlocked by calling dma_resv_unlock().
*/
- See also dma_resv_lock_interruptible() for the interruptible variant.
static inline int dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -119,6 +200,12 @@ static inline int dma_resv_lock(struct dma_resv *obj,
- undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
- is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
- object may be locked by itself by passing NULL as @ctx.
- When a die situation is indicated by returning -EDEADLK all locks held by
- @ctx must be unlocked and then dma_resv_lock_slow_interruptible() called on
- @obj.
*/
- Unlocked by calling dma_resv_unlock().
static inline int dma_resv_lock_interruptible(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -134,6 +221,8 @@ static inline int dma_resv_lock_interruptible(struct dma_resv *obj,
- Acquires the reservation object after a die case. This function
- will sleep until the lock becomes available. See dma_resv_lock() as
- well.
*/
- See also dma_resv_lock_slow_interruptible() for the interruptible variant.
static inline void dma_resv_lock_slow(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -167,7 +256,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj,
- if they overlap with a writer.
- Also note that since no context is provided, no deadlock protection is
- possible.
*/
- possible, which is also not needed for a trylock.
- Returns true if the lock was acquired, false otherwise.
@@ -193,6 +282,11 @@ static inline bool dma_resv_is_locked(struct dma_resv *obj)
- Returns the context used to lock a reservation object or NULL if no context
- was used or the object is not locked at all.
- WARNING: This interface is pretty horrible, but TTM needs it because it
- doesn't pass the struct ww_acquire_ctx around in some very long callchains.
- Everyone else just uses it to check whether they're holding a reservation or
*/
- not.
static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) { -- 2.32.0