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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 30.11.21 um 10:16 schrieb Daniel Vetter:
> 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.
>
> 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
> ---
>   drivers/dma-buf/dma-resv.c | 26 ++++++++++++--------------
>   include/linux/dma-resv.h   | 13 ++++++++++++-
>   2 files changed, 24 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c
> index 9eb2baa387d4..1453b664c405 100644
> --- a/drivers/dma-buf/dma-resv.c
> +++ b/drivers/dma-buf/dma-resv.c
> @@ -323,12 +323,6 @@ 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.
> - */

Maybe instead of completely removing the documentation add a single line 
of what this is supposed to do and not to do.

E.g. "/* 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 +338,6 @@ 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.
> - */

Same here, something /* Walk to the next not signaled fence and grab a 
reference to it */.

Apart from that feel free add Reviewed-by: Christian König 
<christian.koenig@amd.com>

Regards,
Christian.

>   static void dma_resv_iter_walk_unlocked(struct dma_resv_iter *cursor)
>   {
>   	struct dma_resv *obj = cursor->obj;
> @@ -387,6 +373,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 +398,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 +427,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);		\