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@xxxxxxxxx>
> > Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
> > Cc: "Christian König" <christian.koenig@xxxxxxx>
> > Cc: linux-media@xxxxxxxxxxxxxxx
> > Cc: linaro-mm-sig@xxxxxxxxxxxxxxxx
> > ---
> > 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@xxxxxxx>
>
> 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
