| Message ID | 20211130152756.1388106-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-resv: some doc polish for iterators | expand |
On Tue, Nov 30, 2021 at 04:27:55PM +0100, Daniel Vetter wrote: > Hammer it a bit more in that iterators can be restarted and when that > matters, plus suggest to prefer the locked version whenver. > > Also delete the two leftover kerneldoc for static functions plus > sprinkle some more links while at it. > > v2: Keep some comments (Christian) > > Reviewed-by: Christian König <christian.koenig@amd.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 Soooooo behind on random stuff, just noticed I never merged this. Done that now. -Daniel > --- > drivers/dma-buf/dma-resv.c | 29 +++++++++++++++-------------- > include/linux/dma-resv.h | 13 ++++++++++++- > 2 files changed, 27 insertions(+), 15 deletions(-) > > diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c > index 9eb2baa387d4..a62eb8fc33b9 100644 > --- a/drivers/dma-buf/dma-resv.c > +++ b/drivers/dma-buf/dma-resv.c > @@ -323,12 +323,8 @@ 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. > - */ > +/* Restart the iterator by initializing all the necessary fields, but not the > + * relation to the dma_resv object. */ > static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) > { > cursor->seq = read_seqcount_begin(&cursor->obj->seq); > @@ -344,14 +340,7 @@ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) > 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 iteration is started over again. > - */ > +/* Walk to the next not signaled fence and grab a reference to it */ > static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) > { > struct dma_resv *obj = cursor->obj; > @@ -387,6 +376,12 @@ static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) > * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. > * @cursor: the cursor with the current position > * > + * Subsequent fences are iterated with dma_resv_iter_next_unlocked(). > + * > + * Beware that the iterator can be restarted. Code which accumulates statistics > + * or similar needs to check for this with dma_resv_iter_is_restarted(). For > + * this reason prefer the locked dma_resv_iter_first() whenver possible. > + * > * Returns the first fence from an unlocked dma_resv obj. > */ > struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) > @@ -406,6 +401,10 @@ 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 > * > + * Beware that the iterator can be restarted. Code which accumulates statistics > + * or similar needs to check for this with dma_resv_iter_is_restarted(). For > + * this reason prefer the locked dma_resv_iter_next() whenver possible. > + * > * Returns the next fence from an unlocked dma_resv obj. > */ > struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) > @@ -431,6 +430,8 @@ EXPORT_SYMBOL(dma_resv_iter_next_unlocked); > * dma_resv_iter_first - first fence from a locked dma_resv object > * @cursor: cursor to record the current position > * > + * Subsequent fences are iterated with dma_resv_iter_next_unlocked(). > + * > * Return the first fence in the dma_resv object while holding the > * &dma_resv.lock. > */ > diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h > index dbd235ab447f..ebe908592ac3 100644 > --- a/include/linux/dma-resv.h > +++ b/include/linux/dma-resv.h > @@ -153,6 +153,13 @@ struct dma_resv { > * struct dma_resv_iter - current position into the dma_resv fences > * > * Don't touch this directly in the driver, use the accessor function instead. > + * > + * IMPORTANT > + * > + * When using the lockless iterators like dma_resv_iter_next_unlocked() or > + * dma_resv_for_each_fence_unlocked() beware that the iterator can be restarted. > + * Code which accumulates statistics or similar needs to check for this with > + * dma_resv_iter_is_restarted(). > */ > struct dma_resv_iter { > /** @obj: The dma_resv object we iterate over */ > @@ -243,7 +250,11 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) > * &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 held and the RCU lock dropped. > - * When the dma_resv is modified the iteration starts over again. > + * > + * Beware that the iterator can be restarted when the struct dma_resv for > + * @cursor is modified. Code which accumulates statistics or similar needs to > + * check for this with dma_resv_iter_is_restarted(). For this reason prefer the > + * lock iterator dma_resv_for_each_fence() whenever possible. > */ > #define dma_resv_for_each_fence_unlocked(cursor, fence) \ > for (fence = dma_resv_iter_first_unlocked(cursor); \ > -- > 2.33.0 >
diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index 9eb2baa387d4..a62eb8fc33b9 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -323,12 +323,8 @@ 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. - */ +/* Restart the iterator by initializing all the necessary fields, but not the + * relation to the dma_resv object. */ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) { cursor->seq = read_seqcount_begin(&cursor->obj->seq); @@ -344,14 +340,7 @@ static void dma_resv_iter_restart_unlocked(struct dma_resv_iter *cursor) 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 iteration is started over again. - */ +/* Walk to the next not signaled fence and grab a reference to it */ static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) { struct dma_resv *obj = cursor->obj; @@ -387,6 +376,12 @@ static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor) * dma_resv_iter_first_unlocked - first fence in an unlocked dma_resv obj. * @cursor: the cursor with the current position * + * Subsequent fences are iterated with dma_resv_iter_next_unlocked(). + * + * Beware that the iterator can be restarted. Code which accumulates statistics + * or similar needs to check for this with dma_resv_iter_is_restarted(). For + * this reason prefer the locked dma_resv_iter_first() whenver possible. + * * Returns the first fence from an unlocked dma_resv obj. */ struct dma_fence *dma_resv_iter_first_unlocked(struct dma_resv_iter *cursor) @@ -406,6 +401,10 @@ 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 * + * Beware that the iterator can be restarted. Code which accumulates statistics + * or similar needs to check for this with dma_resv_iter_is_restarted(). For + * this reason prefer the locked dma_resv_iter_next() whenver possible. + * * Returns the next fence from an unlocked dma_resv obj. */ struct dma_fence *dma_resv_iter_next_unlocked(struct dma_resv_iter *cursor) @@ -431,6 +430,8 @@ EXPORT_SYMBOL(dma_resv_iter_next_unlocked); * dma_resv_iter_first - first fence from a locked dma_resv object * @cursor: cursor to record the current position * + * Subsequent fences are iterated with dma_resv_iter_next_unlocked(). + * * Return the first fence in the dma_resv object while holding the * &dma_resv.lock. */ diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index dbd235ab447f..ebe908592ac3 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -153,6 +153,13 @@ struct dma_resv { * struct dma_resv_iter - current position into the dma_resv fences * * Don't touch this directly in the driver, use the accessor function instead. + * + * IMPORTANT + * + * When using the lockless iterators like dma_resv_iter_next_unlocked() or + * dma_resv_for_each_fence_unlocked() beware that the iterator can be restarted. + * Code which accumulates statistics or similar needs to check for this with + * dma_resv_iter_is_restarted(). */ struct dma_resv_iter { /** @obj: The dma_resv object we iterate over */ @@ -243,7 +250,11 @@ static inline bool dma_resv_iter_is_restarted(struct dma_resv_iter *cursor) * &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 held and the RCU lock dropped. - * When the dma_resv is modified the iteration starts over again. + * + * Beware that the iterator can be restarted when the struct dma_resv for + * @cursor is modified. Code which accumulates statistics or similar needs to + * check for this with dma_resv_iter_is_restarted(). For this reason prefer the + * lock iterator dma_resv_for_each_fence() whenever possible. */ #define dma_resv_for_each_fence_unlocked(cursor, fence) \ for (fence = dma_resv_iter_first_unlocked(cursor); \