Re: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled

[Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>]

On 09/06/2023 07:32, Christian König wrote:
> Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin:
> > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> >
> > For dma_fence_is_signaled signaling critical path annotations are an
> > annoying cause of false positives when using dma_fence_is_signaled and
> > indirectly higher level helpers such as dma_resv_test_signaled etc.
> >
> > Drop the critical path annotation since the "is signaled" API does not
> > guarantee to ever change the signaled status anyway.
> >
> > We do that by adding a low level _dma_fence_signal helper and use it from
> > dma_fence_is_signaled.
>
> I have been considering dropping the signaling from the 
> dma_fence_is_signaled() function altogether.
>
> Doing this together with the spin locking we have in the dma_fence is 
> just utterly nonsense since the purpose of the external spinlock is to 
> keep the signaling in order while this here defeats that.
>
> The quick check is ok I think, but signaling the dma_fence and issuing 
> the callbacks should always come from the interrupt handler.

What do you think is broken with regards to ordering with the current 
code? The unlocked fast check?

Regards,

Tvrtko

> Christian.
>
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > Cc: Christian König <christian.koenig@xxxxxxx>
> > Cc: Daniel Vetter <daniel.vetter@xxxxxxxx>
> > ---
> >   drivers/dma-buf/dma-fence.c | 26 ++++++++++++++++++++------
> >   include/linux/dma-fence.h   |  3 ++-
> >   2 files changed, 22 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index f177c56269bb..ace1415f0566 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -444,6 +444,25 @@ int dma_fence_signal_locked(struct dma_fence *fence)
> >   }
> >   EXPORT_SYMBOL(dma_fence_signal_locked);
> > +/**
> > + * __dma_fence_signal - signal completion of a fence bypassing 
> > critical section annotation
> > + * @fence: the fence to signal
> > + *
> > + * This low-level helper should not be used by code external to 
> > dma-fence.h|c!
> > + */
> > +int _dma_fence_signal(struct dma_fence *fence)
> > +{
> > +    unsigned long flags;
> > +    int ret;
> > +
> > +    spin_lock_irqsave(fence->lock, flags);
> > +    ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
> > +    spin_unlock_irqrestore(fence->lock, flags);
> > +
> > +    return ret;
> > +}
> > +EXPORT_SYMBOL(_dma_fence_signal);
> > +
> >   /**
> >    * dma_fence_signal - signal completion of a fence
> >    * @fence: the fence to signal
> > @@ -459,7 +478,6 @@ EXPORT_SYMBOL(dma_fence_signal_locked);
> >    */
> >   int dma_fence_signal(struct dma_fence *fence)
> >   {
> > -    unsigned long flags;
> >       int ret;
> >       bool tmp;
> > @@ -467,11 +485,7 @@ int dma_fence_signal(struct dma_fence *fence)
> >           return -EINVAL;
> >       tmp = dma_fence_begin_signalling();
> > -
> > -    spin_lock_irqsave(fence->lock, flags);
> > -    ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
> > -    spin_unlock_irqrestore(fence->lock, flags);
> > -
> > +    ret = _dma_fence_signal(fence);
> >       dma_fence_end_signalling(tmp);
> >       return ret;
> > diff --git a/include/linux/dma-fence.h b/include/linux/dma-fence.h
> > index d54b595a0fe0..d94768ad70e4 100644
> > --- a/include/linux/dma-fence.h
> > +++ b/include/linux/dma-fence.h
> > @@ -387,6 +387,7 @@ static inline void dma_fence_end_signalling(bool 
> > cookie) {}
> >   static inline void __dma_fence_might_wait(void) {}
> >   #endif
> > +int _dma_fence_signal(struct dma_fence *fence);
> >   int dma_fence_signal(struct dma_fence *fence);
> >   int dma_fence_signal_locked(struct dma_fence *fence);
> >   int dma_fence_signal_timestamp(struct dma_fence *fence, ktime_t 
> > timestamp);
> > @@ -452,7 +453,7 @@ dma_fence_is_signaled(struct dma_fence *fence)
> >           return true;
> >       if (fence->ops->signaled && fence->ops->signaled(fence)) {
> > -        dma_fence_signal(fence);
> > +        _dma_fence_signal(fence);
> >           return true;
> >       }