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@xxxxxxx>
> Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> Cc: "Christian König" <christian.koenig@xxxxxxx>
> Cc: linux-media@xxxxxxxxxxxxxxx
> Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
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
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
