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

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

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

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.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 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;
 	}
 
-- 
2.39.2

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

[kernel test robot]

Hi Tvrtko,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-tip/drm-tip linus/master v6.4-rc5 next-20230608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tvrtko-Ursulin/dma-fence-Bypass-signaling-annotation-from-dma_fence_is_signaled/20230608-223804
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230608143059.1038115-1-tvrtko.ursulin%40linux.intel.com
patch subject: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled
config: arm-buildonly-randconfig-r006-20230608 (https://download.01.org/0day-ci/archive/20230609/202306090039.JEYvWyVv-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add drm-misc git://anongit.freedesktop.org/drm/drm-misc
        git fetch drm-misc drm-misc-next
        git checkout drm-misc/drm-misc-next
        b4 shazam https://lore.kernel.org/r/20230608143059.1038115-1-tvrtko.ursulin@linux.intel.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/dma-buf/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306090039.JEYvWyVv-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/dma-buf/dma-fence.c:454: warning: expecting prototype for __dma_fence_signal(). Prototype was for _dma_fence_signal() instead


vim +454 drivers/dma-buf/dma-fence.c

   446	
   447	/**
   448	 * __dma_fence_signal - signal completion of a fence bypassing critical section annotation
   449	 * @fence: the fence to signal
   450	 *
   451	 * This low-level helper should not be used by code external to dma-fence.h|c!
   452	 */
   453	int _dma_fence_signal(struct dma_fence *fence)
 > 454	{
   455		unsigned long flags;
   456		int ret;
   457	
   458		spin_lock_irqsave(fence->lock, flags);
   459		ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
   460		spin_unlock_irqrestore(fence->lock, flags);
   461	
   462		return ret;
   463	}
   464	EXPORT_SYMBOL(_dma_fence_signal);
   465	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[kernel test robot]

Hi Tvrtko,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on drm-tip/drm-tip linus/master v6.4-rc5 next-20230608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Tvrtko-Ursulin/dma-fence-Bypass-signaling-annotation-from-dma_fence_is_signaled/20230608-223804
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230608143059.1038115-1-tvrtko.ursulin%40linux.intel.com
patch subject: [PATCH] dma-fence: Bypass signaling annotation from dma_fence_is_signaled
config: arm-buildonly-randconfig-r006-20230608 (https://download.01.org/0day-ci/archive/20230609/202306090039.JEYvWyVv-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce (this is a W=1 build):
        mkdir -p ~/bin
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        git remote add drm-misc git://anongit.freedesktop.org/drm/drm-misc
        git fetch drm-misc drm-misc-next
        git checkout drm-misc/drm-misc-next
        b4 shazam https://lore.kernel.org/r/20230608143059.1038115-1-tvrtko.ursulin@linux.intel.com
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.3.0 ~/bin/make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/dma-buf/

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306090039.JEYvWyVv-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/dma-buf/dma-fence.c:454: warning: expecting prototype for __dma_fence_signal(). Prototype was for _dma_fence_signal() instead


vim +454 drivers/dma-buf/dma-fence.c

   446	
   447	/**
   448	 * __dma_fence_signal - signal completion of a fence bypassing critical section annotation
   449	 * @fence: the fence to signal
   450	 *
   451	 * This low-level helper should not be used by code external to dma-fence.h|c!
   452	 */
   453	int _dma_fence_signal(struct dma_fence *fence)
 > 454	{
   455		unsigned long flags;
   456		int ret;
   457	
   458		spin_lock_irqsave(fence->lock, flags);
   459		ret = dma_fence_signal_timestamp_locked(fence, ktime_get());
   460		spin_unlock_irqrestore(fence->lock, flags);
   461	
   462		return ret;
   463	}
   464	EXPORT_SYMBOL(_dma_fence_signal);
   465	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Christian König]

Am 08.06.23 um 16:30 schrieb Tvrtko Ursulin:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> 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.

Christian.

>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>   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;
>   	}
>   

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

[Tvrtko Ursulin]

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@intel.com>
>>
>> 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@intel.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> ---
>>   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;
>>       }
> 

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

[Christian König]

Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin:
>
> 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@intel.com>
>>>
>>> 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?

Well it's not broken, the complexity just doesn't make sense.

The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t 
itself. That was done to make sure that all dma_fence objects from a 
single context (or in other words hardware device) signal in the order 
of their sequence number, e.g. 1 first, then 2, then 3 etc...

But when somebody uses the dma_fence_is_signaled() function it's 
perfectly possible that this races with an interrupt handler which wants 
to signal fences on another CPU.

In other words we get:
CPU A:
dma_fence_is_signaled(fence with seqno=3)
fence->ops->signaled() returns true
dma_fence_signal()
spin_lock_irqsave(fence->lock)
signal seqno=3
...

CPU B:
device_driver_interrupt_handler()
spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete
signal seqno=1
signal seqno=2
seqno=3 is already signaled.
signal seqno=4
...

Either fence->lock should not be a pointer or we should not signal the 
fence from dma_fence_is_signaled().

I strongly think that later should be the way to go.

Regards,
Christian.

>
> Regards,
>
> Tvrtko
>
>>
>> Christian.
>>
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>>> ---
>>>   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;
>>>       }
>>

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

[Tvrtko Ursulin]

On 09/06/2023 13:52, Christian König wrote:
> Am 09.06.23 um 14:09 schrieb Tvrtko Ursulin:
>>
>> 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@intel.com>
>>>>
>>>> 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?
> 
> Well it's not broken, the complexity just doesn't make sense.
> 
> The dma_fence->lock is a pointer to a spinlock_t instead of a spinlock_t 
> itself. That was done to make sure that all dma_fence objects from a 
> single context (or in other words hardware device) signal in the order 
> of their sequence number, e.g. 1 first, then 2, then 3 etc...
> 
> But when somebody uses the dma_fence_is_signaled() function it's 
> perfectly possible that this races with an interrupt handler which wants 
> to signal fences on another CPU.
> 
> In other words we get:
> CPU A:
> dma_fence_is_signaled(fence with seqno=3)
> fence->ops->signaled() returns true
> dma_fence_signal()
> spin_lock_irqsave(fence->lock)
> signal seqno=3
> ...
> 
> CPU B:
> device_driver_interrupt_handler()
> spin_lock_irqsave(driver->lock) <- busy waits for CPU A to complete
> signal seqno=1
> signal seqno=2
> seqno=3 is already signaled.
> signal seqno=4
> ...

Right I see. However hm.. would the order of be guaranteed anyway, if 
someone would be observing what CPU B is doing via the 
dma_fence_is_signaled->test_bit? And in which scenarios would it matter 
if out of order signaled status could be observed?

> Either fence->lock should not be a pointer or we should not signal the 
> fence from dma_fence_is_signaled().
> 
> I strongly think that later should be the way to go.

Despite having wrote the above, I don't have any objections to removing 
this either. I don't see anything in the contract that requires it, but 
it was probably a bit before my time to know why it was added so I don't 
know if it could cause any subtle issues. It should be okay to try and see.

Regards,

Tvrtko