| Message ID | 20230608143059.1038115-1-tvrtko.ursulin@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | dma-fence: Bypass signaling annotation from dma_fence_is_signaled | expand |
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
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; > } >
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; >> } >
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; >>> } >>
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
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; }