| Message ID | 20210922091044.2612-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 v4 | expand |
On 22/09/2021 10:10, 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, add dma_resv_iter_begin/end > > v3: restructor the code, move rcu_read_lock()/unlock() into the > iterator, add dma_resv_iter_is_restarted() > > v4: fix NULL deref when no explicit fence exists, drop superflous > rcu_read_lock()/unlock() calls. > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/dma-buf/dma-resv.c | 95 ++++++++++++++++++++++++++++++++++++++ > include/linux/dma-resv.h | 95 ++++++++++++++++++++++++++++++++++++++ > 2 files changed, 190 insertions(+) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 84fbe60629e3..7768140ab36d 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -323,6 +323,101 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) > } > EXPORT_SYMBOL(dma_resv_add_excl_fence); > > +/** > + * dma_resv_iter_restart_unlocked - restart the unlocked iterator > + * @cursor: The dma_resv_iter object to restart > + * > + * Restart the unlocked iteration by initializing the cursor object. > + */ > +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) > +{ > + cursor->seq = read_seqcount_begin(&cursor->obj->seq); > + cursor->index = -1; > + if (cursor->all_fences) > + cursor->fences = dma_resv_shared_list(cursor->obj); > + else > + cursor->fences = NULL; > + cursor->is_restarted = true; > +} > + > +/** > + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj > + * @cursor: cursor to record the current position > + * > + * 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 iterration is started over again. iteration > + */ > +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) > +{ > + struct dma_resv *obj = cursor->obj; > + > + do { > + /* Drop the reference from the previous round */ > + dma_fence_put(cursor->fence); > + > + if (cursor->index++ == -1) { > + cursor->fence = dma_resv_excl_fence(obj); > + > + } else if (!cursor->fences || > + cursor->index >= cursor->fences->shared_count) { > + cursor->fence = NULL; > + > + } else { > + struct dma_resv_list *fences = cursor->fences; > + unsigned int idx = cursor->index; > + > + cursor->fence = rcu_dereference(fences->shared[idx]); > + } > + if (cursor->fence) > + cursor->fence = dma_fence_get_rcu(cursor->fence); > + } while (cursor->fence && dma_fence_is_signaled(cursor->fence)); > +} > + > +/** > + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. > + * @cursor: the cursor with the current position > + * > + * Returns the first fence from an unlocked dma_resv obj. > + */ > +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) > +{ > + rcu_read_lock(); > + do { > + dma_resv_iter_restart_unlocked(cursor); > + dma_resv_iter_walk_unlocked(cursor); > + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); > + rcu_read_unlock(); > + > + return cursor->fence; > +} > +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); Why is this one split from dma_resv_iter_begin and even exported? I couldn't find any users in the series. > + > +/** > + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj. > + * @cursor: the cursor with the current position > + * > + * Returns the next fence from an unlocked dma_resv obj. > + */ > +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) > +{ > + bool restart; > + > + rcu_read_lock(); > + cursor->is_restarted = false; > + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); > + do { > + if (restart) > + dma_resv_iter_restart_unlocked(cursor); > + dma_resv_iter_walk_unlocked(cursor); > + restart = true; > + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); > + rcu_read_unlock(); > + > + return cursor->fence; > +} > +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); Couldn't dma_resv_iter_first_unlocked and dma_resv_iter_next_unlocked share the same implementation? Especially if you are able to replace cursor->is_restarted with cursor->index == -1. > + > /** > * 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..baf77a542392 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -149,6 +149,101 @@ 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 { > + /** @obj: The dma_resv object we iterate over */ > + struct dma_resv *obj; > + > + /** @all_fences: If all fences should be returned */ > + bool all_fences; > + > + /** @fence: the currently handled fence */ > + struct dma_fence *fence; > + > + /** @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_restarted: true if this is the first returned fence */ > + bool is_restarted; > +}; > + > +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); > +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); > + > +/** > + * dma_resv_iter_begin - initialize a dma_resv_iter object > + * @cursor: The dma_resv_iter object to initialize > + * @obj: The dma_resv object which we want to iterator over iterate > + * @all_fences: If all fences should be returned or just the exclusive one > + */ > +static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor, > + struct dma_resv *obj, > + bool all_fences) > +{ > + cursor->obj = obj; > + cursor->all_fences = all_fences; > + cursor->fence = NULL; > +} > + > +/** > + * dma_resv_iter_end - cleanup a dma_resv_iter object > + * @cursor: the dma_resv_iter object which should be cleaned up > + * > + * Make sure that the reference to the fence in the cursor is properly > + * dropped. > + */ > +static inline void dma_resv_iter_end(struct dma_resv_iter *cursor) > +{ > + dma_fence_put(cursor->fence); > +} > + > +/** > + * 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; > +} > + > +/** > + * dma_resv_iter_is_restarted - test if this is the first fence after a restart > + * @cursor: the cursor with the current position > + * > + * Return true if this is the first fence in an interation after a restart. iteration > + */ > +static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > +{ > + return cursor->is_restarted; > +} > + > +/** > + * dma_resv_for_each_fence_unlocked - unlocked fence iterator > + * @cursor: a struct dma_resv_iter pointer > + * @fence: the current fence > + * > + * Iterate over the fences in a struct dma_resv object without holding the > + * &dma_resv.lock and using RCU instead. The cursor needs to be initialized > + * with dma_resv_iter_begin() and cleaned up with dma_resv_iter_end(). Inside > + * the iterator a reference to the dma_fence is hold and the RCU lock dropped. held > + * When the dma_resv is modified the iteration starts over again. > + */ > +#define dma_resv_for_each_fence_unlocked(cursor, fence) \ > + for (fence = dma_resv_iter_first_unlocked(cursor); \ > + fence; fence = dma_resv_iter_next_unlocked(cursor)) > + > #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) > #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base) > > Regards, Tvrtko
Am 22.09.21 um 16:36 schrieb Tvrtko Ursulin: >> + >> +/** >> + * dma_resv_iter_first_unlocked - first fence in an unlocked >> dma_resv obj. >> + * @cursor: the cursor with the current position >> + * >> + * Returns the first fence from an unlocked dma_resv obj. >> + */ >> +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter >> *cursor) >> +{ >> + rcu_read_lock(); >> + do { >> + dma_resv_iter_restart_unlocked(cursor); >> + dma_resv_iter_walk_unlocked(cursor); >> + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); >> + rcu_read_unlock(); >> + >> + return cursor->fence; >> +} >> +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); > > Why is this one split from dma_resv_iter_begin and even exported? I've split it to be able to use dma_resv_iter_begin in both the unlocked and locked iterator. > I couldn't find any users in the series. This is used in the dma_resv_for_each_fence_unlocked() macro to return the first fence. > >> + >> +/** >> + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv >> obj. >> + * @cursor: the cursor with the current position >> + * >> + * Returns the next fence from an unlocked dma_resv obj. >> + */ >> +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter >> *cursor) >> +{ >> + bool restart; >> + >> + rcu_read_lock(); >> + cursor->is_restarted = false; >> + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); >> + do { >> + if (restart) >> + dma_resv_iter_restart_unlocked(cursor); >> + dma_resv_iter_walk_unlocked(cursor); >> + restart = true; >> + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); >> + rcu_read_unlock(); >> + >> + return cursor->fence; >> +} >> +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); > > Couldn't dma_resv_iter_first_unlocked and dma_resv_iter_next_unlocked > share the same implementation? Especially if you are able to replace > cursor->is_restarted with cursor->index == -1. That's what I had initially, but Daniel disliked it for some reason. You then need a centralized walk function instead of first/next. Thanks, Christian. > Regards, > > Tvrtko
On 22/09/2021 15:50, Christian König wrote: > Am 22.09.21 um 16:36 schrieb Tvrtko Ursulin: >>> + >>> +/** >>> + * dma_resv_iter_first_unlocked - first fence in an unlocked >>> dma_resv obj. >>> + * @cursor: the cursor with the current position >>> + * >>> + * Returns the first fence from an unlocked dma_resv obj. >>> + */ >>> +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter >>> *cursor) >>> +{ >>> + rcu_read_lock(); >>> + do { >>> + dma_resv_iter_restart_unlocked(cursor); >>> + dma_resv_iter_walk_unlocked(cursor); >>> + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); >>> + rcu_read_unlock(); >>> + >>> + return cursor->fence; >>> +} >>> +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); >> >> Why is this one split from dma_resv_iter_begin and even exported? > > I've split it to be able to use dma_resv_iter_begin in both the unlocked > and locked iterator. Ok. > >> I couldn't find any users in the series. > > This is used in the dma_resv_for_each_fence_unlocked() macro to return > the first fence. Doh! >>> + >>> +/** >>> + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv >>> obj. >>> + * @cursor: the cursor with the current position >>> + * >>> + * Returns the next fence from an unlocked dma_resv obj. >>> + */ >>> +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter >>> *cursor) >>> +{ >>> + bool restart; >>> + >>> + rcu_read_lock(); >>> + cursor->is_restarted = false; >>> + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); >>> + do { >>> + if (restart) >>> + dma_resv_iter_restart_unlocked(cursor); >>> + dma_resv_iter_walk_unlocked(cursor); >>> + restart = true; >>> + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); >>> + rcu_read_unlock(); >>> + >>> + return cursor->fence; >>> +} >>> +EXPORT_SYMBOL(dma_resv_iter_next_unlocked); >> >> Couldn't dma_resv_iter_first_unlocked and dma_resv_iter_next_unlocked >> share the same implementation? Especially if you are able to replace >> cursor->is_restarted with cursor->index == -1. > > That's what I had initially, but Daniel disliked it for some reason. You > then need a centralized walk function instead of first/next. I had some ideas to only consolidate "first" and "next" helpers but never mind, yours is fine as well. Regards, Tvrtko > > Thanks, > Christian. > >> Regards, >> >> Tvrtko >
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 84fbe60629e3..7768140ab36d 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,6 +323,101 @@ void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) } EXPORT_SYMBOL(dma_resv_add_excl_fence); +/** + * dma_resv_iter_restart_unlocked - restart the unlocked iterator + * @cursor: The dma_resv_iter object to restart + * + * Restart the unlocked iteration by initializing the cursor object. + */ +static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) +{ + cursor->seq = read_seqcount_begin(&cursor->obj->seq); + cursor->index = -1; + if (cursor->all_fences) + cursor->fences = dma_resv_shared_list(cursor->obj); + else + cursor->fences = NULL; + cursor->is_restarted = true; +} + +/** + * dma_resv_iter_walk_unlocked - walk over fences in a dma_resv obj + * @cursor: cursor to record the current position + * + * 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 iterration is started over again. + */ +static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) +{ + struct dma_resv *obj = cursor->obj; + + do { + /* Drop the reference from the previous round */ + dma_fence_put(cursor->fence); + + if (cursor->index++ == -1) { + cursor->fence = dma_resv_excl_fence(obj); + + } else if (!cursor->fences || + cursor->index >= cursor->fences->shared_count) { + cursor->fence = NULL; + + } else { + struct dma_resv_list *fences = cursor->fences; + unsigned int idx = cursor->index; + + cursor->fence = rcu_dereference(fences->shared[idx]); + } + if (cursor->fence) + cursor->fence = dma_fence_get_rcu(cursor->fence); + } while (cursor->fence && dma_fence_is_signaled(cursor->fence)); +} + +/** + * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the first fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) +{ + rcu_read_lock(); + do { + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_first_unlocked); + +/** + * dma_resv_iter_next_unlocked - next fence in an unlocked dma_resv obj. + * @cursor: the cursor with the current position + * + * Returns the next fence from an unlocked dma_resv obj. + */ +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) +{ + bool restart; + + rcu_read_lock(); + cursor->is_restarted = false; + restart = read_seqcount_retry(&cursor->obj->seq, cursor->seq); + do { + if (restart) + dma_resv_iter_restart_unlocked(cursor); + dma_resv_iter_walk_unlocked(cursor); + restart = true; + } while (read_seqcount_retry(&cursor->obj->seq, cursor->seq)); + rcu_read_unlock(); + + return cursor->fence; +} +EXPORT_SYMBOL(dma_resv_iter_next_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..baf77a542392 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -149,6 +149,101 @@ 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 { + /** @obj: The dma_resv object we iterate over */ + struct dma_resv *obj; + + /** @all_fences: If all fences should be returned */ + bool all_fences; + + /** @fence: the currently handled fence */ + struct dma_fence *fence; + + /** @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_restarted: true if this is the first returned fence */ + bool is_restarted; +}; + +struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor); +struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor); + +/** + * dma_resv_iter_begin - initialize a dma_resv_iter object + * @cursor: The dma_resv_iter object to initialize + * @obj: The dma_resv object which we want to iterator over + * @all_fences: If all fences should be returned or just the exclusive one + */ +static inline void dma_resv_iter_begin(struct dma_resv_iter *cursor, + struct dma_resv *obj, + bool all_fences) +{ + cursor->obj = obj; + cursor->all_fences = all_fences; + cursor->fence = NULL; +} + +/** + * dma_resv_iter_end - cleanup a dma_resv_iter object + * @cursor: the dma_resv_iter object which should be cleaned up + * + * Make sure that the reference to the fence in the cursor is properly + * dropped. + */ +static inline void dma_resv_iter_end(struct dma_resv_iter *cursor) +{ + dma_fence_put(cursor->fence); +} + +/** + * 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; +} + +/** + * dma_resv_iter_is_restarted - test if this is the first fence after a restart + * @cursor: the cursor with the current position + * + * Return true if this is the first fence in an interation after a restart. + */ +static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) +{ + return cursor->is_restarted; +} + +/** + * dma_resv_for_each_fence_unlocked - unlocked fence iterator + * @cursor: a struct dma_resv_iter pointer + * @fence: the current fence + * + * Iterate over the fences in a struct dma_resv object without holding the + * &dma_resv.lock and using RCU instead. The cursor needs to be initialized + * with dma_resv_iter_begin() and cleaned up with dma_resv_iter_end(). Inside + * the iterator a reference to the dma_fence is hold and the RCU lock dropped. + * When the dma_resv is modified the iteration starts over again. + */ +#define dma_resv_for_each_fence_unlocked(cursor, fence) \ + for (fence = dma_resv_iter_first_unlocked(cursor); \ + fence; fence = dma_resv_iter_next_unlocked(cursor)) + #define dma_resv_held(obj) lockdep_is_held(&(obj)->lock.base) #define dma_resv_assert_held(obj) lockdep_assert_held(&(obj)->lock.base)
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, add dma_resv_iter_begin/end v3: restructor the code, move rcu_read_lock()/unlock() into the iterator, add dma_resv_iter_is_restarted() v4: fix NULL deref when no explicit fence exists, drop superflous rcu_read_lock()/unlock() calls. Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/dma-buf/dma-resv.c | 95 ++++++++++++++++++++++++++++++++++++++ include/linux/dma-resv.h | 95 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 190 insertions(+)