Re: [PATCH] dma-resv: some doc polish for iterators

[Daniel Vetter <daniel@ffwll.ch>]

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
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch