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

[Daniel Vetter <daniel@ffwll.ch>]

On Tue, Nov 30, 2021 at 11:04:11AM +0100, Christian König wrote:
> 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. */

I figured the names are selfexplanatory enough, but makes sense to keep a
bit more detail.
-Daniel

> 
> >   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);		\
> 

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