| Message ID | 20210916113042.3631-2-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2 | expand |
On Thu, Sep 16, 2021 at 01:30:17PM +0200, Christian König wrote: > Abstract the complexity of iterating over all the fences > in a dma_resv object. > > The new loop handles the whole RCU and retry dance and > returns only fences where we can be sure we grabbed the > right one. > > v2: fix accessing the shared fences while they might be freed, > improve kerneldoc, rename _cursor to _iter, add > dma_resv_iter_is_exclusive > > Signed-off-by: Christian König <christian.koenig@amd.com> Replied in the other thread with the fully typed out example, this really needs iter_init/next/end here. Or it's just way too fragile and tricky for a generic helper that we roll out everywhere. -Daniel > --- > drivers/dma-buf/dma-resv.c | 62 ++++++++++++++++++++++++++++++++++++++ > include/linux/dma-resv.h | 50 ++++++++++++++++++++++++++++++ > 2 files changed, 112 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 84fbe60629e3..77083170ec3b 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -323,6 +323,68 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) > } > EXPORT_SYMBOL(dma_resv_add_excl_fence); > > +/** > + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj > + * @obj: the dma_resv object > + * @cursor: cursor to record the current position > + * @all_fences: true returns also the shared fences > + * @first: if we should start over > + * > + * Return all the fences in the dma_resv object which are not yet signaled. > + * The returned fence has an extra local reference so will stay alive. > + * If a concurrent modify is detected the whole iterator is started over again. > + */ > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_iter *cursor, > + bool all_fences, bool first) > +{ > + struct dma_fence *fence = NULL; > + > + first |= read_seqcount_retry(&obj->seq, cursor->seq); > + do { > + /* Drop the reference from the previous round */ > + dma_fence_put(fence); > + > + cursor->is_first = first; > + if (first) { > + cursor->seq = read_seqcount_begin(&obj->seq); > + cursor->index = -1; > + cursor->fences = dma_resv_shared_list(obj); > + > + fence = dma_resv_excl_fence(obj); > + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + fence = NULL; > + } else { > + fence = NULL; > + } > + > + if (fence) { > + fence = dma_fence_get_rcu(fence); > + } else if (all_fences && cursor->fences) { > + struct dma_resv_list *fences = cursor->fences; > + > + while (++cursor->index < fences->shared_count) { > + fence = rcu_dereference( > + fences->shared[cursor->index]); > + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, > + &fence->flags)) > + break; > + } > + if (cursor->index < fences->shared_count) > + fence = dma_fence_get_rcu(fence); > + else > + fence = NULL; > + } > + > + /* For the eventually next round */ > + first = true; > + } while (read_seqcount_retry(&obj->seq, cursor->seq)); > + > + return fence; > +} > +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); > + > /** > * dma_resv_copy_fences - Copy all fences from src to dst. > * @dst: the destination reservation object > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index 9100dd3dc21f..1cd686384c71 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -149,6 +149,53 @@ struct dma_resv { > struct dma_resv_list __rcu *fence; > }; > > +/** > + * struct dma_resv_iter - current position into the dma_resv fences > + * > + * Don't touch this directly in the driver, use the accessor function instead. > + */ > +struct dma_resv_iter { > + /** @seq: sequence number to check for modifications */ > + unsigned int seq; > + > + /** @index: index into the shared fences */ > + unsigned int index; > + > + /** @fences: the shared fences */ > + struct dma_resv_list *fences; > + > + /** @is_first: true if this is the first returned fence */ > + bool is_first; > +}; > + > +/** > + * dma_resv_for_each_fence_unlocked - fence iterator > + * @obj: a dma_resv object pointer > + * @cursor: a struct dma_resv_iter pointer > + * @all_fences: true if all fences should be returned > + * @fence: the current fence > + * > + * Iterate over the fences in a struct dma_resv object without holding the > + * dma_resv::lock. The RCU read side lock must be hold when using this, but can > + * be dropped and re-taken as necessary inside the loop. @all_fences controls > + * if the shared fences are returned as well. > + */ > +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ > + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ > + fence; dma_fence_put(fence), \ > + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) > + > +/** > + * dma_resv_iter_is_exclusive - test if the current fence is the exclusive one > + * @cursor: the cursor of the current position > + * > + * Returns true if the currently returned fence is the exclusive one. > + */ > +static inline bool dma_resv_iter_is_exclusive(struct dma_resv_iter *cursor) > +{ > + return cursor->index == -1; > +} > + > #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) > #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) > > @@ -366,6 +413,9 @@ void dma_resv_fini(struct dma_resv *obj); > int dma_resv_reserve_shared(struct dma_resv *obj, unsigned int num_fences); > void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence); > void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); > +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, > + struct dma_resv_iter *cursor, > + bool first, bool all_fences); > int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, > unsigned *pshared_count, struct dma_fence ***pshared); > int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src); > -- > 2.25.1 >
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..77083170ec3b 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,68 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_walk_unlocked - walk over fences in a dma_resv obj + * @obj: the dma_resv object + * @cursor: cursor to record the current position + * @all_fences: true returns also the shared fences + * @first: if we should start over + * + * Return all the fences in the dma_resv object which are not yet signaled. + * The returned fence has an extra local reference so will stay alive. + * If a concurrent modify is detected the whole iterator is started over again. + */ +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_iter *cursor, + bool all_fences, bool first) +{ + struct dma_fence *fence = NULL; + + first |= read_seqcount_retry(&obj->seq, cursor->seq); + do { + /* Drop the reference from the previous round */ + dma_fence_put(fence); + + cursor->is_first = first; + if (first) { + cursor->seq = read_seqcount_begin(&obj->seq); + cursor->index = -1; + cursor->fences = dma_resv_shared_list(obj); + + fence = dma_resv_excl_fence(obj); + if (fence && test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + fence = NULL; + } else { + fence = NULL; + } + + if (fence) { + fence = dma_fence_get_rcu(fence); + } else if (all_fences && cursor->fences) { + struct dma_resv_list *fences = cursor->fences; + + while (++cursor->index < fences->shared_count) { + fence = rcu_dereference( + fences->shared[cursor->index]); + if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, + &fence->flags)) + break; + } + if (cursor->index < fences->shared_count) + fence = dma_fence_get_rcu(fence); + else + fence = NULL; + } + + /* For the eventually next round */ + first = true; + } while (read_seqcount_retry(&obj->seq, cursor->seq)); + + return fence; +} +EXPORT_SYMBOL_GPL(dma_resv_walk_unlocked); + /** * dma_resv_copy_fences - Copy all fences from src to dst. * @dst: the destination reservation object diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index 9100dd3dc21f..1cd686384c71 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,53 @@ struct dma_resv { struct dma_resv_list __rcu *fence; }; +/** + * struct dma_resv_iter - current position into the dma_resv fences + * + * Don't touch this directly in the driver, use the accessor function instead. + */ +struct dma_resv_iter { + /** @seq: sequence number to check for modifications */ + unsigned int seq; + + /** @index: index into the shared fences */ + unsigned int index; + + /** @fences: the shared fences */ + struct dma_resv_list *fences; + + /** @is_first: true if this is the first returned fence */ + bool is_first; +}; + +/** + * dma_resv_for_each_fence_unlocked - fence iterator + * @obj: a dma_resv object pointer + * @cursor: a struct dma_resv_iter pointer + * @all_fences: true if all fences should be returned + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * dma_resv::lock. The RCU read side lock must be hold when using this, but can + * be dropped and re-taken as necessary inside the loop. @all_fences controls + * if the shared fences are returned as well. + */ +#define dma_resv_for_each_fence_unlocked(obj, cursor, all_fences, fence) \ + for (fence = dma_resv_walk_unlocked(obj, cursor, all_fences, true); \ + fence; dma_fence_put(fence), \ + fence = dma_resv_walk_unlocked(obj, cursor, all_fences, false)) + +/** + * dma_resv_iter_is_exclusive - test if the current fence is the exclusive one + * @cursor: the cursor of the current position + * + * Returns true if the currently returned fence is the exclusive one. + */ +static inline bool dma_resv_iter_is_exclusive(struct dma_resv_iter *cursor) +{ + return cursor->index == -1; +} + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) @@ -366,6 +413,9 @@ void dma_resv_fini(struct dma_resv *obj); int dma_resv_reserve_shared(struct dma_resv *obj, unsigned int num_fences); void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence); void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence); +struct dma_fence *dma_resv_walk_unlocked(struct dma_resv *obj, + struct dma_resv_iter *cursor, + bool first, bool all_fences); int dma_resv_get_fences(struct dma_resv *obj, struct dma_fence **pfence_excl, unsigned *pshared_count, struct dma_fence ***pshared); int dma_resv_copy_fences(struct dma_resv *dst, struct dma_resv *src);
Abstract the complexity of iterating over all the fences in a dma_resv object. The new loop handles the whole RCU and retry dance and returns only fences where we can be sure we grabbed the right one. v2: fix accessing the shared fences while they might be freed, improve kerneldoc, rename _cursor to _iter, add dma_resv_iter_is_exclusive Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 62 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 50 ++++++++++++++++++++++++++++++ 2 files changed, 112 insertions(+)